Fix JsonMove._get_value to support both string and integer list indices (#4237) [202511] - #4811
Draft
rimunagala wants to merge 3 commits into
Draft
Fix JsonMove._get_value to support both string and integer list indices (#4237) [202511]#4811rimunagala wants to merge 3 commits into
rimunagala wants to merge 3 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>
|
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
5 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 #4237 (Xincun Li).
JsonMove._get_valuemishandled integer list indices, so patches addressing a list positionally (e.g.TC_TO_QUEUE_MAP, or removing an ACL port by index) resolved incorrectly.How I did it
Clean cherry-pick. 3 lines of source plus a new 55-line test module.
How to verify it
Green in the full-suite run (460 passed). On hardware, removing 10 leaf-list members by index went from 10 moves / 6.40 s to 1 move / 0.76 s — though that figure is stack-level, since #4478 and #4476 act on the same path.
Backport notes
Clean; no conflicts.
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.