[sonic-package-manager] update FEATURE entries on upgrade (#1803)

Signed-off-by: Stepan Blyschak stepanb@nvidia.com

What I did
Implemented feature table update on package upgrade.
It could be that upgraded package includes timer service when old one does not.

How I did it
Upgrade logic added in manager.py
Use ConfigDBConnector for config modification as it simplifies config DB operations.

How to verify it
Upgrade package with new feature attributes.

sonic_package_manager/manager.py

@@ -534,7 +534,6 @@ def upgrade_from_source(self,
             )
 
         old_feature = old_package.manifest['service']['name']
-        new_feature = new_package.manifest['service']['name']
         old_version = old_package.manifest['package']['version']
         new_version = new_package.manifest['package']['version']
 
@@ -577,7 +576,12 @@ def upgrade_from_source(self,
                 source.install(new_package)
                 exits.callback(rollback(source.uninstall, new_package))
 
-                if self.feature_registry.is_feature_enabled(old_feature):
+                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'))
@@ -600,14 +604,27 @@ def upgrade_from_source(self,
                     self._get_installed_packages_and(old_package))
                 )
 
-                if self.feature_registry.is_feature_enabled(new_feature):
+                # 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 not skip_host_plugins:
                     self._install_cli_plugins(new_package)
-                    exits.callback(rollback(self._uninstall_cli_plugin, old_package))
+                    exits.callback(rollback(self._uninstall_cli_plugin, new_package))
 
                 self.docker.rmi(old_package.image_id, force=True)
 

sonic_package_manager/metadata.py

@@ -6,31 +6,12 @@
 import tarfile
 from typing import Dict
 
+from sonic_package_manager import utils
 from sonic_package_manager.errors import MetadataError
 from sonic_package_manager.manifest import Manifest
 from sonic_package_manager.version import Version
 
 
-def deep_update(dst: Dict, src: Dict) -> Dict:
-    """ Deep update dst dictionary with src dictionary.
-
-    Args:
-        dst: Dictionary to update
-        src: Dictionary to update with
-
-    Returns:
-        New merged dictionary.
-    """
-
-    for key, value in src.items():
-        if isinstance(value, dict):
-            node = dst.setdefault(key, {})
-            deep_update(node, value)
-        else:
-            dst[key] = value
-    return dst
-
-
 def translate_plain_to_tree(plain: Dict[str, str], sep='.') -> Dict:
     """ Convert plain key/value dictionary into
     a tree by spliting the key with '.'
@@ -62,7 +43,7 @@ def translate_plain_to_tree(plain: Dict[str, str], sep='.') -> Dict:
             continue
         namespace, key = key.split(sep, 1)
         res.setdefault(namespace, {})
-        deep_update(res[namespace], translate_plain_to_tree({key: value}))
+        utils.deep_update(res[namespace], translate_plain_to_tree({key: value}))
     return res
 
 

sonic_package_manager/service_creator/creator.py

@@ -11,6 +11,7 @@
 from prettyprinter import pformat
 from toposort import toposort_flatten, CircularDependencyError
 
+from sonic_package_manager import utils
 from sonic_package_manager.logger import log
 from sonic_package_manager.package import Package
 from sonic_package_manager.service_creator import ETC_SONIC_PATH
@@ -468,21 +469,14 @@ def set_initial_config(self, package):
         """
 
         init_cfg = package.manifest['package']['init-cfg']
+        if not init_cfg:
+            return
 
-        for tablename, content in init_cfg.items():
-            if not isinstance(content, dict):
-                continue
-
-            tables = self._get_tables(tablename)
-
-            for key in content:
-                for table in tables:
-                    cfg = content[key]
-                    exists, old_fvs = table.get(key)
-                    if exists:
-                        cfg.update(old_fvs)
-                    fvs = list(cfg.items())
-                    table.set(key, fvs)
+        for conn in self.sonic_db.get_connectors():
+            cfg = conn.get_config()
+            new_cfg = init_cfg.copy()
+            utils.deep_update(new_cfg, cfg)
+            conn.mod_config(new_cfg)
 
     def remove_config(self, package):
         """ Remove configuration based on init-cfg tables, so having
@@ -497,35 +491,13 @@ def remove_config(self, package):
         """
 
         init_cfg = package.manifest['package']['init-cfg']
+        if not init_cfg:
+            return
 
-        for tablename, content in init_cfg.items():
-            if not isinstance(content, dict):
-                continue
-
-            tables = self._get_tables(tablename)
-
-            for key in content:
-                for table in tables:
-                    table._del(key)
-
-    def _get_tables(self, table_name):
-        """ Return swsscommon Tables for all kinds of configuration DBs """
-
-        tables = []
-
-        running_table = self.sonic_db.running_table(table_name)
-        if running_table is not None:
-            tables.append(running_table)
-
-        persistent_table = self.sonic_db.persistent_table(table_name)
-        if persistent_table is not None:
-            tables.append(persistent_table)
-
-        initial_table = self.sonic_db.initial_table(table_name)
-        if initial_table is not None:
-            tables.append(initial_table)
-
-        return tables
+        for conn in self.sonic_db.get_connectors():
+            for table in init_cfg:
+                for key in init_cfg[table]:
+                    conn.set_entry(table, key, None)
 
     def _post_operation_hook(self):
         """ Common operations executed after service is created/removed. """

sonic_package_manager/service_creator/feature.py

@@ -16,6 +16,14 @@
 }
 
 
