[db_migrator] Fix migration of Loopback data: handle all Loopback interfaces (sonic-net#2560)

vaibhavhd · isabelmsft · commit 44c5d1c23a16 · 2023-03-23T07:28:30.000Z
Fix the issue where cross branch upgrades (base DB version 1_0_1) lead to a OA crash due to a duplicate IP2ME route being added when there are more than one Loopback interfaces.

The issue happens as in current implementation lo is hardcoded to be replaced as Loopback0.
When the base image's APP DB has more than one IP assigned to lo interface, upon migration, all the IPs are assinged to same loopback Loopback0. This is incorrect, as in newer images different IPs are assinged to distinct Loopback interfaces.

How to verify it
Verified on a physical testbed that this change fixes the OA crash issue.
Also added a unit test to catch this issue in PR tests.
diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py
@@ -178,22 +178,39 @@ def migrate_intf_table(self):
         if self.appDB is None:
             return
 
-        data = self.appDB.keys(self.appDB.APPL_DB, "INTF_TABLE:*")
-
-        if data is None:
+        # Get Lo interface corresponding to IP(v4/v6) address from CONFIG_DB.
+        configdb_data = self.configDB.get_keys('LOOPBACK_INTERFACE')
+        lo_addr_to_int = dict()
+        for int_data in configdb_data:
+            if type(int_data) == tuple and len(int_data) > 1:
+                intf_name = int_data[0]
+                intf_addr = int_data[1]
+                lo_addr_to_int.update({intf_addr: intf_name})
+
+        lo_data = self.appDB.keys(self.appDB.APPL_DB, "INTF_TABLE:*")
+        if lo_data is None:
             return
 
         if_db = []
-        for key in data:
-            if_name = key.split(":")[1]
-            if if_name == "lo":
-                self.appDB.delete(self.appDB.APPL_DB, key)
-                key = key.replace(if_name, "Loopback0")
-                log.log_info('Migrating lo entry to ' + key)
-                self.appDB.set(self.appDB.APPL_DB, key, 'NULL', 'NULL')
-
-            if '/' not in key:
-                if_db.append(key.split(":")[1])
+        for lo_row in lo_data:
+            # Example of lo_row: 'INTF_TABLE:lo:10.1.0.32/32'
+            # Delete the old row with name as 'lo'. A new row with name as Loopback will be added
+            lo_name_appdb = lo_row.split(":")[1]
+            if lo_name_appdb == "lo":
+                self.appDB.delete(self.appDB.APPL_DB, lo_row)
+                lo_addr = lo_row.split('INTF_TABLE:lo:')[1]
+                lo_name_configdb = lo_addr_to_int.get(lo_addr)
+                if lo_name_configdb is None or lo_name_configdb == '':
+                    # an unlikely case where a Loopback address is present in APPLDB, but
+                    # there is no corresponding interface for this address in CONFIGDB:
+                    # Default to legacy implementation: hardcode interface name as Loopback0
+                    lo_new_row = lo_row.replace(lo_name_appdb, "Loopback0")
+                else:
+                    lo_new_row = lo_row.replace(lo_name_appdb, lo_name_configdb)
+                self.appDB.set(self.appDB.APPL_DB, lo_new_row, 'NULL', 'NULL')
+
+            if '/' not in lo_row:
+                if_db.append(lo_row.split(":")[1])
                 continue
 
         data = self.appDB.keys(self.appDB.APPL_DB, "INTF_TABLE:*")
diff --git a/tests/db_migrator_input/appl_db/loopback_interface_migrate_from_1_0_1_expected.json b/tests/db_migrator_input/appl_db/loopback_interface_migrate_from_1_0_1_expected.json
@@ -0,0 +1,8 @@
+{
+    "INTF_TABLE:Loopback0" : {"NULL": "NULL"},
+    "INTF_TABLE:Loopback0:10.1.0.32/32" :  {"NULL": "NULL"},
+    "INTF_TABLE:Loopback0:FC00:1::32/128" : {"NULL": "NULL"},
+    "INTF_TABLE:Loopback1" : {"NULL": "NULL"},
+    "INTF_TABLE:Loopback1:10.20.8.199/32" :  {"NULL": "NULL"},
+    "INTF_TABLE:Loopback1:2001:506:28:500::1/128" :  {"NULL": "NULL"}
+}
diff --git a/tests/db_migrator_input/appl_db/loopback_interface_migrate_from_1_0_1_input.json b/tests/db_migrator_input/appl_db/loopback_interface_migrate_from_1_0_1_input.json
@@ -0,0 +1,18 @@
+{
+    "INTF_TABLE:lo:10.1.0.32/32" : {
+        "scope": "global",
+        "family": "IPv4"
+    },
+    "INTF_TABLE:lo:FC00:1::32/128" : {
+        "scope": "global",
+        "family": "IPv6"
+    },
+    "INTF_TABLE:lo:10.20.8.199/32" : {
+        "scope": "global",
+        "family": "IPv4"
+    },
+    "INTF_TABLE:lo:2001:506:28:500::1/128" : {
+        "scope": "global",
+        "family": "IPv6"
+    }
+}
diff --git a/tests/db_migrator_input/config_db/loopback_interface_migrate_from_1_0_1_expected.json b/tests/db_migrator_input/config_db/loopback_interface_migrate_from_1_0_1_expected.json
@@ -0,0 +1,8 @@
+{
+    "LOOPBACK_INTERFACE|Loopback0" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback0|10.1.0.32/32" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback0|FC00:1::32/128" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback1" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback1|10.20.8.199/32" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback1|2001:506:28:500::1/128" : {"NULL": "NULL"}
+}
diff --git a/tests/db_migrator_input/config_db/loopback_interface_migrate_from_1_0_1_input.json b/tests/db_migrator_input/config_db/loopback_interface_migrate_from_1_0_1_input.json
@@ -0,0 +1,7 @@
+{
+    "LOOPBACK_INTERFACE|Loopback0|10.1.0.32/32" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback0|FC00:1::32/128" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback1|10.20.8.199/32" : {"NULL": "NULL"},
+    "LOOPBACK_INTERFACE|Loopback1|2001:506:28:500::1/128" : {"NULL": "NULL"},
+    "VERSIONS|DATABASE": {"VERSION": "version_1_0_1"}
+}
diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py
@@ -476,3 +476,45 @@ def test_warm_upgrade_to_2_0_2(self):
             expected_table = expected_db.cfgdb.get_table(table)
             diff = DeepDiff(resulting_table, expected_table, ignore_order=True)
             assert not diff
+
+class Test_Migrate_Loopback(object):
+    @classmethod
+    def setup_class(cls):
+        os.environ['UTILITIES_UNIT_TESTING'] = "2"
+
+    @classmethod
+    def teardown_class(cls):
+        os.environ['UTILITIES_UNIT_TESTING'] = "0"
+        dbconnector.dedicated_dbs['CONFIG_DB'] = None
+        dbconnector.dedicated_dbs['APPL_DB'] = None
+
+    def test_migrate_loopback_int(self):
+        dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'loopback_interface_migrate_from_1_0_1_input')
+        dbconnector.dedicated_dbs['APPL_DB'] = os.path.join(mock_db_path, 'appl_db', 'loopback_interface_migrate_from_1_0_1_input')
+
+        import db_migrator
+        dbmgtr = db_migrator.DBMigrator(None)
+        dbmgtr.migrate()
+        dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'loopback_interface_migrate_from_1_0_1_expected')
+        dbconnector.dedicated_dbs['APPL_DB'] = os.path.join(mock_db_path, 'appl_db', 'loopback_interface_migrate_from_1_0_1_expected')
+        expected_db = Db()
+
+        # verify migrated configDB
+        resulting_table = dbmgtr.configDB.get_table("LOOPBACK_INTERFACE")
+        expected_table = expected_db.cfgdb.get_table("LOOPBACK_INTERFACE")
+        diff = DeepDiff(resulting_table, expected_table, ignore_order=True)
+        assert not diff
+
+        # verify migrated appDB
+        expected_appl_db = SonicV2Connector(host='127.0.0.1')
+        expected_appl_db.connect(expected_appl_db.APPL_DB)
+        expected_keys = expected_appl_db.keys(expected_appl_db.APPL_DB, "INTF_TABLE:*")
+        expected_keys.sort()
+        resulting_keys = dbmgtr.appDB.keys(dbmgtr.appDB.APPL_DB, "INTF_TABLE:*")
+        resulting_keys.sort()
+        assert expected_keys == resulting_keys
+        for key in expected_keys:
+            resulting_keys = dbmgtr.appDB.get_all(dbmgtr.appDB.APPL_DB, key)
+            expected_keys = expected_appl_db.get_all(expected_appl_db.APPL_DB, key)
+            diff = DeepDiff(resulting_keys, expected_keys, ignore_order=True)
+            assert not diff
