[generic-config-updater] Handling empty tables while sorting a patch (s…

…onic-net#1923)

#### What I did
Fixing issue sonic-net#1908 

#### How I did it
- When the given patch produces empty-table, reject it since ConfigDb cannot show empty tables. Achieved that by:
  - Adding a validation step before patch-sorting
- The patch-sorter should not produce steps which result in empty-tables because it is not possible in ConfigDb, we should instead delete the whole table. Achieved that by:
  - Adding a new validator to reject moves producing empty tables
  - No need to add a generator for deleting the whole table, current generators take care of that. They do by the following:
    - The move to empty a table is rejected by `NoEmptyTableMoveValidator`
    - The previous rejected move is used to generate moves using `UpperLevelMoveExtender` which produces either
      - Remove move for parent -- This is good, we are removing the table
      - Replace move for parent -- This later on will be extended to a Delete move using `DeleteInsteadOfReplaceMoveExtender`

The generators/validators are documented in the `PatchSorter.py`, check the documentation out.

#### How to verify it
unit-test

#### Previous command output (if the output of a command-line utility has changed)
```sh
admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... sonig-yang-mgmt logs
Patch Applier: Sorting patch updates.
... sonig-yang-mgmt logs
Patch Applier: The patch was sorted into 11 changes:
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/bgp_asn"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/buffer_model"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_bgp_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_pfcwd_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/deployment_id"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/docker_routing_config_mode"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hostname"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hwsku"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/mac"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/platform"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost"}]
Patch Applier: Applying changes in order.
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: After applying patch to config, there are still some parts not updated
admin@vlab-01:~$ 
```

The above error occurs because post the update, the whole `DEVICE_METADATA` table is deleted, not just showing as empty i.e. `"DEVICE_METADATA": {}`

#### New command output (if the output of a command-line utility has changed)
```
admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch is not valid because it will result in empty tables which is not allowed in ConfigDb. Table: DEVICE_METADATA
admin@vlab-01:~$ 
```

generic_config_updater/generic_updater.py

@@ -52,7 +52,17 @@ def apply(self, patch):
         self.logger.log_notice("Simulating the target full config after applying the patch.")
         target_config = self.patch_wrapper.simulate_patch(patch, old_config)
 
-        # Validate target config
+        # Validate target config does not have empty tables since they do not show up in ConfigDb
+        self.logger.log_notice("Validating target config does not have empty tables, " \
+                               "since they do not show up in ConfigDb.")
+        empty_tables = self.config_wrapper.get_empty_tables(target_config)
+        if empty_tables: # if there are empty tables
+            empty_tables_txt = ", ".join(empty_tables)
+            raise ValueError("Given patch is not valid because it will result in empty tables " \
+                             "which is not allowed in ConfigDb. " \
+                            f"Table{'s' if len(empty_tables) != 1 else ''}: {empty_tables_txt}")
+
+        # Validate target config according to YANG models
         self.logger.log_notice("Validating target config according to YANG models.")
         if not(self.config_wrapper.validate_config_db_config(target_config)):
             raise ValueError(f"Given patch is not valid because it will result in an invalid config")

generic_config_updater/gu_common.py

@@ -133,6 +133,14 @@ def crop_tables_without_yang(self, config_db_as_json):
         sy._cropConfigDB()
 
         return sy.jIn
+
+    def get_empty_tables(self, config):
+        empty_tables = []
+        for key in config.keys():
+            if not(config[key]):
+                empty_tables.append(key)
+        return empty_tables
+
 
 class DryRunConfigWrapper(ConfigWrapper):
     # TODO: implement DryRunConfigWrapper

generic_config_updater/patch_sorter.py

@@ -573,6 +573,35 @@ def _find_ref_paths(self, paths, config):
             refs.extend(self.path_addressing.find_ref_paths(path, config))
         return refs
 
+class NoEmptyTableMoveValidator:
+    """
+    A class to validate that a move will not result in an empty table, because empty table do not show up in ConfigDB.
+    """
+    def __init__(self, path_addressing):
+        self.path_addressing = path_addressing
+
+    def validate(self, move, diff):
+        simulated_config = move.apply(diff.current_config)
+        op_path = move.path
+
+        if op_path == "": # If updating whole file
+            tables_to_check = simulated_config.keys()
+        else:
+            tokens = self.path_addressing.get_path_tokens(op_path)
+            tables_to_check = [tokens[0]]
+
+        return self._validate_tables(tables_to_check, simulated_config)
+
+    def _validate_tables(self, tables, config):
+        for table in tables:
+            if not(self._validate_table(table, config)):
+                return False
+        return True
+
+    def _validate_table(self, table, config):
+        # the only invalid case is if table exists and is empty
+        return table not in config or config[table]
+
 class LowLevelMoveGenerator:
     """
     A class to generate the low level moves i.e. moves corresponding to differences between current/target config
@@ -969,7 +998,8 @@ def create(self, algorithm=Algorithm.DFS):
                            FullConfigMoveValidator(self.config_wrapper),
                            NoDependencyMoveValidator(self.path_addressing, self.config_wrapper),
                            UniqueLanesMoveValidator(),
-                           CreateOnlyMoveValidator(self.path_addressing) ]
+                           CreateOnlyMoveValidator(self.path_addressing),
+                           NoEmptyTableMoveValidator(self.path_addressing)]
 
         move_wrapper = MoveWrapper(move_generators, move_extenders, move_validators)
 

tests/generic_config_updater/files/config_db_with_interface.json

@@ -4,7 +4,8 @@
         "Ethernet8|10.0.0.1/30": {
             "family": "IPv4",
             "scope": "global"
-        }
+        },
+        "Ethernet9": {}
     },
     "PORT": {
         "Ethernet8": {
@@ -15,6 +16,15 @@
             "lanes": "65",
             "mtu": "9000",
             "speed": "25000"
+        },
+        "Ethernet9": {
+            "admin_status": "up",
+            "alias": "eth9",
+            "description": "Ethernet9",
+            "fec": "rs",
+            "lanes": "6",
+            "mtu": "9000",
+            "speed": "25000"
         }
     }
 }

tests/generic_config_updater/generic_updater_test.py

@@ -19,6 +19,13 @@ def test_apply__invalid_patch_updating_tables_without_yang_models__failure(self)
         # Act and assert
         self.assertRaises(ValueError, patch_applier.apply, Files.MULTI_OPERATION_CONFIG_DB_PATCH)
 
+    def test_apply__invalid_patch_producing_empty_tables__failure(self):
+        # Arrange
+        patch_applier = self.__create_patch_applier(valid_patch_does_not_produce_empty_tables=False)
+
+        # Act and assert
+        self.assertRaises(ValueError, patch_applier.apply, Files.MULTI_OPERATION_CONFIG_DB_PATCH)
+
     def test_apply__invalid_config_db__failure(self):
         # Arrange
         patch_applier = self.__create_patch_applier(valid_config_db=False)
@@ -58,12 +65,16 @@ def __create_patch_applier(self,
                                changes=None,
                                valid_patch_only_tables_with_yang_models=True,
                                valid_config_db=True,
+                               valid_patch_does_not_produce_empty_tables=True,
                                verified_same_config=True):
         config_wrapper = Mock()
         config_wrapper.get_config_db_as_json.side_effect = \
             [Files.CONFIG_DB_AS_JSON, Files.CONFIG_DB_AFTER_MULTI_PATCH]
         config_wrapper.validate_config_db_config.side_effect = \
             create_side_effect_dict({(str(Files.CONFIG_DB_AFTER_MULTI_PATCH),): valid_config_db})
+        empty_tables = [] if valid_patch_does_not_produce_empty_tables else ["AnyTable"]
+        config_wrapper.get_empty_tables.side_effect = \
+            create_side_effect_dict({(str(Files.CONFIG_DB_AFTER_MULTI_PATCH),): empty_tables})
 
         patch_wrapper = Mock()
         patch_wrapper.validate_config_db_patch_has_yang_models.side_effect = \

tests/generic_config_updater/gu_common_test.py

@@ -148,6 +148,39 @@ def test_crop_tables_without_yang__returns_cropped_config_db_as_json(self):
         # Assert
         self.assertDictEqual(expected, actual)
 
+    def test_get_empty_tables__no_empty_tables__returns_no_tables(self):
+        # Arrange
+        config_wrapper = gu_common.ConfigWrapper()
+        config = {"any_table": {"key": "value"}}
+
+        # Act
+        empty_tables = config_wrapper.get_empty_tables(config)
+
+        # Assert
+        self.assertCountEqual([], empty_tables)
+
+    def test_get_empty_tables__single_empty_table__returns_one_table(self):
+        # Arrange
+        config_wrapper = gu_common.ConfigWrapper()
+        config = {"any_table": {"key": "value"}, "another_table":{}}
+
+        # Act
+        empty_tables = config_wrapper.get_empty_tables(config)
+
+        # Assert
+        self.assertCountEqual(["another_table"], empty_tables)
+
+    def test_get_empty_tables__multiple_empty_tables__returns_multiple_tables(self):
+        # Arrange
+        config_wrapper = gu_common.ConfigWrapper()
+        config = {"any_table": {"key": "value"}, "another_table":{}, "yet_another_table":{}}
+
+        # Act
+        empty_tables = config_wrapper.get_empty_tables(config)
+
+        # Assert
+        self.assertCountEqual(["another_table", "yet_another_table"], empty_tables)
+
 class TestPatchWrapper(unittest.TestCase):
     def setUp(self):
         self.config_wrapper_mock = gu_common.ConfigWrapper()

tests/generic_config_updater/patch_sorter_test.py

@@ -1008,6 +1008,121 @@ def test_validate__replace_whole_config_item_same_ref_same__true(self):
     def prepare_config(self, config, patch):
         return patch.apply(config)
 
+class TestNoEmptyTableMoveValidator(unittest.TestCase):
+    def setUp(self):
+        path_addressing = ps.PathAddressing()
+        self.validator = ps.NoEmptyTableMoveValidator(path_addressing)
+
+    def test_validate__no_changes__success(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1", "key2":"value2"}}
+        target_config = {"some_table":{"key1":"value1", "key2":"value22"}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key1"], ["some_table", "key1"])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
+    def test_validate__change_but_no_empty_table__success(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1", "key2":"value2"}}
+        target_config = {"some_table":{"key1":"value1", "key2":"value22"}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key2"], ["some_table", "key2"])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
+    def test_validate__single_empty_table__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1", "key2":"value2"}}
+        target_config = {"some_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table"], ["some_table"])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__whole_config_replace_single_empty_table__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1", "key2":"value2"}}
+        target_config = {"some_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, [], [])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__whole_config_replace_mix_of_empty_and_non_empty__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"some_table":{"key1":"value1"}, "other_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, [], [])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__whole_config_multiple_empty_tables__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"some_table":{}, "other_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REPLACE, [], [])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__remove_key_empties_a_table__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"some_table":{"key1":"value1"}, "other_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__remove_key_but_table_has_other_keys__success(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2", "key3":"value3"}}
+        target_config = {"some_table":{"key1":"value1"}, "other_table":{"key3":"value3"}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
+    def test_validate__remove_whole_table__success(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"some_table":{"key1":"value1"}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table"], [])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
+    def test_validate__add_empty_table__failure(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"new_table":{}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])
+
+        # Act and assert
+        self.assertFalse(self.validator.validate(move, diff))
+
+    def test_validate__add_non_empty_table__success(self):
+        # Arrange
+        current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}}
+        target_config = {"new_table":{"key3":"value3"}}
+        diff = ps.Diff(current_config, target_config)
+        move = ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
 class TestLowLevelMoveGenerator(unittest.TestCase):
     def setUp(self):
         path_addressing = PathAddressing()
@@ -1566,7 +1681,8 @@ def verify(self, algo, algo_class):
                               ps.FullConfigMoveValidator,
                               ps.NoDependencyMoveValidator,
                               ps.UniqueLanesMoveValidator,
-                              ps.CreateOnlyMoveValidator]
+                              ps.CreateOnlyMoveValidator,
+                              ps.NoEmptyTableMoveValidator]
 
         # Act
         sorter = factory.create(algo)
@@ -1712,13 +1828,34 @@ def test_sort__modify_items_with_dependencies_using_must__success(self):
                             {"op":"replace", "path":"/CRM/Config/acl_counter_low_threshold", "value":"60"}],
                     cc_ops=[{"op":"replace", "path":"", "value":Files.CONFIG_DB_WITH_CRM}])
 
+    def test_sort__modify_items_result_in_empty_table__failure(self):
+        # Emptying a table
+        self.assertRaises(GenericConfigUpdaterError,
+                          self.verify,
+                          cc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}],
+                          tc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS},
+                                  {"op":"replace", "path":"/ACL_TABLE", "value":{}}])
+        # Adding an empty table
+        self.assertRaises(GenericConfigUpdaterError,
+                          self.verify,
+                          cc_ops=[{"op":"replace", "path":"", "value":Files.ANY_CONFIG_DB}],
+                          tc_ops=[{"op":"replace", "path":"", "value":Files.ANY_CONFIG_DB},
+                                  {"op":"add", "path":"/VLAN", "value":{}}])
+        # Emptying multiple tables
+        self.assertRaises(GenericConfigUpdaterError,
+                          self.verify,
+                          cc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}],
+                          tc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS},
+                                  {"op":"replace", "path":"/ACL_TABLE", "value":{}},
+                                  {"op":"replace", "path":"/PORT", "value":{}}])
+
     def verify(self, cc_ops=[], tc_ops=[]):
         # Arrange
         config_wrapper=ConfigWrapper()
         target_config=jsonpatch.JsonPatch(tc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON)
         current_config=jsonpatch.JsonPatch(cc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON)
         patch=jsonpatch.make_patch(current_config, target_config)
-        
+
         # Act
         actual = self.create_patch_sorter(current_config).sort(patch)