Skip to content

Commit d3766b1

Browse files
mramezani95mramezani95
authored
Improved the GCU's config validation logic for the WRED_PROFILE table. (#194)
<!-- 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 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](sonic-net/sonic-buildimage#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 1. Modified `gcu_field_operation_validators.conf.json` to remove the profile name `azure_lossless` from `WRED_PROFILE` fields. 2. 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 ... ----------------------- ------- ``` #### 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
1 parent 63b6805 commit d3766b1

3 files changed

Lines changed: 136 additions & 23 deletions

File tree

‎generic_config_updater/field_operation_validators.py‎

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -64,31 +64,52 @@ def get_asic_name():
6464
return asic
6565

6666

67-
def rdma_config_update_validator(patch_element):
67+
def fields_match_exact(cleaned_patch_field, gcu_field):
68+
return cleaned_patch_field == gcu_field
69+
70+
71+
def fields_match_endswith(cleaned_patch_field, gcu_field):
72+
"""
73+
Checks if cleaned_patch_field ends with gcu_field
74+
"""
75+
field = cleaned_patch_field.split('/')[-1]
76+
return field == gcu_field
77+
78+
79+
# If exact_field_match is True, then each field in GCU_TABLE_MOD_CONF_FILE must match exactly with
80+
# the corresponding cleaned field from the patch.
81+
# If exact_field_match is False, then each field in GCU_TABLE_MOD_CONF_FILE must appear at the end of
82+
# the corresponding cleaned fields from the patch.
83+
# remove_port controls the behavior of the _get_fields_in_patch function.
84+
def rdma_config_update_validator_common(patch_element, exact_field_match=False, remove_port=False):
6885
asic = get_asic_name()
6986
if asic == "unknown":
7087
return False
7188
version_info = device_info.get_sonic_version_info()
7289
build_version = version_info.get('build_version')
7390
version_substrings = build_version.split('.')
7491
branch_version = None
75-
92+
7693
for substring in version_substrings:
7794
if substring.isdigit() and re.match(r'^\d{8}$', substring):
7895
branch_version = substring
79-
96+
8097
path = patch_element["path"]
8198
table = jsonpointer.JsonPointer(path).parts[0]
82-
99+
83100
# Helper function to return relevant cleaned paths, considers case where the jsonpatch value is a dict
84-
# 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)
101+
# If remove_port is True, then for paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from
102+
# the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112)
85103
def _get_fields_in_patch():
86104
cleaned_fields = []
87105

88106
field_elements = jsonpointer.JsonPointer(path).parts[1:]
89-
cleaned_field_elements = [elem for elem in field_elements if not any(char.isdigit() for char in elem)]
107+
if remove_port:
108+
cleaned_field_elements = [elem for elem in field_elements if not any(char.isdigit() for char in elem)]
109+
else:
110+
cleaned_field_elements = field_elements
90111
cleaned_field = '/'.join(cleaned_field_elements).lower()
91-
112+
92113

93114
if 'value' in patch_element.keys() and isinstance(patch_element['value'], dict):
94115
for key in patch_element['value']:
@@ -100,7 +121,7 @@ def _get_fields_in_patch():
100121
cleaned_fields.append(cleaned_field)
101122

102123
return cleaned_fields
103-
124+
104125
if os.path.exists(GCU_TABLE_MOD_CONF_FILE):
105126
with open(GCU_TABLE_MOD_CONF_FILE, "r") as s:
106127
gcu_field_operation_conf = json.load(s)
@@ -109,24 +130,27 @@ def _get_fields_in_patch():
109130

110131
tables = gcu_field_operation_conf["tables"]
111132
scenarios = tables[table]["validator_data"]["rdma_config_update_validator"]
112-
113-
cleaned_fields = _get_fields_in_patch()
114-
for cleaned_field in cleaned_fields:
133+
cleaned_patch_fields = _get_fields_in_patch()
134+
fields_match = fields_match_exact if exact_field_match else fields_match_endswith
135+
for cleaned_patch_field in cleaned_patch_fields:
115136
scenario = None
116137
for key in scenarios.keys():
117-
if cleaned_field in scenarios[key]["fields"]:
118-
scenario = scenarios[key]
138+
for gcu_field in scenarios[key]["fields"]:
139+
if fields_match(cleaned_patch_field, gcu_field):
140+
scenario = scenarios[key]
141+
break
142+
if scenario:
119143
break
120-
144+
121145
if scenario is None:
122146
return False
123-
124-
if scenario["platforms"][asic] == "":
147+
148+
if not scenario["platforms"].get(asic): # None or empty string
125149
return False
126150

127151
if patch_element['op'] not in scenario["operations"]:
128152
return False
129-
153+
130154
if branch_version is not None:
131155
if asic in scenario["platforms"]:
132156
if branch_version < scenario["platforms"][asic]:
@@ -137,6 +161,14 @@ def _get_fields_in_patch():
137161
return True
138162

139163

164+
def rdma_config_update_validator(patch_element):
165+
return rdma_config_update_validator_common(patch_element, exact_field_match=True, remove_port=True)
166+
167+
168+
def wred_profile_config_update_validator(patch_element):
169+
return rdma_config_update_validator_common(patch_element)
170+
171+
140172
def read_statedb_entry(table, key, field):
141173
state_db = swsscommon.DBConnector("STATE_DB", 0)
142174
tbl = swsscommon.Table(state_db, table)

‎generic_config_updater/gcu_field_operation_validators.conf.json‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,19 +155,19 @@
155155
}
156156
},
157157
"WRED_PROFILE": {
158-
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ],
158+
"field_operation_validators": [ "generic_config_updater.field_operation_validators.wred_profile_config_update_validator" ],
159159
"validator_data": {
160160
"rdma_config_update_validator": {
161161
"ECN tuning": {
162162
"fields": [
163-
"azure_lossless/green_min_threshold",
164-
"azure_lossless/green_max_threshold",
165-
"azure_lossless/green_drop_probability"
163+
"green_min_threshold",
164+
"green_max_threshold",
165+
"green_drop_probability"
166166
],
167167
"operations": ["replace"],
168168
"platforms": {
169169
"spc1": "20181100",
170-
"spc2": "20191100",
170+
"spc2": "20191100",
171171
"spc3": "20220500",
172172
"spc4": "20221100",
173173
"spc5": "20241200",

‎tests/generic_config_updater/field_operation_validator_test.py‎

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,88 @@ def test_rdma_config_update_validator_spc_asic_invalid_op(self):
145145
def test_rdma_config_update_validator_spc_asic_other_field(self):
146146
patch_element = {"path": "/PFC_WD/Ethernet8/other_field", "op": "add", "value": "sample_value"}
147147
assert generic_config_updater.field_operation_validators.rdma_config_update_validator(patch_element) == False
148-
148+
149+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "20250530.12"}))
150+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="th5"))
151+
@patch("os.path.exists", mock.Mock(return_value=True))
152+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
153+
def test_wred_profile_config_update_validator_lossless(self):
154+
patch_element = {
155+
"path": "/WRED_PROFILE/AZURE_LOSSLESS/green_min_threshold",
156+
"op": "replace",
157+
"value": "1234"
158+
}
159+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == True
160+
161+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "20250530.12"}))
162+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="th5"))
163+
@patch("os.path.exists", mock.Mock(return_value=True))
164+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
165+
def test_wred_profile_config_update_validator_lossy(self):
166+
patch_element = {
167+
"path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold",
168+
"op": "replace",
169+
"value": "1234"
170+
}
171+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == True
172+
173+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "20250530.12"}))
174+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="th5"))
175+
@patch("os.path.exists", mock.Mock(return_value=True))
176+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
177+
def test_wred_profile_config_update_validator_invalid_field(self):
178+
patch_element = {
179+
"path": "/WRED_PROFILE/AZURE_LOSSY/invalid",
180+
"op": "replace",
181+
"value": "1234"
182+
}
183+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == False
184+
185+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="unknown"))
186+
def test_wred_profile_config_update_validator_unknown_asic(self):
187+
patch_element = {
188+
"path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold",
189+
"op": "replace",
190+
"value": "1234"
191+
}
192+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == False
193+
194+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "SONiC.20220530"}))
195+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="th5"))
196+
@patch("os.path.exists", mock.Mock(return_value=True))
197+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
198+
def test_wred_profile_config_update_validator_old_version(self):
199+
patch_element = {
200+
"path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold",
201+
"op": "replace",
202+
"value": "1234"
203+
}
204+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == False
205+
206+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "20250530.12"}))
207+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="th5"))
208+
@patch("os.path.exists", mock.Mock(return_value=True))
209+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
210+
def test_wred_profile_config_update_validator_invalid_op(self):
211+
patch_element = {
212+
"path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold",
213+
"op": "add",
214+
"value": "1234"
215+
}
216+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == False
217+
218+
@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"build_version": "20250530.12"}))
219+
@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="spc1"))
220+
@patch("os.path.exists", mock.Mock(return_value=True))
221+
@patch("builtins.open", mock_open(read_data='{"tables": {"WRED_PROFILE": {"validator_data": {"rdma_config_update_validator": {"ECN tuning": {"fields": ["green_min_threshold", "green_max_threshold", "green_drop_probability"], "operations": ["replace"], "platforms": {"th5": "20240500"}}}}}}}'))
222+
def test_wred_profile_config_update_validator_invalid_asic(self):
223+
patch_element = {
224+
"path": "/WRED_PROFILE/AZURE_LOSSY/green_min_threshold",
225+
"op": "replace",
226+
"value": "1234"
227+
}
228+
assert generic_config_updater.field_operation_validators.wred_profile_config_update_validator(patch_element) == False
229+
149230
def test_validate_field_operation_illegal__pfcwd(self):
150231
old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}}
151232
target_config = {"PFC_WD": {"GLOBAL": {}}}

0 commit comments

Comments
 (0)