From c70df53bab243166d51331c5ceb6217d569d97c3 Mon Sep 17 00:00:00 2001 From: SuvarnaMeenakshi <50386592+SuvarnaMeenakshi@users.noreply.github.com> Date: Thu, 25 Jun 2020 10:19:23 -0700 Subject: [PATCH] Fix undefined variable and warning message (#134) * Fix errors introduced. Add namespace test case for fdb related MIB. * Make sure that db connections are done in MIB class init functions. Multiple connect is causing an increase in run time causing agentx connection to close. * Minor fix * Add mock asic_db. * Remove trailing whitespace. * Revert "Make sure that db connections are done in MIB class init" * Fix to make sure pubsub channel in LLDPRemManAddrUpdater is not created everytime in update_data but is created only if the channel was not created previously. * Fix as per review comments. --- src/sonic_ax_impl/mibs/__init__.py | 15 +- src/sonic_ax_impl/mibs/ieee802_1ab.py | 33 ++-- src/sonic_ax_impl/mibs/ietf/rfc2737.py | 15 +- src/sonic_ax_impl/mibs/ietf/rfc3433.py | 4 +- .../mibs/vendor/cisco/ciscoPfcExtMIB.py | 2 +- tests/mock_tables/asic0/asic_db.json | 25 +++ tests/mock_tables/asic1/asic_db.json | 9 + tests/mock_tables/global_db/asic_db.json | 2 + tests/namespace/test_fdb.py | 156 ++++++++++++++++++ 9 files changed, 226 insertions(+), 35 deletions(-) create mode 100644 tests/mock_tables/global_db/asic_db.json create mode 100644 tests/namespace/test_fdb.py diff --git a/src/sonic_ax_impl/mibs/__init__.py b/src/sonic_ax_impl/mibs/__init__.py index c9154c7f7..e9353861d 100644 --- a/src/sonic_ax_impl/mibs/__init__.py +++ b/src/sonic_ax_impl/mibs/__init__.py @@ -150,7 +150,7 @@ def init_db(): """ # SyncD database connector. THIS MUST BE INITIALIZED ON A PER-THREAD BASIS. # Redis PubSub objects (such as those within swsssdk) are NOT thread-safe. - db_conn = SonicV2Connector(**redis_kwargs) + db_conn = SonicV2Connector(**redis_kwargs) return db_conn @@ -389,6 +389,13 @@ def get_transceiver_sensor_sub_id(ifindex, sensor): transceiver_oid, = get_transceiver_sub_id(ifindex) return (transceiver_oid + SENSOR_PART_ID_MAP[sensor], ) +def get_redis_pubsub(db_conn, db_name, pattern): + redis_client = db_conn.get_redis_client(db_name) + db = db_conn.get_dbid(db_name) + pubsub = redis_client.pubsub() + pubsub.psubscribe("__keyspace@{}__:{}".format(db, pattern)) + return pubsub + class RedisOidTreeUpdater(MIBUpdater): def __init__(self, prefix_str): super().__init__() @@ -419,7 +426,7 @@ def update_data(self): self.oid_list = [] self.oid_map = {} - keys = Namespace.dbs_keys(self.db_conn, SNMP_OVERLAY_DB, self.prefix_str + '*') + keys = Namespace.dbs_keys(self.db_conn, SNMP_OVERLAY_DB, self.prefix_str + '*') # TODO: fix db_conn.keys to return empty list instead of None if there is no match if keys is None: keys = [] @@ -428,7 +435,7 @@ def update_data(self): key = key.decode() oid = oid2tuple(key, dot_prefix=False) self.oid_list.append(oid) - value = Namespace.dbs_get_all(self.db_conn, SNMP_OVERLAY_DB, key) + value = Namespace.dbs_get_all(self.db_conn, SNMP_OVERLAY_DB, key) if value[b'type'] in [b'COUNTER_32', b'COUNTER_64']: self.oid_map[oid] = int(value[b'data']) else: @@ -587,6 +594,6 @@ def dbs_get_bridge_port_map(dbs, db_name): def dbs_get_vlan_id_from_bvid(dbs, bvid): for db_conn in Namespace.get_non_host_dbs(dbs): db_conn.connect('ASIC_DB') - vlan_obj = db.keys('ASIC_DB', "ASIC_STATE:SAI_OBJECT_TYPE_VLAN:" + bvid) + vlan_obj = db_conn.keys('ASIC_DB', "ASIC_STATE:SAI_OBJECT_TYPE_VLAN:" + bvid) if vlan_obj is not None: return port_util.get_vlan_id_from_bvid(db_conn, bvid) diff --git a/src/sonic_ax_impl/mibs/ieee802_1ab.py b/src/sonic_ax_impl/mibs/ieee802_1ab.py index fa4d2b7e2..6b36ff866 100644 --- a/src/sonic_ax_impl/mibs/ieee802_1ab.py +++ b/src/sonic_ax_impl/mibs/ieee802_1ab.py @@ -71,7 +71,6 @@ class ManAddrConst: """ man_addr_oid = (1, 3, 6, 1, 2, 1, 2, 2, 1, 1) - def poll_lldp_entry_updates(pubsub): ret = None, None, None msg = pubsub.get_message() @@ -157,7 +156,7 @@ def __init__(self): # cache of port data # { if_name -> { 'key': 'value' } } self.loc_port_data = {} - self.pubsub = [None] * len(self.db_conn) + self.pubsub = [None] * len(self.db_conn) def reinit_data(self): """ @@ -222,16 +221,10 @@ def get_next(self, sub_id): return None return self.if_range[right] - def _update_per_namespace_data(self, db_conn, pubsub): + def _update_per_namespace_data(self, pubsub): """ Listen to updates in APP DB, update local cache """ - if not pubsub: - redis_client = db_conn.get_redis_client(db_conn.APPL_DB) - db = db_conn.get_dbid(db_conn.APPL_DB) - pubsub = redis_client.pubsub() - pubsub.psubscribe("__keyspace@{}__:{}".format(db, mibs.lldp_entry_table(b'*'))) - while True: data, interface, if_id = poll_lldp_entry_updates(pubsub) @@ -243,7 +236,10 @@ def _update_per_namespace_data(self, db_conn, pubsub): def update_data(self): for i in range(len(self.db_conn)): - self._update_per_namespace_data(self.db_conn[i], self.pubsub[i]) + if not self.pubsub[i]: + pattern = mibs.lldp_entry_table(b'*') + self.pubsub[i] = mibs.get_redis_pubsub(self.db_conn[i], self.db_conn[i].APPL_DB, pattern) + self._update_per_namespace_data(self.pubsub[i]) def local_port_num(self, sub_id): if len(sub_id) == 0: @@ -307,7 +303,7 @@ def reinit_data(self): self.mgmt_ip_str = None # establish connection to application database. - self.db_conn.connect(mibs.APPL_DB) + self.db_conn.connect(mibs.APPL_DB) mgmt_ip_bytes = self.db_conn.get(mibs.APPL_DB, mibs.LOC_CHASSIS_TABLE, b'lldp_loc_man_addr') if not mgmt_ip_bytes: @@ -488,7 +484,7 @@ class LLDPRemManAddrUpdater(MIBUpdater): def __init__(self): super().__init__() - self.db_conn = Namespace.init_namespace_dbs() + self.db_conn = Namespace.init_namespace_dbs() # establish connection to application database. Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB) self.if_range = [] @@ -536,16 +532,10 @@ def update_rem_if_mgmt(self, if_oid, if_name): return self.if_range.sort() - def _update_per_namespace_data(self, db_conn, pubsub): + def _update_per_namespace_data(self, pubsub): """ Listen to updates in APP DB, update local cache """ - if not pubsub: - redis_client = db_conn.get_redis_client(db_conn.APPL_DB) - db = db_conn.get_dbid(db_conn.APPL_DB) - pubsub = redis_client.pubsub() - pubsub.psubscribe("__keyspace@{}__:{}".format(db, mibs.lldp_entry_table(b'*'))) - while True: data, interface, if_index = poll_lldp_entry_updates(pubsub) @@ -561,7 +551,10 @@ def _update_per_namespace_data(self, db_conn, pubsub): def update_data(self): for i in range(len(self.db_conn)): - self._update_per_namespace_data(self.db_conn[i], self.pubsub[i]) + if not self.pubsub[i]: + pattern = mibs.lldp_entry_table(b'*') + self.pubsub[i] = mibs.get_redis_pubsub(self.db_conn[i], self.db_conn[i].APPL_DB, pattern) + self._update_per_namespace_data(self.pubsub[i]) def reinit_data(self): diff --git a/src/sonic_ax_impl/mibs/ietf/rfc2737.py b/src/sonic_ax_impl/mibs/ietf/rfc2737.py index a0134cf99..b503967d1 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc2737.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc2737.py @@ -181,7 +181,7 @@ def reinit_data(self): interface = transceiver_entry.split(mibs.TABLE_NAME_SEPARATOR_VBAR)[-1] self._update_transceiver_cache(interface) - def _update_per_namespace_data(self, statedb, pubsub): + def _update_per_namespace_data(self, pubsub): """ Update cache. Here we listen to changes in STATE_DB TRANSCEIVER_INFO table @@ -190,12 +190,6 @@ def _update_per_namespace_data(self, statedb, pubsub): # This code is not executed in unit test, since mockredis # does not support pubsub - if not pubsub: - redis_client = statedb.get_redis_client(statedb.STATE_DB) - db = statedb.get_dbid(statedb.STATE_DB) - pubsub = redis_client.pubsub() - pubsub.psubscribe("__keyspace@{}__:{}".format(db, self.TRANSCEIVER_KEY_PATTERN)) - while True: msg = pubsub.get_message() @@ -233,8 +227,13 @@ def _update_per_namespace_data(self, statedb, pubsub): self.physical_entities.remove(sub_id) def update_data(self): + # This code is not executed in unit test, since mockredis + # does not support pubsub for i in range(len(self.statedb)): - self._update_per_namespace_data(self.statedb[i], self.pubsub[i]) + if not self.pubsub[i]: + pattern = self.TRANSCEIVER_KEY_PATTERN + self.pubsub[i] = mibs.get_redis_pubsub(self.statedb[i], self.statedb[i].STATE_DB, pattern) + self._update_per_namespace_data(self.pubsub[i]) def _update_transceiver_cache(self, interface): """ diff --git a/src/sonic_ax_impl/mibs/ietf/rfc3433.py b/src/sonic_ax_impl/mibs/ietf/rfc3433.py index 4c0754f99..4e97b2bab 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc3433.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc3433.py @@ -250,7 +250,7 @@ def __init__(self): super().__init__() - self.statedb = Namespace.init_namespace_dbs() + self.statedb = Namespace.init_namespace_dbs() Namespace.connect_all_dbs(self.statedb, mibs.STATE_DB) # list of available sub OIDs @@ -378,7 +378,7 @@ def get_ent_physical_sensor_precision(self, sub_id): """ Get sensor precision value based on sub OID :param sub_id: sub ID of the sensor - :return: sensor precision in range (-8, 9) + :return: sensor precision in range (-8, 9) or None if sub_id not in the cache. """ diff --git a/src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py b/src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py index 7661016c4..77ee7f88e 100644 --- a/src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py +++ b/src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py @@ -191,7 +191,7 @@ def requests_per_priority(self, sub_id): """ :param sub_id: The 0-based sub-identifier query. :return: the counter for the respective sub_id/table. - """ + """ port_oid = '' queue_index = '' try: diff --git a/tests/mock_tables/asic0/asic_db.json b/tests/mock_tables/asic0/asic_db.json index 2c63c0851..590918051 100644 --- a/tests/mock_tables/asic0/asic_db.json +++ b/tests/mock_tables/asic0/asic_db.json @@ -1,2 +1,27 @@ { + "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bridge_id\":\"oid:0x0\",\"bridge_type\":\"SAI_FDB_ENTRY_BRIDGE_TYPE_1Q\",\"mac\":\"7C:FE:90:80:9F:04\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": { + "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000608", + "SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bridge_id\":\"oid:0x0\",\"bridge_type\":\"SAI_FDB_ENTRY_BRIDGE_TYPE_1Q\",\"mac\":\"7C:FE:90:80:9F:06\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": { + "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000616", + "SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bvid\":\"oid:0x26000000000016\",\"mac\":\"7C:FE:90:80:9F:08\",\"switch_id\":\"oid:0x21000000000000\"}": { + "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000620", + "SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_VLAN:oid:0x26000000000016": { + "SAI_VLAN_ATTR_VLAN_ID": "1000" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000608": { + "SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT", + "SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000003", + "SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000616": { + "SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT", + "SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000005", + "SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true" + } } diff --git a/tests/mock_tables/asic1/asic_db.json b/tests/mock_tables/asic1/asic_db.json index 2c63c0851..70dedc520 100644 --- a/tests/mock_tables/asic1/asic_db.json +++ b/tests/mock_tables/asic1/asic_db.json @@ -1,2 +1,11 @@ { + "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bridge_id\":\"oid:0x0\",\"bridge_type\":\"SAI_FDB_ENTRY_BRIDGE_TYPE_1Q\",\"mac\":\"7C:FE:90:80:9F:10\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": { + "SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a0000000006208", + "SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC" + }, + "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000620": { + "SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT", + "SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000010", + "SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true" + } } diff --git a/tests/mock_tables/global_db/asic_db.json b/tests/mock_tables/global_db/asic_db.json new file mode 100644 index 000000000..2c63c0851 --- /dev/null +++ b/tests/mock_tables/global_db/asic_db.json @@ -0,0 +1,2 @@ +{ +} diff --git a/tests/namespace/test_fdb.py b/tests/namespace/test_fdb.py new file mode 100644 index 000000000..ee95bd5c4 --- /dev/null +++ b/tests/namespace/test_fdb.py @@ -0,0 +1,156 @@ +import os +import sys +import importlib + +modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, os.path.join(modules_path, 'src')) + +from unittest import TestCase + +# noinspection PyUnresolvedReferences +import tests.mock_tables.dbconnector + +from ax_interface.mib import MIBTable +from ax_interface.pdu import PDUHeader +from ax_interface.pdu_implementations import GetPDU, GetNextPDU +from ax_interface import ValueType +from ax_interface.encodings import ObjectIdentifier +from ax_interface.constants import PduTypes +from sonic_ax_impl.mibs.ietf import rfc4363 +from sonic_ax_impl.main import SonicMIB +import sonic_ax_impl + +class TestSonicMIB(TestCase): + @classmethod + def setUpClass(cls): + tests.mock_tables.dbconnector.load_namespace_config() + importlib.reload(rfc4363) + + cls.lut = MIBTable(rfc4363.QBridgeMIBObjects) + for updater in cls.lut.updater_instances: + updater.update_data() + updater.reinit_data() + updater.update_data() + + def test_getpdu(self): + oid = ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2, 1000, 124, 254, 144, 128, 159, 4)) + get_pdu = GetPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=[oid] + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.INTEGER) + self.assertEqual(str(value0.name), str(oid)) + self.assertEqual(value0.data, 1) + + def test_getnextpdu(self): + get_pdu = GetNextPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=( + ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2)), + ) + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.INTEGER) + self.assertEqual(value0.data, 1) + + def test_getnextpdu_exactmatch(self): + # oid.include = 1 + oid = ObjectIdentifier(20, 0, 1, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2, 1000, 124, 254, 144, 128, 159, 4)) + get_pdu = GetNextPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=[oid] + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.INTEGER) + print("test_getnextpdu_exactmatch: ", str(oid)) + self.assertEqual(str(value0.name), str(oid)) + self.assertEqual(value0.data, 1) + + def test_getnextpdu_exactmatch_asic0(self): + # oid.include = 1 + oid = ObjectIdentifier(20, 0, 1, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2, 1000, 124, 254, 144, 128, 159, 6)) + get_pdu = GetNextPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=[oid] + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.INTEGER) + print("test_getnextpdu_exactmatch: ", str(oid)) + self.assertEqual(str(value0.name), str(oid)) + self.assertEqual(value0.data, 9000) + + def test_getnextpdu_exactmatch_asic1(self): + # oid.include = 1 + oid = ObjectIdentifier(20, 0, 1, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2, 1000, 124, 254, 144, 128, 159, 8)) + get_pdu = GetNextPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=[oid] + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.INTEGER) + print("test_getnextpdu_exactmatch: ", str(oid)) + self.assertEqual(str(value0.name), str(oid)) + self.assertEqual(value0.data, 9008) + + def test_getpdu_noinstance(self): + get_pdu = GetPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=( + ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 2, 1000, 100001)), + ) + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.NO_SUCH_INSTANCE) + + def test_getnextpdu_empty(self): + get_pdu = GetNextPDU( + header=PDUHeader(1, PduTypes.GET, 16, 0, 42, 0, 0, 0), + oids=( + ObjectIdentifier(20, 0, 0, 0, (1, 3, 6, 1, 2, 1, 17, 7, 1, 2, 2, 1, 3)), + ) + ) + + encoded = get_pdu.encode() + response = get_pdu.make_response(self.lut) + print(response) + + n = len(response.values) + value0 = response.values[0] + self.assertEqual(value0.type_, ValueType.END_OF_MIB_VIEW) +