Skip to content

Commit

Permalink
[generic-config-updater] Handling empty tables while sorting a patch (s…
Browse files Browse the repository at this point in the history
…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:~$ 
```
  • Loading branch information
ghooo authored Nov 18, 2021
1 parent fdedcbf commit 2ec47a5
Show file tree
Hide file tree
Showing 7 changed files with 244 additions and 5 deletions.
12 changes: 11 additions & 1 deletion generic_config_updater/generic_updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
8 changes: 8 additions & 0 deletions generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 31 additions & 1 deletion generic_config_updater/patch_sorter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down
12 changes: 11 additions & 1 deletion tests/generic_config_updater/files/config_db_with_interface.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"Ethernet8|10.0.0.1/30": {
"family": "IPv4",
"scope": "global"
}
},
"Ethernet9": {}
},
"PORT": {
"Ethernet8": {
Expand All @@ -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"
}
}
}
11 changes: 11 additions & 0 deletions tests/generic_config_updater/generic_updater_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 = \
Expand Down
33 changes: 33 additions & 0 deletions tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
141 changes: 139 additions & 2 deletions tests/generic_config_updater/patch_sorter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 2ec47a5

Please sign in to comment.