[generic_config_updater]: Rewrite key-level removes that empty a ConfigDB table - #4856
Verma-Anukul wants to merge 2 commits into
Conversation
…igDB table Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@bhouse-nexthop @saiarcot895 @rimunagala @vaibhavhd @bingwang-ms @qiluo-msft Please review this change |
rimunagala
left a comment
There was a problem hiding this comment.
The intended pure key-removal behavior and protected-field validation look correct, and all CI checks pass. Requesting changes for the config replace regression noted inline; the RFC 6902 from-path case is non-blocking.
| # removes (for example /VLAN/Vlan10) without knowing they delete the last | ||
| # remaining entries. Only those key-level removes are rewritten to a | ||
| # table-level remove; other operations are left unchanged. | ||
| empty_tables = self.config_wrapper.get_empty_tables(target_config) |
There was a problem hiding this comment.
PatchApplier is also used by ConfigReplacer (config replace). If a replacement target contains "VLAN": {}, generate_patch() produces per-key removes; this branch rewrites and applies them as /VLAN, but ConfigReplacer then compares the absent table with the original empty-table target and raises after ConfigDB has already been modified. Before this change, EmptyTableError occurred before any writes. Please restrict this rewrite to the explicit apply-patch flow, or reject/canonicalize empty replacement targets before applying, and add a config replace regression test.
| parsed_ops = [] | ||
| ops_by_table = {table: [] for table in empty_tables} | ||
|
|
||
| for operation in patch: |
There was a problem hiding this comment.
Non-blocking edge: eligibility considers only path, but RFC 6902 move/copy can reference this table through from. For example, remove /VLAN/B, copy /VLAN/A -> /OTHER/Y, then remove /VLAN/A is rewritten to remove /VLAN before the copy, causing re-simulation to fail. Since GCU accepts move/copy and the PR states other patch shapes remain unchanged, please skip rewriting tables referenced through from, or otherwise preserve those dependencies.
What I did
Fixes #4852
config apply-patchnow accepts per-key removes that empty a ConfigDB table (for example/VLAN/Vlan3911) and rewrites them to a table-level remove. Automation can delete keys one by one without inspecting running config or special-casing the empty-table restriction.How I did it
After field-operation validation on the original simulated target,
PatchApplier.applycallsrewrite_patch_emptying_tables(). Onlyop=removewith path/TABLE/KEYis rewritten, and only when every operation on that emptied table is of that form. Other patch shapes are unchanged. Protected paths such as/LOOPBACK_INTERFACE/Loopback0are still validated on the original transition.How to verify it
Unit tests:
tests/generic_config_updater/gu_common_test.py::TestRewritePatchEmptyingTablestests/generic_config_updater/generic_updater_test.pyrewrite and Loopback0 testsDevice verification (VS):
vlan.json: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)