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] Complete RDMA Platform Validation Checks #2791

Merged
Merged
Show file tree
Hide file tree
Changes from 15 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
83 changes: 76 additions & 7 deletions generic_config_updater/field_operation_validators.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,50 @@
from sonic_py_common import device_info
import os
import re
import json
import subprocess
from sonic_py_common import device_info
from .gu_common import GenericConfigUpdaterError

SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json"

def get_asic_name():
asic = "unknown"

if device_info.get_sonic_version_info()['asic_type'] == 'cisco-8000':
asic = "cisco-8000"
elif device_info.get_sonic_version_info()['asic_type'] == 'mellanox':
GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku"
spc1_hwskus = [ 'ACS-MSN2700', 'ACS-MSN2740', 'ACS-MSN2100', 'ACS-MSN2410', 'ACS-MSN2010', 'Mellanox-SN2700', 'Mellanox-SN2700-D48C8' ]
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

spc1_hwskus

These string are hard-coded so not future proof. Better to move into gcu_field_operation_validators.conf.json. #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.

Moved to gcu_field_operation_validators.conf.json.

proc = subprocess.Popen(GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE)
output, err = proc.communicate()
hwsku = output.rstrip('\n')
if hwsku.lower() in [spc1_hwsku.lower() for spc1_hwsku in spc1_hwskus]:
asic = "spc1"
elif device_info.get_sonic_version_info()['asic_type'] == 'broadcom':
command = ["sudo", "lspci"]
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

command

Runtime check is heavy. We could build the mapping offline, and move add the hareware skus into gcu_field_operation_validators.conf.json.

Then in runtime, you just check hwsku. #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.

Moved mapping to gcu_field_operation_validators.conf.json.

proc = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE)
output, err = proc.communicate()
if "Broadcom Limited Device b960" in output or "Broadcom Limited Broadcom BCM56960" in output:
asic = "th"
elif "Broadcom Limited Device b971" in output:
asic = "th2"
elif "Broadcom Limited Device b850" in output or "Broadcom Limited Broadcom BCM56850" in output:
asic = "td2"
elif "Broadcom Limited Device b870" in output or "Broadcom Inc. and subsidiaries Device b870" in output:
asic = "td3"

return asic

def rdma_config_update_validator():
def rdma_config_update_validator(path, operation):
version_info = device_info.get_sonic_version_info()
build_version = version_info.get('build_version')
asic_type = version_info.get('asic_type')
asic = get_asic_name()
path = path.lower()
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

lower

Why change to lowercases? #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.

For consistency, because all fields/paths other than the table in the conf file are lowercase. But in ConfigDB, some fields are uppercase (like AZURE_LOSSLESS)


if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'):
# For paths like /BUFFER_PROFILE/pg_lossless_50000_300m_profile/xoff, remove pg_lossless_50000_300m from the path so that we can clearly determine which fields are modifiable
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

determine

Not quite understand why removing help determine. #Closed

Copy link
Contributor Author

@isabelmsft isabelmsft May 10, 2023

Choose a reason for hiding this comment

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

I changed comment to possibly a clearer example: # 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)

The conf file only saves action because that's the relevant field. Any part of the path with an integer is not relevant, so I don't save that in the conf file. Alternatively, I could change the conf file to define the regex pattern accepted for paths that include integers.

cleaned_path = "/".join([part for part in path.split("/") if not any(char.isdigit() for char in part)])
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

split

Manually split or join is dangerous, you do not consider escaping, etc. Suggest leverage some libraries, such as https://pypi.org/project/jsonpointer/. #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 use jsonpointer library

if asic == "unknown":
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

==

You can compare and return earlier. #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.

Moved comparison to the start of validator function

return False

version_substrings = build_version.split('.')
Expand All @@ -20,7 +58,38 @@ def rdma_config_update_validator():
if branch_version is None:
return False

if asic_type == 'cisco-8000':
return branch_version >= "20201200"
if os.path.exists(GCU_TABLE_MOD_CONF_FILE):
with open(GCU_TABLE_MOD_CONF_FILE, "r") as s:
gcu_field_operation_conf = json.load(s)
else:
raise GenericConfigUpdaterError("GCU table modification validators config file not found")

match = re.search(r'\/([^\/]+)(\/|$)', cleaned_path) # This matches the table name in the path, eg if path if /PFC_WD/GLOBAL, the match would be PFC_WD
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

search

Suggest use library instead of manually parsing. #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 use jsonpointer library

if match is not None:
table = match.group(1)
index = cleaned_path.index(table) + len(table)
field = cleaned_path[index:].lstrip('/')
else:
raise GenericConfigUpdaterError("Invalid jsonpatch path: {}".format(path))

tables = gcu_field_operation_conf["tables"]
scenarios = tables[table]["validator_data"]["rdma_config_update_validator"]
scenario = None
for key in scenarios.keys():
if field in scenarios[key]["fields"]:
scenario = scenarios[key]
break

