[202405][GCU] Backport #3831 perf refactor + #4118/#4335/#4668 fixes (parts 1-8)

.github/workflows/semgrep.yml

@@ -18,4 +18,4 @@ jobs:
       - uses: actions/checkout@v3
       - run: semgrep ci
         env:
-           SEMGREP_RULES: "p/default r/python.lang.security.audit.dangerous-system-call-audit.dangerous-system-call-audit"
+           SEMGREP_RULES: "p/default r/python.lang.security.audit.dangerous-system-call-audit.dangerous-system-call-audit .semgrep/"

.semgrep/no-direct-libyang.yml

@@ -0,0 +1,21 @@
+rules:
+  - id: no-direct-libyang-import
+    patterns:
+      - pattern-either:
+          - pattern: import yang
+          - pattern: import yang as $X
+          - pattern: from yang import $X
+          - pattern: from yang import $X as $Y
+          - pattern: import libyang
+          - pattern: import libyang as $X
+          - pattern: from libyang import $X
+          - pattern: from libyang import $X as $Y
+    message: >-
+      Do not import libyang directly. sonic-utilities must go through
+      sonic_yang / sonic_yang_ext so that libyang API/ABI changes (libyang1
+      vs libyang2 vs libyang3) are isolated to sonic-yang-mgmt.
+    languages: [python]
+    severity: ERROR
+    paths:
+      exclude:
+        - tests/

README.md

@@ -34,6 +34,8 @@ Currently, this list of dependencies is as follows:
 - libyang_1.0.73_amd64.deb
 - libyang-cpp_1.0.73_amd64.deb
 - python3-yang_1.0.73_amd64.deb
+- libyang3_3.*_amd64.deb
+- python3-libyang_3.*_amd64.deb
 - redis_dump_load-1.1-py3-none-any.whl
 - sonic_py_common-1.0-py3-none-any.whl
 - sonic_config_engine-1.0-py3-none-any.whl

config/config_mgmt.py

@@ -8,7 +8,6 @@
 import shutil
 import syslog
 import tempfile
-import yang as ly
 from json import load
 from sys import flags
 from time import sleep as tsleep
@@ -35,8 +34,7 @@ class ConfigMgmt():
     to verify config for the commands which are capable of change in config DB.
     '''
 
-    def __init__(self, source="configDB", debug=False, allowTablesWithoutYang=True,
-                 sonicYangOptions=0, configdb=None):
+    def __init__(self, source="configDB", debug=False, allowTablesWithoutYang=True, configdb=None):
         '''
         Initialise the class, --read the config, --load in data tree.
 
