From fc7c8414afa8de61f28d76e64270e040076dcdc2 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 21 Jan 2023 05:24:41 +0000 Subject: [PATCH 01/20] add hwsku validator --- generic_config_updater/gu_common.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index d2561d0532..52e5c92236 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -8,6 +8,7 @@ import copy import re from sonic_py_common import logger +from sonic_py_common import device_info from enum import Enum YANG_DIR = "/usr/local/yang-models" @@ -155,6 +156,26 @@ 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 is_mellanox_device(): + version_info = device_info.get_sonic_version_info() + asic_type = version_info.get('asic_type') + return asic_type == "mellanox" + + # tables_to_validating_function_map[list of tables] yields a list of validating functions that must return True for modification to be allowed to any table in the list of tables via GCU + tables_to_validating_function_map = { + ('/PFC_WD', '/BUFFER_POOL', '/WRED_PROFILE', 'QUEUE', '/BUFFER_PROFILE'): [is_mellanox_device] + } + + for element in patch: + path = element["path"] + for key in tables_to_validating_function_map: + for table in key: + if table in path: + for func in table_to_validating_function_map[key]: + if not func(): + raise IllegalPatchOperationError("Modification of {} table is illegal due to corresponding validating function {}".format(table, func.__name__)) + + def validate_lanes(self, config_db): if "PORT" not in config_db: return True, None From 8240d3adc8d7ac9359330f231efb02ba1e2a9ce6 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sun, 22 Jan 2023 04:38:06 +0000 Subject: [PATCH 02/20] refactor table validating functions --- .../generic_config_updater.conf.json | 27 +++++++++--- generic_config_updater/gu_common.py | 43 +++++++++++-------- .../{services_validator.py => validators.py} | 9 +++- 3 files changed, 55 insertions(+), 24 deletions(-) rename generic_config_updater/{services_validator.py => validators.py} (92%) diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/generic_config_updater.conf.json index 907b5a6863..a146cd9b5d 100644 --- a/generic_config_updater/generic_config_updater.conf.json +++ b/generic_config_updater/generic_config_updater.conf.json @@ -48,7 +48,22 @@ }, "NTP_SERVER": { "services_to_validate": [ "ntp-service" ] - } + }, + "PFC_WD": { + "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] + }, + "BUFFER_POOL": { + "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] + }, + "WRED_PROFILE": { + "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] + }, + "QUEUE": { + "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] + }, + "BUFFER_PROFILE": { + "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] + } }, "services": { "system_health": { @@ -58,19 +73,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" ] } } } diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 52e5c92236..6d07c3e70c 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -1,5 +1,6 @@ import json import jsonpatch +import importlib from jsonpointer import JsonPointer import sonic_yang import sonic_yang_ext @@ -7,12 +8,14 @@ import yang as ly import copy import re +import os from sonic_py_common import logger -from sonic_py_common import device_info from enum import Enum YANG_DIR = "/usr/local/yang-models" SYSLOG_IDENTIFIER = "GenericConfigUpdater" +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_config_updater.conf.json" class GenericConfigUpdaterError(Exception): pass @@ -156,24 +159,30 @@ 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 is_mellanox_device(): - version_info = device_info.get_sonic_version_info() - asic_type = version_info.get('asic_type') - return asic_type == "mellanox" - - # tables_to_validating_function_map[list of tables] yields a list of validating functions that must return True for modification to be allowed to any table in the list of tables via GCU - tables_to_validating_function_map = { - ('/PFC_WD', '/BUFFER_POOL', '/WRED_PROFILE', 'QUEUE', '/BUFFER_PROFILE'): [is_mellanox_device] - } - + def _invoke_validating_function(cmd): + # cmd is in the format as . + 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() + + if os.path.exists(UPDATER_CONF_FILE): + with open(UPDATER_CONF_FILE, "r") as s: + updater_conf = json.load(s) + else: + raise GenericConfigUpdaterError("GCU Config file not found") + for element in patch: path = element["path"] - for key in tables_to_validating_function_map: - for table in key: - if table in path: - for func in table_to_validating_function_map[key]: - if not func(): - raise IllegalPatchOperationError("Modification of {} table is illegal due to corresponding validating function {}".format(table, func.__name__)) + table = re.search(r'\/([^\/]+)(\/|$)', path).group(1) + validating_functions = set() + tables = updater_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): diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/validators.py similarity index 92% rename from generic_config_updater/services_validator.py rename to generic_config_updater/validators.py index 44a9e095eb..91ecc9f341 100644 --- a/generic_config_updater/services_validator.py +++ b/generic_config_updater/validators.py @@ -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 @@ -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" From e08a8fef6c58958d5e317c47893d6b449fbb26a5 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Mon, 23 Jan 2023 01:42:41 +0000 Subject: [PATCH 03/20] fix UT --- tests/generic_config_updater/change_applier_test.py | 4 ++-- tests/generic_config_updater/service_validator_test.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index afe166b008..b59db2e833 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -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__)) @@ -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") diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py index 2f51771d33..97ca336620 100644 --- a/tests/generic_config_updater/service_validator_test.py +++ b/tests/generic_config_updater/service_validator_test.py @@ -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 @@ -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 From fdb45b33d5c91ff582a98cf28ae805aa74300d4f Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Feb 2023 04:36:19 +0000 Subject: [PATCH 04/20] address PR comments --- ....json => gcu_service_validators.conf.json} | 17 +------- ...cu_table_modification_validators.conf.json | 39 +++++++++++++++++++ generic_config_updater/gu_common.py | 26 ++++++++----- setup.py | 2 +- 4 files changed, 57 insertions(+), 27 deletions(-) rename generic_config_updater/{generic_config_updater.conf.json => gcu_service_validators.conf.json} (78%) create mode 100644 generic_config_updater/gcu_table_modification_validators.conf.json diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/gcu_service_validators.conf.json similarity index 78% rename from generic_config_updater/generic_config_updater.conf.json rename to generic_config_updater/gcu_service_validators.conf.json index a146cd9b5d..9410ed4130 100644 --- a/generic_config_updater/generic_config_updater.conf.json +++ b/generic_config_updater/gcu_service_validators.conf.json @@ -48,22 +48,7 @@ }, "NTP_SERVER": { "services_to_validate": [ "ntp-service" ] - }, - "PFC_WD": { - "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] - }, - "BUFFER_POOL": { - "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] - }, - "WRED_PROFILE": { - "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] - }, - "QUEUE": { - "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] - }, - "BUFFER_PROFILE": { - "table_modification_validators": [ "generic_config_updater.validators.is_mellanox_device_validator" ] - } + } }, "services": { "system_health": { diff --git a/generic_config_updater/gcu_table_modification_validators.conf.json b/generic_config_updater/gcu_table_modification_validators.conf.json new file mode 100644 index 0000000000..0e779c9c00 --- /dev/null +++ b/generic_config_updater/gcu_table_modification_validators.conf.json @@ -0,0 +1,39 @@ +{ + "README": [ + "table_modification_validators provides, module & method name as ", + " .", + "NOTE: module name could have '.'", + " ", + "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": { + "table_modification_validators_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ], + "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": [ ] + } + } +} + diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 6d07c3e70c..1bc5c77a6b 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -15,7 +15,7 @@ YANG_DIR = "/usr/local/yang-models" SYSLOG_IDENTIFIER = "GenericConfigUpdater" SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) -UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_config_updater.conf.json" +GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_table_modification_validators.conf.json" class GenericConfigUpdaterError(Exception): pass @@ -168,21 +168,27 @@ def _invoke_validating_function(cmd): return method_to_call() if os.path.exists(UPDATER_CONF_FILE): - with open(UPDATER_CONF_FILE, "r") as s: - updater_conf = json.load(s) + with open(GCU_TABLE_MOD_CONF_FILE, "r") as s: + gcu_table_modification_conf = json.load(s) else: - raise GenericConfigUpdaterError("GCU Config file not found") + raise GenericConfigUpdaterError("GCU table modification validators config file not found") for element in patch: path = element["path"] - table = re.search(r'\/([^\/]+)(\/|$)', path).group(1) - validating_functions = set() - tables = updater_conf["tables"] - validating_functions.update(tables.get(table, {}).get("table_modification_validators", [])) + table = re.search(r'\/([^\/]+)(\/|$)', path).group(1) # This matches the table name in the path, eg. PFC_WD without the forward slashes + 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: + 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)) + raise IllegalPatchOperationError("Modification of {} table is illegal- validating function {} returned False".format(table, function)) + + if 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): diff --git a/setup.py b/setup.py index 70d7473bd7..36c41f04ca 100644 --- a/setup.py +++ b/setup.py @@ -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/*', From b73ff9afdfbcab747d1d29f98e0738dff45610ec Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Feb 2023 04:44:40 +0000 Subject: [PATCH 05/20] revert naming change --- generic_config_updater/change_applier.py | 3 ++- .../gcu_service_validators.conf.json | 10 +++++----- generic_config_updater/gu_common.py | 4 ++-- tests/generic_config_updater/change_applier_test.py | 4 ++-- tests/generic_config_updater/service_validator_test.py | 4 ++-- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index f5a365d59f..e1932f221b 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -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 @@ -141,6 +141,7 @@ def _report_mismatch(self, run_data, upd_data): def apply(self, change): run_data = self._get_running_config() + print(change) upd_data = prune_empty_table(change.apply(copy.deepcopy(run_data))) upd_keys = defaultdict(dict) diff --git a/generic_config_updater/gcu_service_validators.conf.json b/generic_config_updater/gcu_service_validators.conf.json index 9410ed4130..907b5a6863 100644 --- a/generic_config_updater/gcu_service_validators.conf.json +++ b/generic_config_updater/gcu_service_validators.conf.json @@ -58,19 +58,19 @@ "validate_commands": [ ] }, "rsyslog": { - "validate_commands": [ "generic_config_updater.validators.rsyslog_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.rsyslog_validator" ] }, "dhcp-relay": { - "validate_commands": [ "generic_config_updater.validators.dhcp_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ] }, "vlan-service": { - "validate_commands": [ "generic_config_updater.validators.vlan_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ] }, "caclmgrd-service": { - "validate_commands": [ "generic_config_updater.validators.caclmgrd_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.caclmgrd_validator" ] }, "ntp-service": { - "validate_commands": [ "generic_config_updater.validators.ntp_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.ntp_validator" ] } } } diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 1bc5c77a6b..cc434476da 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -167,7 +167,7 @@ def _invoke_validating_function(cmd): method_to_call = getattr(module, method_name) return method_to_call() - if os.path.exists(UPDATER_CONF_FILE): + 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: @@ -187,7 +187,7 @@ def _invoke_validating_function(cmd): if not _invoke_validating_function(function): raise IllegalPatchOperationError("Modification of {} table is illegal- validating function {} returned False".format(table, function)) - if not any(_invoke_validating_function(function) for function in validating_functions_or): + 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)) diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index b59db2e833..afe166b008 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -7,7 +7,7 @@ from unittest.mock import patch, Mock, call import generic_config_updater.change_applier -import generic_config_updater.validators +import generic_config_updater.services_validator import generic_config_updater.gu_common SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) @@ -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.validators.set_verbose(True) + generic_config_updater.services_validator.set_verbose(True) applier = generic_config_updater.change_applier.ChangeApplier() debug_print("invoked applier") diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py index 97ca336620..2f51771d33 100644 --- a/tests/generic_config_updater/service_validator_test.py +++ b/tests/generic_config_updater/service_validator_test.py @@ -6,7 +6,7 @@ from collections import defaultdict from unittest.mock import patch -from generic_config_updater.validators import vlan_validator, rsyslog_validator, caclmgrd_validator +from generic_config_updater.services_validator import vlan_validator, rsyslog_validator, caclmgrd_validator import generic_config_updater.gu_common @@ -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.validators.time.sleep") + @patch("generic_config_updater.services_validator.time.sleep") def test_change_apply_time_sleep(self, mock_time_sleep): global time_sleep_calls, time_sleep_call_index From 10dd1f5f57b45a205a0df396f7962fae2c33b39a Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Feb 2023 04:45:24 +0000 Subject: [PATCH 06/20] remove print statement --- generic_config_updater/change_applier.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index e1932f221b..eadb38037b 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -141,7 +141,6 @@ def _report_mismatch(self, run_data, upd_data): def apply(self, change): run_data = self._get_running_config() - print(change) upd_data = prune_empty_table(change.apply(copy.deepcopy(run_data))) upd_keys = defaultdict(dict) From 77bb42467177ec3fbc42718ddca62fd3304d860a Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Feb 2023 21:08:03 +0000 Subject: [PATCH 07/20] fix naming convention --- .../gcu_service_validators.conf.json | 10 +++++----- tests/generic_config_updater/change_applier_test.py | 4 ++-- tests/generic_config_updater/service_validator_test.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/generic_config_updater/gcu_service_validators.conf.json b/generic_config_updater/gcu_service_validators.conf.json index 907b5a6863..9410ed4130 100644 --- a/generic_config_updater/gcu_service_validators.conf.json +++ b/generic_config_updater/gcu_service_validators.conf.json @@ -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" ] } } } diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index afe166b008..b59db2e833 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -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__)) @@ -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") diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py index 2f51771d33..97ca336620 100644 --- a/tests/generic_config_updater/service_validator_test.py +++ b/tests/generic_config_updater/service_validator_test.py @@ -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 @@ -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 From 2762e49ec48b1ac2c0a5f07a174dba63d6af29be Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Mon, 13 Feb 2023 09:30:13 +0000 Subject: [PATCH 08/20] address PR comments --- ...cu_table_modification_validators.conf.json | 26 +++---------------- generic_config_updater/gu_common.py | 17 +++++------- generic_config_updater/validators.py | 20 +++++++++++--- .../generic_config_updater/gu_common_test.py | 14 ++++++++-- 4 files changed, 39 insertions(+), 38 deletions(-) diff --git a/generic_config_updater/gcu_table_modification_validators.conf.json b/generic_config_updater/gcu_table_modification_validators.conf.json index 0e779c9c00..b35a353ec2 100644 --- a/generic_config_updater/gcu_table_modification_validators.conf.json +++ b/generic_config_updater/gcu_table_modification_validators.conf.json @@ -9,31 +9,13 @@ "", "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", + "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_and": [ "generic_config_updater.validators.is_mellanox_device_validator" ], - "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": [ ] - } + "PFC_WD": { + "table_modification_validators": [ "generic_config_updater.validators.rdma_config_update_validator" ] + } } } diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index cc434476da..42f7ab6c08 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -172,24 +172,19 @@ def _invoke_validating_function(cmd): 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 - validating_functions_and = set() - validating_functions_or = set() + print('hi') + validating_functions= 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: + 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)) - 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: diff --git a/generic_config_updater/validators.py b/generic_config_updater/validators.py index 91ecc9f341..742f713466 100644 --- a/generic_config_updater/validators.py +++ b/generic_config_updater/validators.py @@ -103,8 +103,22 @@ 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(): +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') - return asic_type == "mellanox" + + 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 diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 398558bd65..1c233fd4d9 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -3,8 +3,10 @@ import jsonpatch import sonic_yang import unittest -from unittest.mock import MagicMock, Mock +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 @@ -57,6 +59,7 @@ 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(self): old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "40"}}} @@ -64,10 +67,17 @@ def test_validate_field_operation_legal(self): config_wrapper.validate_field_operation(old_config, target_config) def test_validate_field_operation_illegal(self): - old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": 60}}} + 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(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_ctor__default_values_set(self): config_wrapper = gu_common.ConfigWrapper() From 3df33e2b5286ace3b513a2e4d18b1acb08a6bea7 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Mon, 13 Feb 2023 21:39:44 +0000 Subject: [PATCH 09/20] remove print statement --- generic_config_updater/gu_common.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 42f7ab6c08..689ae01f54 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -176,7 +176,6 @@ def _invoke_validating_function(cmd): 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 - print('hi') validating_functions= set() tables = gcu_table_modification_conf["tables"] validating_functions.update(tables.get(table, {}).get("table_modification_validators", [])) From 633a8fe47bb9779e711b30af52504f7d82bd475f Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Tue, 14 Feb 2023 23:30:05 +0000 Subject: [PATCH 10/20] refactor based on PR comment --- .../gcu_service_validators.conf.json | 77 ----------- ...cu_table_modification_validators.conf.json | 21 --- generic_config_updater/gu_common.py | 14 +- generic_config_updater/validators.py | 124 ------------------ setup.py | 2 +- .../change_applier_test.py | 5 +- .../service_validator_test.py | 4 +- 7 files changed, 13 insertions(+), 234 deletions(-) delete mode 100644 generic_config_updater/gcu_service_validators.conf.json delete mode 100644 generic_config_updater/gcu_table_modification_validators.conf.json delete mode 100644 generic_config_updater/validators.py diff --git a/generic_config_updater/gcu_service_validators.conf.json b/generic_config_updater/gcu_service_validators.conf.json deleted file mode 100644 index 9410ed4130..0000000000 --- a/generic_config_updater/gcu_service_validators.conf.json +++ /dev/null @@ -1,77 +0,0 @@ -{ - "README": [ - "Validate_commands provides, module & method name as ", - " .", - "NOTE: module name could have '.'", - " ", - "The last element separated by '.' is considered as ", - "method name", - "", - "e.g. 'show.acl.test_acl'", - "", - "Here we load 'show.acl' and call 'test_acl' method on it.", - "", - "called as:", - " .>(, ", - " , )", - " config is in JSON format as in config_db.json", - " affected_keys in same format, but w/o value", - " { 'ACL_TABLE': { 'SNMP_ACL': {} ... }, ...}", - " The affected keys has 'added', 'updated' & 'deleted'", - "", - "Multiple validate commands may be provided.", - "", - "Note: The commands may be called in any order", - "" - ], - "tables": { - "": { - "services_to_validate": [ "system_health" ] - }, - "PORT": { - "services_to_validate": [ "port_service" ] - }, - "SYSLOG_SERVER":{ - "services_to_validate": [ "rsyslog" ] - }, - "DHCP_RELAY": { - "services_to_validate": [ "dhcp-relay" ] - }, - "DHCP_SERVER": { - "services_to_validate": [ "dhcp-relay" ] - }, - "VLAN": { - "services_to_validate": [ "vlan-service" ] - }, - "ACL_RULE": { - "services_to_validate": [ "caclmgrd-service" ] - }, - "NTP_SERVER": { - "services_to_validate": [ "ntp-service" ] - } - }, - "services": { - "system_health": { - "validate_commands": [ ] - }, - "port_service": { - "validate_commands": [ ] - }, - "rsyslog": { - "validate_commands": [ "generic_config_updater.validators.rsyslog_validator" ] - }, - "dhcp-relay": { - "validate_commands": [ "generic_config_updater.validators.dhcp_validator" ] - }, - "vlan-service": { - "validate_commands": [ "generic_config_updater.validators.vlan_validator" ] - }, - "caclmgrd-service": { - "validate_commands": [ "generic_config_updater.validators.caclmgrd_validator" ] - }, - "ntp-service": { - "validate_commands": [ "generic_config_updater.validators.ntp_validator" ] - } - } -} - diff --git a/generic_config_updater/gcu_table_modification_validators.conf.json b/generic_config_updater/gcu_table_modification_validators.conf.json deleted file mode 100644 index b35a353ec2..0000000000 --- a/generic_config_updater/gcu_table_modification_validators.conf.json +++ /dev/null @@ -1,21 +0,0 @@ -{ - "README": [ - "table_modification_validators provides, module & method name as ", - " .", - "NOTE: module name could have '.'", - " ", - "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" ] - } - } -} - diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 128e6beff3..d3386699d5 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -15,7 +15,7 @@ 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" +GCU_FIELD_OP_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" class GenericConfigUpdaterError(Exception): pass @@ -174,18 +174,18 @@ def _invoke_validating_function(cmd): method_to_call = getattr(module, method_name) return method_to_call() - 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) + if os.path.exists(GCU_FIELD_OP_CONF_FILE): + with open(GCU_FIELD_OP_CONF_FILE, "r") as s: + gcu_field_operation_conf = json.load(s) else: - raise GenericConfigUpdaterError("GCU table modification validators config file not found") + raise GenericConfigUpdaterError("GCU field operation 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 validating_functions= set() - tables = gcu_table_modification_conf["tables"] - validating_functions.update(tables.get(table, {}).get("table_modification_validators", [])) + 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): diff --git a/generic_config_updater/validators.py b/generic_config_updater/validators.py deleted file mode 100644 index 742f713466..0000000000 --- a/generic_config_updater/validators.py +++ /dev/null @@ -1,124 +0,0 @@ -import os -import time -from .gu_common import genericUpdaterLogging -from sonic_py_common import device_info - -logger = genericUpdaterLogging.get_logger(title="GCU Validator") - -print_to_console = False - -def set_verbose(verbose=False): - global print_to_console, logger - - print_to_console = verbose - if verbose: - logger.set_min_log_priority_debug() - else: - logger.set_min_log_priority_notice() - - -def _service_restart(svc_name): - rc = os.system(f"systemctl restart {svc_name}") - if rc != 0: - # This failure is likely due to too many restarts - # - rc = os.system(f"systemctl reset-failed {svc_name}") - logger.log(logger.LOG_PRIORITY_ERROR, - f"Service has been reset. rc={rc}; Try restart again...", - print_to_console) - - rc = os.system(f"systemctl restart {svc_name}") - if rc != 0: - # Even with reset-failed, restart fails. - # Give a pause before retry. - # - logger.log(logger.LOG_PRIORITY_ERROR, - f"Restart failed for {svc_name} rc={rc} after reset; Pause for 10s & retry", - print_to_console) - time.sleep(10) - rc = os.system(f"systemctl restart {svc_name}") - - if rc == 0: - logger.log(logger.LOG_PRIORITY_NOTICE, - f"Restart succeeded for {svc_name}", - print_to_console) - else: - logger.log(logger.LOG_PRIORITY_ERROR, - f"Restart failed for {svc_name} rc={rc}", - print_to_console) - return rc == 0 - - -def rsyslog_validator(old_config, upd_config, keys): - rc = os.system("/usr/bin/rsyslog-config.sh") - if rc != 0: - return _service_restart("rsyslog") - else: - return True - - -def dhcp_validator(old_config, upd_config, keys): - return _service_restart("dhcp_relay") - - -def vlan_validator(old_config, upd_config, keys): - old_vlan = old_config.get("VLAN", {}) - upd_vlan = upd_config.get("VLAN", {}) - - for key in set(old_vlan.keys()).union(set(upd_vlan.keys())): - if (old_vlan.get(key, {}).get("dhcp_servers", []) != - upd_vlan.get(key, {}).get("dhcp_servers", [])): - return _service_restart("dhcp_relay") - # No update to DHCP servers. - return True - -def caclmgrd_validator(old_config, upd_config, keys): - old_acltable = old_config.get("ACL_TABLE", {}) - upd_acltable = upd_config.get("ACL_TABLE", {}) - - old_cacltable = [table for table, fields in old_acltable.items() - if fields.get("type", "") == "CTRLPLANE"] - upd_cacltable = [table for table, fields in upd_acltable.items() - if fields.get("type", "") == "CTRLPLANE"] - - old_aclrule = old_config.get("ACL_RULE", {}) - upd_aclrule = upd_config.get("ACL_RULE", {}) - - old_caclrule = [rule for rule in old_aclrule - if rule.split("|")[0] in old_cacltable] - upd_caclrule = [rule for rule in upd_aclrule - if rule.split("|")[0] in upd_cacltable] - - # Only sleep when cacl rule is changed as this will update iptable. - for key in set(old_caclrule).union(set(upd_caclrule)): - if (old_aclrule.get(key, {}) != upd_aclrule.get(key, {})): - # caclmgrd will update in 0.5 sec when configuration stops, - # we sleep 1 sec to make sure it does update. - time.sleep(1) - return True - # No update to ACL_RULE. - return True - - -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 diff --git a/setup.py b/setup.py index dfc03e30ac..42c286d573 100644 --- a/setup.py +++ b/setup.py @@ -64,7 +64,7 @@ 'sonic_cli_gen', ], package_data={ - 'generic_config_updater': ['gcu_service_validators.conf.json', 'gcu_table_modification_validators.conf.json'], + 'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json'], 'show': ['aliases.ini'], 'sonic_installer': ['aliases.ini'], 'tests': ['acl_input/*', diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index b59db2e833..c0a31ee0fb 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -7,7 +7,7 @@ from unittest.mock import patch, Mock, call import generic_config_updater.change_applier -import generic_config_updater.validators +import generic_config_updater.services_validator import generic_config_updater.gu_common SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) @@ -86,6 +86,7 @@ def os_system_cfggen(cmd): return 0 + # mimics config_db.set_entry # def set_entry(config_db, tbl, key, data): @@ -232,7 +233,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.validators.set_verbose(True) + generic_config_updater.services_validator.set_verbose(True) applier = generic_config_updater.change_applier.ChangeApplier() debug_print("invoked applier") diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py index 97ca336620..2f51771d33 100644 --- a/tests/generic_config_updater/service_validator_test.py +++ b/tests/generic_config_updater/service_validator_test.py @@ -6,7 +6,7 @@ from collections import defaultdict from unittest.mock import patch -from generic_config_updater.validators import vlan_validator, rsyslog_validator, caclmgrd_validator +from generic_config_updater.services_validator import vlan_validator, rsyslog_validator, caclmgrd_validator import generic_config_updater.gu_common @@ -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.validators.time.sleep") + @patch("generic_config_updater.services_validator.time.sleep") def test_change_apply_time_sleep(self, mock_time_sleep): global time_sleep_calls, time_sleep_call_index From 987eaa79fae0527fc970f8e6d5355e1b9ff1bba7 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Tue, 14 Feb 2023 23:35:34 +0000 Subject: [PATCH 11/20] address PR comment2 --- .../field_operation_validators.py | 21 +++ .../gcu_field_operation_validators.conf.json | 20 +++ .../gcu_services_validator.conf.json | 77 +++++++++++ generic_config_updater/services_validator.py | 124 ++++++++++++++++++ .../change_applier_test.py | 1 - 5 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 generic_config_updater/field_operation_validators.py create mode 100644 generic_config_updater/gcu_field_operation_validators.conf.json create mode 100644 generic_config_updater/gcu_services_validator.conf.json create mode 100644 generic_config_updater/services_validator.py diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py new file mode 100644 index 0000000000..c550bb679c --- /dev/null +++ b/generic_config_updater/field_operation_validators.py @@ -0,0 +1,21 @@ +from sonic_py_common import device_info + +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 diff --git a/generic_config_updater/gcu_field_operation_validators.conf.json b/generic_config_updater/gcu_field_operation_validators.conf.json new file mode 100644 index 0000000000..f12a14d8eb --- /dev/null +++ b/generic_config_updater/gcu_field_operation_validators.conf.json @@ -0,0 +1,20 @@ +{ + "README": [ + "field_operation_validators provides, module & method name as ", + " .", + "NOTE: module name could have '.'", + " ", + "The last element separated by '.' is considered as ", + "method name", + "", + "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", + "" + ], + "tables": { + "PFC_WD": { + "field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ] + } + } +} diff --git a/generic_config_updater/gcu_services_validator.conf.json b/generic_config_updater/gcu_services_validator.conf.json new file mode 100644 index 0000000000..907b5a6863 --- /dev/null +++ b/generic_config_updater/gcu_services_validator.conf.json @@ -0,0 +1,77 @@ +{ + "README": [ + "Validate_commands provides, module & method name as ", + " .", + "NOTE: module name could have '.'", + " ", + "The last element separated by '.' is considered as ", + "method name", + "", + "e.g. 'show.acl.test_acl'", + "", + "Here we load 'show.acl' and call 'test_acl' method on it.", + "", + "called as:", + " .>(, ", + " , )", + " config is in JSON format as in config_db.json", + " affected_keys in same format, but w/o value", + " { 'ACL_TABLE': { 'SNMP_ACL': {} ... }, ...}", + " The affected keys has 'added', 'updated' & 'deleted'", + "", + "Multiple validate commands may be provided.", + "", + "Note: The commands may be called in any order", + "" + ], + "tables": { + "": { + "services_to_validate": [ "system_health" ] + }, + "PORT": { + "services_to_validate": [ "port_service" ] + }, + "SYSLOG_SERVER":{ + "services_to_validate": [ "rsyslog" ] + }, + "DHCP_RELAY": { + "services_to_validate": [ "dhcp-relay" ] + }, + "DHCP_SERVER": { + "services_to_validate": [ "dhcp-relay" ] + }, + "VLAN": { + "services_to_validate": [ "vlan-service" ] + }, + "ACL_RULE": { + "services_to_validate": [ "caclmgrd-service" ] + }, + "NTP_SERVER": { + "services_to_validate": [ "ntp-service" ] + } + }, + "services": { + "system_health": { + "validate_commands": [ ] + }, + "port_service": { + "validate_commands": [ ] + }, + "rsyslog": { + "validate_commands": [ "generic_config_updater.services_validator.rsyslog_validator" ] + }, + "dhcp-relay": { + "validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ] + }, + "vlan-service": { + "validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ] + }, + "caclmgrd-service": { + "validate_commands": [ "generic_config_updater.services_validator.caclmgrd_validator" ] + }, + "ntp-service": { + "validate_commands": [ "generic_config_updater.services_validator.ntp_validator" ] + } + } +} + diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py new file mode 100644 index 0000000000..24314a3c36 --- /dev/null +++ b/generic_config_updater/services_validator.py @@ -0,0 +1,124 @@ +import os +import time +from .gu_common import genericUpdaterLogging +from sonic_py_common import device_info + +logger = genericUpdaterLogging.get_logger(title="Service Validator") + +print_to_console = False + +def set_verbose(verbose=False): + global print_to_console, logger + + print_to_console = verbose + if verbose: + logger.set_min_log_priority_debug() + else: + logger.set_min_log_priority_notice() + + +def _service_restart(svc_name): + rc = os.system(f"systemctl restart {svc_name}") + if rc != 0: + # This failure is likely due to too many restarts + # + rc = os.system(f"systemctl reset-failed {svc_name}") + logger.log(logger.LOG_PRIORITY_ERROR, + f"Service has been reset. rc={rc}; Try restart again...", + print_to_console) + + rc = os.system(f"systemctl restart {svc_name}") + if rc != 0: + # Even with reset-failed, restart fails. + # Give a pause before retry. + # + logger.log(logger.LOG_PRIORITY_ERROR, + f"Restart failed for {svc_name} rc={rc} after reset; Pause for 10s & retry", + print_to_console) + time.sleep(10) + rc = os.system(f"systemctl restart {svc_name}") + + if rc == 0: + logger.log(logger.LOG_PRIORITY_NOTICE, + f"Restart succeeded for {svc_name}", + print_to_console) + else: + logger.log(logger.LOG_PRIORITY_ERROR, + f"Restart failed for {svc_name} rc={rc}", + print_to_console) + return rc == 0 + + +def rsyslog_validator(old_config, upd_config, keys): + rc = os.system("/usr/bin/rsyslog-config.sh") + if rc != 0: + return _service_restart("rsyslog") + else: + return True + + +def dhcp_validator(old_config, upd_config, keys): + return _service_restart("dhcp_relay") + + +def vlan_validator(old_config, upd_config, keys): + old_vlan = old_config.get("VLAN", {}) + upd_vlan = upd_config.get("VLAN", {}) + + for key in set(old_vlan.keys()).union(set(upd_vlan.keys())): + if (old_vlan.get(key, {}).get("dhcp_servers", []) != + upd_vlan.get(key, {}).get("dhcp_servers", [])): + return _service_restart("dhcp_relay") + # No update to DHCP servers. + return True + +def caclmgrd_validator(old_config, upd_config, keys): + old_acltable = old_config.get("ACL_TABLE", {}) + upd_acltable = upd_config.get("ACL_TABLE", {}) + + old_cacltable = [table for table, fields in old_acltable.items() + if fields.get("type", "") == "CTRLPLANE"] + upd_cacltable = [table for table, fields in upd_acltable.items() + if fields.get("type", "") == "CTRLPLANE"] + + old_aclrule = old_config.get("ACL_RULE", {}) + upd_aclrule = upd_config.get("ACL_RULE", {}) + + old_caclrule = [rule for rule in old_aclrule + if rule.split("|")[0] in old_cacltable] + upd_caclrule = [rule for rule in upd_aclrule + if rule.split("|")[0] in upd_cacltable] + + # Only sleep when cacl rule is changed as this will update iptable. + for key in set(old_caclrule).union(set(upd_caclrule)): + if (old_aclrule.get(key, {}) != upd_aclrule.get(key, {})): + # caclmgrd will update in 0.5 sec when configuration stops, + # we sleep 1 sec to make sure it does update. + time.sleep(1) + return True + # No update to ACL_RULE. + return True + + +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 diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index c0a31ee0fb..afe166b008 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -86,7 +86,6 @@ def os_system_cfggen(cmd): return 0 - # mimics config_db.set_entry # def set_entry(config_db, tbl, key, data): From 735630c38a587231b6e3219b7c264f90f140c878 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 15 Feb 2023 21:54:48 +0000 Subject: [PATCH 12/20] remove dup method --- generic_config_updater/services_validator.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py index 24314a3c36..8172a3f4d4 100644 --- a/generic_config_updater/services_validator.py +++ b/generic_config_updater/services_validator.py @@ -102,23 +102,3 @@ 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 From 0c6c24e6f0e33476ca6a3e4620f924cfd8f59918 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 15 Feb 2023 23:38:27 +0000 Subject: [PATCH 13/20] fix typo --- generic_config_updater/change_applier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index eadb38037b..d0818172f8 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -9,7 +9,7 @@ from .gu_common import genericUpdaterLogging SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) -UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_service_validators.conf.json" +UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_services_validator.conf.json" logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False From 46c45074e30d05b35bbd69435929b4093df06b3f Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 16 Feb 2023 22:47:55 +0000 Subject: [PATCH 14/20] address PR comments --- .../field_operation_validators.py | 24 +++++++++++-------- generic_config_updater/gu_common.py | 8 ++++++- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index c550bb679c..66b8ae38cd 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -8,14 +8,18 @@ def rdma_config_update_validator(): 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: + version_substrings = build_version.split('.') + branch_int = None + + for substring in version_substrings: + if substring.isdigit() and (substring.startswith("202") or substring.startswith("201")): + branch_int = int(substring) + break + + if branch_int is None: return False + + if asic_type == 'cisco-8000': + return branch_int >= 202012 + else: + return branch_int >= 201811 diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index d3386699d5..aa8ecfdcc4 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -170,6 +170,8 @@ def _invoke_validating_function(cmd): # cmd is in the format as . 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() @@ -182,7 +184,11 @@ def _invoke_validating_function(cmd): 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 + match = re.search(r'\/([^\/]+)(\/|$)', path) + if match is not None: + table = match.group(1) + 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", [])) From c8f9249a4d2513de50792c1226edc29bbfc9fd8c Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 16 Feb 2023 22:52:17 +0000 Subject: [PATCH 15/20] add clarifying comment --- generic_config_updater/gu_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index aa8ecfdcc4..e8c66fcbbe 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -184,7 +184,7 @@ def _invoke_validating_function(cmd): for element in patch: path = element["path"] - match = re.search(r'\/([^\/]+)(\/|$)', path) + 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) else: From 9c15a08e723b36a807e1d1f823561d3b36b8c23d Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 16 Feb 2023 23:13:48 +0000 Subject: [PATCH 16/20] fix test --- tests/generic_config_updater/gu_common_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index b13e883b82..0149761ec6 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -71,7 +71,7 @@ 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"})) + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "mellanox", "build_version": "SONiC.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"}}} @@ -84,7 +84,7 @@ def test_validate_field_operation_illegal__pfcwd(self): 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"})) + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "invalid-asic", "build_version": "SONiC.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"}}} From 80bf53f9d2aebb8a829518edc5481c4d24759c6c Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 23 Feb 2023 09:15:23 +0000 Subject: [PATCH 17/20] address comment --- generic_config_updater/field_operation_validators.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 66b8ae38cd..00c179ca53 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -20,6 +20,6 @@ def rdma_config_update_validator(): return False if asic_type == 'cisco-8000': - return branch_int >= 202012 + return branch_int >= 20201200 else: - return branch_int >= 201811 + return branch_int >= 20181100 From 30bd7b2601c52c2b29a11af7615217aa5f4d822a Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Tue, 28 Feb 2023 08:04:23 +0000 Subject: [PATCH 18/20] fix build version in test --- tests/generic_config_updater/gu_common_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 0149761ec6..a319a25ead 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -71,7 +71,7 @@ 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": "SONiC.201811"})) + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "mellanox", "build_version": "SONiC.20181131"})) def test_validate_field_operation_legal__pfcwd(self): old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "40"}}} @@ -84,7 +84,7 @@ def test_validate_field_operation_illegal__pfcwd(self): 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": "SONiC.201811"})) + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "invalid-asic", "build_version": "SONiC.20181131"})) def test_validate_field_modification_illegal__pfcwd(self): old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "80"}}} From 8d17240f3cde22febc31d218cd7925dc9eacdbec Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Mar 2023 00:33:04 +0000 Subject: [PATCH 19/20] change to string comparison --- .../field_operation_validators.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 00c179ca53..befd4b8749 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -1,4 +1,5 @@ from sonic_py_common import device_info +import re def rdma_config_update_validator(): version_info = device_info.get_sonic_version_info() @@ -9,17 +10,17 @@ def rdma_config_update_validator(): return False version_substrings = build_version.split('.') - branch_int = None + branch_version = None for substring in version_substrings: - if substring.isdigit() and (substring.startswith("202") or substring.startswith("201")): - branch_int = int(substring) + if substring.isdigit() and re.match(r'^\d{8}$', substring): + branch_version = substring break - if branch_int is None: + if branch_version is None: return False if asic_type == 'cisco-8000': - return branch_int >= 20201200 + return branch_version >= "20201200" else: - return branch_int >= 20181100 + return branch_version >= "20181100" From 4e29fe703033827d467715a270dab9b0239c733f Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 1 Mar 2023 00:34:36 +0000 Subject: [PATCH 20/20] remove unused import --- generic_config_updater/services_validator.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py index 8172a3f4d4..44a9e095eb 100644 --- a/generic_config_updater/services_validator.py +++ b/generic_config_updater/services_validator.py @@ -1,7 +1,6 @@ import os import time from .gu_common import genericUpdaterLogging -from sonic_py_common import device_info logger = genericUpdaterLogging.get_logger(title="Service Validator")