if scenario is None:
return False

if operation not in scenario["operations"]:
return False

if asic in scenario["platforms"]:
if branch_version < scenario["platforms"][asic]:
return False
else:
return branch_version >= "20181100"
return False

return True
106 changes: 104 additions & 2 deletions generic_config_updater/gcu_field_operation_validators.conf.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,113 @@
"e.g. 'show.acl.test_acl'",
"",
"field_operation_validators for a given table defines a list of validators that all must pass for modification to the specified field and table to be allowed",
"",
"validator_data provides data relevant to each validator",
""
],
"tables": {
"PFC_WD": {
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ]
"pfc_wd": {
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ],
"validator_data": {
"rdma_config_update_validator": {
"PFCWD enable/disable": {
"fields": [
"restoration_time",
"detection_time",
"action"
],
"operations": ["remove", "add", "replace"],
"platforms": {
"spc1": "20181100",
"td2": "20181100",
"th": "20181100",
"th2": "20181100",
"td3": "20181100",
Copy link
Contributor

Choose a reason for hiding this comment

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

realized there is an error in my doc. this should be 202012. td3 is supported only from that release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

"cisco-8000": "20201200"
}
}
}
}
},
"buffer_pool": {
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ],
"validator_data": {
"rdma_config_update_validator": {
"Shared/headroom pool size changes": {
"fields": [
"ingress_lossless_pool/xoff",
"ingress_lossless_pool/size",
"egress_lossy_pool/size"
],
"operations": ["replace"],
"platforms": {
"spc1": "20191100",
"td2": "",
"th": "20221100",
"th2": "20221100",
"td3": "20221100",
"cisco-8000": ""
}
}
}
}
},
"buffer_profile": {
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ],
"validator_data": {
"rdma_config_update_validator": {
"Dynamic threshold tuning": {
"fields": [
"dynamic_th"
],
"operations": ["replace"],
"platforms": {
"spc1": "20181100",
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 still need to check build number less than 20220531? Because GCU support is introduced in 202205.
It by default satisfies this rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but I have this information in the conf file for completion and clarity.

I left the comparison in field_operation_validators.py as-is because this version comparison is always going to be necessary for cases only supported in 202211+. Do you think it's better to not define lower versions in the conf file, and only run this version comparison check if the higher version is defined in the conf file? I figure for this implementation, I would have to add code just to check if the version is defined, which may be an extra unnecessary step.

"td2": "20181100",
"th": "20181100",
"th2": "20181100",
"td3": "20201200",
"cisco-8000": ""
}
},
"PG headroom modification": {
"fields": [
"xoff"
],
"operations": ["replace"],
"platforms": {
"spc1": "20191100",
"td2": "",
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
"th": "20221100",
"th2": "20221100",
"td3": "20221100",
"cisco-8000": ""
}
}
}
}
},
"wred_profile": {
"field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ],
"validator_data": {
"rdma_config_update_validator": {
"ECN tuning": {
"fields": [
"azure_lossless/green_min_threshold",
"azure_lossless/green_max_threshold"
],
"operations": ["replace"],
"platforms": {
"spc1": "20181100",
"td2": "20181100",
"th": "20181100",
"th2": "20181100",
"td3": "20201200",
"cisco-8000": ""
}
}
}
}
}
}
}
9 changes: 5 additions & 4 deletions generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,15 +166,15 @@ 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):
def _invoke_validating_function(cmd, path, operation):
# cmd is in the format as <package/module name>.<method name>
method_name = cmd.split(".")[-1]
module_name = ".".join(cmd.split(".")[0:-1])
if module_name != "generic_config_updater.field_operation_validators" or "validator" not in method_name:
raise GenericConfigUpdaterError("Attempting to call invalid method {} in module {}. Module must be generic_config_updater.field_operation_validators, and method must be a defined validator".format(method_name, module_name))
module = importlib.import_module(module_name, package=None)
method_to_call = getattr(module, method_name)
return method_to_call()
return method_to_call(path, operation)

if os.path.exists(GCU_FIELD_OP_CONF_FILE):
with open(GCU_FIELD_OP_CONF_FILE, "r") as s:
Expand All @@ -184,17 +184,18 @@ def _invoke_validating_function(cmd):

for element in patch:
path = element["path"]
operation = element["op"]
match = re.search(r'\/([^\/]+)(\/|$)', path) # This matches the table name in the path, eg if path if /PFC_WD/GLOBAL, the match would be PFC_WD
if match is not None:
table = match.group(1)
table = match.group(1).lower()
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 26, 2023

Choose a reason for hiding this comment

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

lower()

Why change to lowercases? #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 it back to default text which would be upper case, and also changed the config file we are matching against to have table names in upper case

else:
raise GenericConfigUpdaterError("Invalid jsonpatch path: {}".format(path))
validating_functions= set()
tables = gcu_field_operation_conf["tables"]
validating_functions.update(tables.get(table, {}).get("field_operation_validators", []))

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


Expand Down
Loading