[code sync] Merge code from sonic-net/sonic-utilities:202511 to 202601 - #460
Merged
Merged
Conversation
mssonicbld
commented
Sep 14, 2026
mssonicbld
commented
Collaborator
…es (#4237) (#4811)
What I did:
Issue: #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.
(cherry picked from commit 40260d5)
Signed-off-by: rimunagala <rimunagala@microsoft.com>
Co-authored-by: Xincun Li <147451452+xincunli-sonic@users.noreply.github.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. (cherry picked from commit 369e703) Signed-off-by: rimunagala <rimunagala@microsoft.com> Co-authored-by: Brad House - Nexthop <bhouse@nexthop.ai>
* 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.
* 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.
* Update generic_config_updater/patch_sorter.py
Fix spelling / gramatical error caught by Copilot.
* 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.
---------
(cherry picked from commit 1580ccc)
Signed-off-by: rimunagala <rimunagala@microsoft.com>
Co-authored-by: Brad House - Nexthop <bhouse@nexthop.ai>
…4814) 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 (cherry picked from commit bfc67f5) Signed-off-by: rimunagala <rimunagala@microsoft.com> Co-authored-by: rookie-who <vaibhavcall@gmail.com>
…ANG parsing (#4476) (#4815) The two caches in this PR target different layers: 1. _currently_loaded_hash in SonicYangCfg.loadData() — skips re-parsing when the same config (by content hash) is loaded consecutively. This helps when multiple validators call loadData() with identical config within a single move validation. 2. _validate_config_cache in ConfigWrapper.validate_config_db() — caches the validation result for a given config hash, so if the same config state is validated again later, it returns the cached pass/fail without calling loadData() at all. Per-operation analysis Operation Helps? Why REMOVE (individual) ❌ No Each DFS step removes one item → unique config at each step. Neither cache hits because every state is different. ADD⚠️ Marginal Typically 1 move → few loadData calls total. Cache might save 1 call if FullConfigMoveValidator and NoDependencyMoveValidator validate the same state. REPLACE (scalar)⚠️ Marginal Same as ADD — few moves, small absolute savings. REMOVE (batched via #4478) ✅ Yes #4478 collapses N individual REMOVEs into 1 bulk REPLACE move. That single move still triggers multiple validator calls with the same config. Cache deduplicates those, reducing loads/move from ~10.6x to ~7.7 --------- (cherry picked from commit 5d54e44) Signed-off-by: rimunagala <rimunagala@microsoft.com>
…a leaf-list (#4668) (#4816)
#### Why I did it
The GCU (`generic_config_updater`) patch sorter aborted with an unhandled `ValueError ("'<x>' is not in list")` when a `config replace` / apply-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`.
During move validation the sorter transiently removes the port from the leaf-list in the simulated intermediate config, but a still-present leafref to it then fails to resolve (`list.index` raises `ValueError`). The exception propagated out of `RemoveCreateOnlyDependencyMoveValidator._validate_member` and aborted the whole sort, so any GCU/config-replace-based breakout failed on a realistic switch config (auto-rollback). The native `config interface breakout` path avoids GCU and is unaffected.
#### How I did it
Treat an unresolvable reference in a simulated intermediate config as an *invalid move*: catch the error in `_validate_member` and return `False` so the DFS backtracks to a valid ordering instead of letting the exception propagate and abort the whole sort.
#### How to verify it
Added a regression test that reproduces the create-only PORT field change against a multi-member `ACL_TABLE.ports` leaf-list and asserts the move is rejected rather than raising:
```
python3 -m pytest tests/generic_config_updater/patch_sorter_test.py \
-k test_validate__unresolvable_ref_in_simulated_config__move_rejected
```
Verified the new test **fails** with the original code (raising the reported `ValueError`) and **passes** with the fix.
(cherry picked from commit 0552d0d24f67b287d74ef82ef7922f389f715c5f)
Signed-off-by: rimunagala <rimunagala@microsoft.com>
Co-authored-by: Brad House - Nexthop <bhouse@nexthop.ai>
…ConfigDB target (#4731) (#4817) Emptying a leaf-list through `config apply-patch` left an invalid value in CONFIG_DB and made the patch application fail. Removing the last item of e.g. BGP_ALLOWED_PREFIXES|DEPLOYMENT_ID|0 prefixes_v4 wrote `prefixes_v4@ = ""` and the command reported "after applying patch to config, there are still some parts not updated". Every later patch touching that field then failed YANG validation with `"" does not conform`. Root cause is that CONFIG_DB has no representation for an empty leaf-list. set_entry serializes [] to an empty string, which is read back as [""], so a target config asking for [] can never be satisfied and the final verify_same_json in PatchApplier.apply always mismatches. The canonical way to express "this leaf-list has no items" is for the field to be absent. Add PatchWrapper.simulate_config_db_patch, which simulates the patch and then drops empty leaf-lists, and use it everywhere the result is a ConfigDB target: PatchApplier.apply, all three sorters (StrictPatchSorter.sort, NonStrictPatchSorter.sort and PatchSorter.sort), and convert_config_db_patch_to_sonic_yang_patch. The sorter now produces a field level remove instead of a replace with an empty list, the change applier writes the entry without the field, and the readback matches the target. simulate_patch stays shape agnostic so the SonicYang format path is unaffected. In SonicYang json a list holds yang list entries rather than leaf-list items, so ConfigDB canonicalization must not be applied there. The guard in BulkLeafListMoveGenerator is kept as defence in depth so the generator never emits a bulk replace with an empty list. This intentionally reverses one assertion added by #4478. test_generate__leaf_list_all_items_removed__single_replace_move expected the generator to emit REPLACE with an empty list for an emptied leaf-list. That is the value CONFIG_DB cannot store, so the test is renamed to test_generate__leaf_list_all_items_removed__no_bulk_replace_move and now expects no bulk move, leaving the transition to the granular removal generators. The batching optimisation from #4478 is untouched for every non-empty leaf-list. Verified on docker-sonic-vs built from master using the reproduction steps from sonic-net/sonic-buildimage#27818. Before the change CONFIG_DB ended up with prefixes_v4@ = "" and apply-patch failed; after the change the field is removed and the patch applies successfully, including re-adding the leaf-list afterwards. The full generic_config_updater suite passes in the same container, 558 passed against a 554 passed baseline. (cherry picked from commit 4464c12241d3f87f8d43c71e60a57a438484aa0b) Signed-off-by: rimunagala <rimunagala@microsoft.com>
Collaborator
Author
|
/azp run |
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
2 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.