diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 83ed4a88cb..13436d1174 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -399,10 +399,10 @@ def _get_paths_recursive(self, config, pattern_tokens, matching_tokens, idx, com if token == "*": matching_keys = config.keys() elif token.startswith("*|"): - suffix = token[2:] + suffix = token[1:] # the suffix will be `|...` matching_keys = [key for key in config.keys() if key.endswith(suffix)] elif token.endswith("|*"): - prefix = token[:-2] + prefix = token[:-1] # the prefix will be `...|` matching_keys = [key for key in config.keys() if key.startswith(prefix)] elif token in config: matching_keys = [token] @@ -544,6 +544,66 @@ def _get_default_value_from_settings(self, parent_path, field_name): return None +class LaneReplacementMoveValidator: + def __init__(self, path_addressing): + self.path_addressing = path_addressing + + def validate(self, move, diff): + current_config = diff.current_config + target_config = diff.target_config # Final config after applying whole patch + + if "PORT" not in current_config: + return True + + current_ports = current_config["PORT"] + if not current_ports: + return True + + if "PORT" not in target_config: + return True + + target_ports = target_config["PORT"] + if not target_ports: + return True + + simulated_config = move.apply(current_config) # Config after applying just this move + + for port_name in current_ports: + if port_name not in target_ports: + continue + + if not self._validate_port(port_name, current_config, target_config, simulated_config): + return False + + return True + + def _validate_port(self, port_name, current_config, target_config, simulated_config): + current_lanes = self._get_lanes(current_config, port_name) + target_lanes = self._get_lanes(target_config, port_name) + + if current_lanes == target_lanes: + return True + + simulated_port = self.path_addressing.get_from_path(simulated_config, f"/PORT/{port_name}") + + if simulated_port == None: + return True + + current_admin_status = self.path_addressing.get_from_path(current_config, f"/PORT/{port_name}/admin_status") + simulated_admin_status = self.path_addressing.get_from_path(simulated_config, f"/PORT/{port_name}/admin_status") + if current_admin_status != simulated_admin_status and current_admin_status != "up": + return False + + port_path = f"/PORT/{port_name}" + for ref_path in self.path_addressing.find_ref_paths(port_path, simulated_config): + if not self.path_addressing.has_path(current_config, ref_path): + return False + + return True + + def _get_lanes(self, config, port_name): + return config["PORT"][port_name].get("lanes", None) + class DeleteWholeConfigMoveValidator: """ A class to validate not deleting whole config as it is not supported by JsonPatch lib. @@ -921,6 +981,8 @@ def validate(self, move, diff): for required_path, required_value in data[path]: current_value = self.identifier.get_value_or_default(current_config, required_path) simulated_value = self.identifier.get_value_or_default(simulated_config, required_path) + if simulated_value == None: # Simulated config does not have this value at all. + continue if current_value != simulated_value and simulated_value != required_value: return False @@ -1000,6 +1062,43 @@ def generate(self, diff): for move in single_run_generator.generate(): yield move +class LaneReplacementMoveGenerator: + def __init__(self, path_addressing): + self.path_addressing = path_addressing + + def generate(self, diff): + current_config = diff.current_config + target_config = diff.target_config # Final config after applying whole patch + + current_ports = current_config["PORT"] + if not current_ports: + return + + if "PORT" not in target_config: + return + + target_ports = target_config["PORT"] + if not target_ports: + return + + for port_name in current_ports: + if port_name not in target_ports: + continue + + current_lanes = self._get_lanes(current_config, port_name) + target_lanes = self._get_lanes(target_config, port_name) + + if current_lanes == target_lanes: + continue + + port_path = f"/PORT/{port_name}" + + for ref_path in self.path_addressing.find_ref_paths(port_path, current_config): + yield JsonMove(diff, OperationType.REMOVE, self.path_addressing.get_path_tokens(ref_path)) + + def _get_lanes(self, config, port_name): + return config["PORT"][port_name].get("lanes", None) + class SingleRunLowLevelMoveGenerator: """ A class that can only run once to assist LowLevelMoveGenerator with generating the moves. @@ -1271,6 +1370,8 @@ def extend(self, move, diff): for required_path, required_value in data[path]: current_value = self.identifier.get_value_or_default(current_config, required_path) simulated_value = self.identifier.get_value_or_default(simulated_config, required_path) + if simulated_value == None: # Simulated config does not have this value at all. + continue if current_value != simulated_value and simulated_value != required_value: flip_path_value_tuples.add((required_path, required_value)) @@ -1473,7 +1574,8 @@ def __init__(self, operation_wrapper, config_wrapper, path_addressing): self.path_addressing = path_addressing def create(self, algorithm=Algorithm.DFS): - move_generators = [LowLevelMoveGenerator(self.path_addressing)] + move_generators = [LaneReplacementMoveGenerator(self.path_addressing), + LowLevelMoveGenerator(self.path_addressing)] # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time move_non_extendable_generators = [KeyLevelMoveGenerator()] move_extenders = [RequiredValueMoveExtender(self.path_addressing, self.operation_wrapper), @@ -1485,6 +1587,7 @@ def create(self, algorithm=Algorithm.DFS): NoDependencyMoveValidator(self.path_addressing, self.config_wrapper), CreateOnlyMoveValidator(self.path_addressing), RequiredValueMoveValidator(self.path_addressing), + LaneReplacementMoveValidator(self.path_addressing), NoEmptyTableMoveValidator(self.path_addressing)] move_wrapper = MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators) diff --git a/tests/generic_config_updater/files/config_db_with_port_critical.json b/tests/generic_config_updater/files/config_db_with_port_critical.json index 5853bfe5ea..2b2007d068 100644 --- a/tests/generic_config_updater/files/config_db_with_port_critical.json +++ b/tests/generic_config_updater/files/config_db_with_port_critical.json @@ -36,6 +36,23 @@ "lanes": "41,42,43,44", "pfc_asym": "off", "speed": "40000" + }, + "Ethernet28": { + "alias": "fortyGigE0/28", + "description": "Servers5:eth0", + "index": "6", + "lanes": "53,54,55,56", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet32": { + "alias": "fortyGigE0/32", + "description": "Servers6:eth0", + "index": "7", + "lanes": "57,58,59,60", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" } }, "BUFFER_PG": { @@ -44,6 +61,9 @@ }, "Ethernet12|0": { "profile": "ingress_lossy_profile" + }, + "Ethernet28|0": { + "profile": "ingress_lossy_profile" } } } diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 68a6b09a54..31d7ced015 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -753,6 +753,41 @@ def test_simulate__applies_move(self): # Assert self.assertIs(self.any_diff, actual) +class TestJsonPointerFilter(unittest.TestCase): + def test_get_paths__common_prefix__exact_match_returned(self): + config = { + "BUFFER_PG": { + "Ethernet1|0": {}, + "Ethernet12|0": {}, + "Ethernet120|0": {}, # 'Ethernet12' is a common prefix with the previous line + }, + } + + filter = ps.JsonPointerFilter([["BUFFER_PG", "Ethernet12|*"]], PathAddressing()) + + expected_paths = ["/BUFFER_PG/Ethernet12|0"] + + actual_paths = list(filter.get_paths(config)) + + self.assertCountEqual(expected_paths, actual_paths) + + def test_get_paths__common_suffix__exact_match_returned(self): + config = { + "QUEUE": { + "Ethernet1|0": {}, + "Ethernet1|10": {}, + "Ethernet1|110": {}, # 10 is a common suffix with the previous line + }, + } + + filter = ps.JsonPointerFilter([["QUEUE", "*|10"]], PathAddressing()) + + expected_paths = ["/QUEUE/Ethernet1|10"] + + actual_paths = list(filter.get_paths(config)) + + self.assertCountEqual(expected_paths, actual_paths) + class TestRequiredValueIdentifier(unittest.TestCase): def test_hard_coded_required_value_data(self): identifier = ps.RequiredValueIdentifier(PathAddressing()) @@ -1849,6 +1884,22 @@ def _get_critical_port_change_test_cases(self): } }) }, + # Additional cases where the full port is getting deleted + # If port is getting deleted, there is no point in checking if there are critical changes depending on it + "PORT_UP__STATUS_CHANGING__UNDER_PORT__PORT_EXIST__PORT_DELETION": { + "expected": True, + "config": Files.CONFIG_DB_WITH_PORT_CRITICAL, + "move": ps.JsonMove.from_operation({ "op": "remove", "path": "/PORT/Ethernet32" }) + }, + "PORT_UP__STATUS_CHANGING__NOT_UNDER_PORT__PORT_EXIST__PORT_DELETION": { + "expected": True, + "config": Files.CONFIG_DB_WITH_PORT_CRITICAL, + "target_config": self._apply_operations(Files.CONFIG_DB_WITH_PORT_CRITICAL, [ + { "op": "remove", "path": "/PORT/Ethernet28" }, + { "op": "remove", "path": "/BUFFER_PG/Ethernet28|0" }, + ]), + "move": ps.JsonMove.from_operation({ "op": "remove", "path": "/PORT/Ethernet28" }) + }, } def test_validate__no_critical_port_changes(self): @@ -1922,7 +1973,7 @@ def test_generate__all_tables_exist__no_moves(self): self.verify(current = {"ExistingTable1": { "Key1": "Value1" }, "ExistingTable2": {}}, target = {"ExistingTable1": {}, "ExistingTable2": { "Key2": "Value2" }}, ex_ops = []) - + def test_generate__multiple_cases__deletion_precedes_addition(self): self.verify(current = {"CommonTable": { "Key1": "Value1" }, "CurrentTable": {}}, target = {"CommonTable": {}, "TargetTable": { "Key2": "Value2" }}, @@ -2390,7 +2441,7 @@ def test_extend__multi_port_turn_up_and_critical_move__multi_flip_to_turn_down(s {"op":"add", "path":"/PORT/Ethernet16/admin_status", "value":"up"}, {"op":"add", "path":"/PORT/Ethernet12/mtu", "value":"9000"}, # Will not be part of the move, only in the final target config - {"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"}, + {"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"}, ]) move = ps.JsonMove.from_operation({ "op":"replace", @@ -2444,7 +2495,7 @@ def test_extend__multiple_changes__multiple_extend_moves(self): {"op":"add", "path":"/PORT/Ethernet16/admin_status", "value":"up"}, {"op":"add", "path":"/PORT/Ethernet12/mtu", "value":"9000"}, # Will not be part of the move, only in the final target config - {"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"}, + {"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"}, ]) move = ps.JsonMove.from_operation({ "op":"replace", @@ -2508,6 +2559,26 @@ def test_extend__multiple_changes__multiple_extend_moves(self): # Assert self._verify_moves(expected, actual) + def test_extend__port_deletion__no_extension(self): + # Arrange + move = ps.JsonMove.from_operation({ + "op":"remove", + "path":"/PORT/Ethernet28" + }) + current_config = Files.CONFIG_DB_WITH_PORT_CRITICAL + target_config = self._apply_operations(Files.CONFIG_DB_WITH_PORT_CRITICAL, [ + { "op": "remove", "path": "/PORT/Ethernet28" }, + { "op": "remove", "path": "/BUFFER_PG/Ethernet28|0" } + ]) + diff = ps.Diff(current_config, target_config) + expected = [] + + # Act + actual = self.extender.extend(move, diff) + + # Assert + self._verify_moves(expected, actual) + def _verify_moves(self, ex_ops, moves): moves_ops = [list(move.patch)[0] for move in moves] self.assertCountEqual(ex_ops, moves_ops)