From 2ec47a5952549c8845820969ef6a318906130983 Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Wed, 17 Nov 2021 16:46:39 -0800 Subject: [PATCH] [generic-config-updater] Handling empty tables while sorting a patch (#1923) #### What I did Fixing issue #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 | 12 +- generic_config_updater/gu_common.py | 8 + generic_config_updater/patch_sorter.py | 32 +++- .../files/config_db_with_interface.json | 12 +- .../generic_updater_test.py | 11 ++ .../generic_config_updater/gu_common_test.py | 33 ++++ .../patch_sorter_test.py | 141 +++++++++++++++++- 7 files changed, 244 insertions(+), 5 deletions(-) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index d004abdbcc75..1979faa1d566 100644 --- a/generic_config_updater/generic_updater.py +++ b/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") diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 1f213e437150..3e775f1bb260 100644 --- a/generic_config_updater/gu_common.py +++ b/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 diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index ebc7c572ff14..f781576fab91 100644 --- a/generic_config_updater/patch_sorter.py +++ b/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) diff --git a/tests/generic_config_updater/files/config_db_with_interface.json b/tests/generic_config_updater/files/config_db_with_interface.json index 2e1c488a4a0c..abd97d987505 100644 --- a/tests/generic_config_updater/files/config_db_with_interface.json +++ b/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" } } } diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index f201280062a5..ef6a2192430f 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/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 = \ diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index f69ec08030c3..842e71baaa31 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/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() diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 4da9fb901b42..c51733abe72b 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/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)