[sonic-package-manager] implement blocking feature state change (soni…

…c-net#2035)

Signed-off-by: Stepan Blyschak [email protected]

Motivation for this change is to handle the TODO:

    # TODO: Replace with "config feature" command.
    # The problem with current "config feature" command
    # is that it is asynchronous, thus can't be used
    # for package upgrade purposes where we need to wait
    # till service stops before upgrading docker image.
    # It would be really handy if we could just call
    # something like: "config feature state <name> <state> --wait"
    # instead of operating on systemd service since
    # this is basically a duplicated code from "hostcfgd".
What I did
Implement blocking feature state change for CLI command and sonic-package-manager.

How I did it
Implemented a wait loop that blocks till feature reaches desired state.

How to verify it
Run UT, run on switch.

config/feature.py

@@ -1,8 +1,58 @@
 import sys
 
 import click
+from swsscommon import swsscommon
 from utilities_common.cli import AbbreviationGroup, pass_db
 
+SELECT_TIMEOUT = 1000  # ms
+
+
+def set_feature_state(cfgdb_clients, name, state, block):
+    """Enable/disable a feature"""
+    entry_data_set = set()
+
+    for ns, cfgdb in cfgdb_clients.items():
+        entry_data = cfgdb.get_entry('FEATURE', name)
+        if not entry_data:
+            raise Exception("Feature '{}' doesn't exist".format(name))
+        entry_data_set.add(entry_data['state'])
+
+    if len(entry_data_set) > 1:
+        raise Exception("Feature '{}' state is not consistent across namespaces".format(name))
+
+    if entry_data['state'] == "always_enabled":
+        raise Exception("Feature '{}' state is always enabled and can not be modified".format(name))
+
+    for ns, cfgdb in cfgdb_clients.items():
+        cfgdb.mod_entry('FEATURE', name, {'state': state})
+
+    if block:
+        db = swsscommon.DBConnector('STATE_DB', 0)
+        tbl = swsscommon.SubscriberStateTable(db, 'FEATURE')
+        sel = swsscommon.Select()
+
+        sel.addSelectable(tbl);
+
+        while True:
+            rc, _ = sel.select(SELECT_TIMEOUT)
+
+            if rc == swsscommon.Select.TIMEOUT:
+                continue
+            elif rc == swsscommon.Select.ERROR:
+                raise Exception('Failed to wait till feature reaches desired state: select() failed')
+            else:
+                feature, _, fvs = tbl.pop()
+                if feature != name:
+                    continue
+
+                actual_state = dict(fvs).get('state')
+
+                if actual_state == 'failed':
+                    raise Exception('Feature failed to be {}'.format(state))
+                elif actual_state == state:
+                    break
+
+
 #
 # 'feature' group ('config feature ...')
 #
@@ -17,7 +67,7 @@ def _update_field(db, name, fld, val):
         click.echo("Unable to retrieve {} from FEATURE table".format(name))
         sys.exit(1)
     db.cfgdb.mod_entry('FEATURE', name, { fld: val })
-    
+
 
 #
 # 'owner' command ('config feature owner ...')
@@ -49,29 +99,17 @@ def feature_fallback(db, name, fallback):
 @feature.command('state', short_help="Enable/disable a feature")
 @click.argument('name', metavar='<feature-name>', required=True)
 @click.argument('state', metavar='<state>', required=True, type=click.Choice(["enabled", "disabled"]))
+@click.option('--block', is_flag=True, help='Wait till operation is finished')
 @pass_db
-def feature_state(db, name, state):
+def feature_state(db, name, state, block):
     """Enable/disable a feature"""
-    entry_data_set = set()
 
-    for ns, cfgdb in db.cfgdb_clients.items():
-        entry_data = cfgdb.get_entry('FEATURE', name)
-        if not entry_data:
-            click.echo("Feature '{}' doesn't exist".format(name))
-            sys.exit(1)
-        entry_data_set.add(entry_data['state'])
-
-    if len(entry_data_set) > 1:
-        click.echo("Feature '{}' state is not consistent across namespaces".format(name))
+    try:
+        set_feature_state(db.cfgdb_clients, name, state, block)
+    except Exception as exception:
+        click.echo("{}".format(exception))
         sys.exit(1)
 
-    if entry_data['state'] == "always_enabled":
-        click.echo("Feature '{}' state is always enabled and can not be modified".format(name))
-        return
-
-    for ns, cfgdb in db.cfgdb_clients.items():
-        cfgdb.mod_entry('FEATURE', name, {'state': state})
-
 #
 # 'autorestart' command ('config feature autorestart ...')
 #
@@ -100,3 +138,4 @@ def feature_autorestart(db, name, autorestart):
 
     for ns, cfgdb in db.cfgdb_clients.items():
         cfgdb.mod_entry('FEATURE', name, {'auto_restart': autorestart})
+

show/feature.py

@@ -62,9 +62,10 @@ def feature_status(db, feature_name):
     if feature_name:
         key = "FEATURE|{}".format(feature_name)
         if feature_name in cfg_table:
-            data = cfg_table[feature_name]
+            data = {}
             if keys and (key in keys):
-                data.update(dbconn.get_all(dbconn.STATE_DB, key))
+                data = dbconn.get_all(dbconn.STATE_DB, key)
+            data.update(cfg_table[feature_name])
             ordered_data.append(data)
             fields = set(data.keys())
             names.append(feature_name)
@@ -73,10 +74,11 @@ def feature_status(db, feature_name):
             sys.exit(1)
     else:
         for name in natsorted(cfg_table.keys()):
-            data = cfg_table[name]
+            data = {}
             key = "FEATURE|{}".format(name)
             if keys and (key in keys):
-                data.update(dbconn.get_all(dbconn.STATE_DB, key))
+                data = dbconn.get_all(dbconn.STATE_DB, key)
+            data.update(cfg_table[name])
 
             fields = fields | set(data.keys())
             ordered_data.append(data)

sonic_package_manager/manager.py

@@ -43,10 +43,7 @@
 from sonic_package_manager.reference import PackageReference
 from sonic_package_manager.registry import RegistryResolver
 from sonic_package_manager.service_creator import SONIC_CLI_COMMANDS
-from sonic_package_manager.service_creator.creator import (
-    ServiceCreator,
-    run_command
-)
+from sonic_package_manager.service_creator.creator import ServiceCreator
 from sonic_package_manager.service_creator.feature import FeatureRegistry
 from sonic_package_manager.service_creator.sonic_db import (
     INIT_CFG_JSON,
@@ -302,6 +299,7 @@ def __init__(self,
         self.database = database
         self.metadata_resolver = metadata_resolver
         self.service_creator = service_creator
+        self.sonic_db = service_creator.sonic_db
         self.feature_registry = service_creator.feature_registry
         self.is_multi_npu = device_information.is_multi_npu()
         self.num_npus = device_information.get_num_npus()
@@ -479,17 +477,7 @@ def uninstall(self, name: str,
         # After all checks are passed we proceed to actual uninstallation
 
         try:
-            # Stop and disable the service.
-            # First to make sure we are not uninstalling
-            # package before the service has fully stopped
-            # since "config feature state" command is not blocking.
-            # Second, we make sure the service is in disabled state
-            # so that after reinstall and enablement hostcfgd will enable
-            # it and start it.
-            # TODO: once there is a way to block till hostcfgd will stop
-            # the service, replace it with new approach.
-            self._systemctl_action(package, 'stop')
-            self._systemctl_action(package, 'disable')
+            self._stop_feature(package)
             self._uninstall_cli_plugins(package)
             self.service_creator.remove(package, keep_config=keep_config)
             self.service_creator.generate_shutdown_sequence_files(
@@ -588,12 +576,8 @@ def upgrade_from_source(self,
                 feature_enabled = self.feature_registry.is_feature_enabled(old_feature)
 
                 if feature_enabled:
-                    self._systemctl_action(old_package, 'disable')
-                    exits.callback(rollback(self._systemctl_action,
-                                            old_package, 'enable'))
-                    self._systemctl_action(old_package, 'stop')
-                    exits.callback(rollback(self._systemctl_action,
-                                            old_package, 'start'))
+                    self._stop_feature(old_package)
+                    exits.callback(rollback(self._start_feature, old_package))
 
                 self.service_creator.remove(old_package, **service_remove_opts)
                 exits.callback(rollback(self.service_creator.create, old_package,
@@ -613,24 +597,16 @@ def upgrade_from_source(self,
                     self._get_installed_packages_and(old_package))
                 )
 
-                # If old feature was enabled, the user should have the new feature enabled as well.
-                if feature_enabled:
-                    self._systemctl_action(new_package, 'enable')
-                    exits.callback(rollback(self._systemctl_action,
-                                            new_package, 'disable'))
-                    self._systemctl_action(new_package, 'start')
-                    exits.callback(rollback(self._systemctl_action,
-                                            new_package, 'stop'))
-
-                # Update feature configuration after we have started new service.
-                # If we place it before the above, our service start/stop will
-                # interfere with hostcfgd in rollback path leading to a service
-                # running with new image and not the old one.
                 self.feature_registry.update(old_package.manifest, new_package.manifest)
                 exits.callback(rollback(
                     self.feature_registry.update, new_package.manifest, old_package.manifest)
                 )
 
+                # If old feature was enabled, the user should have the new feature enabled as well.
+                if feature_enabled:
+                    self._start_feature(new_package)
+                    exits.callback(rollback(self._stop_feature, new_package))
+
                 if not skip_host_plugins:
                     self._install_cli_plugins(new_package)
                     exits.callback(rollback(self._uninstall_cli_plugin, new_package))
@@ -947,33 +923,50 @@ def _get_installed_packages_except(self, package: Package) -> Dict[str, Package]
         packages.pop(package.name)
         return packages
 
-    # TODO: Replace with "config feature" command.
-    # The problem with current "config feature" command
-    # is that it is asynchronous, thus can't be used
-    # for package upgrade purposes where we need to wait
-    # till service stops before upgrading docker image.
-    # It would be really handy if we could just call
-    # something like: "config feature state <name> <state> --wait"
-    # instead of operating on systemd service since
-    # this is basically a duplicated code from "hostcfgd".
-    def _systemctl_action(self, package: Package, action: str):
-        """ Execute systemctl action for a service supporting
-        multi-asic services. """
-
-        name = package.manifest['service']['name']
-        host_service = package.manifest['service']['host-service']
-        asic_service = package.manifest['service']['asic-service']
-        single_instance = host_service or (asic_service and not self.is_multi_npu)
-        multi_instance = asic_service and self.is_multi_npu
+    def _start_feature(self, package: Package, block: bool = True):
+        """ Starts the feature and blocks till operation is finished if
+        block argument is set to True.
+
+        Args:
+            package: Package object of the feature that will be started.
+            block: Whether to block for operation completion.
+        """
+
+        self._set_feature_state(package, 'enabled', block)
+
+    def _stop_feature(self, package: Package, block: bool = True):
+        """ Stops the feature and blocks till operation is finished if
+        block argument is set to True.
+
+        Args:
+            package: Package object of the feature that will be stopped.
+            block: Whether to block for operation completion.
+        """
+
+        self._set_feature_state(package, 'disabled', block)
+
+    def _set_feature_state(self, package: Package, state: str, block: bool = True):
+        """ Sets the feature state and blocks till operation is finished if
+        block argument is set to True.
+
+        Args:
+            package: Package object of the feature that will be stopped.
+            state: Feature state to set.
+            block: Whether to block for operation completion.
+        """
 
         if in_chroot():
             return
 
-        if single_instance:
-            run_command(f'systemctl {action} {name}')
-        if multi_instance:
-            for npu in range(self.num_npus):
-                run_command(f'systemctl {action} {name}@{npu}')
+        # import from here otherwise this import will fail when executing
+        # sonic-package-manager from chroot environment as "config" package
+        # tries accessing database at import time.
+        from config.feature import set_feature_state
+
+        feature_name = package.manifest['service']['name']
+        log.info('{} {}'.format(state.replace('ed', 'ing').capitalize(), feature_name))
+        cfgdb_clients = {'': self.sonic_db.get_running_db_connector()}
+        set_feature_state(cfgdb_clients, feature_name, state, block)
 
     def _install_cli_plugins(self, package: Package):
         for command in SONIC_CLI_COMMANDS:

tests/feature_test.py

@@ -1,8 +1,13 @@
 import importlib
+import pytest
+
+from unittest import mock
+from contextlib import ExitStack
 
 from click.testing import CliRunner
 
 from utilities_common.db import Db
+from swsscommon import swsscommon
 
 show_feature_status_output="""\
 Feature     State           AutoRestart     SetOwner
@@ -282,6 +287,24 @@ def test_config_bgp_feature_state(self, get_cmd_module):
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_disabled_status_output
 
+    @pytest.mark.parametrize("actual_state,rc", [("disabled", 0), ("failed", 1)])
+    def test_config_bgp_feature_state_blocking(self, get_cmd_module, actual_state, rc):
+        (config, show) = get_cmd_module
+        db = Db()
+        runner = CliRunner()
+        with ExitStack() as es:
+            es.enter_context(mock.patch("swsscommon.swsscommon.DBConnector"))
+            mock_select = mock.Mock()
+            es.enter_context(mock.patch("swsscommon.swsscommon.Select", return_value=mock_select))
+            mock_tbl = mock.Mock()
+            es.enter_context(mock.patch("swsscommon.swsscommon.SubscriberStateTable", return_value=mock_tbl))
+            mock_select.select = mock.Mock(return_value=(swsscommon.Select.OBJECT, mock_tbl))
+            mock_tbl.pop = mock.Mock(return_value=("bgp", "", [("state", actual_state)]));
+            result = runner.invoke(config.config.commands["feature"].commands["state"], ["bgp", "disabled", "--block"], obj=db)
+        print(result.exit_code)
+        print(result.output)
+        assert result.exit_code == rc
+
     def test_config_snmp_feature_owner(self, get_cmd_module):
         (config, show) = get_cmd_module
         db = Db()
@@ -478,8 +501,8 @@ def test_config_bgp_feature_consistent_autorestart(self, get_cmd_module):
         print(result.exit_code)
         assert result.exit_code == 0
         assert result.output == show_feature_bgp_autorestart_output
- 
- 
+
+
     @classmethod
     def teardown_class(cls):
         print("TEARDOWN")