[code sync] Merge code from sonic-net/sonic-utilities:202605 to 202606 - #433
Merged
Conversation
mssonicbld
commented
Aug 14, 2026
mssonicbld
commented
Collaborator
… ERR log (#4778)
<!--
Please make sure you've read and understood our contributing guidelines:
https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md
** Make sure all your commits include a signature generated with `git commit -s` **
If this is a bug fix, make sure your description includes "closes #xxxx",
"fixes #xxxx" or "resolves #xxxx" so that GitHub automatically closes the related
issue when the PR is merged.
If you are adding/modifying/removing any command or utility script, please also
make sure to add/modify/remove any unit tests from the tests
directory as appropriate.
If you are modifying or removing an existing 'show', 'config' or 'sonic-clear'
subcommand, or you are adding a new subcommand, please make sure you also
update the Command Line Reference Guide (doc/Command-Reference.md) to reflect
your changes.
Please provide the following information:
-->
#### What I did
When GCU Patching tries to remove MGMT_INTERFACE/forced_mgmt_routes, it constructs the forced_mgmt_routes with empty list instead of deleting the 'forced_mgmt_routes' data field from the config Json data. When self.ctx.parse_data_mem(dumps(self.xlateJson) parses the json file which contains a empty list, it returns a failure. This PR modifies function _traverse_current_list() to remove the data field from entry when its value is an empty list. Fixes sonic-net/sonic-buildimage#23872
#### How I did it
MGMT_INTERFACE/eth0|<ipv4_address|ipv6_address>/forced_mgmt_routes attribute is a list in configuration entry. When the last element of this list is removed, the configuration key "forced_mgmt_routes" should be deleted from the entry. But the current GCU 'generator' constructs an empty list instead of deleting this configuration key and value from the configuration entry. An empty list value in the json file causes _yang.Context_parse_data_mem() returns error due to Yang model validation. This PR modifies the _traverse_current_list() check "if len(prt) <= 1" insteaf of "if len(ptr) == 0" to constructs a configuration path ([{"op": "remove", "path": "/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes"}] without index when the last element is removed. This change will generate the config DB without an empty list value in the configuration. And this will pass the Yang model validation when _yang.Context_parse_data_mem() parse the config json file.
#### How to verify it
1. Add a forced_mgmt_route :
```
Patch Content: [
{
"path": "/localhost/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes",
"value": [
"1::2:3:4/64"
],
"op": "add"
}
]
```
2. Remove forced_mgmt_route :
```
Patch Content: [
{
"path": "/localhost/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes",
"op": "remove"
}
]
```
3) Apply both patch files. No error shown
'''
admin@line-card25:~$ sudo config apply-patch eth0-add.json
Patch Applier: localhost: Patch application starting.
Patch Applier: localhost: Patch: [{"path": "/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes", "value": ["1::2:3:4/64"], "op": "add"}]
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 Applier: The localhost patch was converted into 0 changes.
Patch Applier: localhost: applying 0 changes in order.
Patch Applier: localhost: verifying patch updates are reflected on ConfigDB.
Patch Applier: localhost patch application completed.
Patch applied successfully.
admin@line-card25:~$ sudo config apply-patch eth0-del.json
Patch Applier: localhost: Patch application starting.
Patch Applier: localhost: Patch: [{"path": "/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes", "op": "remove"}]
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 Applier: The localhost patch was converted into 1 change:
Patch Applier: localhost: applying 1 change in order:
Patch Applier: * [{"op": "remove", "path": "/MGMT_INTERFACE/eth0|FC00:2::32~164/forced_mgmt_routes"}]
Patch Applier: localhost: verifying patch updates are reflected on ConfigDB.
Patch Applier: localhost patch application completed.
Patch applied successfully.
admin@line-card25:~$
'''
#### Which release branch to backport (provide reason below if selected)
<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->
- [ ] 202305
- [ ] 202311
- [ ] 202405
- [ ] 202411
- [ ] 202505
- [x] 202511
- [x] 202601
- [x] 202605
#### Tested branch (Please provide the tested image version)
<!--
- Please provide tested image version
- e.g.
- [x] 20201231.100
-->
- [x] 202511
- [x] 202601 private image
- [x] 202605, docker-sonic-vs `0b2faef94`
#### Test result
202605: PASS. Tested by @rimunagala, because 202605 needs this change as a prerequisite for #4731. Everything in this section is my own run on 202605; the 202511 and 202601 entries above are @mlok-nokia's.
Image: 202605 `docker-sonic-vs`, `branch: '202605'`, `commit_id: '0b2faef94'`.
Unit tests, `tests/generic_config_updater/`:
| 202605 tree | result |
| --- | --- |
| baseline, nothing picked | 549 passed |
| this PR only | 549 passed, no regression |
| #4731 only | 552 passed, 1 failed |
| this PR + #4731 | 553 passed, 0 failed |
The failure without this PR is `test_patch_sorter__remove_last_bgp_allowed_prefix__removes_field_instead_of_emptying_it`, which asserts the single change form that `len(ptr) <= 1` produces. So this PR has to reach 202605 before #4731, otherwise the cherry-pick of #4731 fails its own unit test in CI.
Functional check on 202605 using the removal from **How to verify it** above:
```
202605 without this PR:
Patch Applier: The localhost patch was converted into 2 changes:
Patch Applier: * [{"op": "remove", "path": ".../forced_mgmt_routes/0"}]
Patch Applier: * [{"op": "remove", "path": ".../forced_mgmt_routes"}]
202605 with this PR:
Patch Applier: The localhost patch was converted into 1 change:
Patch Applier: * [{"op": "remove", "path": ".../forced_mgmt_routes"}]
```
`forced_mgmt_routes` ends up absent from CONFIG_DB either way, so on this image the difference is the change count, and with this PR 202605 matches master. The cherry-pick applies cleanly on 202605.
Signed-off-by: Sonic Build Admin <sonicbld@microsoft.com>
#### Previous command output (if the output of a command-line utility has changed)
#### New command output (if the output of a command-line utility has changed)
… value to CONFIG_DB (#4777) #### What I did Fixes [sonic-net/sonic-buildimage#27818](sonic-net/sonic-buildimage#27818). 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 `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`. #### How I did it 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 that asks 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. The target config is always derived through `PatchWrapper`, so a ConfigDB specific simulation entry point was added there, and empty leaf-lists are dropped in it: ```python def simulate_config_db_patch(self, patch, config_db): return remove_empty_leaf_lists(self.simulate_patch(patch, config_db)) ``` This is used everywhere the result is meant to be a ConfigDB target: `PatchApplier.apply`, all three sorters (`StrictPatchSorter.sort`, `NonStrictPatchSorter.sort`, `PatchSorter.sort`) and `convert_config_db_patch_to_sonic_yang_patch`. The sorter then 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` itself stays shape agnostic, so the `--format SONICYANG` 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 that generator never emits a bulk `replace` with an empty list. > Note on scope: this only reproduces on images whose `sonic_yang` uses the libyang3 python bindings (`import libyang as ly`). That holds for master and for 202605, so both are affected. On branches still on libyang 1.0.73 (`import yang as ly`) the empty list is rejected during validation before the sorter runs, so the bad value never reaches CONFIG_DB. #### How to verify it Verified end to end on `docker-sonic-vs` built from master (`db028339e`) using the reproduction steps from the issue. Before, step 2 emits a `replace` with an empty list, `apply-patch` fails, and CONFIG_DB is left corrupted: ``` === STEP 2: remove the LAST prefixes_v4 item (empties the leaf-list) === apply-patch failed for localhost: localhost: after applying patch to config, there are still some parts not updated Error: Failed to apply patch on the following scopes: Patch Applier: The localhost patch was converted into 1 change: Patch Applier: * [{"op": "replace", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4", "value": []}] === STEP 3: CONFIG_DB state === {'prefixes_v4@': '', 'prefixes_v6@': 'fc01:20::/64'} ``` After, step 2 emits a field level `remove`, the patch applies, and the field is gone: ``` === STEP 2: remove the LAST prefixes_v4 item (empties the leaf-list) === Patch Applier: The localhost patch was converted into 1 change: Patch Applier: * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4"}] Patch applied successfully. === STEP 3: CONFIG_DB state === {'prefixes_v6@': 'fc01:20::/64'} ``` Re-adding the leaf-list afterwards works, so the config is fully recoverable: ``` === re-add via add === Patch applied successfully. {'prefixes_v6@': 'fc01:20::/64', 'prefixes_v4@': '10.40.0.0/16'} ``` Edge case where the leaf-list is the only field in the entry also applies cleanly, the entry is kept as `NULL: NULL`, which is the normal ConfigDB representation of an entry with no fields. Unit tests, run in the same container against `tests/generic_config_updater/`: | | result | | --- | --- | | master baseline | 554 passed, 81 subtests passed | | this PR | 558 passed, 81 subtests passed | Five tests are added and one pre-existing test is updated, so the net delta is four. The updated one is `test_generate__leaf_list_all_items_removed__single_replace_move` from #4478. It asserted that emptying a leaf-list produces a single bulk `replace` with an empty list, which is exactly the value CONFIG_DB cannot store, so that assertion has to be reversed. It 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, which is still covered by `test_generate__leaf_list_items_removed__single_replace_move` and `test_generate__leaf_list_items_added__single_replace_move`. #### Which release branch to backport <!-- - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [ ] 202111 - [ ] 202205 - [ ] 202211 - [ ] 202305 --> - [x] 202605 Updated following @dgsudharsan's request. 202605 ships the libyang3 based `sonic_yang`, so it is affected and the fix is needed there. Reproduction and fix are both confirmed on a real 202605 image, see **Test result**. The cherry-pick applies without conflict, but #4423 (`33c1c482`) should be picked first. It is a one line change to `_traverse_current_list` that 202605 never received, and without it the test added here fails. Details in **Test result**. #### Tested branch (Please provide the tested image version) - [x] master, `docker-sonic-vs` at `db028339e` - [x] 202605, `docker-sonic-vs` at `0b2faef94` #### Test result master: PASS. `docker-sonic-vs` at `db028339e`. Issue reproduction end to end plus `tests/generic_config_updater/` at 558 passed against a 554 passed baseline, as described in **How to verify it**. 202605: PASS, with #4423 as a prerequisite. Image is `docker-sonic-vs` built from 202605, reporting `branch: '202605'`, `commit_id: '0b2faef94'`. That image has `BulkLeafListMoveGenerator` from #4478 and the libyang3 binding (`sonic_yang.py:3:import libyang as ly`) and does not have this fix, so it is exposed. Stock 202605, bug present: ``` === STEP 2: remove the LAST prefixes_v4 item (empties the leaf-list) === apply-patch failed for localhost: localhost: after applying patch to config, there are still some parts not updated Patch Applier: The localhost patch was converted into 1 change: Patch Applier: * [{"op": "replace", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4", "value": []}] === STEP 3: CONFIG_DB state === {'prefixes_v4@': '', 'prefixes_v6@': 'fc01:20::/64'} ``` 202605 with this PR cherry-picked, bug fixed: ``` === STEP 2: remove the LAST prefixes_v4 item (empties the leaf-list) === Patch Applier: The localhost patch was converted into 2 changes: Patch Applier: * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/0"}] Patch Applier: * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4"}] Patch applied successfully. === STEP 3: CONFIG_DB state === {'prefixes_v6@': 'fc01:20::/64'} ``` CONFIG_DB ends correct, but the sorter takes two changes instead of one, because 202605 is missing #4423 which relaxed `_traverse_current_list` from `len(ptr) == 0` to `len(ptr) <= 1`. That difference is visible to the test added here, `test_patch_sorter__remove_last_bgp_allowed_prefix__removes_field_instead_of_emptying_it`, which asserts the single change form. `tests/generic_config_updater/` run in the same 202605 container: | 202605 tree | result | | --- | --- | | baseline, nothing picked | 549 passed, 81 subtests passed | | this PR only | 552 passed, 81 subtests passed, 1 failed | | #4423 + this PR | 553 passed, 81 subtests passed, 0 failed | With #4423 picked first, 202605 matches master exactly, a single field level `remove`: ``` === STEP 2: remove the LAST prefixes_v4 item (empties the leaf-list) === Patch Applier: The localhost patch was converted into 1 change: Patch Applier: * [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4"}] Patch applied successfully. === STEP 3: CONFIG_DB state === {'prefixes_v6@': 'fc01:20::/64'} ``` #4423 is already included in 202511 and has a cherry-pick PR created for msft-202601, but it was never taken to 202605, so picking it there is a gap fix in its own right. Both cherry-picks apply without conflict. #### Description for the changelog Fix `config apply-patch` leaving an invalid empty value in CONFIG_DB when a patch empties a leaf-list. Signed-off-by: Sonic Build Admin <sonicbld@microsoft.com> Co-authored-by: rimunagala <rimunagala@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to 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
1 participant
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.