YANG: remove uses clause handling, now part of sonic-yang-mgmt#3814
YANG: remove uses clause handling, now part of sonic-yang-mgmt#3814qiluo-msft merged 2 commits intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
kalash-nexthop
left a comment
There was a problem hiding this comment.
Just leaving a comment here that I accidentally hit approve button. Hopefully this will be enough for "unapproving" this. Although it doesn't matter since I am not an official reviewer anyway.
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
|
@adyeung , please help tag the YANG and UMF group. |
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352) fix formatting for lines touched (but not caused by me)
I forced the rebuild too early, the new sonic-buildimage assets aren't available yet so tests fail. Will retry in a few hours. |
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352) fix formatting for lines touched (but not caused by me)
sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
bcea496 to
b1ef321
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@qiluo-msft all test pass now since the artifacts from the merged PR sonic-net/sonic-buildimage#21907 are available. Can this be merged? |
|
FYI, all PRs are failing tests in sonic-utilities due to this not being merged! |
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352)
By words "Just leaving a comment here that I accidentally hit approve button. Hopefully this will be enough for "unapproving" this. Although it doesn't matter since I am not an official reviewer anyway."
Just dismissing "request changes" status.
|
The depenent PR sonic-net/sonic-buildimage#21907 is marked as 202505 not needed, removing 202505 request label |
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352) fix formatting for lines touched (but not caused by me)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352) fix formatting for lines touched (but not caused by me)
…nic-net#3814) sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. Signed-off-by: Brad House (@bradh352) fix formatting for lines touched (but not caused by me)
#### Why I did it When a uses clause imports a grouping, it was only processing leaf entries and ignoring leaf-list and choice clauses. That means that for instance in bgp route maps, route_map_in and route_map_out validations would fail. Honoring the uses `refine` clause is now also honored which is depended upon in sonic-utilities. This now precompiles the uses clause and integrates it as if the uses clause was not part of the schema as multiple end users were having to do this additional processing. This fixes that behavior and adds test cases to ensure it doesn't regress in the future. This is the proper fix, replacing #21078 that just worked around it. Removal of `uses` logic in sonic-utilities here: sonic-net/sonic-utilities#3814 Fixes sonic-net/sonic-buildimage#22382 ##### Work item tracking - Microsoft ADO **(number only)**: #### How I did it Added leaf-list lookup. #### How to verify it See test cases pass #### Which release branch to backport (provide reason below if selected) - [X] 202405 - [X] 202411 #### Tested branch (Please provide the tested image version) master as of 20250304 #### Description for the changelog sonic-yang-mgmt: uses clause with leaf-list, choice not honored #### Link to config_db schema for YANG module changes N/A #### A picture of a cute animal (not mandatory but encouraged) Signed-off-by: Brad House (@bradh352)
…ce not honored (#1731) #### Why I did it When a uses clause imports a grouping, it was only processing leaf entries and ignoring leaf-list and choice clauses. That means that for instance in bgp route maps, route_map_in and route_map_out validations would fail. Honoring the uses `refine` clause is now also honored which is depended upon in sonic-utilities. This now precompiles the uses clause and integrates it as if the uses clause was not part of the schema as multiple end users were having to do this additional processing. This fixes that behavior and adds test cases to ensure it doesn't regress in the future. This is the proper fix, replacing #21078 that just worked around it. Removal of `uses` logic in sonic-utilities here: sonic-net/sonic-utilities#3814 Fixes sonic-net/sonic-buildimage#22382 ##### Work item tracking - Microsoft ADO **(number only)**: #### How I did it Added leaf-list lookup. #### How to verify it See test cases pass #### Which release branch to backport (provide reason below if selected) - [X] 202405 - [X] 202411 #### Tested branch (Please provide the tested image version) master as of 20250304 #### Description for the changelog sonic-yang-mgmt: uses clause with leaf-list, choice not honored #### Link to config_db schema for YANG module changes N/A #### A picture of a cute animal (not mandatory but encouraged) Signed-off-by: Brad House (@bradh352)
Generic Configuration Updater (GCU) performance enhancements #### What I did 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: #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).
***Important***: Please review each commit (including commit message) individually. Looking at the patch-set as a whole may cause confusion. #### What I did 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/sonic-utilities#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). #### How I did it Profiling via cProfiler #### How to verify it Run sonic-utilities test cases, they still pass #### Previous command output (if the output of a command-line utility has changed) N/A #### New command output (if the output of a command-line utility has changed) N/A Fixes sonic-net/sonic-buildimage#22372
***Important***: Please review each commit (including commit message) individually. Looking at the patch-set as a whole may cause confusion. #### What I did 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: failure_prs.log sonic-yang-mgmt enhancements: sonic-net/sonic-buildimage#22254 failure_prs.log sonic-yang-mgmt parse uses/grouping: sonic-net/sonic-buildimage#21907 failure_prs.log sonic-utilities rely on sonic-yang-mgmt uses/grouping handling: sonic-net/sonic-utilities#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). #### How I did it Profiling via cProfiler #### How to verify it Run sonic-utilities test cases, they still pass #### Previous command output (if the output of a command-line utility has changed) N/A #### New command output (if the output of a command-line utility has changed) N/A Fixes sonic-net/sonic-buildimage#22372
…ce not honored (#1731) #### Why I did it When a uses clause imports a grouping, it was only processing leaf entries and ignoring leaf-list and choice clauses. That means that for instance in bgp route maps, route_map_in and route_map_out validations would fail. Honoring the uses `refine` clause is now also honored which is depended upon in sonic-utilities. This now precompiles the uses clause and integrates it as if the uses clause was not part of the schema as multiple end users were having to do this additional processing. This fixes that behavior and adds test cases to ensure it doesn't regress in the future. This is the proper fix, replacing #21078 that just worked around it. Removal of `uses` logic in sonic-utilities here: sonic-net/sonic-utilities#3814 Fixes sonic-net/sonic-buildimage#22382 ##### Work item tracking - Microsoft ADO **(number only)**: #### How I did it Added leaf-list lookup. #### How to verify it See test cases pass #### Which release branch to backport (provide reason below if selected) - [X] 202405 - [X] 202411 #### Tested branch (Please provide the tested image version) master as of 20250304 #### Description for the changelog sonic-yang-mgmt: uses clause with leaf-list, choice not honored #### Link to config_db schema for YANG module changes N/A #### A picture of a cute animal (not mandatory but encouraged) Signed-off-by: Brad House (@bradh352)
…c-net#3814) What I did sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities. Depends on sonic-net/sonic-buildimage#21907 Fixes sonic-net/sonic-buildimage#22382 How I did it Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses. How to verify it Run tests/cli_autogen_yang_parser_test.py after applying both this PR and sonic-net/sonic-buildimage#21907
What I did
sonic-yang-mgmt now integrates uses clauses as if a uses clause didn't exist. No need to have this complex handling in sonic-utilities.
Depends on sonic-net/sonic-buildimage#21907
Fixes sonic-net/sonic-buildimage#22382
How I did it
Besides removal of the code, the only test case changes are the removal of the 'grouping' clause as it is specific to 'uses' which is not relevant and some minor reordering of expected responses.
How to verify it
Run
tests/cli_autogen_yang_parser_test.pyafter applying both this PR and sonic-net/sonic-buildimage#21907Previous command output (if the output of a command-line utility has changed)
N/A
New command output (if the output of a command-line utility has changed)
N/A
Signed-off-by: Brad House (@bradh352)