[config load]: do not stop/reset/reload service if it is disabled (so…

…nic-net#1028)

also introduce fixtures to setup the cmd as well as asic context

Signed-off-by: Guohan Lu <lguohan@gmail.com>

config/main.py

@@ -337,6 +337,24 @@ def run_command(command, display_cmd=False, ignore_error=False):
     if proc.returncode != 0 and not ignore_error:
         sys.exit(proc.returncode)
 
+def _get_device_type():
+    """
+    Get device type
+
+    TODO: move to sonic-py-common
+    """
+
+    command = "{} -m -v DEVICE_METADATA.localhost.type".format(SONIC_CFGGEN_PATH)
+    proc = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)
+    device_type, err = proc.communicate()
+    if err:
+        click.echo("Could not get the device type from minigraph, setting device type to Unknown")
+        device_type = 'Unknown'
+    else:
+        device_type = device_type.strip()
+
+    return device_type
+
 # Validate whether a given namespace name is valid in the device.
 def validate_namespace(namespace):
     if not sonic_device_util.is_multi_npu():
@@ -676,8 +694,6 @@ def _abort_if_false(ctx, param, value):
 def _get_disabled_services_list():
     disabled_services_list = []
 
-    config_db = ConfigDBConnector()
-    config_db.connect()
     feature_table = config_db.get_table('FEATURE')
     if feature_table is not None:
         for feature_name in feature_table.keys():
@@ -697,7 +713,6 @@ def _get_disabled_services_list():
 
     return disabled_services_list
 
-
 def _stop_services():
     # This list is order-dependent. Please add services in the order they should be stopped
     # on Mellanox platform pmon is stopped by syncd
@@ -715,6 +730,12 @@ def _stop_services():
     if asic_type == 'mellanox' and 'pmon' in services_to_stop:
         services_to_stop.remove('pmon')
 
+    disabled_services = _get_disabled_services_list()
+
+    for service in disabled_services:
+        if service in services_to_stop:
+            services_to_stop.remove(service)
+
     execute_systemctl(services_to_stop, SYSTEMCTL_ACTION_STOP)
 
 
@@ -741,6 +762,12 @@ def _reset_failed_services():
         'telemetry'
     ]
 
+    disabled_services = _get_disabled_services_list()
+
+    for service in disabled_services:
+        if service in services_to_reset:
+            services_to_reset.remove(service)
+
     execute_systemctl(services_to_reset, SYSTEMCTL_ACTION_RESET_FAILED)
 
 
@@ -763,9 +790,9 @@ def _restart_services():
         'telemetry'
     ]
 
-    disable_services = _get_disabled_services_list()
+    disabled_services = _get_disabled_services_list()
 
-    for service in disable_services:
+    for service in disabled_services:
         if service in services_to_restart:
             services_to_restart.remove(service)
 
@@ -1185,16 +1212,6 @@ def load_minigraph(no_service_restart):
     """Reconfigure based on minigraph."""
     log_info("'load_minigraph' executing...")
 
-    # get the device type
-    command = "{} -m -v DEVICE_METADATA.localhost.type".format(SONIC_CFGGEN_PATH)
-    proc = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)
-    device_type, err = proc.communicate()
-    if err:
-        click.echo("Could not get the device type from minigraph, setting device type to Unknown")
-        device_type = 'Unknown'
-    else:
-        device_type = device_type.strip()
-
     #Stop services before config push
     if not no_service_restart:
         log_info("'load_minigraph' stopping services...")
@@ -1223,10 +1240,13 @@ def load_minigraph(no_service_restart):
         if os.path.isfile('/etc/sonic/init_cfg.json'):
             command = "{} -H -m -j /etc/sonic/init_cfg.json {} --write-to-db".format(SONIC_CFGGEN_PATH, cfggen_namespace_option)
         else:
-            command = "{} -H -m --write-to-db {} ".format(SONIC_CFGGEN_PATH,cfggen_namespace_option)
+            command = "{} -H -m --write-to-db {}".format(SONIC_CFGGEN_PATH, cfggen_namespace_option)
         run_command(command, display_cmd=True)
         client.set(config_db.INIT_INDICATOR, 1)
 
