Skip to content

[fast-reboot]: Fix fail to execute fast-reboot problem#1047

Merged
lguohan merged 2 commits intosonic-net:masterfrom
kuanyu99:ky_modify_fast-reboot-dump
Aug 21, 2020
Merged

[fast-reboot]: Fix fail to execute fast-reboot problem#1047
lguohan merged 2 commits intosonic-net:masterfrom
kuanyu99:ky_modify_fast-reboot-dump

Conversation

@kuanyu99
Copy link
Contributor

@kuanyu99 kuanyu99 commented Aug 12, 2020 •
edited
Loading

- What I did

Try to fix fast-reboot function from error

- How I did it

Separate IPv4 and IPv6 neighbor address when fast-reboot try to send ARP to them

- How to verify it

Make it learn some IPv6 neighbor address and ask it to fast-reboot. It should not return error.

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

- Error log during executing fast-reboot

Jul 23 21:18:47.612045 as5835-54x ERR fast-reboot-dump: Got an exception illegal IP address string passed to inet_aton: Traceback: Traceback (most recent call last):#012  File "/usr/bin/fast-reboot-dump.py", line 283, in <module>#012    res = main()#012  File "/usr/bin/fast-reboot-dump.py", line 276, in main#012    garp_send(arp_entries, map_mac_ip_per_vlan)#012  File "/usr/bin/fast-reboot-dump.py", line 223, in garp_send#012    send_arp(sockets[src_if], src_mac_addrs[src_if], src_ip_addrs[vlan_name], dst_mac, dst_ip)#012  File "/usr/bin/fast-reboot-dump.py", line 191, in send_arp#012    dst_ip = socket.inet_aton(dst_ip_s)#012error: illegal IP address string passed to inet_aton
Jul 23 21:18:47.648276 as5835-54x NOTICE admin: fast-reboot failure (12) cleanup ...

@kuanyu99
[fast-reboot]: Fix fail to execute fast-reboot problem
027c7c2 
* Fix the fast-reboot-dump.py error when it try to use inet_aton to translate ipv6 address
* Add send_ndp as TODO in fast-reboot-dump.py to ipv6 target
@lguohan lguohan requested a review from tahmed-dev August 12, 2020 22:47
@tahmed-dev
Copy link
Contributor

tahmed-dev commented Aug 13, 2020

Thanks @kuanyu99 for your effort. Can you please add some details about the error that was seen during fast-reboot?

@kuanyu99
Copy link
Contributor Author

kuanyu99 commented Aug 14, 2020

Thanks @kuanyu99 for your effort. Can you please add some details about the error that was seen during fast-reboot?

Hi Tahmed,
I replenish the error log into the original comment.

tahmed-dev
tahmed-dev suggested changes Aug 14, 2020
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2020

This pull request introduces 1 alert when merging 2fe3c55dd43388d7e7e725710bf790747ebad96a into d5fdd74 - view on LGTM.com

new alerts:

  • 1 for Unused import

tahmed-dev
tahmed-dev reviewed Aug 15, 2020
View reviewed changes
@kuanyu99
[fast-reboot-dump]: Enhance some ip address check method
3401616 
* Use ipaddress to check if address is ipv4 address
@kuanyu99 kuanyu99 force-pushed the ky_modify_fast-reboot-dump branch from 55328e3 to 3401616 Compare August 17, 2020 00:38
tahmed-dev
tahmed-dev approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@tahmed-dev tahmed-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Thanks!

@lguohan lguohan merged commit 56a97a6 into sonic-net:master Aug 21, 2020
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Apr 2, 2021

Is this needed for 201911?

abdosi pushed a commit that referenced this pull request Apr 4, 2021
@kuanyu99 @abdosi
[fast-reboot]: Fix fail to execute fast-reboot problem (#1047)
4993a36 
* Fix the fast-reboot-dump.py error when it try to use inet_aton to translate ipv6 address
* Add send_ndp as TODO in fast-reboot-dump.py to ipv6 target
kuanyu99
kuanyu99 commented Apr 6, 2021
View reviewed changes
scripts/fast-reboot-dump.py
arp_output.append(obj)

ip_addr = key.split(':')[2]
if ipaddress.ip_interface(ip_addr).ip.version != 4:
Copy link
Contributor Author

@kuanyu99 kuanyu99 Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is a necessary modification in 201911 to handle IPv6 entries.
But there is some problem using the ipaddress.ip_interface(ip_addr).ip.version to parse it.
From my opinion, I will consider using some simple checking like
if '.' not in ip_addr:
If no '.' inside the ip_addr, it is a IPv6 address.

Copy link
Contributor

@prsunny prsunny May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we identified there is problem with script, why is it taken to 201911? @qiluo-msft, @tahmed-dev, @abdosi . This is breaking the 201911 fastboot.

Copy link
Contributor

@qiluo-msft qiluo-msft May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prsunny Do you have a Github issue? Are you suggesting revert?

Copy link
Contributor Author

@kuanyu99 kuanyu99 May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will suggest replace the line if ipaddress.ip_interface(ip_addr).ip.version != 4: with this line if '.' not in ip_addr: to handle the IPv6 entries. And also fix the problem here.
Simple is powerful.

Copy link
Contributor

@prsunny prsunny May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking with @bingwang-ms to see if this can be cherry-picked - #1164

@prsunny prsunny mentioned this pull request May 27, 2021
Closed
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
@abdosi
[Submodule update] sonic-utilities
9143cca 
ad9022e (HEAD -> 201911, origin/201911) Reduce time taken by show commands on multi-asic platforms (sonic-net#1544)
4993a36 [fast-reboot]: Fix fail to execute fast-reboot problem (sonic-net#1047)

Signed-off-by: Abhishek Dosi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qiluo-msft qiluo-msft qiluo-msft left review comments

@prsunny prsunny prsunny left review comments

+1 more reviewer

@tahmed-dev tahmed-dev tahmed-dev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Included in 201911 Branch Request for 201911 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kuanyu99 @tahmed-dev @qiluo-msft @prsunny @lguohan @abdosi