From 3a85220f470a5360f5f68c7d2db679a11f04aae3 Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Mon, 26 Jun 2023 21:58:23 +0000 Subject: [PATCH 1/8] [db migrator] Remove hardcoded migration and replace w/ config from minigraph.py --- scripts/db_migrator.py | 25 +++++++++++++++++++++++-- scripts/db_migrator_constants.py | 32 -------------------------------- setup.py | 1 - 3 files changed, 23 insertions(+), 35 deletions(-) delete mode 100644 scripts/db_migrator_constants.py diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 761f858fe8..62f13e7656 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -9,9 +9,10 @@ from sonic_py_common import device_info, logger from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector, SonicDBConfig -from db_migrator_constants import RESTAPI, TELEMETRY, CONSOLE_SWITCH +from minigraph import parse_xml INIT_CFG_FILE = '/etc/sonic/init_cfg.json' +MINIGRAPH_FILE = '/etc/sonic/minigraph.xml' # mock the redis for unit test purposes # try: @@ -51,6 +52,12 @@ def __init__(self, namespace, socket=None): self.TABLE_KEY = 'DATABASE' self.TABLE_FIELD = 'VERSION' + # load config data from minigraph to get the default/hardcoded values from minigraph.py + # this is to avoid duplicating the hardcoded these values in db_migrator + self.minigraph_data = None + if os.path.isfile(MINIGRAPH_FILE): + self.minigraph_data = parse_xml("/etc/sonic/minigraph.xml") + db_kwargs = {} if socket: db_kwargs['unix_socket_path'] = socket @@ -527,6 +534,9 @@ def migrate_vxlan_config(self): def migrate_restapi(self): # RESTAPI - add missing key + if not self.minigraph_data: + return + RESTAPI = self.minigraph_data['RESTAPI'] log.log_notice('Migrate RESTAPI configuration') config = self.configDB.get_entry('RESTAPI', 'config') if not config: @@ -537,6 +547,9 @@ def migrate_restapi(self): def migrate_telemetry(self): # TELEMETRY - add missing key + if not self.minigraph_data: + return + TELEMETRY = self.minigraph_data['TELEMETRY'] log.log_notice('Migrate TELEMETRY configuration') gnmi = self.configDB.get_entry('TELEMETRY', 'gnmi') if not gnmi: @@ -547,6 +560,9 @@ def migrate_telemetry(self): def migrate_console_switch(self): # CONSOLE_SWITCH - add missing key + if not self.minigraph_data: + return + CONSOLE_SWITCH = self.minigraph_data['CONSOLE_SWITCH'] log.log_notice('Migrate CONSOLE_SWITCH configuration') console_mgmt = self.configDB.get_entry('CONSOLE_SWITCH', 'console_mgmt') if not console_mgmt: @@ -555,10 +571,15 @@ def migrate_console_switch(self): def migrate_device_metadata(self): # DEVICE_METADATA - synchronous_mode entry + if not self.minigraph_data: + return log.log_notice('Migrate DEVICE_METADATA missing configuration (synchronous_mode=enable)') metadata = self.configDB.get_entry('DEVICE_METADATA', 'localhost') + synchronous_mode = self.minigraph_data["DEVICE_METADATA"]["localhost"]["synchronous_mode"] + docker_routing_config_mode = self.minigraph_data["DEVICE_METADATA"]["localhost"]["docker_routing_config_mode"] if 'synchronous_mode' not in metadata: - metadata['synchronous_mode'] = 'enable' + metadata['synchronous_mode'] = synchronous_mode + metadata['docker_routing_config_mode'] = docker_routing_config_mode self.configDB.set_entry('DEVICE_METADATA', 'localhost', metadata) def migrate_port_qos_map_global(self): diff --git a/scripts/db_migrator_constants.py b/scripts/db_migrator_constants.py deleted file mode 100644 index 71237a5655..0000000000 --- a/scripts/db_migrator_constants.py +++ /dev/null @@ -1,32 +0,0 @@ -RESTAPI = { - "config": { - "client_auth": "true", - "log_level": "info", - "allow_insecure": "false" - }, - "certs": { - "server_key": "/etc/sonic/credentials/restapiserver.key", - "ca_crt": "/etc/sonic/credentials/AME_ROOT_CERTIFICATE.pem", - "server_crt": "/etc/sonic/credentials/restapiserver.crt", - "client_crt_cname": "client.restapi.sonic.gbl" - } - } - -TELEMETRY = { - "gnmi": { - "client_auth": "true", - "log_level": "2", - "port": "50051" - }, - "certs": { - "server_key": "/etc/sonic/telemetry/streamingtelemetryserver.key", - "ca_crt": "/etc/sonic/telemetry/dsmsroot.cer", - "server_crt": "/etc/sonic/telemetry/streamingtelemetryserver.cer" - } -} - -CONSOLE_SWITCH = { - "console_mgmt": { - "enabled": "no" - } -} diff --git a/setup.py b/setup.py index 547b0fac7a..837e356c4a 100644 --- a/setup.py +++ b/setup.py @@ -117,7 +117,6 @@ 'scripts/coredump-compress', 'scripts/configlet', 'scripts/db_migrator.py', - 'scripts/db_migrator_constants.py', 'scripts/decode-syseeprom', 'scripts/dropcheck', 'scripts/disk_check.py', From 7ae4c945ab0392beb579d820cba76f1c6339641e Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Tue, 27 Jun 2023 21:20:03 +0000 Subject: [PATCH 2/8] Remove routing config mode from this PR --- scripts/db_migrator.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 62f13e7656..660ff14514 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -534,7 +534,7 @@ def migrate_vxlan_config(self): def migrate_restapi(self): # RESTAPI - add missing key - if not self.minigraph_data: + if not self.minigraph_data or 'RESTAPI' not in self.minigraph_data: return RESTAPI = self.minigraph_data['RESTAPI'] log.log_notice('Migrate RESTAPI configuration') @@ -547,7 +547,7 @@ def migrate_restapi(self): def migrate_telemetry(self): # TELEMETRY - add missing key - if not self.minigraph_data: + if not self.minigraph_data or 'TELEMETRY' not in self.minigraph_data: return TELEMETRY = self.minigraph_data['TELEMETRY'] log.log_notice('Migrate TELEMETRY configuration') @@ -560,7 +560,7 @@ def migrate_telemetry(self): def migrate_console_switch(self): # CONSOLE_SWITCH - add missing key - if not self.minigraph_data: + if not self.minigraph_data or 'CONSOLE_SWITCH' not in self.minigraph_data: return CONSOLE_SWITCH = self.minigraph_data['CONSOLE_SWITCH'] log.log_notice('Migrate CONSOLE_SWITCH configuration') @@ -571,7 +571,7 @@ def migrate_console_switch(self): def migrate_device_metadata(self): # DEVICE_METADATA - synchronous_mode entry - if not self.minigraph_data: + if not self.minigraph_data or 'DEVICE_METADATA' not in self.minigraph_data: return log.log_notice('Migrate DEVICE_METADATA missing configuration (synchronous_mode=enable)') metadata = self.configDB.get_entry('DEVICE_METADATA', 'localhost') @@ -579,7 +579,6 @@ def migrate_device_metadata(self): docker_routing_config_mode = self.minigraph_data["DEVICE_METADATA"]["localhost"]["docker_routing_config_mode"] if 'synchronous_mode' not in metadata: metadata['synchronous_mode'] = synchronous_mode - metadata['docker_routing_config_mode'] = docker_routing_config_mode self.configDB.set_entry('DEVICE_METADATA', 'localhost', metadata) def migrate_port_qos_map_global(self): From 6e28286ee590f0237e07599b79fa1b21c933ed40 Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Wed, 28 Jun 2023 19:13:02 +0000 Subject: [PATCH 3/8] Add sample minigraph.xml for UT --- scripts/db_migrator.py | 1 + tests/db_migrator_input/minigraph.xml | 29 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/db_migrator_input/minigraph.xml diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 660ff14514..a4a370637a 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -23,6 +23,7 @@ sys.path.insert(0, modules_path) sys.path.insert(0, tests_path) INIT_CFG_FILE = os.path.join(mocked_db_path, "init_cfg.json") + MINIGRAPH_FILE = os.path.join(mocked_db_path, "minigraph.xml") except KeyError: pass diff --git a/tests/db_migrator_input/minigraph.xml b/tests/db_migrator_input/minigraph.xml new file mode 100644 index 0000000000..dcadac306e --- /dev/null +++ b/tests/db_migrator_input/minigraph.xml @@ -0,0 +1,29 @@ + + + + + + + SONiC-Dummy + + + + + + + + + + SONiC-Dummy + + + + + + + + + + + SONiC-Dummy + \ No newline at end of file From 2b66fa3a8b9d1bc964ee097415fe0b57aa446a2a Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Wed, 28 Jun 2023 20:00:24 +0000 Subject: [PATCH 4/8] Fix test failures --- scripts/db_migrator.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index a4a370637a..ae27457bb6 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -57,7 +57,7 @@ def __init__(self, namespace, socket=None): # this is to avoid duplicating the hardcoded these values in db_migrator self.minigraph_data = None if os.path.isfile(MINIGRAPH_FILE): - self.minigraph_data = parse_xml("/etc/sonic/minigraph.xml") + self.minigraph_data = parse_xml(MINIGRAPH_FILE) db_kwargs = {} if socket: @@ -574,12 +574,11 @@ def migrate_device_metadata(self): # DEVICE_METADATA - synchronous_mode entry if not self.minigraph_data or 'DEVICE_METADATA' not in self.minigraph_data: return - log.log_notice('Migrate DEVICE_METADATA missing configuration (synchronous_mode=enable)') + log.log_notice('Migrate DEVICE_METADATA missing configuration') metadata = self.configDB.get_entry('DEVICE_METADATA', 'localhost') - synchronous_mode = self.minigraph_data["DEVICE_METADATA"]["localhost"]["synchronous_mode"] - docker_routing_config_mode = self.minigraph_data["DEVICE_METADATA"]["localhost"]["docker_routing_config_mode"] + DEVICE_METADATA = self.minigraph_data["DEVICE_METADATA"]["localhost"] if 'synchronous_mode' not in metadata: - metadata['synchronous_mode'] = synchronous_mode + metadata['synchronous_mode'] = DEVICE_METADATA.get("synchronous_mode") self.configDB.set_entry('DEVICE_METADATA', 'localhost', metadata) def migrate_port_qos_map_global(self): From fcfd1fb93b629f20d56edd579bc0d867570368df Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Wed, 28 Jun 2023 21:50:57 +0000 Subject: [PATCH 5/8] Fix test input --- .../cross_branch_upgrade_to_version_2_0_2_expected.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/db_migrator_input/config_db/cross_branch_upgrade_to_version_2_0_2_expected.json b/tests/db_migrator_input/config_db/cross_branch_upgrade_to_version_2_0_2_expected.json index c975229287..946a9812b7 100644 --- a/tests/db_migrator_input/config_db/cross_branch_upgrade_to_version_2_0_2_expected.json +++ b/tests/db_migrator_input/config_db/cross_branch_upgrade_to_version_2_0_2_expected.json @@ -6,9 +6,9 @@ }, "RESTAPI|certs": { "server_key": "/etc/sonic/credentials/restapiserver.key", - "ca_crt": "/etc/sonic/credentials/AME_ROOT_CERTIFICATE.pem", - "server_crt": "/etc/sonic/credentials/restapiserver.crt", - "client_crt_cname": "client.restapi.sonic.gbl" + "ca_crt": "/etc/sonic/credentials/restapica.crt", + "server_crt": "/etc/sonic/credentials/restapiserver.crt", + "client_crt_cname": "client.restapi.sonic" }, "TELEMETRY|gnmi": { "client_auth": "true", From 94bc8bda230ebdbfcb0a3aadb5ae140a970d66d1 Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Thu, 29 Jun 2023 07:30:11 +0000 Subject: [PATCH 6/8] Review addressed: added a UT for missing minigraph --- scripts/db_migrator.py | 28 ++++++++++++++++------------ tests/db_migrator_test.py | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index ae27457bb6..244eb12495 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -56,8 +56,12 @@ def __init__(self, namespace, socket=None): # load config data from minigraph to get the default/hardcoded values from minigraph.py # this is to avoid duplicating the hardcoded these values in db_migrator self.minigraph_data = None - if os.path.isfile(MINIGRAPH_FILE): - self.minigraph_data = parse_xml(MINIGRAPH_FILE) + try: + if os.path.isfile(MINIGRAPH_FILE): + self.minigraph_data = parse_xml(MINIGRAPH_FILE) + except Exception: + log.log_error('Caught exception while trying to parse minigraph: ' + str(e)) + pass db_kwargs = {} if socket: @@ -537,38 +541,38 @@ def migrate_restapi(self): # RESTAPI - add missing key if not self.minigraph_data or 'RESTAPI' not in self.minigraph_data: return - RESTAPI = self.minigraph_data['RESTAPI'] + restapi_data = self.minigraph_data['RESTAPI'] log.log_notice('Migrate RESTAPI configuration') config = self.configDB.get_entry('RESTAPI', 'config') if not config: - self.configDB.set_entry("RESTAPI", "config", RESTAPI.get("config")) + self.configDB.set_entry("RESTAPI", "config", restapi_data.get("config")) certs = self.configDB.get_entry('RESTAPI', 'certs') if not certs: - self.configDB.set_entry("RESTAPI", "certs", RESTAPI.get("certs")) + self.configDB.set_entry("RESTAPI", "certs", restapi_data.get("certs")) def migrate_telemetry(self): # TELEMETRY - add missing key if not self.minigraph_data or 'TELEMETRY' not in self.minigraph_data: return - TELEMETRY = self.minigraph_data['TELEMETRY'] + telemetry_data = self.minigraph_data['TELEMETRY'] log.log_notice('Migrate TELEMETRY configuration') gnmi = self.configDB.get_entry('TELEMETRY', 'gnmi') if not gnmi: - self.configDB.set_entry("TELEMETRY", "gnmi", TELEMETRY.get("gnmi")) + self.configDB.set_entry("TELEMETRY", "gnmi", telemetry_data.get("gnmi")) certs = self.configDB.get_entry('TELEMETRY', 'certs') if not certs: - self.configDB.set_entry("TELEMETRY", "certs", TELEMETRY.get("certs")) + self.configDB.set_entry("TELEMETRY", "certs", telemetry_data.get("certs")) def migrate_console_switch(self): # CONSOLE_SWITCH - add missing key if not self.minigraph_data or 'CONSOLE_SWITCH' not in self.minigraph_data: return - CONSOLE_SWITCH = self.minigraph_data['CONSOLE_SWITCH'] + console_switch_data = self.minigraph_data['CONSOLE_SWITCH'] log.log_notice('Migrate CONSOLE_SWITCH configuration') console_mgmt = self.configDB.get_entry('CONSOLE_SWITCH', 'console_mgmt') if not console_mgmt: self.configDB.set_entry("CONSOLE_SWITCH", "console_mgmt", - CONSOLE_SWITCH.get("console_mgmt")) + console_switch_data.get("console_mgmt")) def migrate_device_metadata(self): # DEVICE_METADATA - synchronous_mode entry @@ -576,9 +580,9 @@ def migrate_device_metadata(self): return log.log_notice('Migrate DEVICE_METADATA missing configuration') metadata = self.configDB.get_entry('DEVICE_METADATA', 'localhost') - DEVICE_METADATA = self.minigraph_data["DEVICE_METADATA"]["localhost"] + device_metadata_data = self.minigraph_data["DEVICE_METADATA"]["localhost"] if 'synchronous_mode' not in metadata: - metadata['synchronous_mode'] = DEVICE_METADATA.get("synchronous_mode") + metadata['synchronous_mode'] = device_metadata_data.get("synchronous_mode") self.configDB.set_entry('DEVICE_METADATA', 'localhost', metadata) def migrate_port_qos_map_global(self): diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index f063e43e4a..e3ef451dab 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -509,6 +509,25 @@ def test_warm_upgrade_to_2_0_2(self): diff = DeepDiff(resulting_table, expected_table, ignore_order=True) assert not diff + def test_warm_upgrade__without_mg_to_2_0_2(self): + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'cross_branch_upgrade_to_version_2_0_2_input') + import db_migrator + dbmgtr = db_migrator.DBMigrator(None) + # set minigraph_data to None to mimic the missing minigraph.xml scenario + dbmgtr.minigraph_data = None + dbmgtr.migrate() + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'cross_branch_upgrade_without_mg_2_0_2_expected.json') + expected_db = Db() + + expected_db + new_tables = ["RESTAPI", "TELEMETRY", "CONSOLE_SWITCH"] + for table in new_tables: + resulting_table = dbmgtr.configDB.get_table(table) + expected_table = expected_db.cfgdb.get_table(table) + print(resulting_table) + diff = DeepDiff(resulting_table, expected_table, ignore_order=True) + assert not diff + class Test_Migrate_Loopback(object): @classmethod def setup_class(cls): From 126c1872964f7cac597767147eb6b73ea027d3c6 Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Thu, 29 Jun 2023 16:15:15 +0000 Subject: [PATCH 7/8] Fix exception handling --- scripts/db_migrator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 244eb12495..0f009985cd 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -59,7 +59,7 @@ def __init__(self, namespace, socket=None): try: if os.path.isfile(MINIGRAPH_FILE): self.minigraph_data = parse_xml(MINIGRAPH_FILE) - except Exception: + except Exception as e: log.log_error('Caught exception while trying to parse minigraph: ' + str(e)) pass From 2240d104d458876c222e3722a5fb0afbf2492085 Mon Sep 17 00:00:00 2001 From: Vaibhav Hemant Dixit Date: Thu, 29 Jun 2023 19:04:17 +0000 Subject: [PATCH 8/8] Remove unwanted line from unit test --- tests/db_migrator_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index e3ef451dab..e038db93bb 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -501,7 +501,6 @@ def test_warm_upgrade_to_2_0_2(self): dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'cross_branch_upgrade_to_version_2_0_2_expected') expected_db = Db() - expected_db new_tables = ["RESTAPI", "TELEMETRY", "CONSOLE_SWITCH"] for table in new_tables: resulting_table = dbmgtr.configDB.get_table(table) @@ -519,7 +518,6 @@ def test_warm_upgrade__without_mg_to_2_0_2(self): dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', 'cross_branch_upgrade_without_mg_2_0_2_expected.json') expected_db = Db() - expected_db new_tables = ["RESTAPI", "TELEMETRY", "CONSOLE_SWITCH"] for table in new_tables: resulting_table = dbmgtr.configDB.get_table(table)