+        # get the device type
+        device_type = _get_device_type()
+
         # These commands are not run for host on multi asic platform
         if num_npus == 1 or namespace is not DEFAULT_NAMESPACE:
             if device_type != 'MgmtToRRouter':

tests/config_test.py

@@ -0,0 +1,72 @@
+from click.testing import CliRunner
+
+load_minigraph_command_output="""\
+Executing stop of service telemetry...
+Executing stop of service swss...
+Executing stop of service lldp...
+Executing stop of service pmon...
+Executing stop of service bgp...
+Executing stop of service hostcfgd...
+Executing stop of service nat...
+Running command: /usr/local/bin/sonic-cfggen -H -m --write-to-db
+Running command:   pfcwd start_default
+Running command:   config qos reload
+Executing reset-failed of service bgp...
+Executing reset-failed of service dhcp_relay...
+Executing reset-failed of service hostcfgd...
+Executing reset-failed of service hostname-config...
+Executing reset-failed of service interfaces-config...
+Executing reset-failed of service lldp...
+Executing reset-failed of service nat...
+Executing reset-failed of service ntp-config...
+Executing reset-failed of service pmon...
+Executing reset-failed of service radv...
+Executing reset-failed of service rsyslog-config...
+Executing reset-failed of service snmp...
+Executing reset-failed of service swss...
+Executing reset-failed of service syncd...
+Executing reset-failed of service teamd...
+Executing reset-failed of service telemetry...
+Executing restart of service hostname-config...
+Executing restart of service interfaces-config...
+Executing restart of service ntp-config...
+Executing restart of service rsyslog-config...
+Executing restart of service swss...
+Executing restart of service bgp...
+Executing restart of service pmon...
+Executing restart of service lldp...
+Executing restart of service hostcfgd...
+Executing restart of service nat...
+Executing restart of service telemetry...
+Please note setting loaded from minigraph will be lost after system reboot. To preserve setting, run `config save`.
+"""
+
+class TestLoadMinigraph(object):
+    @classmethod
+    def setup_class(cls):
+        print("SETUP")
+
+    def test_load_minigraph(self, get_cmd_module, setup_single_broacom_asic):
+        (config, show) = get_cmd_module
+        runner = CliRunner()
+        result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
+        print result.exit_code
+        print result.output
+        assert result.exit_code == 0
+        assert "\n".join([ l.rstrip() for l in result.output.split('\n')]) == load_minigraph_command_output
+
+    def test_load_minigraph_with_disabled_telemetry(self, get_cmd_module, setup_single_broacom_asic):
+        (config, show) = get_cmd_module
+        runner = CliRunner()
+        runner.invoke(config.config.commands["feature"].commands["state"], ["telemetry", "disabled"])
+        result = runner.invoke(show.cli.commands["feature"].commands["status"], ["telemetry"])
+        print result.output
+        result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
+        print result.exit_code
+        print result.output
+        assert result.exit_code == 0
+        assert "telemetry" not in result.output
+
+    @classmethod
+    def teardown_class(cls):
+        print("TEARDOWN")

tests/conftest.py

