From 8ed097813d9c2d2124c12b5122c0d8aeadcf38b0 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi Date: Mon, 13 Jul 2020 16:05:07 -0700 Subject: [PATCH 1/5] [Namespace]: Fix interface counters in RFC 1213 for multi-asic platforms. In multi-asic platform, SAI OID is not unique for the whole device. Fix implementation to make sure that interfaces counters is keyed based on interface index. Signed-off-by: SuvarnaMeenakshi --- src/sonic_ax_impl/mibs/__init__.py | 28 ++++++++++++------ src/sonic_ax_impl/mibs/ietf/rfc1213.py | 39 +++++++++++++------------- tests/namespace/test_interfaces.py | 2 -- 3 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index 5c4e8de6a..33d0b9db9 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -300,11 +300,17 @@ def init_sync_d_rif_tables(db_conn): if not rif_port_map: return {}, {} - port_rif_map = {port: rif for rif, port in rif_port_map.items()} - logger.debug("Rif port map:\n" + pprint.pformat(rif_port_map, indent=2)) + rif_port_map_updated = {} + port_rif_map = {} + for rif, port in rif_port_map.items(): + rif_key = get_sai_id_key(db_conn.namespace, rif) + port_key = get_sai_id_key(db_conn.namespace, port) + rif_port_map_updated[rif_key] = port_key + port_rif_map[port_key] = rif_key - return rif_port_map, port_rif_map + logger.debug("Rif port map:\n" + pprint.pformat(rif_port_map, indent=2)) + return rif_port_map_updated, port_rif_map def init_sync_d_vlan_tables(db_conn): """ @@ -314,22 +320,28 @@ def init_sync_d_vlan_tables(db_conn): vlan_name_map = port_util.get_vlan_interface_oid_map(db_conn) + vlan_name_map_updated = {} + + for sai_id in vlan_name_map: + sai_id_key = get_sai_id_key(db_conn.namespace, sai_id) + vlan_name_map_updated[sai_id_key] = vlan_name_map[sai_id] + logger.debug("Vlan oid map:\n" + pprint.pformat(vlan_name_map, indent=2)) # { OID -> sai_id } - oid_sai_map = {get_index(if_name): sai_id for sai_id, if_name in vlan_name_map.items() + oid_sai_map = {get_index(if_name): sai_id for sai_id, if_name in vlan_name_map_updated.items() # only map the interface if it's a style understood to be a SONiC interface. if get_index(if_name) is not None} logger.debug("OID sai map:\n" + pprint.pformat(oid_sai_map, indent=2)) # { OID -> if_name (SONiC) } - oid_name_map = {get_index(if_name): if_name for sai_id, if_name in vlan_name_map.items() + oid_name_map = {get_index(if_name): if_name for sai_id, if_name in vlan_name_map_updated.items() # only map the interface if it's a style understood to be a SONiC interface. if get_index(if_name) is not None} logger.debug("OID name map:\n" + pprint.pformat(oid_name_map, indent=2)) - return vlan_name_map, oid_sai_map, oid_name_map + return vlan_name_map_updated, oid_sai_map, oid_name_map def init_sync_d_lag_tables(db_conn): @@ -346,7 +358,7 @@ def init_sync_d_lag_tables(db_conn): if_name_lag_name_map = {} # { OID -> lag_name (SONiC) } oid_lag_name_map = {} - # { lag_name (SONiC) -> lag_oid (SAI) } + # { lag_name (SONiC) -> namespace: lag_oid (SAI) } lag_sai_map = {} lag_entries = db_conn.keys(APPL_DB, b"LAG_TABLE:*") @@ -355,7 +367,7 @@ def init_sync_d_lag_tables(db_conn): return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map lag_sai_map = db_conn.get_all(COUNTERS_DB, b"COUNTERS_LAG_NAME_MAP") - lag_sai_map = {name: sai_id.lstrip(b"oid:0x") for name, sai_id in lag_sai_map.items()} + lag_sai_map = {name: get_sai_id_key(db_conn.namespace, sai_id.lstrip(b"oid:0x")) for name, sai_id in lag_sai_map.items()} for lag_entry in lag_entries: lag_name = lag_entry[len(b"LAG_TABLE:"):] diff --git a/src/sonic_ax_impl/mibs/ietf/rfc1213.py b/src/sonic_ax_impl/mibs/ietf/rfc1213.py index 6333b966e..c3b199f6b 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc1213.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc1213.py @@ -9,6 +9,7 @@ from ax_interface.mib import MIBMeta, ValueType, MIBUpdater, MIBEntry, SubtreeMIBEntry, OverlayAdpaterMIBEntry, OidMIBEntry from ax_interface.encodings import ObjectIdentifier from ax_interface.util import mac_decimals, ip2tuple_v4 +from swsssdk.port_util import get_index_from_str @unique class DbTables(int, Enum): @@ -154,7 +155,7 @@ class InterfacesUpdater(MIBUpdater): def __init__(self): super().__init__() - self.db_conn = Namespace.init_namespace_dbs() + self.db_conn = Namespace.init_namespace_dbs() self.lag_name_if_name_map = {} self.if_name_lag_name_map = {} @@ -176,6 +177,7 @@ def __init__(self): self.oid_sai_map = {} self.oid_name_map = {} self.rif_counters = {} + self.namespace_db_map = Namespace.get_namespace_db_map(self.db_conn) def reinit_data(self): """ @@ -206,17 +208,19 @@ def update_data(self): Update redis (caches config) Pulls the table references for each interface. """ - self.if_counters = \ - {sai_id: Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True) - for sai_id in self.if_id_map} + for sai_id_key in self.if_id_map: + namespace, sai_id = mibs.split_sai_id_key(sai_id_key) + if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode()) + self.if_counters[if_idx] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, \ + mibs.counter_table(sai_id), blocking=True) rif_sai_ids = list(self.rif_port_map) + list(self.vlan_name_map) - self.rif_counters = \ - {sai_id: Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True) - for sai_id in rif_sai_ids} + for sai_id_key in rif_sai_ids: + namespace, sai_id = mibs.split_sai_id_key(sai_id_key) + self.rif_counters[sai_id_key] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True) - if self.rif_counters: + if self.rif_counters: self.aggregate_counters() self.lag_name_if_name_map, \ @@ -282,18 +286,11 @@ def _get_counter(self, oid, table_name): :param table_name: the redis table (either IntEnum or string literal) to query. :return: the counter for the respective sub_id/table. """ - sai_id = '' - if oid in self.oid_sai_map: - sai_id = self.oid_sai_map[oid] - elif oid in self.vlan_oid_sai_map: - sai_id = self.vlan_oid_sai_map[oid] - else: - logger.warning("Unexpected oid {}".format(oid)) # Enum.name or table_name = 'name_of_the_table' _table_name = bytes(getattr(table_name, 'name', table_name), 'utf-8') try: - counter_value = self.if_counters[sai_id][_table_name] + counter_value = self.if_counters[oid][_table_name] # truncate to 32-bit counter (database implements 64-bit counters) counter_value = int(counter_value) & 0x00000000ffffffff # done! @@ -311,16 +308,18 @@ def aggregate_counters(self): """ for rif_sai_id, port_sai_id in self.rif_port_map.items(): if port_sai_id in self.if_id_map: + if_idx = get_index_from_str(self.if_id_map[port_sai_id].decode()) for port_counter_name, rif_counter_name in mibs.RIF_DROPS_AGGR_MAP.items(): - self.if_counters[port_sai_id][port_counter_name] = \ - int(self.if_counters[port_sai_id][port_counter_name]) + \ + self.if_counters[if_idx][port_counter_name] = \ + int(self.if_counters[if_idx][port_counter_name]) + \ int(self.rif_counters[rif_sai_id][rif_counter_name]) for vlan_sai_id in self.vlan_name_map: for port_counter_name, rif_counter_name in mibs.RIF_COUNTERS_AGGR_MAP.items(): try: - self.if_counters.setdefault(vlan_sai_id, {}) - self.if_counters[vlan_sai_id][port_counter_name] = \ + if_idx = get_index_from_str(self.vlan_name_map[vlan_sai_id].decode()) + self.if_counters.setdefault(if_idx, {}) + self.if_counters[if_idx][port_counter_name] = \ int(self.rif_counters[vlan_sai_id][rif_counter_name]) except KeyError as e: logger.warning("Not able to aggregate counters for {}: {}\n {}".format(vlan_sai_id, rif_counter_name, e)) diff --git a/tests/namespace/test_interfaces.py b/tests/namespace/test_interfaces.py index 784a324ea..ea51339f8 100644 --- a/tests/namespace/test_interfaces.py +++ b/tests/namespace/test_interfaces.py @@ -22,8 +22,6 @@ class TestGetNextPDU(TestCase): @classmethod def setUpClass(cls): - cls.skipTest(cls, "TODO: Need to update corresponding MIB implementation \ - in the Snmp Agent for multiple namespaces/multi-asic") tests.mock_tables.dbconnector.load_namespace_config() importlib.reload(rfc1213) cls.lut = MIBTable(rfc1213.InterfacesMIB) From a7718df226477f45152469a14e1698b7c5097931 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi Date: Tue, 14 Jul 2020 13:18:56 -0700 Subject: [PATCH 2/5] Remove extra space as per review comment. Signed-off-by: SuvarnaMeenakshi --- src/sonic_ax_impl/mibs/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index 33d0b9db9..0de36e31b 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -170,7 +170,7 @@ def get_sai_id_key(namespace, sai_id): Return value: namespace:sai id or sai id """ if namespace != '': - return namespace.encode() + b':' + sai_id + return namespace.encode() + b':' + sai_id else: return sai_id From 978b39dd27eb75c09f383335a38132112d2128b7 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi Date: Tue, 14 Jul 2020 14:06:32 -0700 Subject: [PATCH 3/5] Fix review comment. Signed-off-by: SuvarnaMeenakshi --- src/sonic_ax_impl/mibs/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index 0de36e31b..6751f4989 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -367,7 +367,8 @@ def init_sync_d_lag_tables(db_conn): return lag_name_if_name_map, if_name_lag_name_map, oid_lag_name_map, lag_sai_map lag_sai_map = db_conn.get_all(COUNTERS_DB, b"COUNTERS_LAG_NAME_MAP") - lag_sai_map = {name: get_sai_id_key(db_conn.namespace, sai_id.lstrip(b"oid:0x")) for name, sai_id in lag_sai_map.items()} + for name, sai_id in lag_sai_map.items(): + lag_sai_map[name] = get_sai_id_key(db_conn.namespace, sai_id.lstrip(b"oid:0x")) for lag_entry in lag_entries: lag_name = lag_entry[len(b"LAG_TABLE:"):] From 7bb9c992e16e5f1b1039c2d26ce5fc34f183d4b1 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi Date: Tue, 14 Jul 2020 17:36:34 -0700 Subject: [PATCH 4/5] Decode the interface name in init function to avoid decoding in different functions. Signed-off-by: SuvarnaMeenakshi --- src/sonic_ax_impl/mibs/__init__.py | 2 +- src/sonic_ax_impl/mibs/ietf/rfc1213.py | 7 +++---- src/sonic_ax_impl/mibs/ietf/rfc2863.py | 3 +-- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index 6751f4989..b836cc8ea 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -246,7 +246,7 @@ def init_sync_d_interface_tables(db_conn): # namespace to get a unique key. Assuming that ':' is not present in namespace # string or in sai id. # sai_id_key = namespace : sai_id - if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name for sai_id, if_name in if_id_map.items() if \ + if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name.decode() for sai_id, if_name in if_id_map.items() if \ (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name.decode()) or \ re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name.decode()))} logger.debug("Port name map:\n" + pprint.pformat(if_name_map, indent=2)) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc1213.py b/src/sonic_ax_impl/mibs/ietf/rfc1213.py index c3b199f6b..689094965 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc1213.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc1213.py @@ -9,7 +9,6 @@ from ax_interface.mib import MIBMeta, ValueType, MIBUpdater, MIBEntry, SubtreeMIBEntry, OverlayAdpaterMIBEntry, OidMIBEntry from ax_interface.encodings import ObjectIdentifier from ax_interface.util import mac_decimals, ip2tuple_v4 -from swsssdk.port_util import get_index_from_str @unique class DbTables(int, Enum): @@ -210,7 +209,7 @@ def update_data(self): """ for sai_id_key in self.if_id_map: namespace, sai_id = mibs.split_sai_id_key(sai_id_key) - if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode()) + if_idx = mibs.get_index_from_str(self.if_id_map[sai_id_key]) self.if_counters[if_idx] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, \ mibs.counter_table(sai_id), blocking=True) @@ -308,7 +307,7 @@ def aggregate_counters(self): """ for rif_sai_id, port_sai_id in self.rif_port_map.items(): if port_sai_id in self.if_id_map: - if_idx = get_index_from_str(self.if_id_map[port_sai_id].decode()) + if_idx = mibs.get_index_from_str(self.if_id_map[port_sai_id]) for port_counter_name, rif_counter_name in mibs.RIF_DROPS_AGGR_MAP.items(): self.if_counters[if_idx][port_counter_name] = \ int(self.if_counters[if_idx][port_counter_name]) + \ @@ -317,7 +316,7 @@ def aggregate_counters(self): for vlan_sai_id in self.vlan_name_map: for port_counter_name, rif_counter_name in mibs.RIF_COUNTERS_AGGR_MAP.items(): try: - if_idx = get_index_from_str(self.vlan_name_map[vlan_sai_id].decode()) + if_idx = mibs.get_index_from_str(self.vlan_name_map[vlan_sai_id].decode()) self.if_counters.setdefault(if_idx, {}) self.if_counters[if_idx][port_counter_name] = \ int(self.rif_counters[vlan_sai_id][rif_counter_name]) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc2863.py b/src/sonic_ax_impl/mibs/ietf/rfc2863.py index 90d16867e..451675421 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc2863.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc2863.py @@ -4,7 +4,6 @@ from sonic_ax_impl import mibs from ax_interface.mib import MIBMeta, MIBUpdater, ValueType, SubtreeMIBEntry, OverlayAdpaterMIBEntry, OidMIBEntry from sonic_ax_impl.mibs import Namespace -from swsssdk.port_util import get_index_from_str @unique class DbTables32(int, Enum): @@ -103,7 +102,7 @@ def update_data(self): """ for sai_id_key in self.if_id_map: namespace, sai_id = mibs.split_sai_id_key(sai_id_key) - if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode()) + if_idx = mibs.get_index_from_str(self.if_id_map[sai_id_key]) self.if_counters[if_idx] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, \ mibs.counter_table(sai_id), blocking=True) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index e9aa46af6..3caa89547 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -74,7 +74,7 @@ def update_data(self): port_id = self.if_bpid_map[bridge_port_id] vlanmac = self.fdb_vlanmac(fdb) - self.vlanmac_ifindex_map[vlanmac] = mibs.get_index(self.if_id_map[port_id]) + self.vlanmac_ifindex_map[vlanmac] = mibs.get_index_from_str(self.if_id_map[port_id]) self.vlanmac_ifindex_list.append(vlanmac) self.vlanmac_ifindex_list.sort() From fdeab59f45888c1de204492caeeb6e6dc767ee76 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi Date: Tue, 14 Jul 2020 23:25:46 -0700 Subject: [PATCH 5/5] Fix one of the functions to avoid using multiple decode functions. Signed-off-by: SuvarnaMeenakshi --- src/sonic_ax_impl/mibs/__init__.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index b836cc8ea..9654ad025 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -235,20 +235,26 @@ def init_sync_d_interface_tables(db_conn): Initializes interface maps for SyncD-connected MIB(s). :return: tuple(if_name_map, if_id_map, oid_map, if_alias_map) """ + if_id_map = {} + if_name_map = {} # { if_name (SONiC) -> sai_id } # ex: { "Ethernet76" : "1000000000023" } - if_name_map, if_id_map = port_util.get_interface_oid_map(db_conn) - if_name_map = {if_name: sai_id for if_name, sai_id in if_name_map.items() if \ - (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name.decode()) or \ - re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name.decode()))} + if_name_map_util, if_id_map_util = port_util.get_interface_oid_map(db_conn) + for if_name, sai_id in if_name_map_util.items(): + if_name_str = if_name.decode() + if (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name_str) or \ + re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name_str)): + if_name_map[if_name] = sai_id # As sai_id is not unique in multi-asic platform, concatenate it with # namespace to get a unique key. Assuming that ':' is not present in namespace # string or in sai id. # sai_id_key = namespace : sai_id - if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name.decode() for sai_id, if_name in if_id_map.items() if \ - (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name.decode()) or \ - re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name.decode()))} + for sai_id, if_name in if_id_map_util.items(): + if_name = if_name.decode() + if (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name) or \ + re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name)): + if_id_map[get_sai_id_key(db_conn.namespace, sai_id)] = if_name logger.debug("Port name map:\n" + pprint.pformat(if_name_map, indent=2)) logger.debug("Interface name map:\n" + pprint.pformat(if_id_map, indent=2))