+def is_enabled(cfg):
+    return cfg.get('state', 'disabled').lower() == 'enabled'
+
+
+def is_multi_instance(cfg):
+    return str(cfg.get('has_per_asic_scope', 'False')).lower() == 'true'
+
+
 class FeatureRegistry:
     """ FeatureRegistry class provides an interface to
     register/de-register new feature persistently. """
@@ -27,51 +35,93 @@ def register(self,
                  manifest: Manifest,
                  state: str = 'disabled',
                  owner: str = 'local'):
+        """ Register feature in CONFIG DBs.
+
+        Args:
+            manifest: Feature's manifest.
+            state: Desired feature admin state.
+            owner: Owner of this feature (kube/local).
+        Returns:
+            None.
+        """
+
         name = manifest['service']['name']
-        for table in self._get_tables():
-            cfg_entries = self.get_default_feature_entries(state, owner)
-            non_cfg_entries = self.get_non_configurable_feature_entries(manifest)
+        db_connectors = self._sonic_db.get_connectors()
+        cfg_entries = self.get_default_feature_entries(state, owner)
+        non_cfg_entries = self.get_non_configurable_feature_entries(manifest)
 
-            exists, current_cfg = table.get(name)
+        for conn in db_connectors:
+            current_cfg = conn.get_entry(FEATURE, name)
 
             new_cfg = cfg_entries.copy()
             # Override configurable entries with CONFIG DB data.
-            new_cfg = {**new_cfg, **dict(current_cfg)}
+            new_cfg = {**new_cfg, **current_cfg}
             # Override CONFIG DB data with non configurable entries.
             new_cfg = {**new_cfg, **non_cfg_entries}
 
-            table.set(name, list(new_cfg.items()))
+            conn.set_entry(FEATURE, name, new_cfg)
 
     def deregister(self, name: str):
-        for table in self._get_tables():
-            table._del(name)
+        """ Deregister feature by name.
+
+        Args:
+            name: Name of the feature in CONFIG DB.
+        Returns:
+            None
+        """
+
+        db_connetors = self._sonic_db.get_connectors()
+        for conn in db_connetors:
+            conn.set_entry(FEATURE, name, None)
+
+    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
+        the new feature introduces a service timer or name of the service has
+        changed, but user configurable entries are not changed).
+
+        Args:
+            old_manifest: Old feature manifest.
+            new_manifest: New feature manifest.
+        Returns:
+            None
+        """
+
+        old_name = old_manifest['service']['name']
+        new_name = new_manifest['service']['name']
+        db_connectors = self._sonic_db.get_connectors()
+        non_cfg_entries = self.get_non_configurable_feature_entries(new_manifest)
+
+        for conn in db_connectors:
+            current_cfg = conn.get_entry(FEATURE, old_name)
+            conn.set_entry(FEATURE, old_name, None)
+
+            new_cfg = current_cfg.copy()
+            # Override CONFIG DB data with non configurable entries.
+            new_cfg = {**new_cfg, **non_cfg_entries}
+
+            conn.set_entry(FEATURE, new_name, new_cfg)
 
     def is_feature_enabled(self, name: str) -> bool:
         """ Returns whether the feature is current enabled
         or not. Accesses running CONFIG DB. If no running CONFIG_DB
         table is found in tables returns False. """
 
-        running_db_table = self._sonic_db.running_table(FEATURE)
-        if running_db_table is None:
+        conn = self._sonic_db.get_running_db_connector()
+        if conn is None:
             return False
 
-        exists, cfg = running_db_table.get(name)
-        if not exists:
-            return False
-        cfg = dict(cfg)
-        return cfg.get('state').lower() == 'enabled'
+        cfg = conn.get_entry(FEATURE, name)
+        return is_enabled(cfg)
 
     def get_multi_instance_features(self):
-        res = []
-        init_db_table = self._sonic_db.initial_table(FEATURE)
-        for feature in init_db_table.keys():
-            exists, cfg = init_db_table.get(feature)
-            assert exists
-            cfg = dict(cfg)
-            asic_flag = str(cfg.get('has_per_asic_scope', 'False'))
-            if asic_flag.lower() == 'true':
-                res.append(feature)
-        return res
+        """ Returns a list of features which run in asic namespace. """
+
+        conn = self._sonic_db.get_initial_db_connector()
+        features = conn.get_table(FEATURE)
+        return [feature for feature, cfg in features.items() if is_multi_instance(cfg)]
 
     @staticmethod
     def get_default_feature_entries(state=None, owner=None) -> Dict[str, str]: