Fix pre-commit check error and address review comments

Author: JunhongMao

config/chassis_modules.py

@@ -3,9 +3,12 @@
 import click
 import time
 import re
-from swsscommon.swsscommon import SonicV2Connector
+import subprocess
 import utilities_common.cli as clicommon
 
+TIMEOUT_SECS = 10
+
+
 #
 # 'chassis_modules' group ('config chassis_modules ...')
 #
@@ -19,6 +22,7 @@ def modules():
     """Configure chassis modules"""
     pass
 
+
 def get_config_module_state(db, chassis_module_name):
     config_db = db.cfgdb
     fvs = config_db.get_entry('CHASSIS_MODULE', chassis_module_name)
@@ -27,22 +31,22 @@ def get_config_module_state(db, chassis_module_name):
     else:
         return fvs['admin_status']
 
-TIMEOUT_SECS = 10
 
 #
-# Name: get_config_module_state_timeout
+# Name: check_config_module_state_with_timeout
 # return: True: timeout, False: not timeout
 #
-def get_config_module_state_timeout(ctx, db, chassis_module_name, state):
+def check_config_module_state_with_timeout(ctx, db, chassis_module_name, state):
     counter = 0
-    while  get_config_module_state(db, chassis_module_name) != state:
+    while get_config_module_state(db, chassis_module_name) != state:
         time.sleep(1)
         counter += 1
         if counter >= TIMEOUT_SECS:
             ctx.fail("get_config_module_state {} timeout".format(chassis_module_name))
             return True
     return False
 
+
 def get_asic_list_from_db(chassisdb, chassis_module_name):
     asic_list = []
     asics_keys_list = chassisdb.keys("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE*")
@@ -53,11 +57,12 @@ def get_asic_list_from_db(chassisdb, chassis_module_name):
             asic_list.append(asic_id)
     return asic_list
 
+
 #
 # Syntax: fabric_module_set_admin_status <chassis_module_name> <'up'/'down'>
 #
-def fabric_module_set_admin_status(chassis_module_name, state):
-    chassisdb = SonicV2Connector(host="127.0.0.1")
+def fabric_module_set_admin_status(db, chassis_module_name, state):
+    chassisdb = db.db
     chassisdb.connect("CHASSIS_STATE_DB")
     asic_list = get_asic_list_from_db(chassisdb, chassis_module_name)
 
@@ -66,16 +71,30 @@ def fabric_module_set_admin_status(chassis_module_name, state):
 
     if state == "down":
         for asic in asic_list:
-            click.echo("Stop swss@{} and syncd@{} ...".format(asic, asic))
+            click.echo("Stop swss@{} and syncd@{} services".format(asic, asic))
             clicommon.run_command('sudo systemctl stop swss@{}.service'.format(asic))
-            # wait for service is down
-            time.sleep(2)
-            chassisdb.delete("CHASSIS_STATE_DB","CHASSIS_FABRIC_ASIC_TABLE|asic" + str(asic))
-            click.echo("Start swss@{} and syncd@{} ...".format(asic, asic))
+
+        is_active = subprocess.call(["systemctl", "is-active", "--quiet", "swss@{}.service".format(asic)])
+
+        if is_active == 0:  # zero active,  non-zero, inactive
+            click.echo("stop swss@{} and syncd@{} services failed".format(asic, asic))
+            return
+
+        click.echo("Delete related CAHSSIS_FABRIC_ASIC_TABLE entries")
+
+        for asic in asic_list:
+            chassisdb.delete("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic" + str(asic))
+
+        # Start the services in case of the users just execute issue command "systemctl stop swss@/syncd@"
+        # without bring down the hardware
+        for asic in asic_list:
+            click.echo("Start swss@{} and syncd@{} services".format(asic, asic))
+            # To address systemd service restart limit by resetting the count
+            clicommon.run_command('sudo systemctl reset-failed swss@{}.service'.format(asic))
             clicommon.run_command('sudo systemctl start swss@{}.service'.format(asic))
-    else:
+    elif state == "up":
         for asic in asic_list:
-            click.echo("Start swss@{} and syncd@{} ...".format(asic, asic))
+            click.echo("Start swss@{} and syncd@{} services".format(asic, asic))
             clicommon.run_command('sudo systemctl start swss@{}.service'.format(asic))
 
 #
