From ed53a5ced371aefd64636c066877a8c4659a9919 Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Fri, 21 Jul 2023 06:23:27 +0000 Subject: [PATCH 1/6] Fix MIBUpdater crash when SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID attribute missing --- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index f48863082..d4134ec5d 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -85,8 +85,13 @@ def update_data(self): if not ent: continue - # Example output: oid:0x3a000000000608 - bridge_port_id = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:] + try: + # Example output: oid:0x3a000000000608 + bridge_port_id = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:] + except: + mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id.".format(fdb_str)) + continue + if bridge_port_id not in self.if_bpid_map: continue port_id = self.if_bpid_map[bridge_port_id] From 35b86ed2d67baa1f3be84745ddeeaa5e77c697e1 Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Fri, 21 Jul 2023 06:28:32 +0000 Subject: [PATCH 2/6] Fix code --- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index d4134ec5d..adaa74ae6 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -85,6 +85,7 @@ def update_data(self): if not ent: continue + bridge_port_id = "" try: # Example output: oid:0x3a000000000608 bridge_port_id = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:] From 45aabe0fe0c3bc9e627c3c70f427ebe324cd6624 Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Thu, 27 Jul 2023 02:59:43 +0000 Subject: [PATCH 3/6] Improve code --- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index adaa74ae6..5fc5ff6cb 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -85,14 +85,15 @@ def update_data(self): if not ent: continue - bridge_port_id = "" + bridge_port_id_attr = "" try: - # Example output: oid:0x3a000000000608 - bridge_port_id = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:] - except: - mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id.".format(fdb_str)) + bridge_port_id_attr = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"] + except KeyError as e: + mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id, exception: {}".format(fdb_str, e)) continue + # Example output: oid:0x3a000000000608 + bridge_port_id = bridge_port_id_attr[6:] if bridge_port_id not in self.if_bpid_map: continue port_id = self.if_bpid_map[bridge_port_id] From 0fb0f1d33d225c0e133d88dac699e27bbcff941a Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Thu, 27 Jul 2023 03:33:27 +0000 Subject: [PATCH 4/6] Improve UT coverage --- tests/test_rfc4363.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 tests/test_rfc4363.py diff --git a/tests/test_rfc4363.py b/tests/test_rfc4363.py new file mode 100644 index 000000000..9349ab0ca --- /dev/null +++ b/tests/test_rfc4363.py @@ -0,0 +1,28 @@ +import os +import sys +from unittest import TestCase + +if sys.version_info.major == 3: + from unittest import mock +else: + import mock + +modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, os.path.join(modules_path, 'src')) + +from sonic_ax_impl.mibs.ietf.rfc4363 import FdbUpdater + +class TestFdbUpdater(TestCase): + + @mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(['ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{"bvid":"oid:0x26000000000b6c","mac":"60:45:BD:98:6F:48","switch_id":"oid:0x21000000000000"}']))) + @mock.patch('sonic_ax_impl.mibs.Namespace.dbs_get_all', mock.MagicMock(return_value=({"nexthop": "10.0.0.1,10.0.0.3", "ifname": "Ethernet0,Ethernet4"}))) + def test_FdbUpdater_ent_bridge_port_id_attr_missing(self): + updater = FdbUpdater() + + with mock.patch('sonic_ax_impl.mibs.logger.error') as mocked_error: + updater.update_data() + + # check warning + mocked_error.assert_called() + + self.assertTrue(len(updater.vlanmac_ifindex_list) == 0) From 793483236b950a3cb615b40a0d955f9b49a0f5d7 Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 22 Aug 2023 02:17:30 +0000 Subject: [PATCH 5/6] Improve code by PR comments --- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index 5fc5ff6cb..d37a06b2d 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -20,6 +20,7 @@ def __init__(self): self.vlanmac_ifindex_list = [] self.if_bpid_map = {} self.bvid_vlan_map = {} + self.broken_fdbs = [] def fdb_vlanmac(self, fdb): if 'vlan' in fdb: @@ -89,7 +90,10 @@ def update_data(self): try: bridge_port_id_attr = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"] except KeyError as e: - mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id, exception: {}".format(fdb_str, e)) + # Only write error log once + if fdb_str not in self.broken_fdbs: + mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id, exception: {}".format(fdb_str, e)) + self.broken_fdbs.append(fdb_str) continue # Example output: oid:0x3a000000000608 From 7b1aad1cb1e4432750a1ec65f19201754ac5cf0c Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Wed, 30 Aug 2023 06:46:41 +0000 Subject: [PATCH 6/6] Update PR comments --- src/sonic_ax_impl/mibs/ietf/rfc4363.py | 5 +++-- tests/test_rfc4363.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/sonic_ax_impl/mibs/ietf/rfc4363.py b/src/sonic_ax_impl/mibs/ietf/rfc4363.py index d37a06b2d..c37e01ca1 100644 --- a/src/sonic_ax_impl/mibs/ietf/rfc4363.py +++ b/src/sonic_ax_impl/mibs/ietf/rfc4363.py @@ -61,6 +61,7 @@ def reinit_data(self): self.if_bpid_map = Namespace.dbs_get_bridge_port_map(self.db_conn, mibs.ASIC_DB) self.bvid_vlan_map.clear() + self.broken_fdbs.clear() def update_data(self): """ @@ -90,9 +91,9 @@ def update_data(self): try: bridge_port_id_attr = ent["SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"] except KeyError as e: - # Only write error log once + # Only write warning log once if fdb_str not in self.broken_fdbs: - mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id, exception: {}".format(fdb_str, e)) + mibs.logger.warn("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': failed to get bridge_port_id, exception: {}".format(fdb_str, e)) self.broken_fdbs.append(fdb_str) continue diff --git a/tests/test_rfc4363.py b/tests/test_rfc4363.py index 9349ab0ca..d31e65abe 100644 --- a/tests/test_rfc4363.py +++ b/tests/test_rfc4363.py @@ -19,10 +19,10 @@ class TestFdbUpdater(TestCase): def test_FdbUpdater_ent_bridge_port_id_attr_missing(self): updater = FdbUpdater() - with mock.patch('sonic_ax_impl.mibs.logger.error') as mocked_error: + with mock.patch('sonic_ax_impl.mibs.logger.warn') as mocked_warn: updater.update_data() # check warning - mocked_error.assert_called() + mocked_warn.assert_called() self.assertTrue(len(updater.vlanmac_ifindex_list) == 0)