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

Fix: correctly handle that lldp_loc_man_addr contains only IPv6 address without IPv4 address #164

Merged
merged 5 commits into from
Oct 12, 2020
Merged

Fix: correctly handle that lldp_loc_man_addr contains only IPv6 address without IPv4 address #164

merged 5 commits into from
Oct 12, 2020

Conversation

qiluo-msft
Copy link
Contributor

@lguohan
Copy link
Contributor

lguohan commented Oct 8, 2020

what is the miss here? can you add unit test to prevent future regression?

@qiluo-msft
Refix
5d3b8f9 
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
@qiluo-msft
Reimplement LLDPLocManAddrUpdater
83cdb0d
@qiluo-msft
Copy link
Contributor Author

qiluo-msft commented Oct 9, 2020 •
edited
Loading

The issue is that LLDP_LOC_CHASSIS.lldp_loc_man_addr has only one IPv6 address, which happens outside the snmp container. The fix is just to handle this unexpected data peacefully.

We are trying to figure out a way to unit test.

@qiluo-msft qiluo-msft marked this pull request as draft October 9, 2020 05:40
@lguohan
Copy link
Contributor

lguohan commented Oct 9, 2020

what changed? is this a regression?

@qiluo-msft
Fix test case
db0b843
@qiluo-msft
Revert some back and fix lookup()
dc327db 
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
@qiluo-msft qiluo-msft mentioned this pull request Oct 11, 2020
Closed
@qiluo-msft qiluo-msft marked this pull request as ready for review October 11, 2020 06:58
@qiluo-msft
Copy link
Contributor Author

I have not investigate outside snmp container. I believe it is an image regression issue.

@lguohan
Copy link
Contributor

lguohan commented Oct 12, 2020

The issue is that LLDP_LOC_CHASSIS.lldp_loc_man_addr has only one IPv6 address, which happens outside the snmp container

how many ipv6 addresses does this have before?

@qiluo-msft
Copy link
Contributor Author

Sorry for misleading. The statement should be:
The issue is that LLDP_LOC_CHASSIS.lldp_loc_man_addr has no IPv4 address(es) at all but only one or more IPv6 address(es), which happens outside the snmp container. The fix is just to handle this unexpected data peacefully.

@lguohan
Copy link
Contributor

lguohan commented Oct 12, 2020

what is the situation before?

@qiluo-msft
Copy link
Contributor Author

In a good system, I see one IPv4 plus one IPv6 address.

@lguohan
Copy link
Contributor

lguohan commented Oct 12, 2020

make sense, can you put this into the issue and ask @gen-hwa to look at it further? why we are missing ipv4 address now?

@qiluo-msft qiluo-msft changed the title Fix mgmt_ip_sub_oid default value to prevent later exception Fix: correctly handle that lldp_loc_man_addr contains only IPv6 address without IPv4 address Oct 12, 2020
SuvarnaMeenakshi
SuvarnaMeenakshi reviewed Oct 12, 2020
View reviewed changes
src/sonic_ax_impl/mibs/ieee802_1ab.py
@@ -334,12 +337,12 @@ def update_data(self):

def get_next(self, sub_id):
right = bisect_right(self.man_addr_list, sub_id)
if right == len(self.man_addr_list):
if right >= len(self.man_addr_list):
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Oct 12, 2020 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

Why is this change required? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not required. It just make code more robust.

In reply to: 503552127 [](ancestors = 503552127)

SuvarnaMeenakshi
SuvarnaMeenakshi reviewed Oct 12, 2020
View reviewed changes
src/sonic_ax_impl/mibs/ieee802_1ab.py
@@ -317,6 +317,9 @@ def reinit_data(self):
if '.' in mgmt_ip:
mgmt_ip_sub_oid = (addr_subtype_sub_oid, *[int(i) for i in mgmt_ip.split('.')])
break
else:
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Oct 12, 2020 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

else block is not intended to match the right if block. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for-else structure, which is a critical change in this PR. If there is no IPv4 address matched in the for loop, we need to return soon. Otherwise future statement will fail since mgmt_ip_sub_oid==None

In reply to: 503552324 [](ancestors = 503552324)

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thank you.

@qiluo-msft qiluo-msft merged commit 6e4a796 into sonic-net:master Oct 12, 2020
@qiluo-msft qiluo-msft deleted the qiluo/fixmgmtlldp branch October 12, 2020 22:13
abdosi pushed a commit that referenced this pull request Dec 31, 2020
@qiluo-msft @abdosi
Fix: correctly handle that lldp_loc_man_addr contains only IPv6 addre…
005cf2e 
…ss without IPv4 address (#164)

* Fix mgmt_ip_sub_oid default value to prevent later exception
* Refix
* Reimplement LLDPLocManAddrUpdater
* Fix test case
* Revert some back and fix lookup()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lguohan lguohan lguohan approved these changes

@SuvarnaMeenakshi SuvarnaMeenakshi SuvarnaMeenakshi 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.

None yet
4 participants
@qiluo-msft @lguohan @SuvarnaMeenakshi @abdosi