Updated lldpRemManAddrTable to use all the management ip address associated with interface. #201

abdosi · 2021-03-12T19:55:50Z

Why I did:

Fixes:-
[snmp] LLDPRemManAddrTable does not gets populated if Remote Interface has both ipv4/6 address sonic-buildimage#7036
lldpRemManAddrTable SNMP failure due to DHCP on Mgmt interface sonic-buildimage#6636
Updated to use prefix of 16 byte instead of 6 byte for Ipv6 Management Address. Confirmed with other Vendor Nos
Fixed lookup() so that get-next work correctly from test-cases.
Updated all files to use new utility API ip2byte_tuple()

How I did:

Loop through all valid and unique management ip address associated
with remote lldp interface and append to the the oid key list.
Removed some of utility API as they were not being used.

How I verify:

Extend Unit test case to cover all possible combination of management ip address
Verify the snmpwalk when both v4 and v6 address are present

admin@str-sxxxx-on-2:~$ redis-cli -n 0
127.0.0.1:6379> hgetall "LLDP_ENTRY_TABLE:eth0"
 1) "lldp_rem_sys_name"
 2) "str-22ex-235ab08"
 3) "lldp_rem_index"
 4) "1"
 5) "lldp_rem_sys_desc"
 6) "Juniper Networks, Inc. ex2200-48t-4g , version 12.3R4.6 Build date: 2013-09-13 02:53:16 UTC "
 7) "lldp_rem_man_addr"
 8) "10.224.25.100,2603:10e2:290:5016::"
 9) "lldp_rem_time_mark"
10) "84379"
11) "lldp_rem_port_id"
12) "ge-0/0/12"
13) "lldp_rem_sys_cap_supported"
14) "28 00"
15) "lldp_rem_sys_cap_enabled"
16) "28 00"
17) "lldp_rem_chassis_id_subtype"
18) "4"
19) "lldp_rem_port_id_subtype"
20) "5"
21) "lldp_rem_port_desc"
22) "ge-0/0/12.0"
23) "lldp_rem_chassis_id"
24) "f4:b5:2f:56:2d:80"


root@str-sxxxx-on-2:/# snmpwalk -v2c -c msft 127.0.0.1 1.0.8802.1.1.2.1.4.2
iso.0.8802.1.1.2.1.4.2.1.5.84379.10000.1.1.4.10.224.25.100 = OID: iso.3.6.1.2.1.2.2.1.1
iso.0.8802.1.1.2.1.4.2.1.5.84379.10000.1.2.6.9731.4322.656.20502.0.0 = OID: iso.3.6.1.2.1.2.2.1.1

sonic-net/sonic-buildimage#7036 sonic-net/sonic-buildimage#6636 Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

lgtm-com · 2021-03-12T20:07:08Z

This pull request introduces 1 alert when merging 4dbc4dc into 9b83459 - view on LGTM.com

new alerts:

1 for Unused local variable

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi

Can you add multi-asic test case changes in tests/namespace/test_lldp.py also add namespace mock data in tests/mock_tables/asic0... etc as required.

@SuvarnaMeenakshi Done

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

src/sonic_ax_impl/mibs/ieee802_1ab.py

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi

LGTM.

SuvarnaMeenakshi · 2021-03-12T22:43:13Z

tests/namespace/test_lldp.py

+ for entry in range(3, 6):
+ mib_entry = self.lut[(1, 0, 8802, 1, 1, 2, 1, 4, 2, 1, entry)]
+ ret = mib_entry(sub_id=(1, 5))
+ self.assertIsNotNone(ret)


How is the return value checked if both ipv4 and ipv6 exits?

intention of testcase is to check when both ip address are present we can get the data from first key (can be either v4 or v6) for given interface. Basically to check we are not ignoring management ip information in such case.

@SuvarnaMeenakshi Updated test case for this. Fixed lookup() API.

qiluo-msft · 2021-03-13T00:45:58Z

src/sonic_ax_impl/mibs/ieee802_1ab.py

- self.mgmt_ips.update({if_name: {"ip_str": mgmt_ip_str,
- "addr_subtype": subtype,
- "addr_hex": ip_hex}})
+ for mgmt_ip in set(mgmt_ip_str.split(',')):


set [](start = 27, length = 3)

What is the benefit of set ? Just use splited list? #Closed

this is just extra check in case if we have same ip's

Please be aware that different ipv6 strings could be the same ipv6 address.

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

@qiluo-msft Updated.

qiluo-msft · 2021-03-13T01:14:55Z

src/sonic_ax_impl/mibs/ieee802_1ab.py

