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.

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

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

Merged
merged 13 commits into from
Mar 15, 2021
79 changes: 40 additions & 39 deletions src/sonic_ax_impl/mibs/ieee802_1ab.py
Original file line number Diff line number Diff line change
Expand Up @@ -494,10 +494,9 @@ def __init__(self):
# establish connection to application database.
Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB)
self.if_range = []
self.mgmt_ips = {}
self.if_with_mgmt_ips = set()
self.oid_name_map = {}
self.mgmt_oid_name_map = {}
self.mgmt_ip_str = None
self.pubsub = [None] * len(self.db_conn)

def update_rem_if_mgmt(self, if_oid, if_name):
Expand All @@ -511,28 +510,39 @@ def update_rem_if_mgmt(self, if_oid, if_name):
if len(mgmt_ip_str) == 0:
# the peer advertise an emtpy mgmt address
return
time_mark = int(lldp_kvs['lldp_rem_time_mark'])
remote_index = int(lldp_kvs['lldp_rem_index'])
subtype = self.get_subtype(mgmt_ip_str)
ip_hex = self.get_ip_hex(mgmt_ip_str, subtype)
if subtype == ManAddrConst.man_addr_subtype_ipv4:
addr_subtype_sub_oid = 4
mgmt_ip_sub_oid = (addr_subtype_sub_oid, *[int(i) for i in mgmt_ip_str.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_str.split(':')])
else:
logger.warning("Invalid management IP {}".format(mgmt_ip_str))
return
self.if_range.append((time_mark,
if_oid,
remote_index,
subtype,
*mgmt_ip_sub_oid))

self.mgmt_ips.update({if_name: {"ip_str": mgmt_ip_str,
"addr_subtype": subtype,
"addr_hex": ip_hex}})

mgmt_ip_set=set()
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)
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 13, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


# Invalid management IP
if not subtype or not exploded_mgmt_ip:
logger.warning("Invalid management IP {}".format(mgmt_ip))
continue
# Non-Unique management IP
elif exploded_mgmt_ip in mgmt_ip_set:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@abdosi abdosi Mar 15, 2021

Choose a reason for hiding this comment

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

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

# IPv4 management IP
elif subtype == ManAddrConst.man_addr_subtype_ipv4:
addr_subtype_sub_oid = 4
mgmt_ip_sub_oid = (addr_subtype_sub_oid, *[int(i) for i in exploded_mgmt_ip.split('.')])
mgmt_ip_set.add(exploded_mgmt_ip)
# IPv6 management IP
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 exploded_mgmt_ip.split(':')])
mgmt_ip_set.add(exploded_mgmt_ip)
else:
pass
self.if_range.append((time_mark,
if_oid,
remote_index,
subtype,
*mgmt_ip_sub_oid))
self.if_with_mgmt_ips.add(if_name)

except (KeyError, AttributeError) as e:
logger.warning("Error updating remote mgmt addr: {}".format(e))
return
Expand Down Expand Up @@ -577,7 +587,7 @@ def reinit_data(self):
Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB)

self.if_range = []
self.mgmt_ips = {}
self.if_with_mgmt_ips = set()
for if_oid, if_name in self.oid_name_map.items():
self.update_rem_if_mgmt(if_oid, if_name)

Expand All @@ -594,7 +604,7 @@ def lookup(self, sub_id, callable):
if sub_id not in self.oid_name_map:
return None
if_name = self.oid_name_map[sub_id]
if if_name not in self.mgmt_ips:
if if_name not in self.if_with_mgmt_ips:
# no data for this interface
return None
return callable(sub_id, if_name)
Expand All @@ -608,30 +618,21 @@ def get_ip_hex(self, mgmt_ip_str, subtype):
hex_ip = None
return hex_ip

def get_subtype(self, ip_str):
def get_subtype_and_exploded_ip(self, ip_str):
try:
ipaddress.IPv4Address(ip_str)
return ManAddrConst.man_addr_subtype_ipv4
return ManAddrConst.man_addr_subtype_ipv4, ip_str
except ipaddress.AddressValueError:
# not a valid IPv4
pass
try:
ipaddress.IPv6Address(ip_str)
return ManAddrConst.man_addr_subtype_ipv6
return ManAddrConst.man_addr_subtype_ipv6, ipaddress.IPv6Address(ip_str).exploded
except ipaddress.AddressValueError:
# not a valid IPv6
logger.warning("Invalid mgmt IP {}".format(ip_str))
return None

def man_addr_subtype(self, sub_id, if_name):
return self.mgmt_ips[if_name]['addr_subtype']
return None, None

def man_addr(self, sub_id, if_name):
"""
:param sub_id:
:return: MGMT IP in HEX
"""
return self.mgmt_ips[if_name]['addr_hex']

