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 11 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
21 changes: 21 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,21 @@
{
"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 for a given table defines a list of validators that all must pass for modification to the table to be allowed",
""
],
"tables": {
"PFC_WD": {
"table_modification_validators": [ "generic_config_updater.validators.rdma_config_update_validator" ]
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.

generic_config_updater.validators.rdma_config_update_validator

-> generic_config_updater.field_operation_validator.rdma_validator #Closed

}
}
}

30 changes: 30 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 @@ -162,6 +166,32 @@ 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= set()
tables = gcu_table_modification_conf["tables"]
validating_functions.update(tables.get(table, {}).get("table_modification_validators", []))

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


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,23 @@ def caclmgrd_validator(old_config, upd_config, keys):

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

def rdma_config_update_validator():
version_info = device_info.get_sonic_version_info()
build_version = version_info.get('build_version')
asic_type = version_info.get('asic_type')

if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
return False

if len(build_version) >= 6:
if build_version[:6].isdigit():
branch_int = int(build_version[:6])
else:
return False
if asic_type == 'cisco-8000':
return branch_int >= 202012
else:
return branch_int >= 201811
else:
return False
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
26 changes: 18 additions & 8 deletions tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import jsonpatch
import sonic_yang
import unittest
from unittest.mock import MagicMock, Mock, patch
import mock

from unittest.mock import MagicMock, Mock
from mock import patch
from .gutest_helpers import create_side_effect_dict, Files
import generic_config_updater.gu_common as gu_common

Expand Down Expand Up @@ -69,11 +71,25 @@ 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)

@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "mellanox", "build_version": "201811"}))
def test_validate_field_operation_legal__pfcwd(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__pfcwd(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)

@patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "invalid-asic", "build_version": "201811"}))
def test_validate_field_modification_illegal__pfcwd(self):
old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}}
target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "80"}}}
config_wrapper = gu_common.ConfigWrapper()
self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config)

def test_validate_field_operation_legal__rm_loopback1(self):
old_config = {
Expand All @@ -92,13 +108,7 @@ def test_validate_field_operation_legal__rm_loopback1(self):
}
config_wrapper = gu_common.ConfigWrapper()
config_wrapper.validate_field_operation(old_config, target_config)

def test_validate_field_operation_illegal__pfcwd(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_validate_field_operation_illegal__rm_loopback0(self):
old_config = {
"LOOPBACK_INTERFACE": {
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