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.

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] Add PFC_WD RDMA validator #2619

Merged
merged 22 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion generic_config_updater/change_applier.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from .gu_common import genericUpdaterLogging

SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_config_updater.conf.json"
UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_service_validators.conf.json"
logger = genericUpdaterLogging.get_logger(title="Change Applier")

print_to_console = False
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,19 @@
"validate_commands": [ ]
},
"rsyslog": {
"validate_commands": [ "generic_config_updater.services_validator.rsyslog_validator" ]
"validate_commands": [ "generic_config_updater.validators.rsyslog_validator" ]
},
"dhcp-relay": {
"validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ]
"validate_commands": [ "generic_config_updater.validators.dhcp_validator" ]
},
"vlan-service": {
"validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ]
"validate_commands": [ "generic_config_updater.validators.vlan_validator" ]
},
"caclmgrd-service": {
"validate_commands": [ "generic_config_updater.services_validator.caclmgrd_validator" ]
"validate_commands": [ "generic_config_updater.validators.caclmgrd_validator" ]
},
"ntp-service": {
"validate_commands": [ "generic_config_updater.services_validator.ntp_validator" ]
"validate_commands": [ "generic_config_updater.validators.ntp_validator" ]
}
}
}
Expand Down
39 changes: 39 additions & 0 deletions generic_config_updater/gcu_table_modification_validators.conf.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"README": [
"table_modification_validators provides, module & method name as ",
" <module name>.<method name>",
"NOTE: module name could have '.'",
" ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need testcase for this file? Like tests in service_validator_test.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added additional unit testing for the new RDMA-specific validator as suggested offline

"The last element separated by '.' is considered as ",
"method name",
"",
"e.g. 'show.acl.test_acl'",
"",
"table_modification_validators_and for a given table defines a list of validators that all must pass for modification to the table to be allowed",
"table_modification_validators_or for a given table defines a list of validators, at least one of which must pass for modification of the table to be allowed",
""
],
"tables": {
"PFC_WD": {
Copy link
Contributor

@qiluo-msft qiluo-msft Feb 8, 2023

Choose a reason for hiding this comment

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

could you reformat? mixing tabs and spaces now. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to all spaces

"table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ],
Copy link
Contributor

@qiluo-msft qiluo-msft Feb 8, 2023

Choose a reason for hiding this comment

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

table_modification_validators_and

There is no design doc for these new field table_modification_validators_and table_modification_validators_or. It is easy to understand if either of them has value, but what is the semantics if both of them appear? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed back to just table_modification_validators
So now, all listed validators must pass in order for modification to the table to be allowed. This is the same logic as MoveValidators, but MoveValidators checks on every intermediary move during patch sorting.

"table_modification_validators_or": [ ]
},
"BUFFER_POOL": {
"table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ],
"table_modification_validators_or": [ ]
},
"WRED_PROFILE": {
"table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ],
"table_modification_validators_or": [ ]
},
"QUEUE": {
"table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ],
"table_modification_validators_or": [ ]
},
"BUFFER_PROFILE": {
"table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ],
"table_modification_validators_or": [ ]
}
}
}

36 changes: 36 additions & 0 deletions generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
import json
import jsonpatch
import importlib
from jsonpointer import JsonPointer
import sonic_yang
import sonic_yang_ext
import subprocess
import yang as ly
import copy
import re
import os
from sonic_py_common import logger
from enum import Enum

YANG_DIR = "/usr/local/yang-models"
SYSLOG_IDENTIFIER = "GenericConfigUpdater"
SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_table_modification_validators.conf.json"

class GenericConfigUpdaterError(Exception):
pass
Expand Down Expand Up @@ -155,6 +159,38 @@ def validate_field_operation(self, old_config, target_config):
if any(op['op'] == operation and field == op['path'] for op in patch):
raise IllegalPatchOperationError("Given patch operation is invalid. Operation: {} is illegal on field: {}".format(operation, field))

def _invoke_validating_function(cmd):
# cmd is in the format as <package/module name>.<method name>
method_name = cmd.split(".")[-1]
module_name = ".".join(cmd.split(".")[0:-1])
module = importlib.import_module(module_name, package=None)
method_to_call = getattr(module, method_name)
return method_to_call()
Copy link
Contributor

@qiluo-msft qiluo-msft Feb 14, 2023

Choose a reason for hiding this comment

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

method_to_call

There is potential risk to run ambiguous code. We can proactively check the module, and method_names. Maybe we can have allowlist or similar?
#Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added code to ensure the module_name is generic_config_updater.field_operation_validators and the method_name contains the string validator