@staticmethod
def man_addr_if_subtype(sub_id, _): return ManAddrConst.man_addr_if_subtype
Expand Down
8 changes: 4 additions & 4 deletions tests/mock_tables/appl_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet1",
"lldp_rem_man_addr": "10.224.25.100"
"lldp_rem_man_addr": "10.224.25.100,2603:10e2:290:5016::"
},
"LLDP_ENTRY_TABLE:Ethernet4": {
"lldp_rem_port_id_subtype": "5",
Expand All @@ -25,7 +25,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet2",
"lldp_rem_man_addr": "fe80::268a:7ff:fe3f:834c"
"lldp_rem_man_addr": "fe80::268a::7ff:fe3f:834c,10.224.25.101"
},
"LLDP_ENTRY_TABLE:Ethernet8": {
"lldp_rem_port_id_subtype": "5",
Expand Down Expand Up @@ -53,7 +53,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet4",
"lldp_rem_man_addr": "10.224.25.103"
"lldp_rem_man_addr": "fe80:268a::7ff:fe3f:834c"
},
"LLDP_ENTRY_TABLE:Ethernet16": {
"lldp_rem_port_id_subtype": "5",
Expand All @@ -67,7 +67,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet5",
"lldp_rem_man_addr": "10.224.25.104"
"lldp_rem_man_addr": ""
},
"LLDP_ENTRY_TABLE:Ethernet20": {
"lldp_rem_port_id_subtype": "5",
Expand Down
4 changes: 2 additions & 2 deletions tests/mock_tables/asic0/appl_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet1",
"lldp_rem_man_addr": "10.224.25.100"
"lldp_rem_man_addr": "10.224.25.101,fe80:268a::7ff:fe3f:834c"
},
"LLDP_ENTRY_TABLE:Ethernet4": {
"lldp_rem_port_id_subtype": "5",
Expand All @@ -25,7 +25,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet2",
"lldp_rem_man_addr": "fe80::268a:7ff:fe3f:834c"
"lldp_rem_man_addr": "fe80::268a::7ff:fe3f:834c,10.224.25.102"
},
"LLDP_LOC_CHASSIS": {
"lldp_loc_chassis_id_subtype": "5",
Expand Down
4 changes: 2 additions & 2 deletions tests/mock_tables/asic1/appl_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet3",
"lldp_rem_man_addr": "fe80::268a:7ff:fe3f:834c"
"lldp_rem_man_addr": "10.224.25.102"
},
"LLDP_ENTRY_TABLE:Ethernet12": {
"lldp_rem_port_id_subtype": "5",
Expand All @@ -25,7 +25,7 @@
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet4",
"lldp_rem_man_addr": "10.224.25.102"
"lldp_rem_man_addr": "fe80::268a:7ff:fe3f:834c"
},
"LLDP_LOC_CHASSIS": {
"lldp_loc_chassis_id_subtype": "5",
Expand Down
19 changes: 19 additions & 0 deletions tests/mock_tables/asic2/appl_db.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,18 @@
{
"LLDP_ENTRY_TABLE:Ethernet16": {
"lldp_rem_port_id_subtype": "5",
"lldp_rem_sys_cap_supported": "28 00",
"lldp_rem_index": "1",
"lldp_rem_chassis_id": "00:11:22:33:44:55",
"lldp_rem_sys_desc": "I'm a little teapot.",
"lldp_rem_time_mark": "18543",
"lldp_rem_sys_cap_enabled": "28 00",
"lldp_rem_port_desc": " ",
"lldp_rem_chassis_id_subtype": "4",
"lldp_rem_sys_name": "switch13",
"lldp_rem_port_id": "Ethernet5",
"lldp_rem_man_addr": ""
},
"LLDP_LOC_CHASSIS": {
"lldp_loc_chassis_id_subtype": "5",
"lldp_loc_chassis_id": "00:11:22:AB:CD:EF",
Expand All @@ -8,6 +22,11 @@
"lldp_loc_sys_cap_supported": "28 00",
"lldp_loc_man_addr": "fe80::ce37:abff:feec:de9c"
},
"PORT_TABLE:Ethernet16": {
"description": "snowflake",
"alias": "etp5",
"speed": 100000
},
"PORT_TABLE:Ethernet-BP16": {
"description": "backplane",
"alias": "etp9",
Expand Down
25 changes: 25 additions & 0 deletions tests/namespace/test_lldp.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,36 @@ def test_subtype_lldp_loc_man_addr_table(self):


def test_subtype_lldp_rem_man_addr_table(self):
# Verfiy both valid ipv4 and ipv6 address exit
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, 1))
self.assertIsNotNone(ret)
print(ret)
# Verfiy both valid ipv4 and invalid ipv6 address exit
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)
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Mar 12, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

print(ret)
# Verfiy only valid ipv4 address exit
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, 9))
self.assertIsNotNone(ret)
print(ret)
# Verfiy only valid ipv6 address exit
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, 13))
self.assertIsNotNone(ret)
print(ret)
# Verfiy no mgmt address exit
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, 17))
self.assertIsNone(ret)
print(ret)

def test_local_port_identification(self):
mib_entry = self.lut[(1, 0, 8802, 1, 1, 2, 1, 3, 7, 1, 3)]
Expand Down
25 changes: 25 additions & 0 deletions tests/test_lldp.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,36 @@ def test_subtype_lldp_loc_man_addr_table(self):


def test_subtype_lldp_rem_man_addr_table(self):
# Verfiy both valid ipv4 and ipv6 address exist
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, 1))
self.assertIsNotNone(ret)
print(ret)
# Verfiy valid ipv4 and invalid ipv6 address exist
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)
abdosi marked this conversation as resolved.
Show resolved Hide resolved
print(ret)
# Verfiy only valid ipv4 address exist
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, 9))
self.assertIsNotNone(ret)
print(ret)
# Verfiy only valid ipv6 address exist
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, 13))
self.assertIsNotNone(ret)
print(ret)
# Verfiy no mgmt address exist
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, 17))
self.assertIsNone(ret)
print(ret)

def test_local_port_identification(self):
mib_entry = self.lut[(1, 0, 8802, 1, 1, 2, 1, 3, 7, 1, 3)]
Expand Down