[202412] Remove skip-sort bypass and backport GCU patch sorter crash fix (#4668)

generic_config_updater/generic_updater.py

@@ -1,4 +1,3 @@
-import fnmatch
 import json
 import jsonpatch
 import jsonpointer
@@ -129,31 +128,6 @@ def apply(self, patch, sort=True):
                                 which is not allowed in ConfigDb. \
                                 Table{'s' if len(empty_tables) != 1 else ''}: {empty_tables_txt}")
         # Generate list of changes to apply
-
-        # Skip sorting for tables listed in skip_sort_tables.txt
-        if sort:
-            skip_sort_tables_file = os.path.join(os.path.dirname(__file__), "skip_sort_tables.txt")
-            skip_sort_tables = []
-            if os.path.isfile(skip_sort_tables_file):
-                with open(skip_sort_tables_file) as f:
-                    skip_sort_tables = [line.strip() for line in f if line.strip()]
-                    self.logger.log_notice(f"{scope}: tables to skip sorting: {skip_sort_tables}")
-            if skip_sort_tables:
-                all_match = True
-                for operation in patch:
-                    op_path = operation.get("path", "")
-                    matched = any(fnmatch.fnmatch(op_path, pattern) for pattern in skip_sort_tables)
-                    if matched:
-                        self.logger.log_notice(
-                            f"{scope}: patch path {op_path} matches skip-sort pattern."
-                        )
-                    else:
-                        all_match = False
-                        break
-                if all_match:
-                    self.logger.log_notice(f"{scope}: all patch operations match skip-sort patterns, skipping sort.")
-                    sort = False
-
         if sort:
             self.logger.log_notice(f"{scope}: sorting patch updates.")
             changes = self.patchsorter.sort(patch)

generic_config_updater/patch_sorter.py

@@ -742,6 +742,7 @@ class RemoveCreateOnlyDependencyMoveValidator:
     def __init__(self, path_addressing):
         self.path_addressing = path_addressing
         self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter()
+        self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - RemoveCreateOnly")
 
     def validate(self, group: JsonMoveGroup, diff, simulated_config):
         # Note: group is not used by this validator
@@ -816,7 +817,28 @@ def _validate_member(self, tokens, member_name, current_config, target_config, s
                 return False
 
         member_path = f"/{table_to_check}/{member_name}"
-        for ref_path in self.path_addressing.find_ref_paths(member_path, simulated_config, reload_config=reload_config):
+        try:
+            ref_paths = self.path_addressing.find_ref_paths(
+                member_path, simulated_config, reload_config=reload_config)
+        except (ValueError, KeyError) as e:
+            # An unresolvable or malformed reference against the simulated intermediate config
+            # raises here. The motivating case: a create-only field change (e.g. a PORT breakout
+            # that rewrites lanes) has transiently removed this member from a multi-member leaf-list
+            # (e.g. ACL_TABLE.ports) while a leafref to it is still present, so the dangling leafref
+            # fails to resolve (list.index raises ValueError). Other resolution failures in the
+            # xpath<->configdb-path conversion (schema/key-count mismatches raise ValueError; a table
+            # with no YANG model raises KeyError) likewise indicate the reference cannot be resolved
+            # against this intermediate config. In every case the ordering is an invalid intermediate
+            # move, so reject it and let the sort backtrack rather than aborting the whole sort.
+            # Scope is deliberately limited to reference-resolution errors: a loadData failure
+            # (sonic_yang.SonicYangException) is not caught here because FullConfigMoveValidator has
+            # already loaded this config into the sy singleton, so find_ref_paths skips loadData.
+            self.logger.log_debug(
+                f"Rejecting move: reference resolution failed against simulated config "
+                f"for '{member_path}': {type(e).__name__}: {e}")
+            return False
+
+        for ref_path in ref_paths:
             if not self.path_addressing.has_path(current_config, ref_path):
                 return False
 
@@ -957,6 +979,7 @@ class NoDependencyMoveValidator:
     def __init__(self, path_addressing, config_wrapper):
         self.path_addressing = path_addressing
         self.config_wrapper = config_wrapper
+        self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - NoDependency")
 
     def validate(self, group: JsonMoveGroup, diff, simulated_config):
         reload_config = True
@@ -973,7 +996,8 @@ def __validate_move(self, move, diff, simulated_config, reload_config: bool = Tr
 
         if operation_type == OperationType.ADD:
             # For add operation, we check the simulated config has no dependencies between nodes under the added path
-            if not self._validate_paths_config([path], simulated_config, reload_config):
+            if not self._validate_paths_config([path], simulated_config, reload_config,
+                                               reject_on_unresolvable_ref=True):
                 return False
         elif operation_type == OperationType.REMOVE:
             # For remove operation, we check the current config has no dependencies between nodes under the removed path
@@ -1024,7 +1048,8 @@ def _validate_replace(self, move, diff, simulated_config):
         # so _currently_loaded_hash will match and find_ref_paths skips loadData.
         # Then validate deleted_paths against current_config (requires a fresh loadData).
         # This ordering gives 2 loadData calls instead of 3 for REPLACE operations.
-        if not self._validate_paths_config(added_paths, simulated_config, reload_config=True):
+        if not self._validate_paths_config(added_paths, simulated_config, reload_config=True,
+                                           reject_on_unresolvable_ref=True):
             return False
 
         if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True):
@@ -1094,11 +1119,31 @@ def _get_list_paths(self, current_list, target_list, tokens):
 
         return deleted_paths, added_paths
 
-    def _validate_paths_config(self, paths, config, reload_config: bool = True):
+    def _validate_paths_config(self, paths, config, reload_config: bool = True,
+                               reject_on_unresolvable_ref: bool = False):
         """
         validates all config under paths do not have config and its references
+
+        reject_on_unresolvable_ref: set only when 'config' is the transient simulated intermediate
+        state. A dangling leafref in that state (e.g. a create-only PORT change that has transiently
+        removed the port from a leaf-list such as ACL_TABLE.ports while a reference to it lingers)
+        makes find_ref_paths raise ValueError; a table with no YANG model makes it raise KeyError.
+        Either way it is an invalid intermediate move, so reject it and let the sort backtrack rather
+        than aborting. When 'config' is diff.current_config (a valid committed state) the flag stays
+        False: such an error there is genuine and must surface instead of being silently swallowed.
+        Scope is limited to reference-resolution errors; a loadData failure
+        (sonic_yang.SonicYangException) is not caught because the config is already loaded into the sy
+        singleton by FullConfigMoveValidator, so find_ref_paths skips loadData for it.
         """
-        refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config)
+        try:
+            refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config)
+        except (ValueError, KeyError) as e:
+            if reject_on_unresolvable_ref:
+                self.logger.log_debug(
+                    f"Rejecting move: reference resolution failed against simulated config "
+                    f"for {paths}: {type(e).__name__}: {e}")
+                return False
+            raise
         for ref in refs:
             for path in paths:
                 if ref.startswith(path):

