GCU sort: batch leaf-list changes into single REPLACE move (#4478) [202511] - #4814
Draft
rimunagala wants to merge 6 commits into
Draft
GCU sort: batch leaf-list changes into single REPLACE move (#4478) [202511]#4814rimunagala wants to merge 6 commits into
rimunagala wants to merge 6 commits into
Conversation
…gcu-perf Generic Configuration Updater (GCU) performance enhancements 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: sonic-net#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). (cherry picked from commit bd3de9d) Signed-off-by: rimunagala <rimunagala@microsoft.com>
…11-only) 202511-only hardening. Not required on master. PR sonic-net#3831 adds a `reload_config` gate to PathAddressing.find_ref_paths() so that bulk operations skip redundant sy.loadData() calls: if reload_config: sy.loadData(config) That gate was authored against master's tree, where PR sonic-net#4118 ("Remove direct dependency on libyang", 2026-04-27) had already deleted _get_inner_leaf_xpaths() and with it the only sy.root dereference in this function. On master, skipping the load is therefore harmless by construction. 202511 does not contain sonic-net#4118 and retains _get_inner_leaf_xpaths(), which does: nodes = sy.root.find_path(xpath).data() so on this branch find_ref_paths() depends on YANG data already being loaded. That requirement is satisfied today only by statement ordering: every caller (patch_sorter.py:757, 991, 1687) seeds reload_config=True and flips it to False only after the first call has loaded. sy is a process-lifetime singleton (create_sonic_yang_with_loaded_models() calls loadYangModel() once and never loadData()), so sy.root is None only before the first-ever load in the process. The invariant holds today, but it is implicit, undocumented, and not something master has any reason to preserve. Any future reordering, new caller, or new move generator that reaches a reload_config=False call site first would fail with AttributeError: 'NoneType' object has no attribute 'find_path'. Make the requirement explicit instead of relying on call order: if reload_config or sy.root is None: The sonic-net#4476 config-hash caching is preserved, so redundant loads are still skipped. No behavioural change is expected or observed. On cisco-8000 (56 ports, 6 ACL tables) the pre-fix and post-fix patched arms are equal within noise -- 0.74/0.75/14.31/2.00s vs 0.73/0.76/14.09/2.11s -- with identical move counts. 460 unit tests + 81 subtests pass. NOTE: this change is defensive. No production failure has been reproduced. An instrumented end-to-end sort() of a create-only PORT lanes change on a port referenced by an ACL ports leaf-list reaches the reload_config=False sites only after a load has already happened, and behaves identically with and without this change. The alternative -- backporting sonic-net#4118 -- was evaluated and rejected: it touches config/config_mgmt.py and sonic_package_manager/manager.py and adds a semgrep CI gate, all outside the GCU-only scope agreed for this backport. Signed-off-by: rimunagala <rimunagala@microsoft.com>
…es (sonic-net#4237) What I did: Issue: sonic-net#4221 Updated JsonMove._get_value to handle both string and integer indices when traversing lists in config data. Adjusted related unit tests to reflect the new behavior. How I did it: Modified the traversal logic to convert string tokens to integers when accessing lists, allowing both "1" and 1 as valid indices. Removed the test expecting a TypeError for integer indices and added assertions for both string and integer index access. How to verify it: Patched change in lab device, confirmed. admin@STR-SN5640-RDMA-1:~$ cat /usr/local/lib/python3.11/dist-packages/generic_config_updater/patch_sorter.py | grep -C 2 "int(token)" for token in tokens: if isinstance(config, list): token = int(token) config = config[token] admin@STR-SN5640-RDMA-1:~$ cat t_tc_to_queue_map_modify.json [ { "op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8" }, { "op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7" } ] admin@STR-SN5640-RDMA-1:~$ sudo config apply-patch -v t_tc_to_queue_map_modify.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}, {"op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Sorter - Strict: Validating patch is not making changes to tables without YANG models. Patch Sorter - Strict: Validating target config according to YANG models. Patch Sorter - Strict: Sorting patch updates. Patch Applier: The localhost patch was converted into 1 change: Patch Applier: localhost: applying 1 change in order: Patch Applier: * [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}, {"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}] Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch applied successfully. Also run the updated unit tests and all tests should pass, confirming the fix. Signed-off-by: Xincun Li <stli@microsoft.com> (cherry picked from commit 40260d5) Signed-off-by: rimunagala <rimunagala@microsoft.com>
GCU uses a very complex set of generators and validators. When a suboptimal plan is created, it can be hard to determine why the path was generated the way it was and how best to optimize the generators. This patch adds the ability to pass an IO object to all attempted paths with annotations such as `valid`, `invalid`, `recurse_reject` (source and target were already attempted), and `path_issue` (patch was good, but a generator further down the hierarchy failed). It will also list the generator used for each patch, and if `invalid` it will list the validator which failed it with a possibly extended error message. For callers to `config replace` or `config apply-patch` a new command line option of `--path-trace` which takes a filename to dump the json output. This patch also increases the diff output shown during test run failures to make it easier to debug issues based on logging generated via CI/CD. Signed-off-by: Brad House <bhouse@nexthop.ai> (cherry picked from commit 369e703) Signed-off-by: rimunagala <rimunagala@microsoft.com>
* GCU generates suboptimal plan for CreateOnly paths
When GCU hits a CreateOnly entry that has changed, it generates a suboptimal
plan. One example is a simple change of:
```
[{"op": "replace", "path": "/MIRROR_SESSION/EVERFLOW_TUNNEL/dst_ip", "value": "200.1.1.203"}]
```
Should generate an optimal plan of:
```
[
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}],
[{"op": "remove", "path": "/MIRROR_SESSION"}],
[{"op": "add", "path": "/MIRROR_SESSION", "value": {"EVERFLOW_TUNNEL": {"dscp": "8", "dst_ip": "200.1.1.203", "src_ip": "100.1.1.1", "ttl": "255", "type": "ERSPAN"}}}]
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}]
]
```
But instead generates this plan (which removes all ACLs):
```
[
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}],
[{"op": "remove", "path": "/ACL_RULE"}],
[{"op": "add", "path": "/ACL_RULE", "value": {"EVERFLOW|RULE_1": {"PRIORITY": "1000", "IP_TYPE": "IP", "MIRROR_INGRESS_ACTION": "EVERFLOW_TUNNEL"}}}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/L4_DST_PORT", "value": "22"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}],
[{"op": "remove", "path": "/ACL_RULE/DATAACL|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/PACKET_ACTION", "value": "DROP"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}],
[{"op": "remove", "path": "/ACL_RULE/DATAACL|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}],
[{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}],
[{"op": "remove", "path": "/MIRROR_SESSION"}],
[{"op": "add", "path": "/MIRROR_SESSION", "value": {"EVERFLOW_TUNNEL": {"dscp": "8", "dst_ip": "200.1.1.203", "src_ip": "100.1.1.1", "ttl": "255", "type": "ERSPAN"}}}],
[{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/L4_DST_PORT", "value": "22"}, {"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/PACKET_ACTION", "value": "DROP"}],
[{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}, {"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}]
]
```
Modified`RemoveCreateOnlyDependencyMoveGenerator`:
* it would previously short-circuit early due to only processing one child
leaf in the same table.
* it would previously attempt to iterate across all members of the table even
though there was a complete path list.
* it was missing logic to remove the create only path itself (and was relying
on extenders to do that which was inefficient and wouldn't generate the
right plan)
* when removing dependents it wasn't recursing to ensure it would remove
dependents of dependents
Since this generator is now full and doesn't rely on any extenders, it has
been moved to a non-extendable generator.
These changes caused some existing (suboptimal) plans that got generated to
change so those test cases have also been updated.
Added test case to validate this behavior and ensure it does not regress.
Signed-off-by: Brad House <bhouse@nexthop.ai>
* Update generic_config_updater/patch_sorter.py
Prevent possible infinite loop scenario Copilot identified,
however it shouldn't be possible given self.__get_path_count()
can't return 1 in that scenario to allow the loop to continue.
But hardening isn't a bad practice.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Brad House <bhouse@nexthop.ai>
* Update generic_config_updater/patch_sorter.py
Fix spelling / gramatical error caught by Copilot.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Brad House <bhouse@nexthop.ai>
* Code Review Comments
1. Add recursion depth protection in removing dependents because of
a concern of recursive dependencies, which isn't actually possible
with yang. None-the-less, implemented.
2. Remove a duplicate move that gets generated as when using it with a
Depth-first sorter its not necessary though could be on other sorters.
3. The old code depended on 3 levels of depth for the create only
leafs. Reworked the logic to not be dependent on depth.
4. __get_path_count() can no longer return a KeyError even though the
caller paths would make that impossible, but future use cases
may need it to not throw an exception when the path is not found.
Signed-off-by: Brad House <bhouse@nexthop.ai>
---------
Signed-off-by: Brad House <bhouse@nexthop.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
(cherry picked from commit 1580ccc)
Signed-off-by: rimunagala <rimunagala@microsoft.com>
…#4478) Add BulkLeafListMoveGenerator that produces a single REPLACE move for leaf-list fields whose items differ between current and target configs, instead of decomposing into N individual REMOVE/ADD moves. This is registered as a non-extendable generator (tried before individual moves in DFS). If validation fails, DFS falls through to per-item moves. Impact: For a 512-port ACL table where half the ports are removed, this reduces ~256 individual moves (each triggering 2 loadData calls at ~1.4s each = ~717s) to 1 move (1 loadData = ~1.4s). Conservative scope: - Only handles leaf-lists (lists of scalars, not lists of dicts) - Only replaces lists that exist in both current and target - Falls through to individual moves if the bulk replace fails validation Signed-off-by: vaibhavhd <vaibhav.dixit@microsoft.com> Co-authored-by: rookie-who <rookie-who@users.noreply.github.com> (cherry picked from commit bfc67f5) Signed-off-by: rimunagala <rimunagala@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
rimunagala
added
the
Tested for 202511 branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
Cherry-pick of #4478.
202511emits one move per leaf-list element, so shrinking a 512-memberACL_TABLE.portsproduces 510 moves. This batches them into a singleREPLACE.Must merge before #4476.
How I did it
Clean cherry-pick. +57 lines of source.
How to verify it
Sorter micro-benchmark, clean
202511vs the stack, in the faithful container:Before is clearly
O(N²); after is flat. Upstream #4478 claimed ~250×; we independently measure 300× on202511. On hardware, 19 moves → 1 and 57 moves → 3.Correctness held in every run: resulting
portsvalue identical on both arms.Backport notes
Clean. Attribution note: the wall-clock figures are stack-level, since #4478 (fewer moves) and #4476 (fewer parses per move) compound.
Stack-level verification
Measured on the assembled stack, in a container built from the genuine
202511sonic_yang_mgmtwheel (libyang 1.0.73, SWIGimport yang as ly):202511: 434 passed, 80 subtests)flake8 --diff, pre-commit hook semantics (4.0.1,--max-line-length=120)Not run:
sonic-mgmt. I looked into runningtests/generic_config_updater/test_apply_patch_perf.pyand it skips on every LAG topology - its fixture builds the port list fromPORTminusPORTCHANNEL_MEMBER, so on a T1 it finds 0 usable ports and bails withNeed at least 2 admin-up ports, have 0. Across the last 45 days of nightly runs, every202511execution of it landed on at1-*-lagbed and skipped, so no202511baseline for that test exists. It does run cleanly on t0 / m0 / mx / dualtor. Happy to book one of those beds and run it before merge - just ask.