Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Aug 21, 2020
Merged

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

merged 2 commits into from
Aug 21, 2020

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

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

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

@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

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

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

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

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

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

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

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

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