+ mgmt_ip_sub_oid = (addr_subtype_sub_oid, *[int(i) for i in mgmt_ip.split('.')])
+ elif subtype == ManAddrConst.man_addr_subtype_ipv6:
+ addr_subtype_sub_oid = 6
+ mgmt_ip_sub_oid = (addr_subtype_sub_oid, *[int(i, 16) if i else 0 for i in mgmt_ip.split(':')])


[int(i, 16) if i else 0 for i in mgmt_ip.split(':')] [](start = 62, length = 52)

Seems you are trying to parse ipv6

[int(i, 16) if i else 0 for i in mgmt_ip.split(':')]

but this is not working for general cases #Closed

@qiluo-msft Updated to use exploded API of ipaddress module.
https://docs.python.org/3/library/ipaddress.html

qiluo-msft

As comments

@qiluo-msft Made required changes.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

qiluo-msft · 2021-03-13T05:26:53Z

src/sonic_ax_impl/mibs/ieee802_1ab.py

+ for mgmt_ip in mgmt_ip_str.split(','):
+ time_mark = int(lldp_kvs['lldp_rem_time_mark'])
+ remote_index = int(lldp_kvs['lldp_rem_index'])
+ subtype, exploded_mgmt_ip = self.get_subtype_and_exploded_ip(mgmt_ip)


get_subtype_and_exploded_ip [](start = 49, length = 27)

The code is okay, I am think about more readable and concise.
I found some sample code: https://github.com/Azure/sonic-snmpagent/blob/9b83459d647622a12387085f01d2256fd69430e2/src/sonic_ax_impl/mibs/vendor/cisco/bgp4.py#L41-L46

Suggest you extract a common method and reuse. #Closed

Thanks @qiluo-msft . Updated to use updated utility API.

tests/test_lldp.py

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

qiluo-msft · 2021-03-14T20:25:35Z

tests/test_lldp.py

+ self.assertEqual(value0.type_, ValueType.INTEGER)
+ self.assertEqual(value0.data, 2)
+
+ # Verfiy only valid ipv6 address exiit. Ethernet12 has this config.


exiit [](start = 41, length = 5)

typo #Closed

qiluo-msft · 2021-03-14T20:25:41Z

tests/test_lldp.py

+ self.assertEqual(value0.type_, ValueType.INTEGER)
+ self.assertEqual(value0.data, 2)
+
+ # Verfiy only valid ipv4 address exit. Ethernet8 has this config.


exit [](start = 41, length = 4)

typo #Closed

qiluo-msft · 2021-03-14T20:25:44Z

tests/test_lldp.py

+ self.assertEqual(value0.type_, ValueType.OBJECT_IDENTIFIER)
+ self.assertEqual(str(value0.data), str(ObjectIdentifier(5, 2, 0, 0, (1, 2, 2, 1, 1))))
+
+ # Verfiy both valid ipv4 and invalid ipv6 address exit. Ethernet5 has this config.


exit [](start = 58, length = 4)

typo #Closed

qiluo-msft · 2021-03-14T20:25:51Z

tests/test_lldp.py

+ self.assertEqual(value0.type_, ValueType.INTEGER)
+ self.assertEqual(value0.data, 2)
+
+ # Verfiy no mgmt address exit. Ethernet16 has this config.


exit [](start = 33, length = 4)

typo #Closed

qiluo-msft

Minor typos in code comments

Sorry fixed now.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi · 2021-03-15T17:52:36Z

src/sonic_ax_impl/mibs/ieee802_1ab.py

+ continue
+ mgmt_ip_tuple = ip2byte_tuple(mgmt_ip)
+ if mgmt_ip_tuple in mgmt_ip_set:
+ continue


if mgmt_ip_set is present in mgmt_ip_tuple, should the data be still updated instead of continuing?
So that the time_mark gets updated?

we keep key tuple (eg: time_mark) only for unique ip's. lldp_rem_time_mark is associated with all unique ip's in lldp_rem_man_addr. All the data is constant for this SNMP table entry

qiluo-msft

lgtm

