Improved GCU's field validation logic for the WRED_PROFILE table#3910
Conversation
Signed-off-by: Mahdi Ramezani <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Could you add testcase to prevent regression? #Closed |
Signed-off-by: Mahdi Ramezani <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I added a few test cases for |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the field validation logic for the WRED_PROFILE table in the GCU by introducing a dedicated validator function and updating the corresponding configuration and tests. Key changes include:
- Adding the new wred_profile_config_update_validator function that reuses the common validation logic.
- Modifying the configuration file to remove the explicit profile name from the field lists.
- Updating and adding tests that cover multiple patch scenarios for the WRED_PROFILE table.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/generic_config_updater/field_operation_validator_test.py | New tests added to verify the updated validation logic for various WRED_PROFILE patch scenarios. |
| generic_config_updater/gcu_field_operation_validators.conf.json | Updated to remove profile-specific prefixes and to point to the new validator function. |
| generic_config_updater/field_operation_validators.py | Introduced new helper functions and the wred_profile_config_update_validator that reuses existing common validation logic. |
| "field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ], | ||
| "field_operation_validators": [ "generic_config_updater.field_operation_validators.wred_profile_config_update_validator" ], | ||
| "validator_data": { | ||
| "rdma_config_update_validator": { |
There was a problem hiding this comment.
Consider updating the validator_data key under WRED_PROFILE to reflect the new validator identifier (for example, renaming it from 'rdma_config_update_validator' to 'wred_profile_config_update_validator') for consistency and clarity.
| "rdma_config_update_validator": { | |
| "wred_profile_config_update_validator": { |
| # For paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112) | ||
| # If remove_port is True, then for paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from | ||
| # the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112) | ||
| def _get_fields_in_patch(): |
There was a problem hiding this comment.
Consider extracting _get_fields_in_patch as a shared helper function outside of the common validator to reduce code duplication and simplify maintenance if similar logic is needed in other validators.
Signed-off-by: Mahdi Ramezani <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
picked to 202412: Azure/sonic-utilities.msft#194 |
|
Cherry-pick PR to 202505: #3934 |
… a WRED profile named 'AZURE_LOSSLESS'. (#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
<!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Microsoft ADO ID: 32849826 Modified `test_ecn_config_update.py` so that it no longer requires a `WRED_PROFILE` named `AZURE_LOSSLESS`. ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [x] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### Approach #### What is the motivation for this PR? The `test_ecn_config_update.py` test fails on devices that do not have a `WRED_PROFILE` named `AZURE_LOSSLESS`. #### How did you do it? Instead of updating the `WRED_PROFILE` named `AZURE_LOSSLESS`, the test now updates all WRED profiles found in `CONFIG DB` and then verifies that these updates are applied to `ASIC DB`. **Note:** In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 #### How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named `AZURE_LOSSLESS`. The old version of the test failed, while the new version passed. ``` generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_min_threshold] PASSED [ 25%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_max_threshold] PASSED [ 50%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_drop_probability] PASSED [ 75%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_min_threshold,green_max_threshold,green_drop_probability] PASSED [100%] ``` #### Any platform specific information? N/A #### Supported testbed topology if it's a new test case? N/A ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A
…ly test failures (#494) <!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Microsoft ADO ID: 32849826 Modified `test_ecn_config_update.py` so that it no longer requires a `WRED_PROFILE` named `AZURE_LOSSLESS`. ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [x] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### Approach #### What is the motivation for this PR? The `test_ecn_config_update.py` test fails on devices that do not have a `WRED_PROFILE` named `AZURE_LOSSLESS`. #### How did you do it? Instead of updating the `WRED_PROFILE` named `AZURE_LOSSLESS`, the test now updates all WRED profiles found in `CONFIG DB` and then verifies that these updates are applied to `ASIC DB`. **Note:** In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 #### How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named `AZURE_LOSSLESS`. The old version of the test failed, while the new version passed. ``` generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_min_threshold] PASSED [ 25%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_max_threshold] PASSED [ 50%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_drop_probability] PASSED [ 75%] generic_config_updater/test_ecn_config_update.py::test_ecn_config_updates[replace-green_min_threshold,green_max_threshold,green_drop_probability] PASSED [100%] ``` #### Any platform specific information? N/A #### Supported testbed topology if it's a new test case? N/A ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A
… a WRED profile named 'AZURE_LOSSLESS'. (#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]> Signed-off-by: Guy Shemesh <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]> Signed-off-by: Aharon Malkin <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]> Signed-off-by: Guy Shemesh <[email protected]>
…ic-net#3910) What I did When using config apply-patch to update CONFIG_DB, GCU tries to validate the patch according to the rules in gcu_field_operation_validators.conf.json. As explained in Issue 22295, since fields for the WRED_PROFILE table in this file contain the profile name azure_lossless, patches that target other WRED profiles will be rejected with the following error: Error: Failed to apply patch on the following scopes: - localhost: Modification of WRED_PROFILE table is illegal- validating function generic_config_updater.field_operation_validators.rdma_config_update_validator returned False How I did it Modified gcu_field_operation_validators.conf.json to remove the profile name azure_lossless from WRED_PROFILE fields. In order to minimize side effect for other tables, we created a new validation function specifically for the WRED_PROFILE table named wred_profile_config_update_validator. This function modifies the path in patch by removing the table name (i.e., WRED_PROFILE), and then converting the whole path to lowercase. Then it verifies that the last part of this path matches a field defined in gcu_field_operation_validators.conf.json for the WRED_PROFILE table. For example, a path like /WRED_PROFILE/AZURE_LOSSY/green_min_threshold will be converted to azure_lossy/green_min_threshold, which will match green_min_threshold. How to verify it Store a JSON patch that targets a WRED profile different from azure_lossless in a file, and then run sudo config apply-patch {your_patch_name.json}. Test 1: Wrong WRED profile name Patch: [{"op": "replace", "path": "/WRED_PROFILE/DUMMY/green_min_threshold", "value": "1234"}] Result: Failed to apply patch due to: Validate json patch: [{"op": "replace", "path": "/WRED_PROFILE/DUMMY/green_min_threshold", "value": "1234"}] failed due to:member 'DUMMY' not found Test 2: Existing field name that is not in gcu_field_operation_validators.conf.json Patch: [{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/red_min_threshold", "value": "1234"}] Result: Error: Failed to apply patch on the following scopes: - localhost: Modification of WRED_PROFILE table is illegal- validating function generic_config_updater.field_operation_validators.wred_profile_config_update_validator returned False Test 3: Correct patch for 1 field Patch: [{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold", "value": "1234"}] Result: Patch applied successfully. $ ecnconfig -l Profile: AZURE_LOSSY ----------------------- ------- ... green_min_threshold 1234 ... ----------------------- ------- Test 4: Non-existent field name Patch: [{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/min_threshold", "value": "2468"}] Result: Error: Validate json patch: [{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/min_threshold", "value": "2468"}] failed due to:can't replace a non-existent object 'min_threshold' Test 5: Correct patch for multiple fields Patch: [{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold", "value": "2468"},{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_max_threshold", "value": "9876"},{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_drop_probability", "value": "10"}] Result: Patch applied successfully. $ ecnconfig -l Profile: AZURE_LOSSY ----------------------- ------- ... green_drop_probability 10 green_max_threshold 9876 green_min_threshold 2468 ... ----------------------- -------
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]> Signed-off-by: Guy Shemesh <[email protected]>
… a WRED profile named 'AZURE_LOSSLESS'. (sonic-net#19246) What is the motivation for this PR? The test_ecn_config_update.py test fails on devices that do not have a WRED_PROFILE named AZURE_LOSSLESS. How did you do it? Instead of updating the WRED_PROFILE named AZURE_LOSSLESS, the test now updates all WRED profiles found in CONFIG DB and then verifies that these updates are applied to ASIC DB. Note: In order for this test to pass, changes on the GCU side are also needed. Here is the PR in sonic-utilities for GCU changes: sonic-net/sonic-utilities#3910 How did you verify/test it? Tested on a Mellanox switch with 3 WRED profiles, none of which were named AZURE_LOSSLESS. The old version of the test failed, while the new version passed. Signed-off-by: Mahdi Ramezani <[email protected]> Signed-off-by: Yael Tzur <[email protected]>
What I did
When using
config apply-patchto update CONFIG_DB, GCU tries to validate the patch according to the rules ingcu_field_operation_validators.conf.json. As explained in Issue 22295, since fields for theWRED_PROFILEtable in this file contain the profile nameazure_lossless, patches that target other WRED profiles will be rejected with the following error:How I did it
gcu_field_operation_validators.conf.jsonto remove the profile nameazure_losslessfromWRED_PROFILEfields.WRED_PROFILEtable namedwred_profile_config_update_validator. This function modifies the path in patch by removing the table name (i.e.,WRED_PROFILE), and then converting the whole path to lowercase. Then it verifies that the last part of this path matches a field defined ingcu_field_operation_validators.conf.jsonfor theWRED_PROFILEtable. For example, a path like/WRED_PROFILE/AZURE_LOSSY/green_min_thresholdwill be converted toazure_lossy/green_min_threshold, which will matchgreen_min_threshold.How to verify it
Store a JSON patch that targets a WRED profile different from
azure_losslessin a file, and then runsudo config apply-patch {your_patch_name.json}.Test 1: Wrong WRED profile name
Patch:
[{"op": "replace", "path": "/WRED_PROFILE/DUMMY/green_min_threshold", "value": "1234"}]Result:
Test 2: Existing field name that is not in
gcu_field_operation_validators.conf.jsonPatch:
[{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/red_min_threshold", "value": "1234"}]Result:
Test 3: Correct patch for 1 field
Patch:
[{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold", "value": "1234"}]Result:
Test 4: Non-existent field name
Patch:
[{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/min_threshold", "value": "2468"}]Result:
Test 5: Correct patch for multiple fields
Patch:
[{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold", "value": "2468"},{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_max_threshold", "value": "9876"},{"op": "replace", "path": "/WRED_PROFILE/AZURE_LOSSY/green_drop_probability", "value": "10"}]Result:
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