Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCU] Prohibit removal of PFC_WD POLL_INTERVAL field #2545

Merged
merged 4 commits into from
Jan 19, 2023
Merged

[GCU] Prohibit removal of PFC_WD POLL_INTERVAL field #2545

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2929d01
Prohibit removal of PFCWD POLL_INTERVAL field
isabelmsft Dec 8, 2022
1fa9150
add UT
isabelmsft Dec 9, 2022
b4d9df0
add illegal removal
isabelmsft Dec 9, 2022
401fba7
change to exact match
isabelmsft Jan 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions generic_config_updater/generic_updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ def apply(self, patch):
# Generate target config
self.logger.log_notice("Simulating the target full config after applying the patch.")
target_config = self.patch_wrapper.simulate_patch(patch, old_config)

# Validate all JsonPatch operations on specified fields
self.logger.log_notice("Validating all JsonPatch operations are permitted on the specified fields")
self.config_wrapper.validate_field_operation(old_config, target_config)

# Validate target config does not have empty tables since they do not show up in ConfigDb
self.logger.log_notice("Validating target config does not have empty tables, " \
Expand Down
19 changes: 19 additions & 0 deletions generic_config_updater/gu_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
class GenericConfigUpdaterError(Exception):
pass

class IllegalPatchOperationError(ValueError):
pass

class EmptyTableError(ValueError):
pass

Expand Down Expand Up @@ -136,6 +139,22 @@ def validate_config_db_config(self, config_db_as_json):

return True, None

def validate_field_operation(self, old_config, target_config):
"""
Some fields in ConfigDB are restricted and may not allow third-party addition, replacement, or removal.
Because YANG only validates state and not transitions, this method helps to JsonPatch operations/transitions for the specified fields.
"""
patch = jsonpatch.JsonPatch.from_diff(old_config, target_config)

# illegal_operations_to_fields_map['remove'] yields a list of fields for which `remove` is an illegal operation
illegal_operations_to_fields_map = {'add':[],
'replace': [],
'remove': ['/PFC_WD/GLOBAL/POLL_INTERVAL']}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw in sonic-pfcwd.yang that POLL_INTERVAL will only be valid if its table's key is GLOBAL.

Is it guaranteed that GLOBAL will always have the POLL_INTERVAL field?
In that case, we can add a statement in YANG file if we want to avoid deletion of the POLL_INTERVAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neethajohn , can you help confirm if there will ever be a situation where GLOBAL will be present without POLL_INTERVAL field?

@wen587 If this approach is possible, then we must also prohibit removal of GLOBAL. So I think we also need to consider if PFC_WD must always have GLOBAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bingwang-ms , Could you help address the question of " if there will ever be a situation where GLOBAL will be present without POLL_INTERVAL field?"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bingwang-ms , Could you help address the question of " if there will ever be a situation where GLOBAL will be present without POLL_INTERVAL field?"

Yes, I think so. In the GLOBAL table, we can also have another entry named BIG_RED_SWITCH. If PFC watchdog is not enabled, the POLL_INTERVAL can be absent, while the BIG_RED_SWITCH can be present. So the GLOBAL table is present without POLL_INTERVAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bingwang-ms .

In this case, @wen587 , does implementation as-is look alright to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bingwang-ms, does that mean POLL_INTERVAL could be deleted if PFC watchdog state changes from enable to disable?
Hi @isabelmsft , if Bing confirmed it cannot be deleted. I think it is good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I confirm that.

for operation, field_list in illegal_operations_to_fields_map.items():
for field in field_list:
if any(op['op'] == operation and field in op['path'] for op in patch):
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
raise IllegalPatchOperationError("Given patch operation is invalid. Operation: {} is illegal on field: {}".format(operation, field))

def validate_lanes(self, config_db):
if "PORT" not in config_db:
return True, None
Expand Down
12 changes: 12 additions & 0 deletions tests/generic_config_updater/gu_common_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ def setUp(self):
self.config_wrapper_mock = gu_common.ConfigWrapper()
self.config_wrapper_mock.get_config_db_as_json=MagicMock(return_value=Files.CONFIG_DB_AS_JSON)

def test_validate_field_operation_legal(self):
old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}}
target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "40"}}}
config_wrapper = gu_common.ConfigWrapper()
config_wrapper.validate_field_operation(old_config, target_config)

def test_validate_field_operation_illegal(self):
old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": 60}}}
target_config = {"PFC_WD": {"GLOBAL": {}}}
config_wrapper = gu_common.ConfigWrapper()
self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config)

def test_ctor__default_values_set(self):
config_wrapper = gu_common.ConfigWrapper()

Expand Down