…ciated with interface. (#201) Fixes:- sonic-net/sonic-buildimage#7036 sonic-net/sonic-buildimage#6636 Updated to use prefix of 16 byte instead of 6 byte for Ipv6 Management Address. Confirmed with other Vendor Nos Fixed lookup() so that get-next work correctly from test-cases. Updated all files to use new utility API ip2byte_tuple()

…ciated with interface. (sonic-net#201) Fixes:- sonic-net/sonic-buildimage#7036 sonic-net/sonic-buildimage#6636 Updated to use prefix of 16 byte instead of 6 byte for Ipv6 Management Address. Confirmed with other Vendor Nos Fixed lookup() so that get-next work correctly from test-cases. Updated all files to use new utility API ip2byte_tuple()

Fixes:

3194691

sonic-net/sonic-buildimage#7036 sonic-net/sonic-buildimage#6636 Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi requested review from qiluo-msft and SuvarnaMeenakshi March 12, 2021 19:55

abdosi added Bug Request for 201911 Branch labels Mar 12, 2021

Fix typo in comment

4dbc4dc

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

FIX LGTM

a0e0ef1

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi requested changes Mar 12, 2021

View reviewed changes

Updated for Multi-asic

9749374

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi reviewed Mar 12, 2021

View reviewed changes

src/sonic_ax_impl/mibs/ieee802_1ab.py Outdated Show resolved Hide resolved

Fix

2b3ffc7

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi previously approved these changes Mar 12, 2021

View reviewed changes

SuvarnaMeenakshi self-requested a review March 12, 2021 22:42

SuvarnaMeenakshi reviewed Mar 12, 2021

View reviewed changes

qiluo-msft reviewed Mar 13, 2021

View reviewed changes

qiluo-msft requested changes Mar 13, 2021

View reviewed changes

Address Review Comments

7f74185

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi dismissed SuvarnaMeenakshi’s stale review via 7f74185 March 13, 2021 02:43

abdosi added 4 commits March 13, 2021 02:46

Fix

448cad1

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Fix error case

4aaa793

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Fix

6956f42

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Fix typo in comment

09d41cf

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

qiluo-msft reviewed Mar 13, 2021

View reviewed changes

tests/test_lldp.py Outdated Show resolved Hide resolved

abdosi added 2 commits March 14, 2021 02:29

Address Review Comments and more fixes in code and test

341f548

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Verified GET also.

80417ee

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

qiluo-msft reviewed Mar 14, 2021

View reviewed changes

Fix Typo

bab90f4

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

SuvarnaMeenakshi reviewed Mar 15, 2021

View reviewed changes

qiluo-msft approved these changes Mar 15, 2021

View reviewed changes

SuvarnaMeenakshi approved these changes Mar 15, 2021

View reviewed changes

abdosi merged commit 292024a into sonic-net:master Mar 15, 2021

abdosi deleted the snmp_lldp_fix branch March 15, 2021 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated lldpRemManAddrTable to use all the management ip address associated with interface. #201

Updated lldpRemManAddrTable to use all the management ip address associated with interface. #201

abdosi commented Mar 12, 2021 •

edited

Loading

lgtm-com bot commented Mar 12, 2021

SuvarnaMeenakshi left a comment •

edited by abdosi

Loading

SuvarnaMeenakshi left a comment

SuvarnaMeenakshi Mar 12, 2021 •

edited

Loading

abdosi Mar 12, 2021

abdosi Mar 14, 2021

qiluo-msft Mar 13, 2021 •

edited

Loading

abdosi Mar 13, 2021

qiluo-msft Mar 13, 2021

abdosi Mar 13, 2021

qiluo-msft Mar 13, 2021 •

edited

Loading

abdosi Mar 13, 2021

qiluo-msft left a comment •

edited by abdosi

Loading

qiluo-msft Mar 13, 2021 •

edited

Loading

abdosi Mar 14, 2021

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft left a comment •

edited by abdosi

Loading

SuvarnaMeenakshi Mar 15, 2021

abdosi Mar 15, 2021 •

edited

Loading

qiluo-msft left a comment

Updated lldpRemManAddrTable to use all the management ip address associated with interface. #201

Updated lldpRemManAddrTable to use all the management ip address associated with interface. #201

Conversation

abdosi commented Mar 12, 2021 • edited Loading

lgtm-com bot commented Mar 12, 2021

SuvarnaMeenakshi left a comment • edited by abdosi Loading

Choose a reason for hiding this comment

SuvarnaMeenakshi left a comment

Choose a reason for hiding this comment

SuvarnaMeenakshi Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment • edited by abdosi Loading

Choose a reason for hiding this comment

qiluo-msft Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft left a comment • edited by abdosi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdosi Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

abdosi commented Mar 12, 2021 •

edited

Loading

SuvarnaMeenakshi left a comment •

edited by abdosi

Loading

SuvarnaMeenakshi Mar 12, 2021 •

edited

Loading

qiluo-msft Mar 13, 2021 •

edited

Loading

qiluo-msft Mar 13, 2021 •

edited

Loading

qiluo-msft left a comment •

edited by abdosi

Loading

qiluo-msft Mar 13, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft Mar 14, 2021 •

edited

Loading

qiluo-msft left a comment •

edited by abdosi

Loading

abdosi Mar 15, 2021 •

edited

Loading