Merge pull request #3831 from bhouse-nexthop/bhouse-nexthop/gcu-perf

Generic Configuration Updater (GCU) performance enhancements

#### What I did

Generic Configuration Updater is extremely slow, using the python profiler it was possible to determine the worst offenders where changes could be made without affecting the overall algorithm and HLD design documentation.

Brief overview of changes:
* Prevent copy.deepcopy() calls where possible
* Don't run validation twice back to back
* Move configdb path <> xpath conversion logic to sonic-yang-mgmt where it belongs and enhance it to support schema conversion (not just data) and add caching.
* Sort table keys by the number of schema backlinks and must statements for the node to try better guess the right order of the patches to generate rather than doing it in alphabetical order which is likely to cause validation failures.
* Add ability to Group patches together in some commits where its known they will not cause issues, these are things like grouping parameter updates under the same key.
* When applying changes, do not re-read the configuration from redis twice between each applied patch (this is **extremely** slow, and actually hid a race condition).  We are mutating the configuration and a lock is held so we know the expected before and after.  There is still a final validation to ensure something didn't go sideways.

Dependencies:
 * sonic-yang-mgmt enhancements: sonic-net/sonic-buildimage#22254
 * sonic-yang-mgmt parse uses/grouping: sonic-net/sonic-buildimage#21907
 * sonic-utilities rely on sonic-yang-mgmt uses/grouping handling: #3814

Stats below ... (stats need both this and the sonic-utilities PR to be relevant)...

<ins>**Original Performance:**</ins>
Dry Run:
```
time sudo config replace -d ./config_db.json
...
real	2m51.588s
user	2m23.777s
sys	0m25.300s
```

Full:
```
time sudo config replace ./config_db.json
...
real	14m53.772s
user	12m2.376s
sys	2m8.908s
```

<ins>**With Patch**:</ins>
Dry Run:
```
time sudo config replace -d ./config_db.json
...
real	0m59.602s
user	0m56.434s
sys	0m2.110s
```

Full:
```
time sudo config replace ./config_db.json
...
real	1m54.303s
user	0m58.482s
sys	0m2.545s
```

So that's roughly 3x improvement for dry-run, and 7.5x improvement for full commit.  There is room for improvement on the full commit due to a `sleep(1)` being used between each patch because of a race condition found in the prior code (that was hidden due to a costly sanity check that has been removed).

Author: lguohan

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

@@ -142,9 +142,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.")