From 7d02fc4fbc698e55b11ac9bc63c748c461067f7b Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Mon, 30 Oct 2023 08:07:29 +0000 Subject: [PATCH 1/6] Revert "Revert "[config]config reload should generate sysinfo if missing (#2778)" (#2865)" This reverts commit 7d803aedf68f8deb7dc989c90cbf3bae235910b2. --- config/main.py | 13 ++++++++++ tests/config_test.py | 61 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/config/main.py b/config/main.py index f336fb5818..9bf3f8ddc6 100644 --- a/config/main.py +++ b/config/main.py @@ -1540,6 +1540,19 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form click.echo("The config file {} doesn't exist".format(file)) continue + if file_format == 'config_db': + file_input = read_json_file(file) + + platform = file_input.get("DEVICE_METADATA", {}).\ + get("localhost", {}).get("platform") + mac = file_input.get("DEVICE_METADATA", {}).\ + get("localhost", {}).get("mac") + + if not platform or not mac: + log.log_warning("Input file does't have platform or mac. platform: {}, mac: {}" + .format(None if platform is None else platform, None if mac is None else mac)) + load_sysinfo = True + if load_sysinfo: try: command = [SONIC_CFGGEN_PATH, "-j", file, '-v', "DEVICE_METADATA.localhost.hwsku"] diff --git a/tests/config_test.py b/tests/config_test.py index 81c4e404d4..660394ab8a 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -464,9 +464,66 @@ def setup_class(cls): print("SETUP") import config.main importlib.reload(config.main) - open(cls.dummy_cfg_file, 'w').close() + + def add_sysinfo_to_cfg_file(self): + with open(self.dummy_cfg_file, 'w') as f: + device_metadata = { + "DEVICE_METADATA": { + "localhost": { + "platform": "some_platform", + "mac": "02:42:f0:7f:01:05" + } + } + } + f.write(json.dumps(device_metadata)) + + def test_reload_config_invalid_input(self, get_cmd_module, setup_single_broadcom_asic): + open(self.dummy_cfg_file, 'w').close() + with mock.patch( + "utilities_common.cli.run_command", + mock.MagicMock(side_effect=mock_run_command_side_effect) + ) as mock_run_command: + (config, show) = get_cmd_module + runner = CliRunner() + + result = runner.invoke( + config.config.commands["reload"], + [self.dummy_cfg_file, '-y', '-f']) + + print(result.exit_code) + print(result.output) + traceback.print_tb(result.exc_info[2]) + assert result.exit_code != 0 + + def test_reload_config_no_sysinfo(self, get_cmd_module, setup_single_broadcom_asic): + with open(self.dummy_cfg_file, 'w') as f: + device_metadata = { + "DEVICE_METADATA": { + "localhost": { + "hwsku": "some_hwsku" + } + } + } + f.write(json.dumps(device_metadata)) + + with mock.patch( + "utilities_common.cli.run_command", + mock.MagicMock(side_effect=mock_run_command_side_effect) + ) as mock_run_command: + (config, show) = get_cmd_module + runner = CliRunner() + + result = runner.invoke( + config.config.commands["reload"], + [self.dummy_cfg_file, '-y', '-f']) + + print(result.exit_code) + print(result.output) + traceback.print_tb(result.exc_info[2]) + assert result.exit_code == 0 def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic): + self.add_sysinfo_to_cfg_file() with mock.patch( "utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect) @@ -486,6 +543,7 @@ def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic): == RELOAD_CONFIG_DB_OUTPUT def test_config_reload_disabled_service(self, get_cmd_module, setup_single_broadcom_asic): + self.add_sysinfo_to_cfg_file() with mock.patch( "utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect_disabled_timer) @@ -505,6 +563,7 @@ def test_config_reload_disabled_service(self, get_cmd_module, setup_single_broad assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == reload_config_with_disabled_service_output def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic): + self.add_sysinfo_to_cfg_file() with mock.patch( "utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect) From 1a8da598050b1ac838f1019fd1ef3751d7d060f9 Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Tue, 31 Oct 2023 08:25:55 +0000 Subject: [PATCH 2/6] Save tmpfile incase L2 scenario --- config/main.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/config/main.py b/config/main.py index 9bf3f8ddc6..89a9d60e10 100644 --- a/config/main.py +++ b/config/main.py @@ -14,6 +14,7 @@ import time import itertools import copy +import tempfile from jsonpatch import JsonPatchConflict from jsonpointer import JsonPointerException @@ -142,6 +143,19 @@ def read_json_file(fileName): raise Exception(str(e)) return result +# write given JSON file +def write_json_file(json_input, fileName): + try: + with open(fileName, 'w') as f: + json.dump(json_input, fileName) + result = json.load(f) + except FileNotFoundError: + click.echo("{}".format(str(e)), err=True) + raise click.Abort() + except Exception as e: + click.echo("{}\n{}".format(type(e), str(e)), err=True) + raise click.Abort() + def _get_breakout_options(ctx, args, incomplete): """ Provides dynamic mode option as per user argument i.e. interface name """ all_mode_options = [] @@ -1524,7 +1538,12 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form # Get the file from user input, else take the default file /etc/sonic/config_db{NS_id}.json if cfg_files: + # Save to tmpfile in case of stdin input which can only be read once file = cfg_files[inst+1] + file_input = read_json_file(file) + (_, tmpfname) = tempfile.mkstemp(dir="/tmp", suffix="_configReload") + write_json_file(file_input, tmpfname) + file = tmpfname else: if file_format == 'config_db': if namespace is None: @@ -1611,6 +1630,13 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form clicommon.run_command(command, display_cmd=True) client.set(config_db.INIT_INDICATOR, 1) + if os.path.exists(file) and file.endswith("_configReload"): + # Remove tmpfile + try: + os.remove(file) + except OSError as e: + click.echo("An error occurred while removing the temporary file: {}".format(str(e)), err=True) + # Migrate DB contents to latest version db_migrator='/usr/local/bin/db_migrator.py' if os.path.isfile(db_migrator) and os.access(db_migrator, os.X_OK): From 68afebfd9335938adb5555ec27458ebc06592a34 Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Tue, 31 Oct 2023 10:12:16 +0000 Subject: [PATCH 3/6] fix ut --- config/main.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/config/main.py b/config/main.py index 89a9d60e10..ed06d5afac 100644 --- a/config/main.py +++ b/config/main.py @@ -147,8 +147,7 @@ def read_json_file(fileName): def write_json_file(json_input, fileName): try: with open(fileName, 'w') as f: - json.dump(json_input, fileName) - result = json.load(f) + json.dump(json_input, f, indent=4) except FileNotFoundError: click.echo("{}".format(str(e)), err=True) raise click.Abort() @@ -1538,12 +1537,13 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form # Get the file from user input, else take the default file /etc/sonic/config_db{NS_id}.json if cfg_files: - # Save to tmpfile in case of stdin input which can only be read once file = cfg_files[inst+1] - file_input = read_json_file(file) - (_, tmpfname) = tempfile.mkstemp(dir="/tmp", suffix="_configReload") - write_json_file(file_input, tmpfname) - file = tmpfname + # Save to tmpfile in case of stdin input which can only be read once + if file.endswith("stdin"): + file_input = read_json_file(file) + (_, tmpfname) = tempfile.mkstemp(dir="/tmp", suffix="_configReloadStdin") + write_json_file(file_input, tmpfname) + file = tmpfname else: if file_format == 'config_db': if namespace is None: @@ -1630,7 +1630,7 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form clicommon.run_command(command, display_cmd=True) client.set(config_db.INIT_INDICATOR, 1) - if os.path.exists(file) and file.endswith("_configReload"): + if os.path.exists(file) and file.endswith("_configReloadStdin"): # Remove tmpfile try: os.remove(file) From 659f8b8a47583bc4340498a8ca09eb205cefb84a Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Tue, 31 Oct 2023 12:38:49 +0000 Subject: [PATCH 4/6] cov --- tests/config_test.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/config_test.py b/tests/config_test.py index 660394ab8a..66339f5be4 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -8,6 +8,7 @@ import sys import unittest import ipaddress +import shutil from unittest import mock from jsonpatch import JsonPatchConflict @@ -261,6 +262,34 @@ def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic): assert "\n".join([l.rstrip() for l in result.output.split('\n')][:1]) == reload_config_with_sys_info_command_output + def test_config_reload_stdin(self, get_cmd_module, setup_single_broadcom_asic): + with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command: + (config, show) = get_cmd_module + + dev_stdin = os.path.join(mock_db_path, "dev_stdin") + jsonfile_config = os.path.join(mock_db_path, "config_db.json") + jsonfile_init_cfg = os.path.join(mock_db_path, "init_cfg.json") + shutil.copy(jsonfile_config, dev_stdin) + + # create object + config.INIT_CFG_FILE = jsonfile_init_cfg + config.DEFAULT_CONFIG_DB_FILE = dev_stdin + + db = Db() + runner = CliRunner() + obj = {'config_db': db.cfgdb} + + # simulate 'config reload' to provoke load_sys_info option + result = runner.invoke(config.config.commands["reload"], [dev_stdin, "-l", "-n", "-y"], obj=obj) + + print(result.exit_code) + print(result.output) + traceback.print_tb(result.exc_info[2]) + + assert result.exit_code == 0 + + assert "\n".join([l.rstrip() for l in result.output.split('\n')][:1]) == reload_config_with_sys_info_command_output + @classmethod def teardown_class(cls): print("TEARDOWN") From 663aebfd89eebe872bab5a3c483587cc9d8304b0 Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Tue, 31 Oct 2023 14:57:59 +0000 Subject: [PATCH 5/6] cov --- config/main.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/config/main.py b/config/main.py index ed06d5afac..63f67faa1b 100644 --- a/config/main.py +++ b/config/main.py @@ -148,12 +148,8 @@ def write_json_file(json_input, fileName): try: with open(fileName, 'w') as f: json.dump(json_input, f, indent=4) - except FileNotFoundError: - click.echo("{}".format(str(e)), err=True) - raise click.Abort() except Exception as e: - click.echo("{}\n{}".format(type(e), str(e)), err=True) - raise click.Abort() + raise Exception(str(e)) def _get_breakout_options(ctx, args, incomplete): """ Provides dynamic mode option as per user argument i.e. interface name """ From f6dd185b6288ff13df2442028d620cf94c350934 Mon Sep 17 00:00:00 2001 From: jingwenxie Date: Wed, 1 Nov 2023 07:49:52 +0000 Subject: [PATCH 6/6] fix ut --- config/main.py | 2 +- tests/config_test.py | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/config/main.py b/config/main.py index 63f67faa1b..494f0cf68e 100644 --- a/config/main.py +++ b/config/main.py @@ -1535,7 +1535,7 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form if cfg_files: file = cfg_files[inst+1] # Save to tmpfile in case of stdin input which can only be read once - if file.endswith("stdin"): + if file == "/dev/stdin": file_input = read_json_file(file) (_, tmpfname) = tempfile.mkstemp(dir="/tmp", suffix="_configReloadStdin") write_json_file(file_input, tmpfname) diff --git a/tests/config_test.py b/tests/config_test.py index 66339f5be4..c773ad29cb 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -263,17 +263,29 @@ def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic): assert "\n".join([l.rstrip() for l in result.output.split('\n')][:1]) == reload_config_with_sys_info_command_output def test_config_reload_stdin(self, get_cmd_module, setup_single_broadcom_asic): - with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command: + def mock_json_load(f): + device_metadata = { + "DEVICE_METADATA": { + "localhost": { + "docker_routing_config_mode": "split", + "hostname": "sonic", + "hwsku": "Seastone-DX010-25-50", + "mac": "00:e0:ec:89:6e:48", + "platform": "x86_64-cel_seastone-r0", + "type": "ToRRouter" + } + } + } + return device_metadata + with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command,\ + mock.patch("json.load", mock.MagicMock(side_effect=mock_json_load)): (config, show) = get_cmd_module - dev_stdin = os.path.join(mock_db_path, "dev_stdin") - jsonfile_config = os.path.join(mock_db_path, "config_db.json") + dev_stdin = "/dev/stdin" jsonfile_init_cfg = os.path.join(mock_db_path, "init_cfg.json") - shutil.copy(jsonfile_config, dev_stdin) # create object config.INIT_CFG_FILE = jsonfile_init_cfg - config.DEFAULT_CONFIG_DB_FILE = dev_stdin db = Db() runner = CliRunner()