From 20b450002bf27690c992a7d9f45303caa9264f76 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Mon, 10 Apr 2023 18:38:29 -0700 Subject: [PATCH] [config reload]Config Reload Enhancement (#2693) Code changes for HLD: https://github.com/sonic-net/SONiC/pull/1203 Removed the timer based checks for config reload Added db_migrator to migrate from "has_timer" to "delayed" Modified package-manager to migrate from "has_timer" to "delayed" Modified relevant files Added UT to verify --- config/main.py | 27 +----- scripts/db_migrator.py | 24 +++++- .../service_creator/feature.py | 6 +- tests/config_test.py | 85 +------------------ tests/counterpoll_input/config_db.json | 30 +++---- .../config_db/feature-expected.json | 6 +- .../config_db/feature-input.json | 3 +- tests/db_migrator_input/init_cfg.json | 6 +- tests/mock_tables/t1/config_db.json | 30 +++---- .../test_service_creator.py | 10 +-- 10 files changed, 70 insertions(+), 157 deletions(-) diff --git a/config/main.py b/config/main.py index e9dc378bbd..c0a47ea804 100644 --- a/config/main.py +++ b/config/main.py @@ -869,23 +869,8 @@ def _get_sonic_services(): return (unit.strip() for unit in out.splitlines()) -def _get_delayed_sonic_units(get_timers=False): - rc1, _ = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True) - rc2, _ = clicommon.run_command("systemctl is-enabled {}".format(rc1.replace("\n", " ")), return_cmd=True) - timer = [line.strip() for line in rc1.splitlines()] - state = [line.strip() for line in rc2.splitlines()] - services = [] - for unit in timer: - if state[timer.index(unit)] == "enabled": - if not get_timers: - services.append(re.sub('\.timer$', '', unit, 1)) - else: - services.append(unit) - return services - - def _reset_failed_services(): - for service in itertools.chain(_get_sonic_services(), _get_delayed_sonic_units()): + for service in _get_sonic_services(): clicommon.run_command("systemctl reset-failed {}".format(service)) @@ -904,12 +889,6 @@ def _restart_services(): click.echo("Reloading Monit configuration ...") clicommon.run_command("sudo monit reload") -def _delay_timers_elapsed(): - for timer in _get_delayed_sonic_units(get_timers=True): - out, _ = clicommon.run_command("systemctl show {} --property=LastTriggerUSecMonotonic --value".format(timer), return_cmd=True) - if out.strip() == "0": - return False - return True def _per_namespace_swss_ready(service_name): out, _ = clicommon.run_command("systemctl show {} --property ActiveState --value".format(service_name), return_cmd=True) @@ -1526,10 +1505,6 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form click.echo("System is not up. Retry later or use -f to avoid system checks") sys.exit(CONFIG_RELOAD_NOT_READY) - if not _delay_timers_elapsed(): - click.echo("Relevant services are not up. Retry later or use -f to avoid system checks") - sys.exit(CONFIG_RELOAD_NOT_READY) - if not _swss_ready(): click.echo("SwSS container is not ready. Retry later or use -f to avoid system checks") sys.exit(CONFIG_RELOAD_NOT_READY) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index c22bbc2e5d..c8d452c938 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -45,7 +45,7 @@ def __init__(self, namespace, socket=None): none-zero values. build: sequentially increase within a minor version domain. """ - self.CURRENT_VERSION = 'version_4_0_1' + self.CURRENT_VERSION = 'version_4_0_2' self.TABLE_NAME = 'VERSIONS' self.TABLE_KEY = 'DATABASE' @@ -575,6 +575,18 @@ def migrate_port_qos_map_global(self): self.configDB.set_entry('PORT_QOS_MAP', 'global', {"dscp_to_tc_map": dscp_to_tc_map_table_names[0]}) log.log_info("Created entry for global DSCP_TO_TC_MAP {}".format(dscp_to_tc_map_table_names[0])) + def migrate_feature_timer(self): + ''' + Migrate feature 'has_timer' field to 'delayed' + ''' + feature_table = self.configDB.get_table('FEATURE') + for feature, config in feature_table.items(): + state = config.get('has_timer') + if state is not None: + config['delayed'] = state + config.pop('has_timer') + self.configDB.set_entry('FEATURE', feature, config) + def update_edgezone_aggregator_config(self): """ Update cable length configuration in ConfigDB for T0 neighbor interfaces @@ -909,9 +921,17 @@ def version_4_0_0(self): def version_4_0_1(self): """ Version 4_0_1. + """ + self.migrate_feature_timer() + self.set_version('version_4_0_2') + return 'version_4_0_2' + + def version_4_0_2(self): + """ + Version 4_0_2. This is the latest version for master branch """ - log.log_info('Handling version_4_0_1') + log.log_info('Handling version_4_0_2') return None def get_version(self): diff --git a/sonic_package_manager/service_creator/feature.py b/sonic_package_manager/service_creator/feature.py index 90378d378f..43b6c309fe 100644 --- a/sonic_package_manager/service_creator/feature.py +++ b/sonic_package_manager/service_creator/feature.py @@ -105,7 +105,7 @@ def update(self, old_manifest: Manifest, new_manifest: Manifest): """ Migrate feature configuration. It can be that non-configurable - feature entries have to be updated. e.g: "has_timer" for example if + feature entries have to be updated. e.g: "delayed" for example if the new feature introduces a service timer or name of the service has changed, but user configurable entries are not changed). @@ -227,12 +227,12 @@ def get_default_feature_entries(state=None, owner=None) -> Dict[str, str]: @staticmethod def get_non_configurable_feature_entries(manifest) -> Dict[str, str]: - """ Get non-configurable feature table entries: e.g. 'has_timer' """ + """ Get non-configurable feature table entries: e.g. 'delayed' """ return { 'has_per_asic_scope': str(manifest['service']['asic-service']), 'has_global_scope': str(manifest['service']['host-service']), - 'has_timer': str(manifest['service']['delayed']), + 'delayed': str(manifest['service']['delayed']), 'check_up_status': str(manifest['service']['check_up_status']), 'support_syslog_rate_limit': str(manifest['service']['syslog']['support-rate-limit']), } diff --git a/tests/config_test.py b/tests/config_test.py index fff66d47e6..f8ba56da19 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -114,10 +114,6 @@ Reloading Monit configuration ... """ -reload_config_with_untriggered_timer_output="""\ -Relevant services are not up. Retry later or use -f to avoid system checks -""" - def mock_run_command_side_effect(*args, **kwargs): command = args[0] @@ -154,41 +150,6 @@ def mock_run_command_side_effect_disabled_timer(*args, **kwargs): else: return '', 0 -def mock_run_command_side_effect_untriggered_timer(*args, **kwargs): - command = args[0] - - if kwargs.get('display_cmd'): - click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green')) - - if kwargs.get('return_cmd'): - if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'": - return 'snmp.timer', 0 - elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'": - return 'swss', 0 - elif command == "systemctl is-enabled snmp.timer": - return 'enabled', 0 - elif command == "systemctl show snmp.timer --property=LastTriggerUSecMonotonic --value": - return '0', 0 - else: - return '', 0 - -def mock_run_command_side_effect_gnmi(*args, **kwargs): - command = args[0] - - if kwargs.get('display_cmd'): - click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green')) - - if kwargs.get('return_cmd'): - if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'": - return 'gnmi.timer', 0 - elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'": - return 'swss', 0 - elif command == "systemctl is-enabled gnmi.timer": - return 'enabled', 0 - else: - return '', 0 - - # Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension. sonic_cfggen = load_module_from_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen') @@ -234,32 +195,6 @@ 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_untriggered_timer(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_untriggered_timer)) as mock_run_command: - (config, show) = get_cmd_module - - jsonfile_config = os.path.join(mock_db_path, "config_db.json") - jsonfile_init_cfg = os.path.join(mock_db_path, "init_cfg.json") - - # create object - config.INIT_CFG_FILE = jsonfile_init_cfg - config.DEFAULT_CONFIG_DB_FILE = jsonfile_config - - 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"], ["-l", "-y"], obj=obj) - - print(result.exit_code) - print(result.output) - traceback.print_tb(result.exc_info[2]) - - assert result.exit_code == 1 - - assert "\n".join([l.rstrip() for l in result.output.split('\n')][:2]) == reload_config_with_untriggered_timer_output - @classmethod def teardown_class(cls): print("TEARDOWN") @@ -292,25 +227,7 @@ def test_load_minigraph(self, get_cmd_module, setup_single_broadcom_asic): assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output # Verify "systemctl reset-failed" is called for services under sonic.target mock_run_command.assert_any_call('systemctl reset-failed swss') - # Verify "systemctl reset-failed" is called for services under sonic-delayed.target - mock_run_command.assert_any_call('systemctl reset-failed snmp') - assert mock_run_command.call_count == 11 - - def test_load_minigraph_with_gnmi_timer(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_gnmi)) as mock_run_command: - (config, show) = get_cmd_module - runner = CliRunner() - result = runner.invoke(config.config.commands["load_minigraph"], ["-y"]) - 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')]) == load_minigraph_command_output - # Verify "systemctl reset-failed" is called for services under sonic.target - mock_run_command.assert_any_call('systemctl reset-failed swss') - # Verify "systemctl reset-failed" is called for services under sonic-delayed.target - mock_run_command.assert_any_call('systemctl reset-failed gnmi') - assert mock_run_command.call_count == 11 + assert mock_run_command.call_count == 8 def test_load_minigraph_with_port_config_bad_format(self, get_cmd_module, setup_single_broadcom_asic): with mock.patch( diff --git a/tests/counterpoll_input/config_db.json b/tests/counterpoll_input/config_db.json index 40ff750db6..38cde7c15e 100644 --- a/tests/counterpoll_input/config_db.json +++ b/tests/counterpoll_input/config_db.json @@ -2235,7 +2235,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "pmon": { "has_per_asic_scope": "False", @@ -2243,7 +2243,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "sflow": { "has_per_asic_scope": "False", @@ -2251,7 +2251,7 @@ "auto_restart": "enabled", "state": "disabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "database": { "has_per_asic_scope": "True", @@ -2259,7 +2259,7 @@ "auto_restart": "disabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "telemetry": { "has_per_asic_scope": "False", @@ -2268,7 +2268,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "True" + "delayed": "True" }, "snmp": { "has_per_asic_scope": "False", @@ -2276,7 +2276,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "True" + "delayed": "True" }, "bgp": { "has_per_asic_scope": "True", @@ -2284,7 +2284,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "False", - "has_timer": "False" + "delayed": "False" }, "radv": { "has_per_asic_scope": "False", @@ -2292,7 +2292,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "mgmt-framework": { "has_per_asic_scope": "False", @@ -2300,7 +2300,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "True" + "delayed": "True" }, "nat": { "has_per_asic_scope": "False", @@ -2308,7 +2308,7 @@ "auto_restart": "enabled", "state": "disabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "teamd": { "has_per_asic_scope": "True", @@ -2316,7 +2316,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "False", - "has_timer": "False" + "delayed": "False" }, "dhcp_relay": { "has_per_asic_scope": "False", @@ -2324,7 +2324,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "True", - "has_timer": "False" + "delayed": "False" }, "swss": { "has_per_asic_scope": "True", @@ -2332,7 +2332,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "False", - "has_timer": "False" + "delayed": "False" }, "syncd": { "has_per_asic_scope": "True", @@ -2340,7 +2340,7 @@ "auto_restart": "enabled", "state": "enabled", "has_global_scope": "False", - "has_timer": "False" + "delayed": "False" } }, "DSCP_TO_TC_MAP": { @@ -2669,4 +2669,4 @@ "size": "56368" } } -} \ No newline at end of file +} diff --git a/tests/db_migrator_input/config_db/feature-expected.json b/tests/db_migrator_input/config_db/feature-expected.json index 92653771fc..baf051a8bd 100644 --- a/tests/db_migrator_input/config_db/feature-expected.json +++ b/tests/db_migrator_input/config_db/feature-expected.json @@ -3,7 +3,7 @@ "auto_restart": "disabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" }, @@ -11,7 +11,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" }, @@ -19,7 +19,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "True", "high_mem_alert": "disabled", "state": "enabled" } diff --git a/tests/db_migrator_input/config_db/feature-input.json b/tests/db_migrator_input/config_db/feature-input.json index c6d512dad1..46a6cae613 100644 --- a/tests/db_migrator_input/config_db/feature-input.json +++ b/tests/db_migrator_input/config_db/feature-input.json @@ -8,7 +8,8 @@ "high_mem_alert": "disabled" }, "FEATURE|telemetry": { - "status": "enabled" + "status": "enabled", + "has_timer": "True" }, "FEATURE|syncd": { "state": "enabled" diff --git a/tests/db_migrator_input/init_cfg.json b/tests/db_migrator_input/init_cfg.json index 634477a4f9..a714b8cdfe 100644 --- a/tests/db_migrator_input/init_cfg.json +++ b/tests/db_migrator_input/init_cfg.json @@ -4,7 +4,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" }, @@ -12,7 +12,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" }, @@ -20,7 +20,7 @@ "auto_restart": "disabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "disabled" } diff --git a/tests/mock_tables/t1/config_db.json b/tests/mock_tables/t1/config_db.json index f1f835182f..42a0e2da6c 100644 --- a/tests/mock_tables/t1/config_db.json +++ b/tests/mock_tables/t1/config_db.json @@ -1798,7 +1798,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1811,7 +1811,7 @@ "auto_restart": "disabled", "has_global_scope": "True", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1824,7 +1824,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1837,7 +1837,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1850,7 +1850,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "True", + "delayed": "True", "high_mem_alert": "disabled", "state": "enabled" } @@ -1863,7 +1863,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "disabled" } @@ -1876,7 +1876,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1889,7 +1889,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1902,7 +1902,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "disabled" } @@ -1915,7 +1915,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "True", + "delayed": "True", "high_mem_alert": "disabled", "state": "enabled" } @@ -1928,7 +1928,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1941,7 +1941,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1954,7 +1954,7 @@ "auto_restart": "enabled", "has_global_scope": "False", "has_per_asic_scope": "True", - "has_timer": "False", + "delayed": "False", "high_mem_alert": "disabled", "state": "enabled" } @@ -1967,7 +1967,7 @@ "auto_restart": "enabled", "has_global_scope": "True", "has_per_asic_scope": "False", - "has_timer": "True", + "delayed": "True", "high_mem_alert": "disabled", "state": "enabled", "status": "enabled" @@ -3510,4 +3510,4 @@ "VERSION": "version_1_0_4" } } -} \ No newline at end of file +} diff --git a/tests/sonic_package_manager/test_service_creator.py b/tests/sonic_package_manager/test_service_creator.py index c97d362626..689a635411 100644 --- a/tests/sonic_package_manager/test_service_creator.py +++ b/tests/sonic_package_manager/test_service_creator.py @@ -218,7 +218,7 @@ def test_feature_registration(mock_sonic_db, manifest): 'set_owner': 'local', 'has_per_asic_scope': 'False', 'has_global_scope': 'True', - 'has_timer': 'False', + 'delayed': 'False', 'check_up_status': 'False', 'support_syslog_rate_limit': 'False', }) @@ -232,7 +232,7 @@ def test_feature_update(mock_sonic_db, manifest): 'set_owner': 'local', 'has_per_asic_scope': 'False', 'has_global_scope': 'True', - 'has_timer': 'False', + 'delayed': 'False', 'check_up_status': 'False', 'support_syslog_rate_limit': 'False', } @@ -256,7 +256,7 @@ def test_feature_update(mock_sonic_db, manifest): 'set_owner': 'local', 'has_per_asic_scope': 'False', 'has_global_scope': 'True', - 'has_timer': 'True', + 'delayed': 'True', 'check_up_status': 'False', 'support_syslog_rate_limit': 'False', }), @@ -278,7 +278,7 @@ def test_feature_registration_with_timer(mock_sonic_db, manifest): 'set_owner': 'local', 'has_per_asic_scope': 'False', 'has_global_scope': 'True', - 'has_timer': 'True', + 'delayed': 'True', 'check_up_status': 'False', 'support_syslog_rate_limit': 'False', }) @@ -298,7 +298,7 @@ def test_feature_registration_with_non_default_owner(mock_sonic_db, manifest): 'set_owner': 'kube', 'has_per_asic_scope': 'False', 'has_global_scope': 'True', - 'has_timer': 'False', + 'delayed': 'False', 'check_up_status': 'False', 'support_syslog_rate_limit': 'False', })