setup.py

@@ -90,7 +90,7 @@
         'sonic_cli_gen',
     ],
     package_data={
-        'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json', 'skip_sort_tables.txt'],
+        'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json'],
         'show': ['aliases.ini'],
         'sonic_installer': ['aliases.ini'],
         'tests': ['acl_input/*',

tests/generic_config_updater/generic_updater_test.py

@@ -1,5 +1,4 @@
 import json
-import jsonpatch
 import os
 import shutil
 import unittest
@@ -46,113 +45,6 @@ def test_apply__no_errors__update_successful(self):
         patch_applier.patch_wrapper.verify_same_json.assert_has_calls(
             [call(Files.CONFIG_DB_AFTER_MULTI_PATCH, Files.CONFIG_DB_AFTER_MULTI_PATCH)])
 
-    @patch("builtins.open", create=True)
-    @patch("os.path.isfile", return_value=True)
-    def test_apply__all_ops_match_skip_sort__sort_skipped(self, mock_isfile, mock_open):
-        """All operations match skip_sort_tables.txt patterns,
-        sort should be skipped."""
-        from unittest.mock import mock_open as _mock_open
-        mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect
-
-        deny_patch = jsonpatch.JsonPatch([
-            {
-                "op": "add",
-                "path": "/ACL_TABLE/FAIRWATER_DACL_MITIGATION_V2/ports",
-                "value": ["Ethernet0", "Ethernet1"]
-            }
-        ])
-        patch_applier = self.__create_patch_applier_for_patch(deny_patch)
-
-        patch_applier.apply(deny_patch)
-
-        patch_applier.patchsorter.sort.assert_not_called()
-
-    @patch("builtins.open", create=True)
-    @patch("os.path.isfile", return_value=True)
-    def test_apply__no_ops_match_skip_sort__sort_not_skipped(self, mock_isfile, mock_open):
-        """No operations match skip_sort_tables.txt patterns,
-        sort should still happen."""
-        from unittest.mock import mock_open as _mock_open
-        mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect
-
-        normal_patch = jsonpatch.JsonPatch([
-            {
-                "op": "add",
-                "path": "/ACL_TABLE/SOME_OTHER_TABLE/ports",
-                "value": ["Ethernet0"]
-            }
-        ])
-        changes = [Mock()]
-        patch_applier = self.__create_patch_applier_for_patch(
-            normal_patch, changes=changes
-        )
-
-        patch_applier.apply(normal_patch)
-
-        patch_applier.patchsorter.sort.assert_called_once()
-
-    @patch("builtins.open", create=True)
-    @patch("os.path.isfile", return_value=True)
-    def test_apply__partial_ops_match_skip_sort__sort_not_skipped(self, mock_isfile, mock_open):
-        """Only some operations match skip_sort_tables.txt patterns,
-        sort should still happen since not ALL ops match."""
-        from unittest.mock import mock_open as _mock_open
-        mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect
-
-        mixed_patch = jsonpatch.JsonPatch([
-            {
-                "op": "add",
-                "path": "/ACL_TABLE/FAIRWATER_DACL_MITIGATION_V2/ports",
-                "value": ["Ethernet0"]
-            },
-            {
-                "op": "add",
-                "path": "/ACL_TABLE/SOME_OTHER_TABLE/ports",
-                "value": ["Ethernet1"]
-            }
-        ])
-        changes = [Mock()]
-        patch_applier = self.__create_patch_applier_for_patch(
-            mixed_patch, changes=changes
-        )
-
-        patch_applier.apply(mixed_patch)
-
-        patch_applier.patchsorter.sort.assert_called_once()
-
-    def __create_patch_applier_for_patch(
-        self, patch_obj, changes=None
-    ):
-        """Helper to create PatchApplier with mocks for any patch."""
-        config_wrapper = Mock()
-        old_config = {"ACL_TABLE": {}}
-        new_config = {"ACL_TABLE": {
-            "DENY_NEW_INGRESS_TABLE": {
-                "ports": ["Ethernet0", "Ethernet1"]
-            }
-        }}
-        config_wrapper.get_config_db_as_json.side_effect = [
-            old_config, new_config
-        ]
-        config_wrapper.get_empty_tables.return_value = []
-
-        patch_wrapper = Mock()
-        patch_wrapper.simulate_patch.return_value = new_config
-        patch_wrapper.verify_same_json.return_value = True
-
-        if changes is None:
-            changes = [Mock()]
-        patchsorter = Mock()
-        patchsorter.sort.return_value = changes
-
-        changeapplier = Mock()
-        changeapplier.apply.return_value = new_config
-
-        return gu.PatchApplier(
-            patchsorter, changeapplier,
-            config_wrapper, patch_wrapper
-        )
-
     def __create_patch_applier(self,
                                changes=None,
                                valid_patch_does_not_produce_empty_tables=True,