[config reload]Config Reload Enhancement (sonic-net#2693)

Code changes for HLD: sonic-net/SONiC#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

@@ -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)

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):

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']),
         }

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(

tests/counterpoll_input/config_db.json

@@ -2235,31 +2235,31 @@
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "pmon": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "sflow": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "disabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "database": {
             "has_per_asic_scope": "True", 
             "high_mem_alert": "disabled", 
             "auto_restart": "disabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "telemetry": {
             "has_per_asic_scope": "False", 
@@ -2268,79 +2268,79 @@
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "True"
+            "delayed": "True"
         }, 
         "snmp": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "True"
+            "delayed": "True"
         }, 
         "bgp": {
             "has_per_asic_scope": "True", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "False", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "radv": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "mgmt-framework": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "True"
+            "delayed": "True"
         }, 
         "nat": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "disabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "teamd": {
             "has_per_asic_scope": "True", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "False", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "dhcp_relay": {
             "has_per_asic_scope": "False", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "True", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "swss": {
             "has_per_asic_scope": "True", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "False", 
-            "has_timer": "False"
+            "delayed": "False"
         }, 
         "syncd": {
             "has_per_asic_scope": "True", 
             "high_mem_alert": "disabled", 
             "auto_restart": "enabled", 
             "state": "enabled", 
             "has_global_scope": "False", 
-            "has_timer": "False"
+            "delayed": "False"
         }
     }, 
     "DSCP_TO_TC_MAP": {
@@ -2669,4 +2669,4 @@
             "size": "56368"
         }
     }
-}
+}

tests/db_migrator_input/config_db/feature-expected.json

@@ -3,23 +3,23 @@
 		"auto_restart": "disabled",
 		"has_global_scope": "False",
 		"has_per_asic_scope": "True",
-		"has_timer": "False",
+		"delayed": "False",
 		"high_mem_alert": "disabled",
 		"state": "enabled"
 	},
 	"FEATURE|syncd": {
 		"auto_restart": "enabled",
 		"has_global_scope": "False",
 		"has_per_asic_scope": "True",
-		"has_timer": "False",
+		"delayed": "False",
 		"high_mem_alert": "disabled",
 		"state": "enabled"
 	},
 	"FEATURE|telemetry": {
 		"auto_restart": "enabled",
 		"has_global_scope": "False",
 		"has_per_asic_scope": "True",
-		"has_timer": "False",
+		"delayed": "True",
 		"high_mem_alert": "disabled",
 		"state": "enabled"
 	}