[GCU] Moving UniqueLanes from only validating moves, to be a supplemental YANG validator (#2234)

#### What I did
- Added a new supplemental YANG validator to validate UniqueLanes in `validate_config_db_config`
- Removed UniqueLanesMoveValidator as the lanes validation will be taken care of by FullConfigMoveValidator which uses `validate_config_db_config`

The benefit of this is at the beginning of `apply-patch` we make a call to `validate_config_db_config` to check if the given patch is valid or not ([code](https://github.com/Azure/sonic-utilities/blob/e6e4f8ceb9a59fb7b3767a65ffc4f017d0807832/generic_config_updater/patch_sorter.py#L1522)). Now we will fail early, instead of going for the move generation and not being able to generate a moves.

#### How I did it
Check code.

#### How to verify it
Added unit-tests.

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)

Author: ghooo

generic_config_updater/gu_common.py

@@ -113,18 +113,57 @@ def validate_sonic_yang_config(self, sonic_yang_as_json):
     def validate_config_db_config(self, config_db_as_json):
         sy = self.create_sonic_yang_with_loaded_models()
 
+        # TODO: Move these validators to YANG models
+        supplemental_yang_validators = [self.validate_bgp_peer_group,
+                                        self.validate_lanes]
+
         try:
             tmp_config_db_as_json = copy.deepcopy(config_db_as_json)
 
             sy.loadData(tmp_config_db_as_json)
 
             sy.validate_data_tree()
 
-            # TODO: modularize custom validations better or move directly to sonic-yang module
-            return self.validate_bgp_peer_group(config_db_as_json)
+            for supplemental_yang_validator in supplemental_yang_validators:
+                success, error = supplemental_yang_validator(config_db_as_json)
+                if not success:
+                    return success, error
         except sonic_yang.SonicYangException as ex:
             return False, ex
 
+        return True, None
+
+    def validate_lanes(self, config_db):
+        if "PORT" not in config_db:
+            return True, None
+
+        ports = config_db["PORT"]
+
+        # Validate each lane separately, make sure it is not empty, and is a number
+        port_to_lanes_map = {}
+        for port in ports:
+            attrs = ports[port]
+            if "lanes" in attrs:
+                lanes_str = attrs["lanes"]
+                lanes_with_whitespaces = lanes_str.split(",")
+                lanes = [lane.strip() for lane in lanes_with_whitespaces]
+                for lane in lanes:
+                    if not lane:
+                        return False, f"PORT '{port}' has an empty lane"
+                    if not lane.isdigit():
+                        return False, f"PORT '{port}' has an invalid lane '{lane}'"
+                port_to_lanes_map[port] = lanes
+
+        # Validate lanes are unique
+        existing = {}
+        for port in port_to_lanes_map:
+            lanes = port_to_lanes_map[port]
+            for lane in lanes:
+                if lane in existing:
+                    return False, f"'{lane}' lane is used multiple times in PORT: {set([port, existing[lane]])}"
+                existing[lane] = port
+        return True, None
+
     def validate_bgp_peer_group(self, config_db):
         if "BGP_PEER_RANGE" not in config_db:
             return True, None

generic_config_updater/patch_sorter.py

@@ -565,30 +565,6 @@ def validate(self, move, diff):
         is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config)
         return is_valid
 
-# TODO: Add this validation to YANG models instead
-class UniqueLanesMoveValidator:
-    """
-    A class to validate lanes and any port are unique between all ports.
-    """
-    def validate(self, move, diff):
-        simulated_config = move.apply(diff.current_config)
-
-        if "PORT" not in simulated_config:
-            return True
-
-        ports = simulated_config["PORT"]
-        existing = set()
-        for port in ports:
-            attrs = ports[port]
-            if "lanes" in attrs:
-                lanes_str = attrs["lanes"]
-                lanes = lanes_str.split(", ")
-                for lane in lanes:
-                    if lane in existing:
-                        return False
-                    existing.add(lane)
-        return True
-
 class CreateOnlyMoveValidator:
     """
     A class to validate create-only fields are only created, but never modified/updated. In other words:
@@ -1507,7 +1483,6 @@ def create(self, algorithm=Algorithm.DFS):
         move_validators = [DeleteWholeConfigMoveValidator(),
                            FullConfigMoveValidator(self.config_wrapper),
                            NoDependencyMoveValidator(self.path_addressing, self.config_wrapper),
-                           UniqueLanesMoveValidator(),
                            CreateOnlyMoveValidator(self.path_addressing),
                            RequiredValueMoveValidator(self.path_addressing),
                            NoEmptyTableMoveValidator(self.path_addressing)]

tests/generic_config_updater/gu_common_test.py

@@ -260,6 +260,84 @@ def check_validate_bgp_peer_group(self, ip_range, other_ip_range=[], duplicated_
         self.assertFalse(actual)
         self.assertTrue(duplicated_ip in error)
 
+    def test_validate_lanes__no_port_table__success(self):
+        config = {"ACL_TABLE": {}}
+        self.validate_lanes(config)
+
+    def test_validate_lanes__empty_port_table__success(self):
+        config = {"PORT": {}}
+        self.validate_lanes(config)
+
+    def test_validate_lanes__empty_lane__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "", "speed":"10000"}}}
+        self.validate_lanes(config, 'has an empty lane')
+
+    def test_validate_lanes__whitespace_lane__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": " ", "speed":"10000"}}}
+        self.validate_lanes(config, 'has an empty lane')
+
+    def test_validate_lanes__non_digits_lane__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "10g", "speed":"10000"}}}
+        self.validate_lanes(config, "has an invalid lane '10g'")
+
+    def test_validate_lanes__space_between_digits_lane__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": " 1 0  ", "speed":"10000"}}}
+        self.validate_lanes(config, "has an invalid lane '1 0'")
+
+    def test_validate_lanes__single_valid_lane__success(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "66", "speed":"10000"}}}
+        self.validate_lanes(config)
+
+    def test_validate_lanes__different_valid_lanes_single_port__success(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "66, 67, 68", "speed":"10000"}}}
+        self.validate_lanes(config)
+
+    def test_validate_lanes__different_valid_and_invalid_empty_lanes_single_port__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "66, , 68", "speed":"10000"}}}
+        self.validate_lanes(config, 'has an empty lane')
+
+    def test_validate_lanes__different_valid_and_invalid_non_digit_lanes_single_port__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "66, 67, 10g", "speed":"10000"}}}
+        self.validate_lanes(config, "has an invalid lane '10g'")
+
+    def test_validate_lanes__different_valid_lanes_multi_ports__success(self):
+        config = {"PORT": {
+            "Ethernet0": {"lanes": " 64 , 65 \t", "speed":"10000"},
+            "Ethernet1": {"lanes": " 66 , 67 \r\t\n, 68 ", "speed":"10000"},
+            }}
+        self.validate_lanes(config)
+
+    def test_validate_lanes__same_valid_lanes_single_port__failure(self):
+        config = {"PORT": {"Ethernet0": {"lanes": "65 \r\t\n, 65", "speed":"10000"}}}
+        self.validate_lanes(config, '65')
+
+    def test_validate_lanes__same_valid_lanes_multi_ports__failure(self):
+        config = {"PORT": {
+            "Ethernet0": {"lanes": "64, 65, 67", "speed":"10000"},
+            "Ethernet1": {"lanes": "66, 67, 68", "speed":"10000"},
+            }}
+        self.validate_lanes(config, '67')
+
+    def test_validate_lanes__same_valid_lanes_multi_ports_no_spaces__failure(self):
+        config = {"PORT": {
+            "Ethernet0": {"lanes": "64,65,67", "speed":"10000"},
+            "Ethernet1": {"lanes": "66,67,68", "speed":"10000"},
+            }}
+        self.validate_lanes(config, '67')
+
+    def validate_lanes(self, config_db, expected_error=None):
+        # Arrange
+        config_wrapper = gu_common.ConfigWrapper()
+        expected = expected_error is None # if expected_error is None, then the input is valid
+
+        # Act
+        actual, error = config_wrapper.validate_lanes(config_db)
+
+        # Assert
+        self.assertEqual(expected, actual)
+        if expected_error:
+            self.assertTrue(expected_error in error)
+
     def test_crop_tables_without_yang__returns_cropped_config_db_as_json(self):
         # Arrange
         config_wrapper = gu_common.ConfigWrapper()

tests/generic_config_updater/patch_sorter_test.py

@@ -868,49 +868,6 @@ def verify(self, operation_type, path, expected):
         # Assert
         self.assertEqual(expected, actual)
 
-class TestUniqueLanesMoveValidator(unittest.TestCase):
-    def setUp(self):
-        self.validator = ps.UniqueLanesMoveValidator()
-
-    def test_validate__no_port_table__success(self):
-        config = {"ACL_TABLE": {}}
-        self.validate_target_config(config)
-
-    def test_validate__empty_port_table__success(self):
-        config = {"PORT": {}}
-        self.validate_target_config(config)
-
-    def test_validate__single_lane__success(self):
-        config = {"PORT": {"Ethernet0": {"lanes": "66", "speed":"10000"}}}
-        self.validate_target_config(config)
-
-    def test_validate__different_lanes_single_port___success(self):
-        config = {"PORT": {"Ethernet0": {"lanes": "66, 67, 68", "speed":"10000"}}}
-        self.validate_target_config(config)
-
-    def test_validate__different_lanes_multi_ports___success(self):
-        config = {"PORT": {
-            "Ethernet0": {"lanes": "64, 65", "speed":"10000"},
-            "Ethernet1": {"lanes": "66, 67, 68", "speed":"10000"},
-            }}
-        self.validate_target_config(config)
-
-    def test_validate__same_lanes_single_port___success(self):
-        config = {"PORT": {"Ethernet0": {"lanes": "65, 65", "speed":"10000"}}}
-        self.validate_target_config(config, False)
-
-    def validate_target_config(self, target_config, expected=True):
-        # Arrange
-        current_config = {}
-        diff = ps.Diff(current_config, target_config)
-        move = ps.JsonMove(diff, OperationType.REPLACE, [], [])
-
-        # Act
-        actual = self.validator.validate(move, diff)
-
-        # Assert
-        self.assertEqual(expected, actual)
-
 class TestFullConfigMoveValidator(unittest.TestCase):
     def setUp(self):
         self.any_current_config = Mock()
@@ -3038,7 +2995,6 @@ def verify(self, algo, algo_class):
         expected_validator = [ps.DeleteWholeConfigMoveValidator,
                               ps.FullConfigMoveValidator,
                               ps.NoDependencyMoveValidator,
-                              ps.UniqueLanesMoveValidator,
                               ps.CreateOnlyMoveValidator,
                               ps.RequiredValueMoveValidator,
                               ps.NoEmptyTableMoveValidator]