fixing required value validator, and adding replacelane generator

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)

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"
         }
     }
 }

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)