@@ -94,7 +113,7 @@ def shutdown_chassis_module(db, chassis_module_name):
        not chassis_module_name.startswith("FABRIC-CARD"):
         ctx.fail("'module_name' has to begin with 'SUPERVISOR', 'LINE-CARD' or 'FABRIC-CARD'")
 
-    #To avoid duplicate operation
+    # To avoid duplicate operation
     if get_config_module_state(db, chassis_module_name) == 'down':
         click.echo("Module {} is already in down state".format(chassis_module_name))
         return
@@ -103,8 +122,8 @@ def shutdown_chassis_module(db, chassis_module_name):
     fvs = {'admin_status': 'down'}
     config_db.set_entry('CHASSIS_MODULE', chassis_module_name, fvs)
     if chassis_module_name.startswith("FABRIC-CARD"):
-        if not get_config_module_state_timeout(ctx, db, chassis_module_name, 'down'):
-            fabric_module_set_admin_status(chassis_module_name, 'down')
+        if not check_config_module_state_with_timeout(ctx, db, chassis_module_name, 'down'):
+            fabric_module_set_admin_status(db, chassis_module_name, 'down')
 
 #
 # 'startup' subcommand ('config chassis_modules startup ...')
@@ -117,13 +136,13 @@ def startup_chassis_module(db, chassis_module_name):
     config_db = db.cfgdb
     ctx = click.get_current_context()
 
-    #To avoid duplicate operation
+    # To avoid duplicate operation
     if get_config_module_state(db, chassis_module_name) == 'up':
         click.echo("Module {} is already set to up state".format(chassis_module_name))
         return
 
     click.echo("Starting up chassis module {}".format(chassis_module_name))
     config_db.set_entry('CHASSIS_MODULE', chassis_module_name, None)
     if chassis_module_name.startswith("FABRIC-CARD"):
-        if not get_config_module_state_timeout(ctx, db, chassis_module_name, 'up'):
-            fabric_module_set_admin_status(chassis_module_name, 'up')
+        if not check_config_module_state_with_timeout(ctx, db, chassis_module_name, 'up'):
+            fabric_module_set_admin_status(db, chassis_module_name, 'up')

tests/chassis_modules_test.py

@@ -7,6 +7,8 @@
 import tests.mock_tables.dbconnector
 from utilities_common.db import Db
 from .utils import get_result_and_return_code
+from unittest import mock
+sys.modules['clicommon'] = mock.Mock()
 
 show_linecard0_shutdown_output="""\
 LINE-CARD0 line-card 1 Empty down LC1000101
@@ -16,11 +18,11 @@
 LINE-CARD0 line-card 1 Empty up LC1000101
 """
 
-show_fabriccard0_shutdown_output="""\
+show_fabriccard0_shutdown_output = """\
 FABRIC-CARD0 fabric-card 17 Online down FC1000101
 """
 
-show_fabriccard0_startup_output="""\
+show_fabriccard0_startup_output = """\
 FABRIC-CARD0 fabric-card 17 Online up FC1000101
 """
 
