From 7ec5b305cfa57b7474dd6b1296883c8e9c34b1c9 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 18 Aug 2026 10:39:14 -0400 Subject: [PATCH 1/3] [202412] Remove skip-sort bypass from GCU patch application Removes the skip_sort_tables mechanism added by #293, #300 and #305 so that every patch goes through the normal sorting path. The bypass was an interim mitigation from March/April 2026, added while GCU apply-patch was too slow for the Fairwater DACL scenario. The underlying performance work has since landed on this branch: sonic-net/sonic-utilities#3831 (via #254) and sonic-net/sonic-utilities#4310, #4476, #4478 and #4554 (via #352). Beyond simply being redundant, the bypass actively obscures the signal we need. While it is in place, any patch whose operations all match /ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports never reaches the sorter, so healthy apply times on those patches cannot be used as evidence that sorting performance is fixed. Removed: - the skip-sort block and `import fnmatch` in generic_updater.py - generic_config_updater/skip_sort_tables.txt - the corresponding setup.py package_data entry - the three unit tests covering the bypass, and their helper Deliberately kept: `import jsonpatch` and `JsonChange`, which #293 also added. These are not part of the bypass. The `else:` branch of `if sort:` already referenced both symbols without importing them, so the sort=False path carried a latent NameError that #293 incidentally fixed. Removing them would reintroduce that bug. Verified: generic_updater.py now differs from its pre-#293 state by exactly those two imports and nothing else. Signed-off-by: rimunagala --- generic_config_updater/generic_updater.py | 26 ----- generic_config_updater/skip_sort_tables.txt | 1 - setup.py | 2 +- .../generic_updater_test.py | 108 ------------------ 4 files changed, 1 insertion(+), 136 deletions(-) delete mode 100644 generic_config_updater/skip_sort_tables.txt diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 1d9bce530..671213bc5 100644 --- a/generic_config_updater/generic_updater.py +++ b/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) diff --git a/generic_config_updater/skip_sort_tables.txt b/generic_config_updater/skip_sort_tables.txt deleted file mode 100644 index 600791bb1..000000000 --- a/generic_config_updater/skip_sort_tables.txt +++ /dev/null @@ -1 +0,0 @@ -/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports diff --git a/setup.py b/setup.py index 06380858e..423db53b5 100644 --- a/setup.py +++ b/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/*', diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index d41616e73..d86930062 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/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, From 3802c7695c2af01bf69d8dc38a649b8538985719 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 18 Aug 2026 12:24:35 -0400 Subject: [PATCH 2/3] [202412] Backport GCU patch sorter crash fix (#4668) Backport of sonic-net/sonic-utilities#4668, originally authored by Brad House - Nexthop , cherry-picked from upstream commit 0552d0d24f67b287d74ef82ef7922f389f715c5f. Applied cleanly with no conflicts. The patch sorter aborted with an unhandled ValueError ("'' is not in list") when a patch changed a create-only PORT field (for example `lanes` during a breakout) while that port was a member of a multi-member leaf-list such as ACL_TABLE.ports. During move validation the sorter transiently removes the port from the leaf-list in the simulated intermediate config, and a still-present leafref to it then fails to resolve. The exception escaped RemoveCreateOnlyDependencyMoveValidator._validate_member and aborted the entire sort, failing the apply and triggering auto-rollback. The fix treats an unresolvable reference in a simulated intermediate config as an invalid move: the error is caught in _validate_member and False is returned, so the DFS backtracks to a valid ordering instead of aborting. This is needed on 202412 because the preceding commit removes the skip-sort bypass, which means ACL_TABLE.ports patches now go through the sorter rather than around it. ACL_TABLE.ports is the exact leaf-list named in the upstream report, so without this fix that newly-enabled code path carries a known crash. Includes the upstream regression test: pytest tests/generic_config_updater/patch_sorter_test.py \ -k test_validate__unresolvable_ref_in_simulated_config__move_rejected Co-authored-by: Brad House - Nexthop Signed-off-by: rimunagala --- generic_config_updater/patch_sorter.py | 55 +++++- .../patch_sorter_test.py | 156 ++++++++++++++++++ 2 files changed, 206 insertions(+), 5 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 75f20849c..62fc7476d 100644 --- a/generic_config_updater/patch_sorter.py +++ b/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): diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index bb9e61ba7..e8454725d 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1222,6 +1222,78 @@ def setUp(self): path_addressing = ps.PathAddressing(config_wrapper) self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper) + def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self): + # A dangling leafref in the transient simulated intermediate config makes find_ref_paths + # raise ValueError. For a simulated-config-facing check (ADD here), the validator must reject + # the move (return False) so the sort backtracks, rather than letting the exception abort the + # whole sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet0' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the exact arguments so a + # regression swapping simulated_config <-> diff.current_config in the ADD branch is caught + # (simulated_config is value-distinct from current_config here: it carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self): + # REPLACE is the operation type for a PORT breakout (rewriting lanes while an ACL reference + # lingers). _validate_replace validates added_paths against the simulated intermediate config, + # so an unresolvable reference there (KeyError here, e.g. a table with no YANG model) must + # reject the move rather than aborting the sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the config argument so a + # regression routing REPLACE added_paths through diff.current_config instead of the simulated + # config is caught (the two configs are value-distinct: simulated carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__value_error_on_current_config__propagates(self): + # A check against diff.current_config (a valid committed state) is not simulated, so a + # ValueError from find_ref_paths signals a genuine schema error and must propagate rather + # than being silently swallowed as a move rejection. + current_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + target_config = {"PORT": {"Ethernet0": {}}} + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("Keys in configDb not matching keys in SonicYang")) + + with self.assertRaises(ValueError): + self.validator.validate(move, diff, simulated_config) + # Pin that the REMOVE validation ran against diff.current_config (unguarded), not + # simulated_config: the re-raise must be the guaranteed-current-config path, not a lucky + # raise that a config swap could mask. Configs are value-distinct (current has ACL_TABLE). + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], diff.current_config, reload_config=True) + def test_validate__add_full_config_has_dependencies__failure(self): # Arrange # CROPPED_CONFIG_DB_AS_JSON has dependencies between PORT and ACL_TABLE @@ -2024,6 +2096,90 @@ def test_validate__lane_replacement_change(self): with self.subTest(name=test_case_name): self._run_single_test(test_cases[test_case_name]) + def test_validate__unresolvable_ref_in_simulated_config__move_rejected(self): + # A create-only PORT field change (breakout rewriting lanes) can transiently + # remove the port from a multi-member leaf-list (e.g. ACL_TABLE.ports) in the simulated + # intermediate config while a leafref to it is still present. Resolving that dangling + # leafref raises ValueError ("'' is not in list"). The validator must treat this as + # an invalid intermediate move (return False) so the sort backtracks, rather than letting + # the exception propagate and abort the whole sort. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet280"]} + } + } + # The transient intermediate config the sorter is validating: Ethernet312's create-only + # 'lanes' field has already been rewritten toward the target, but the ACL_TABLE leafref to + # it has not been removed yet, so the reference is momentarily dangling. Resolving it raises + # ValueError. Kept value-distinct from current_config (different lanes) so the argument + # assertion below would catch a regression that passed current_config to find_ref_paths. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + + move = JsonMoveGroup("", Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet312' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + # Pin the assertion to the code path under test: the rejection must come from the guarded + # find_ref_paths call raising, resolved against the simulated (intermediate) config. Asserting + # the exact arguments also catches a regression that swapped simulated_config <-> current_config. + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + + def test_validate__keyerror_in_simulated_config__move_rejected(self): + # find_ref_paths also raises KeyError (not just ValueError) when a referenced path's table + # has no YANG model in the simulated intermediate config. The guard must treat that the same + # way as an unresolvable reference: reject the move so the sort backtracks, not abort it. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + # Value-distinct from current_config (lanes already rewritten toward the target) so the + # argument assertion below catches a regression that passed current_config instead. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + + move = JsonMoveGroup("", Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + def _run_single_test(self, test_case): # Arrange expected = test_case['expected'] From 2f1253c123ab625bdf07967d7a54e5a80ae3d2b9 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 18 Aug 2026 12:48:59 -0400 Subject: [PATCH 3/3] [202412] Adapt #4668 regression tests to the 202412 patch sorter API The backport in the preceding commit applied cleanly at the text level, but none of its five new tests could run on this branch. Two API differences exist between upstream master and 202412, both confined to the tests. 1. JsonMoveGroup signature TypeError: JsonMoveGroup.__init__() takes from 1 to 2 positional arguments but 3 were given Upstream's JsonMoveGroup takes a leading name argument, so the upstream tests construct it as JsonMoveGroup("", move). On 202412 the signature is still JsonMoveGroup(move: JsonMove = None). Dropped the empty name argument at the five new call sites. 2. Validator return type TypeError: 'bool' object is not subscriptable Upstream's MoveValidator.validate() returns a Tuple[bool, Optional[str]], so the upstream tests assert on validate(...)[0]. On 202412 validate() returns a plain bool. Dropped the [0] subscript at the four affected assertions. Both forms now match how every pre-existing test in this file already calls JsonMoveGroup and validate(). The production hunks of #4668 needed no adaptation: they reference JsonMoveGroup only in type annotations and do not depend on the validate() return shape. Verified in a container with the 202412 GCU test dependencies: - the five #4668 tests: 5 passed - full tests/generic_config_updater suite, 202412 baseline (4c36b195): 63 failed, 479 passed - full suite with these three commits applied: 62 failed, 482 passed - regressions introduced: 0 The failure count drops by one because the removed skip-sort test test_apply__all_ops_match_skip_sort__sort_skipped was already failing on the 202412 baseline. The remaining 62 failures are pre-existing on 202412 and unrelated to these commits. Signed-off-by: rimunagala --- .../patch_sorter_test.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index e8454725d..bf7574afe 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1233,7 +1233,7 @@ def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self): "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} } diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) simulated_config = move.apply(diff.current_config) self.validator.path_addressing.find_ref_paths = Mock( @@ -1242,7 +1242,7 @@ def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self): # Must not raise; the move is rejected so the DFS can backtrack. Pin the exact arguments so a # regression swapping simulated_config <-> diff.current_config in the ADD branch is caught # (simulated_config is value-distinct from current_config here: it carries the added ACL_TABLE). - self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.assertFalse(self.validator.validate(move, diff, simulated_config)) self.validator.path_addressing.find_ref_paths.assert_called_once_with( ["/ACL_TABLE"], simulated_config, reload_config=True) @@ -1257,7 +1257,7 @@ def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self): "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} } diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) simulated_config = move.apply(diff.current_config) self.validator.path_addressing.find_ref_paths = Mock( @@ -1266,7 +1266,7 @@ def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self): # Must not raise; the move is rejected so the DFS can backtrack. Pin the config argument so a # regression routing REPLACE added_paths through diff.current_config instead of the simulated # config is caught (the two configs are value-distinct: simulated carries the added ACL_TABLE). - self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.assertFalse(self.validator.validate(move, diff, simulated_config)) self.validator.path_addressing.find_ref_paths.assert_called_once_with( ["/ACL_TABLE"], simulated_config, reload_config=True) @@ -1280,7 +1280,7 @@ def test_validate__value_error_on_current_config__propagates(self): } target_config = {"PORT": {"Ethernet0": {}}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) simulated_config = move.apply(diff.current_config) self.validator.path_addressing.find_ref_paths = Mock( @@ -2133,14 +2133,14 @@ def test_validate__unresolvable_ref_in_simulated_config__move_rejected(self): } } - move = JsonMoveGroup("", Mock()) + move = JsonMoveGroup(Mock()) diff = ps.Diff(current_config, target_config) self.validator.path_addressing.find_ref_paths = Mock( side_effect=ValueError("'Ethernet312' is not in list")) # Must not raise; the move is rejected so the DFS can backtrack. - self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.assertFalse(self.validator.validate(move, diff, simulated_config)) # Pin the assertion to the code path under test: the rejection must come from the guarded # find_ref_paths call raising, resolved against the simulated (intermediate) config. Asserting # the exact arguments also catches a regression that swapped simulated_config <-> current_config. @@ -2169,14 +2169,14 @@ def test_validate__keyerror_in_simulated_config__move_rejected(self): } } - move = JsonMoveGroup("", Mock()) + move = JsonMoveGroup(Mock()) diff = ps.Diff(current_config, target_config) self.validator.path_addressing.find_ref_paths = Mock( side_effect=KeyError("ACL_TABLE")) # Must not raise; the move is rejected so the DFS can backtrack. - self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.assertFalse(self.validator.validate(move, diff, simulated_config)) self.validator.path_addressing.find_ref_paths.assert_called_once_with( "/PORT/Ethernet312", simulated_config, reload_config=True)