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 PORT table StateDB Validator #2936

Merged
merged 11 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
72 changes: 70 additions & 2 deletions generic_config_updater/field_operation_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import subprocess
from sonic_py_common import device_info
from .gu_common import GenericConfigUpdaterError

from swsscommon.swsscommon import SonicV2Connector

SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json"
Expand Down Expand Up @@ -71,7 +71,7 @@ def rdma_config_update_validator(patch_element):
path = patch_element["path"]
table = jsonpointer.JsonPointer(path).parts[0]

# Helper function to return relevant cleaned paths, consdiers case where the jsonpatch value is a dict
# Helper function to return relevant cleaned paths, considers case where the jsonpatch value is a dict
# 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)
def _get_fields_in_patch():
cleaned_fields = []
Expand Down Expand Up @@ -126,3 +126,71 @@ def _get_fields_in_patch():
return False

return True


def read_statedb_entry(table, field):
state_db = SonicV2Connector(host="127.0.0.1")
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 11, 2023

Choose a reason for hiding this comment

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

SonicV2Connector

If you use Table class, you do not need to concat table name and key. #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 see.. is Table class recommended over SonicV2Connector use_unix_socket_path? In the future, this function may be extended to include tables other than PORT

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, the benefit is no need to concat table name/separator/key. You can create multiple Table from one DBConnector, and they are not heavy.

state_db.connect(state_db.STATE_DB)
return state_db.get(state_db.STATE_DB, table, field)


def port_config_update_validator(patch_element):

def _validate_key(key, port, value):
if key == "fec":
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs")
if supported_fecs_str:
if supported_fecs_str != 'N/A':
supported_fecs_list = [element.strip() for element in supported_fecs_str.split(',')]
else:
supported_fecs_list = []
else:
supported_fecs_list = [ 'rs', 'fc', 'none']
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
if value.strip() not in supported_fecs_list:
return False
return True
if key == "speed":
supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or ''
supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s]
if supported_speeds and int(value) not in supported_speeds:
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 11, 2023

Choose a reason for hiding this comment

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

int

may throw during casting #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.

Thank you, I added exception handling

return False
return True
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved

def _parse_port_from_path(path):
match = re.search(r"Ethernet\d+", path)
if match:
port = match.group(0)
return port
return None

if patch_element["op"] == "remove":
return True

# for PORT speed and fec configs, need to ensure value is allowed based on StateDB
patch_element_str = json.dumps(patch_element)
path = patch_element["path"]
value = patch_element.get("value")
keys = ['fec', 'speed']
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 11, 2023

Choose a reason for hiding this comment

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

keys

-> fields #Closed

for key in keys:
if key in patch_element_str:
if path.endswith(key):
port = _parse_port_from_path(path)
if not _validate_key(key, port, value):
return False
elif isinstance(value, dict):
if key in value.keys():
port = _parse_port_from_path(path)
value = value[key]
if not _validate_key(key, port, value):
return False
else:
for port_name, port_info in value.items():
if isinstance(port_info, dict):
port = port_name
if key in port_info.keys():
value = port_info[key]
if not _validate_key(key, port, value):
return False
else:
continue
return True
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@
}
}
}
},
"PORT": {
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 11, 2023

Choose a reason for hiding this comment

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

mixing tab and spaces. #Closed

"field_operation_validators": [ "generic_config_updater.field_operation_validators.port_config_update_validator" ]
}
}
}
74 changes: 74 additions & 0 deletions tests/generic_config_updater/field_operation_validator_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,80 @@

class TestValidateFieldOperation(unittest.TestCase):

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value=""))
def test_port_config_update_validator_valid_speed_no_state_db(self):
patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_valid_speed_existing_state_db(self):
patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_valid_speed_existing_state_db(self):
patch_element = {"path": "/PORT/Ethernet3/speed", "op": "add", "value": "234"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_invalid_speed_existing_state_db(self):
patch_element = {"path": "/PORT/Ethernet3/speed", "op": "add", "value": "235"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_invalid_speed_existing_state_db_nested(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "235"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_valid_speed_existing_state_db_nested(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "234"}, "Ethernet4": {"alias": "Eth4", "speed": "234"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234"))
def test_port_config_update_validator_invalid_speed_existing_state_db_nested_2(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "234"}, "Ethernet4": {"alias": "Eth4", "speed": "236"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

def test_port_config_update_validator_remove(self):
patch_element = {"path": "/PORT/Ethernet3", "op": "remove"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc"))
def test_port_config_update_validator_invalid_fec_existing_state_db(self):
patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "asf"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc"))
def test_port_config_update_validator_invalid_fec_existing_state_db_nested(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "none"}, "Ethernet4": {"alias": "Eth4", "fec": "fs"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc"))
def test_port_config_update_validator_valid_fec_existing_state_db_nested(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "fc"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc"))
def test_port_config_update_validator_valid_fec_existing_state_db_nested_2(self):
patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "rs"}, "Ethernet4": {"alias": "Eth4", "fec": "fc"}}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc"))
def test_port_config_update_validator_valid_fec_existing_state_db(self):
patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rs"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value=""))
def test_port_config_update_validator_valid_fec_no_state_db(self):
patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fec": "rs"}}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True

@patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value=""))
def test_port_config_update_validator_invalid_fec_no_state_db(self):
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rsf"}
assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False

@patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="unknown"))
def test_rdma_config_update_validator_unknown_asic(self):
patch_element = {"path": "/PFC_WD/Ethernet4/restoration_time", "op": "replace", "value": "234234"}
Expand Down
Loading