[config reload] Fixing config reload when timer based services are di…

…sabled (#2200)

- What I did
Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

- How I did it
Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

- How to verify it
Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

Signed-off-by: Sudharsan Dhamal Gopalarathnam <[email protected]>

config/main.py

@@ -812,20 +812,23 @@ def _get_sonic_services():
     return (unit.strip() for unit in out.splitlines())
 
 
-def _get_delayed_sonic_services():
+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":
-            services.append(re.sub('\.timer$', '', unit, 1))
+            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_services()):
+    for service in itertools.chain(_get_sonic_services(), _get_delayed_sonic_units()):
         clicommon.run_command("systemctl reset-failed {}".format(service))
 
 
@@ -844,12 +847,8 @@ def _restart_services():
     click.echo("Reloading Monit configuration ...")
     clicommon.run_command("sudo monit reload")
 
-def _get_delay_timers():
-    out = clicommon.run_command("systemctl list-dependencies sonic-delayed.target --plain |sed '1d'", return_cmd=True)
-    return [timer.strip() for timer in out.splitlines()]
-
 def _delay_timers_elapsed():
-    for timer in _get_delay_timers():
+    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
@@ -1447,18 +1446,19 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, disable_arp_cach
     """Clear current configuration and import a previous saved config DB dump file.
        <filename> : Names of configuration file(s) to load, separated by comma with no spaces in between
     """
+    CONFIG_RELOAD_NOT_READY = 1
     if not force and not no_service_restart:
         if _is_system_starting():
             click.echo("System is not up. Retry later or use -f to avoid system checks")
-            return
+            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")
-            return
+            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")
-            return
+            sys.exit(CONFIG_RELOAD_NOT_READY)
 
     if filename is None:
         message = 'Clear current config and reload config in {} format from the default config file(s) ?'.format(file_format)

tests/config_test.py

@@ -73,6 +73,18 @@
 Running command: rm -rf /tmp/dropstat-*
 Running command: /usr/local/bin/sonic-cfggen -H -k Seastone-DX010-25-50 --write-to-db"""
 
+reload_config_with_disabled_service_output="""\
+Running command: rm -rf /tmp/dropstat-*
+Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  --write-to-db
+Restarting SONiC target ...
+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]
 
@@ -89,6 +101,44 @@ def mock_run_command_side_effect(*args, **kwargs):
         else:
             return ''
 
+def mock_run_command_side_effect_disabled_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'
+        elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
+            return 'swss'
+        elif command == "systemctl is-enabled snmp.timer":
+            return 'masked'
+        elif command == "systemctl show swss.service --property ActiveState --value":
+            return 'active'
+        elif command == "systemctl show swss.service --property ActiveEnterTimestampMonotonic --value":
+            return '0'
+        else:
+            return ''
+
+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'
+        elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
+            return 'swss'
+        elif command == "systemctl is-enabled snmp.timer":
+            return 'enabled'
+        elif command == "systemctl show snmp.timer --property=LastTriggerUSecMonotonic --value":
+            return '0'
+        else:
+            return ''
+
 def mock_run_command_side_effect_gnmi(*args, **kwargs):
     command = args[0]
 
@@ -111,6 +161,8 @@ def mock_run_command_side_effect_gnmi(*args, **kwargs):
 
 
 class TestConfigReload(object):
+    dummy_cfg_file = os.path.join(os.sep, "tmp", "config.json")
+
     @classmethod
     def setup_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "1"
@@ -121,6 +173,7 @@ def setup_class(cls):
 
         import config.main
         importlib.reload(config.main)
+        open(cls.dummy_cfg_file, 'w').close()
 
     def test_config_reload(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:
@@ -148,6 +201,32 @@ def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic):
 
             assert "\n".join([l.rstrip() for l in result.output.split('\n')][:2]) == 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")
@@ -312,6 +391,25 @@ def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
             assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
                 == RELOAD_CONFIG_DB_OUTPUT
 
+    def test_config_reload_disabled_service(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_disabled_timer)
+        ) as mock_run_command:
+            (config, show) = get_cmd_module
+
+            runner = CliRunner()
+            result = runner.invoke(config.config.commands["reload"], [self.dummy_cfg_file, "-y"])
+
+            print(result.exit_code)
+            print(result.output)
+            print(reload_config_with_disabled_service_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')]) == reload_config_with_disabled_service_output
+
     def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic):
         with mock.patch(
                 "utilities_common.cli.run_command",