@@ -55,7 +53,6 @@ def __init__(self, source="configDB", debug=False, allowTablesWithoutYang=True,
             self.configdbJsonOut = None
             self.source = source
             self.allowTablesWithoutYang = allowTablesWithoutYang
-            self.sonicYangOptions = sonicYangOptions
             self.configdb = configdb
 
             # logging vars
@@ -71,7 +68,7 @@ def __init__(self, source="configDB", debug=False, allowTablesWithoutYang=True,
         return
 
     def __init_sonic_yang(self):
-        self.sy = sonic_yang.SonicYang(YANG_DIR, debug=self.DEBUG, sonic_yang_options=self.sonicYangOptions)
+        self.sy = sonic_yang.SonicYang(YANG_DIR, debug=self.DEBUG)
         # load yang models
         self.sy.loadYangModel()
         # load jIn from config DB or from config DB json file.
@@ -291,8 +288,7 @@ def get_module_name(yang_module_str):
 
         # Instantiate new context since parse_module_mem() loads the module into context.
         sy = sonic_yang.SonicYang(YANG_DIR)
-        module = sy.ctx.parse_module_mem(yang_module_str, ly.LYS_IN_YANG)
-        return module.name()
+        return sy.load_module_str_name(yang_module_str)
 
 
 # End of Class ConfigMgmt

generic_config_updater/change_applier.py

@@ -5,11 +5,12 @@
 import importlib
 import os
 import tempfile
+import time
 from collections import defaultdict
 from swsscommon.swsscommon import ConfigDBConnector
 from sonic_py_common import multi_asic
 from .gu_common import GenericConfigUpdaterError, genericUpdaterLogging
-from .gu_common import get_config_db_as_json
+from .gu_common import JsonChange
 
 SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
 UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_services_validator.conf.json"
@@ -64,8 +65,8 @@ class DryRunChangeApplier:
     def __init__(self, config_wrapper):
         self.config_wrapper = config_wrapper
 
-    def apply(self, change):
-        self.config_wrapper.apply_change_to_config_db(change)
+    def apply(self, current_configdb: dict, change: JsonChange) -> dict:
+        return self.config_wrapper.apply_change_to_config_db(current_configdb, change)
 
     def remove_backend_tables_from_config(self, data):
         return data
@@ -137,25 +138,45 @@ def _report_mismatch(self, run_data, upd_data):
         log_error("run_data vs expected_data: {}".format(
             str(jsondiff.diff(run_data, upd_data))[0:40]))
 
-    def apply(self, change):
-        run_data = get_config_db_as_json(self.scope)
-        upd_data = prune_empty_table(change.apply(copy.deepcopy(run_data)))
+    def apply(self, current_configdb: dict, change: JsonChange) -> dict:
+        run_data = current_configdb
+        upd_data = prune_empty_table(change.apply(run_data, in_place=False))
         upd_keys = defaultdict(dict)
 
         for tbl in sorted(set(run_data.keys()).union(set(upd_data.keys()))):
             self._upd_data(tbl, run_data.get(tbl, {}), upd_data.get(tbl, {}), upd_keys)
 
         ret = self._services_validate(run_data, upd_data, upd_keys)
-        if not ret:
-            run_data = get_config_db_as_json(self.scope)
-            self.remove_backend_tables_from_config(upd_data)
-            self.remove_backend_tables_from_config(run_data)
-            if upd_data != run_data:
-                self._report_mismatch(run_data, upd_data)
-                ret = -1
-        if ret:
+        # The above function returns 0 on success as it uses shell return codes
+        if ret != 0:
             log_error("Failed to apply Json change")
-        return ret
+
+        # There was a sanity check in this position originally that appeared
+        # to be development-time code to ensure things were operating correctly.
+        # It would retrieve the configdb from Redis and perform transformation
+        # and comparison.  Its not possible for the configuration to not be what
+        # we expect since we have a known state we are mutating with a lock.
+        # That said we are leaving in the final configuration comparison in
+        # PatchApplier "just in case".
+        #
+        # However, this code did hide a pretty nasty race condition since there
+        # is no feedback loop for when config_db changes are actually consumed.
+        # This check would consume high CPU and would take a good amount of
+        # time (0.5s - 1s).
+        #
+        # The below sleep is functionally equivalent in terms of preventing the
+        # race condition (without the high CPU that might cause other control
+        # plane issues), but is of course not the proper fix.
+        #
+        # An upstream SONiC issue will be opened for the race condition, and
+        # until resolved leaving this comment in place for future reference.
+        time.sleep(1)
+
+        # Interestingly this function returns the updated data and doesn't
+        # propagate an error.  Maybe it should?  Or are exceptions thrown
+        # from _upd_data on failure?  We seem to intentionally only log on
+        # _services_validate()
+        return upd_data
 
     def remove_backend_tables_from_config(self, data):
         for key in self.backend_tables:

generic_config_updater/generic_updater.py

@@ -1,11 +1,12 @@
 import json
+import jsonpatch
 import jsonpointer
 import os
 import subprocess
 
 from enum import Enum
 from .gu_common import HOST_NAMESPACE, GenericConfigUpdaterError, EmptyTableError, ConfigWrapper, \
-                    DryRunConfigWrapper, PatchWrapper, genericUpdaterLogging
+                    DryRunConfigWrapper, JsonChange, PatchWrapper, genericUpdaterLogging
 from .patch_sorter import StrictPatchSorter, NonStrictPatchSorter, ConfigSplitter, \
                         TablesWithoutYangConfigSplitter, IgnorePathsFromYangConfigSplitter
 from .change_applier import ChangeApplier, DryRunChangeApplier
@@ -138,9 +139,10 @@ def apply(self, patch, sort=True):
         # Apply changes in order
         self.logger.log_notice(f"{scope}: applying {changes_len} change{'s' if changes_len != 1 else ''} " \
                                f"in order{':' if changes_len > 0 else '.'}")
+        current_config = old_config
         for change in changes:
             self.logger.log_notice(f"  * {change}")
-            self.changeapplier.apply(change)
+            current_config = self.changeapplier.apply(current_config, change)
 
         # Validate config updated successfully
         self.logger.log_notice(f"{scope}: verifying patch updates are reflected on ConfigDB.")