@@ -122,6 +124,11 @@
 Linecard4|Asic2|PortChannel0001         2           22  Linecard4|Asic2|Ethernet29, Linecard4|Asic2|Ethernet30
 """
 
+
+def mock_run_command_side_effect(*args, **kwargs):
+    return '', 0
+
+
 class TestChassisModules(object):
     @classmethod
     def setup_class(cls):
@@ -196,21 +203,45 @@ def test_config_shutdown_module(self):
         #db.get_data("CHASSIS_MODULE", "LINE-CARD0")
 
     def test_config_shutdown_module_fabric(self):
-        runner = CliRunner()
-        db = Db()
-        result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["shutdown"], ["FABRIC-CARD0"], obj=db)
-        print(result.exit_code)
-        print(result.output)
-        assert result.exit_code != 0
-
-        result = runner.invoke(show.cli.commands["chassis"].commands["modules"].commands["status"], ["FABRIC-CARD0"], obj=db)
-        print(result.exit_code)
-        print(result.output)
-        result_lines = result.output.strip('\n').split('\n')
-        assert result.exit_code == 0
-        header_lines = 2
-        result_out = " ".join((result_lines[header_lines]).split())
-        assert result_out.strip('\n') == show_fabriccard0_shutdown_output.strip('\n')
+        with mock.patch("utilities_common.cli.run_command",
+                        mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command:
+            runner = CliRunner()
+            db = Db()
+
+            chassisdb = db.db
+            chassisdb.connect("CHASSIS_STATE_DB")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "asic_id_in_module", "0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "asic_pci_address", "nokia-bdb:4:0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "name", "FABRIC-CARD0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "asic_id_in_module", "1")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "asic_pci_address", "nokia-bdb:4:1")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "name", "FABRIC-CARD0")
+            chassisdb.close("CHASSIS_STATE_DB")
+
+            result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["shutdown"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            assert result.exit_code == 0
+
+            result = runner.invoke(show.cli.commands["chassis"].commands["modules"].commands["status"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            result_lines = result.output.strip('\n').split('\n')
+            assert result.exit_code == 0
+            header_lines = 2
+            result_out = " ".join((result_lines[header_lines]).split())
+            assert result_out.strip('\n') == show_fabriccard0_shutdown_output.strip('\n')
+
+            fvs = {'admin_status': 'down'}
+            db.cfgdb.set_entry('CHASSIS_MODULE', "FABRIC-CARD0", fvs)
+            result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["shutdown"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            assert result.exit_code == 0
+            assert mock_run_command.call_count == 6
 
     def test_config_startup_module(self):
         runner = CliRunner()
@@ -229,20 +260,60 @@ def test_config_startup_module(self):
         assert result_out.strip('\n') == show_linecard0_startup_output.strip('\n')
 
     def test_config_startup_module_fabric(self):
-        runner = CliRunner()
-        db = Db()
-        result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["startup"], ["FABRIC-CARD0"], obj=db)
-        print(result.exit_code)
-        print(result.output)
-        assert result.exit_code == 0
-
-        result = runner.invoke(show.cli.commands["chassis"].commands["modules"].commands["status"], ["FABRIC-CARD0"], obj=db)
-        print(result.exit_code)
-        print(result.output)
-        result_lines = result.output.strip('\n').split('\n')
-        assert result.exit_code == 0
-        result_out = " ".join((result_lines[header_lines]).split())
-        assert result_out.strip('\n') == show_fabriccard0_startup_output.strip('\n')
+        with mock.patch("utilities_common.cli.run_command",
+                        mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command:
+            runner = CliRunner()
+            db = Db()
+
+            chassisdb = db.db
+            chassisdb.connect("CHASSIS_STATE_DB")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "asic_id_in_module", "0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "asic_pci_address", "nokia-bdb:4:0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic6", "name", "FABRIC-CARD0")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "asic_id_in_module", "1")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "asic_pci_address", "nokia-bdb:4:1")
+            chassisdb.set("CHASSIS_STATE_DB", "CHASSIS_FABRIC_ASIC_TABLE|asic7", "name", "FABRIC-CARD0")
+            chassisdb.close("CHASSIS_STATE_DB")
+
+            # FC is down and doing startup
+            fvs = {'admin_status': 'down'}
+            db.cfgdb.set_entry('CHASSIS_MODULE', "FABRIC-CARD0", fvs)
+
+            result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["startup"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            assert result.exit_code == 0
+
+            result = runner.invoke(show.cli.commands["chassis"].commands["modules"].commands["status"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            result_lines = result.output.strip('\n').split('\n')
+            assert result.exit_code == 0
+            result_out = " ".join((result_lines[header_lines]).split())
+            assert result_out.strip('\n') == show_fabriccard0_startup_output.strip('\n')
+            assert mock_run_command.call_count == 2
+
+            # FC is up and doing startup
+            fvs = {'admin_status': 'up'}
+            db.cfgdb.set_entry('CHASSIS_MODULE', "FABRIC-CARD0", fvs)
+
+            result = runner.invoke(config.config.commands["chassis"].commands["modules"].commands["startup"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            assert result.exit_code == 0
+
+            result = runner.invoke(show.cli.commands["chassis"].commands["modules"].commands["status"],
+                                   ["FABRIC-CARD0"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+            result_lines = result.output.strip('\n').split('\n')
+            assert result.exit_code == 0
+            result_out = " ".join((result_lines[header_lines]).split())
+            assert result_out.strip('\n') == show_fabriccard0_startup_output.strip('\n')
+            assert mock_run_command.call_count == 2
 
     def test_config_incorrect_module(self):
         runner = CliRunner()