if os.path.exists(GCU_TABLE_MOD_CONF_FILE):
with open(GCU_TABLE_MOD_CONF_FILE, "r") as s:
gcu_table_modification_conf = json.load(s)
else:
raise GenericConfigUpdaterError("GCU table modification validators config file not found")

for element in patch:
path = element["path"]
table = re.search(r'\/([^\/]+)(\/|$)', path).group(1) # This matches the table name in the path, eg. PFC_WD without the forward slashes
Copy link
Contributor

@qiluo-msft qiluo-msft Feb 14, 2023

Choose a reason for hiding this comment

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

This matches the table name in the path, eg. PFC_WD without the forward slashes

Regex is hard to read. Would you provide more code comment, like sample match case. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I added a sample match case in the comment

Copy link
Contributor

@qiluo-msft qiluo-msft Feb 14, 2023

Choose a reason for hiding this comment

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

group

Better be cautious about mismatch. Check matching result before getting group item. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not be able to reach this part of the code if there is a mismatch.

When creating the jsonpatch before this part of the code is reached, the path must be a string that matches the defined regex. Else, an error will be thrown.
https://github.com/sonic-net/sonic-utilities/blob/master/config/main.py#L1391

I did add extra code to check for mismatch again if that would be more complete.

validating_functions_and = set()
validating_functions_or = set()
tables = gcu_table_modification_conf["tables"]
validating_functions_and.update(tables.get(table, {}).get("table_modification_validators_and", []))
validating_functions_or.update(tables.get(table, {}).get("table_modification_validators_or", []))
# For added flexibility in the future, we can extend the table to store a list of lists, to account for (X AND Y AND Z) OR (S AND T)

for function in validating_functions_and:
if not _invoke_validating_function(function):
raise IllegalPatchOperationError("Modification of {} table is illegal- validating function {} returned False".format(table, function))

if validating_functions_or and not any(_invoke_validating_function(function) for function in validating_functions_or):
raise IllegalPatchOperationError("Modification of {} table is illegal- all validating functions returned False".format(table))


def validate_lanes(self, config_db):
if "PORT" not in config_db:
return True, None
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import os
import time
from .gu_common import genericUpdaterLogging
from sonic_py_common import device_info

logger = genericUpdaterLogging.get_logger(title="Service Validator")
logger = genericUpdaterLogging.get_logger(title="GCU Validator")

print_to_console = False

Expand Down Expand Up @@ -101,3 +102,9 @@ def caclmgrd_validator(old_config, upd_config, keys):

def ntp_validator(old_config, upd_config, keys):
return _service_restart("ntp-config")


def is_mellanox_device_validator():
version_info = device_info.get_sonic_version_info()
asic_type = version_info.get('asic_type')
return asic_type == "mellanox"
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
'sonic_cli_gen',
],
package_data={
'generic_config_updater': ['generic_config_updater.conf.json'],
'generic_config_updater': ['gcu_service_validators.conf.json', 'gcu_table_modification_validators.conf.json'],
'show': ['aliases.ini'],
'sonic_installer': ['aliases.ini'],
'tests': ['acl_input/*',
Expand Down
4 changes: 2 additions & 2 deletions tests/generic_config_updater/change_applier_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from unittest.mock import patch, Mock, call

import generic_config_updater.change_applier
import generic_config_updater.services_validator
import generic_config_updater.validators
import generic_config_updater.gu_common

SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
Expand Down Expand Up @@ -232,7 +232,7 @@ def test_change_apply(self, mock_set, mock_db, mock_os_sys):

generic_config_updater.change_applier.UPDATER_CONF_FILE = CONF_FILE
generic_config_updater.change_applier.set_verbose(True)
generic_config_updater.services_validator.set_verbose(True)
generic_config_updater.validators.set_verbose(True)

applier = generic_config_updater.change_applier.ChangeApplier()
debug_print("invoked applier")
Expand Down
4 changes: 2 additions & 2 deletions tests/generic_config_updater/service_validator_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from collections import defaultdict
from unittest.mock import patch

from generic_config_updater.services_validator import vlan_validator, rsyslog_validator, caclmgrd_validator
from generic_config_updater.validators import vlan_validator, rsyslog_validator, caclmgrd_validator
import generic_config_updater.gu_common


Expand Down Expand Up @@ -177,7 +177,7 @@ def test_change_apply_os_system(self, mock_os_sys):
rc = rsyslog_validator("", "", "")
assert not rc, "rsyslog_validator expected to fail"

@patch("generic_config_updater.services_validator.time.sleep")
@patch("generic_config_updater.validators.time.sleep")
def test_change_apply_time_sleep(self, mock_time_sleep):
global time_sleep_calls, time_sleep_call_index

Expand Down