@@ -0,0 +1,81 @@
+import os
+import sys
+
+import mock
+import click
+import pytest
+
+import mock_tables.dbconnector
+
+import sonic_device_util
+from swsssdk import ConfigDBConnector
+
+test_path = os.path.dirname(os.path.abspath(__file__))
+modules_path = os.path.dirname(test_path)
+sys.path.insert(0, modules_path)
+
+generated_services_list = [
+    'ntp-config.service',
+    'warmboot-finalizer.service',
+    'watchdog-control.service',
+    'rsyslog-config.service',
+    'interfaces-config.service',
+    'hostcfgd.service',
+    'hostname-config.service',
+    'topology.service',
+    'updategraph.service',
+    'config-setup.service',
+    'caclmgrd.service',
+    'procdockerstatsd.service',
+    'pcie-check.service',
+    'process-reboot-cause.service',
+    'dhcp_relay.service',
+    'snmp.service',
+    'sflow.service',
+    'bgp.service',
+    'telemetry.service',
+    'swss.service',
+    'database.service',
+    'database.service',
+    'lldp.service',
+    'lldp.service',
+    'pmon.service',
+    'radv.service',
+    'mgmt-framework.service',
+    'nat.service',
+    'teamd.service',
+    'syncd.service',
+    'snmp.timer',
+    'telemetry.timer']
+
+
+def _dummy_run_command(command, display_cmd=False, return_cmd=False):
+    if display_cmd == True:
+        click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))
+
+@pytest.fixture
+def get_cmd_module():
+    import config.main as config
+    import show.main as show
+
+    config_db = ConfigDBConnector()
+    config_db.connect()
+
+    config.config_db = config_db
+    show.config_db = config_db
+
+    config.run_command = _dummy_run_command
+
+    return (config, show)
+
+@pytest.fixture
+def setup_single_broacom_asic():
+    import config.main as config
+    import show.main as show
+
+    sonic_device_util.get_num_npus = mock.MagicMock(return_value = 1)
+    config._get_sonic_generated_services = \
+            mock.MagicMock(return_value = (generated_services_list, []))
+
+    config.asic_type = mock.MagicMock(return_value = "broadcom")
+    config._get_device_type = mock.MagicMock(return_value = "ToRRouter")

tests/feature_test.py

@@ -1,25 +1,4 @@
-import os
-import sys
-
-import mock
 from click.testing import CliRunner
-from swsssdk import ConfigDBConnector
-
-test_path = os.path.dirname(os.path.abspath(__file__))
-modules_path = os.path.dirname(test_path)
-sys.path.insert(0, modules_path)
-
-import show.main as show
-import config.main as config
-
-config.asic_type = mock.MagicMock(return_value = "broadcom")
-config._get_device_type = mock.MagicMock(return_value = "ToRRouter")
-
-config_db = ConfigDBConnector()
-config_db.connect()
-
-show.config_db = config_db
-config.config_db = config_db
 
 show_feature_status_output="""\
 Feature     State     AutoRestart
@@ -89,53 +68,60 @@ class TestFeature(object):
     def setup_class(cls):
         print("SETUP")
 
-    def test_show_feature_status(self):
+    def test_show_feature_status(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["status"], [])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 0
         assert result.output == show_feature_status_output
 
-    def test_show_bgp_feature_status(self):
+    def test_show_bgp_feature_status(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["status"], ["bgp"])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_status_output
 
-    def test_show_unknown_feature_status(self):
+    def test_show_unknown_feature_status(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["status"], ["foo"])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 1
 
-    def test_show_feature_autorestart(self):
+    def test_show_feature_autorestart(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], [])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 0
         assert result.output == show_feature_autorestart_output
 
-    def test_show_bgp_autorestart_status(self):
+    def test_show_bgp_autorestart_status(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], ["bgp"])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_autorestart_output
 
-    def test_show_unknown_autorestart_status(self):
+    def test_show_unknown_autorestart_status(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], ["foo"])
         print(result.exit_code)
         print(result.output)
         assert result.exit_code == 1
 
-    def test_config_bgp_feature_state(self):
+    def test_config_bgp_feature_state(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(config.config.commands["feature"].commands["state"], ["bgp", "disabled"])
         print(result.exit_code)
@@ -145,7 +131,8 @@ def test_config_bgp_feature_state(self):
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_disabled_status_output
 
-    def test_config_bgp_autorestart(self):
+    def test_config_bgp_autorestart(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(config.config.commands["feature"].commands["autorestart"], ["bgp", "disabled"])
         print(result.exit_code)
@@ -155,7 +142,8 @@ def test_config_bgp_autorestart(self):
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_disabled_autorestart_output
 
-    def test_config_unknown_feature(self):
+    def test_config_unknown_feature(self, get_cmd_module):
+        (config, show) = get_cmd_module
         runner = CliRunner()
         result = runner.invoke(config.config.commands["feature"].commands['state'], ["foo", "enabled"])
         print(result.output)

tests/mock_tables/dbconnector.py

@@ -32,6 +32,8 @@ def listen(self):
     def punsubscribe(self, *args, **kwargs):
         pass
 
+    def clear(self):
+        pass
 
 INPUT_DIR = os.path.dirname(os.path.abspath(__file__))