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

[rimunagala]

Why I did it

The skip_sort_tables bypass was added to 202412 in March/April 2026 (#293, #300, #305) as an
interim mitigation, at a time when GCU apply-patch was too slow for the Fairwater DACL scenario
and was tripping Hw-proxy inband / NDM WCF timeouts.

That mitigation has since been overtaken by the actual performance work, which is now on this
branch:

Change	Landed on 202412 via
sonic-net/sonic-utilities#3831 — GCU performance enhancements	#254
sonic-net/sonic-utilities#4310 — GCU wheel	#352
sonic-net/sonic-utilities#4476 — cache loadData() calls	#352
sonic-net/sonic-utilities#4478 — batch leaf-list changes into a single REPLACE move	#352
sonic-net/sonic-utilities#4554 — init SonicDBConfig for multi-ASIC	#352

Beyond being redundant, the bypass actively suppresses 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 at all — so a healthy apply time on those patches cannot be used as evidence
that sorting performance is fixed. Removing it is a prerequisite for producing that evidence.

How I did it

Commit 1 — remove the skip-sort bypass

• drop the skip-sort block and import fnmatch from generic_updater.py
• delete generic_config_updater/skip_sort_tables.txt
• drop the corresponding setup.py package_data entry
• remove the three unit tests covering the bypass, and their helper

import jsonpatch and JsonChange are deliberately kept, even though #293 added them
alongside the bypass. They are not part of it: 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.

Commit 2 — backport sonic-net/sonic-utilities#4668

The patch sorter aborted with an unhandled ValueError ("'<x>' is not in list") when a patch
changed a create-only PORT field (e.g. lanes during a breakout) while that port was a member of
a multi-member leaf-list such as ACL_TABLE.ports. The exception escaped
RemoveCreateOnlyDependencyMoveValidator._validate_member and aborted the whole sort, failing the
apply and triggering auto-rollback. The fix treats an unresolvable reference in a simulated
intermediate config as an invalid move, so the DFS backtracks instead of aborting.

This is required here rather than optional: commit 1 routes ACL_TABLE.ports patches into the
sorter, and ACL_TABLE.ports is the exact leaf-list named in the upstream report. Without it, the
newly-enabled code path carries a known crash.

Cherry-picked cleanly from upstream 0552d0d24f67b287d74ef82ef7922f389f715c5f with no conflicts.
Re-authored to match the backport convention used by #352; original author credited via
Co-authored-by.

Commit 3 — adapt #4668's regression tests to the 202412 API

Commit 2 applied cleanly at the text level, but that turned out to be misleading: none of the five
tests it adds could actually execute on this branch. Two API differences exist between upstream
master and 202412, and git's 3-way merge did not surface either of them because the differing
signatures appear only as unchanged context lines.

	upstream master	202412	effect on the backported tests
JsonMoveGroup.__init__	takes a leading name argument	(move: JsonMove = None)	TypeError: takes from 1 to 2 positional arguments but 3 were given
MoveValidator.validate()	returns Tuple[bool, Optional[str]]	returns a plain bool	TypeError: 'bool' object is not subscriptable

Commit 3 drops the empty name argument at the five new JsonMoveGroup(...) call sites, and the
[0] subscript at the four affected validate(...) assertions. Both now match how every
pre-existing test in this file already calls those APIs.

The production hunks of #4668 required no adaptation — they reference JsonMoveGroup only in
type annotations, and do not depend on the validate() return shape. The behaviour being
backported is unchanged from upstream.

How to verify it

pytest tests/generic_config_updater/

This repository does not run unit tests in CI (the required checks are CodeQL, Semgrep and CLA),
so the suite was run manually in a container carrying the 202412 GCU test dependencies
(libyang 1.0.73, click==7.0, swsssdk, mockredispy, pyfakefs, responses, deepdiff).

	failed	passed
202412 baseline (4c36b195)	63	479
this branch (all three commits)	62	482

Regressions introduced: 0. All five of #4668's tests pass.

The failure count decreases by one because test_apply__all_ops_match_skip_sort__sort_skipped was
already failing on the 202412 baseline, and commit 1 deletes it. See the note below — that test
never actually worked.

Every one of the 63 baseline failures was traced to root cause rather than assumed to be noise.
They fall into four groups, none of which moves in either direction as a result of these commits:

count	cause	environment, or pre-existing defect on 202412?
57	libyang binding API skew	environment (my container)
3	TestSortAlgorithmFactory generator classification	pre-existing defect
2	main_test expects a --time option main.py does not define	pre-existing defect
1	broken mock_open in the skip-sort test	pre-existing defect (deleted by commit 1)

libyang (57). The sonic-yang-mgmt installed in my test image does import libyang as ly
(CESNET python3-libyang 3.1.0), so sy.root is a libyang.data.DContainer. 202412's
gu_common.py does import yang as ly and calls sy.root.tree_for() and
sy.root.find_path(xpath).data() — the older binding's API — giving
AttributeError: 'DContainer' object has no attribute 'tree_for'. The image happens to carry both
stacks (python3-yang 1.0.73 providing module yang, and python3-libyang 3.1.0 providing module
libyang). This is the skew that upstream #4118 ("remove direct dependency on libyang") resolves,
and #4118 is deliberately not part of this PR. A sonic-slave 202412 environment pins the older
pair, so these should not appear there. Purely an artefact of my environment; unrelated to this
change.

Generator classification (3). patch_sorter_test.py lists
RemoveCreateOnlyDependencyMoveGenerator under expected_generators, while patch_sorter.py
registers it under move_non_extendable_generators. The test file and the source disagree on this
branch today.

--time (2). main_test.py contains test_list_checkpoints_with_time and asserts args.time,
but the list-checkpoints subparser in main.py defines only -v/--verbose, so argparse exits
with unrecognized arguments: --time. The test file is ahead of the source on this branch.

Both of the above look like test files being taken from a newer revision than the corresponding
source change. They are untouched by this PR and are left as-is rather than fixed here, to keep the
diff to the stated scope. Happy to file them separately if useful.

Broken skip-sort mock (1) — worth calling out. The test removed by commit 1 did this:

mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect

This copies only side_effect from the helper and discards its return_value, which is the
configured file handle. The patched open() therefore returns a bare MagicMock, whose default
__iter__ is empty, so

skip_sort_tables = [line.strip() for line in f if line.strip()]

evaluates to [], the bypass is never entered, and sort is called — failing the assertion.
Confirmed directly: reproducing the pattern as written yields [], whereas
patch("builtins.open", new_callable=mock_open, read_data=...) yields the expected entry.

The same flaw makes the sibling test test_apply__no_ops_match_skip_sort__sort_not_skipped pass
vacuously — an empty skip list means sorting happens, which is exactly what that test asserts.
So the skip-sort feature has had no effective unit-test coverage since it was introduced, which went
unnoticed because this repository does not run the unit tests in CI. Commit 1 removes both tests
along with the feature, so nothing further is needed here; noting it only because it is relevant to
how much confidence the previous green-looking state deserved.

Other checks performed locally:

• no residual skip_sort / skip-sort reference anywhere in the tree
• generic_updater.py now differs from its pre-[generic_config_updater]: Skip patch sorting for ACL_TABLE/DENY_NEW_INGRESS_TABLE #293 state by exactly the two intentionally
  retained imports and nothing else
• the test-file deletion in commit 1 was produced by reverse-applying [generic_config_updater]: Skip patch sorting for ACL_TABLE/DENY_NEW_INGRESS_TABLE #293's own diff — 108
  deletions, 0 insertions
• py_compile clean; flake8 code profile unchanged from the 202412 baseline
  (generic_updater.py 16/16, generic_updater_test.py 30/30, patch_sorter.py 72/72)

Rollout note

Merging this removes the mitigation from every subsequent 202412 image build, not just from
piloted devices. The intent is to build a GCU addon container from this branch and pilot it on a
small number of 20241212.61 devices with the MRC isolation scenario owners before it reaches
wider fleet use.

Not included

• sonic-net/sonic-utilities#4335 (suboptimal plan for CreateOnly paths) — deliberately deferred.
  It is +74/-40 in patch_sorter.py and rewrites ~135 lines of expected-plan test fixtures, i.e.
  it intentionally changes generated plans. That deserves its own validation cycle rather than
  riding along here. Cherry-pick conflicts are small (3 hunks / 29 lines) if we decide to take it.
• sonic-net/sonic-utilities#4118 (remove direct libyang dependency) — dependency refactor,
  unrelated to this change.

Which release branch to port

• 202412

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.