Skip to content

Commit

Permalink
[GCU] Copying config_db before callding sonic_yang.loadData (#1983)
Browse files Browse the repository at this point in the history
#### What I did
Adding unit-test for cases where `PatchSorter.sort` is called to update config that has tables without YANG. This would sometimes throw the error
```
KeyError: 'NameOfTableWithoutYang'
```
This error happened because `find_ref_paths` would remove tables without yang as a side-effect.

Earlier this was fixed by cropping tables without yang models from current and target configs. That fix had 2 issues:
- It is not addressing the real problem which is cropping the config as a side effect to `find_ref_paths`
- If the move generated by the inner logic of `PatchSorter` was to replace whole config, it would result in replacing whole config with the config with cropped non-yang tables

#### How I did it
- Fixing `find_ref_paths` to not crop the given config as a side-effect
  - Added a unit-test for that
- Revert the change to crop current config and target config in `PatchSorter.sort`
  - Added a unit-test to verify replace whole config move does not result unintentionally in removing tables without yang
- Added unit-test to verify calling `PatchSorter.sort` on a config with non-yang tables does not result in the exception: 
  ```
  KeyError: 'NameOfTableWithoutYang'
  ```

#### How to verify it
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)
  • Loading branch information
ghooo authored Dec 27, 2021
1 parent a98858d commit 35cb524
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 6 deletions.
4 changes: 3 additions & 1 deletion generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,9 @@ def find_ref_paths(self, path, config):
def _find_leafref_paths(self, path, config):
sy = self.config_wrapper.create_sonic_yang_with_loaded_models()

sy.loadData(config)
tmp_config = copy.deepcopy(config)

sy.loadData(tmp_config)

xpath = self.convert_path_to_xpath(path, config, sy)

Expand Down
12 changes: 8 additions & 4 deletions generic_config_updater/patch_sorter.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ def apply_move(self, move):
def has_no_diff(self):
return self.current_config == self.target_config

def __str__(self):
return f"""current_config: {self.current_config}
target_config: {self.target_config}"""

def __repr__(self):
return str(self)

class JsonMove:
"""
A class similar to JsonPatch operation, but it allows the path to refer to non-existing middle elements.
Expand Down Expand Up @@ -1333,10 +1340,7 @@ def sort(self, patch, algorithm=Algorithm.DFS, preloaded_current_config=None):
current_config = preloaded_current_config if preloaded_current_config else self.config_wrapper.get_config_db_as_json()
target_config = self.patch_wrapper.simulate_patch(patch, current_config)

cropped_current_config = self.config_wrapper.crop_tables_without_yang(current_config)
cropped_target_config = self.config_wrapper.crop_tables_without_yang(target_config)

diff = Diff(cropped_current_config, cropped_target_config)
diff = Diff(current_config, target_config)

sort_algorithm = self.sort_algorithm_factory.create(algorithm)
moves = sort_algorithm.sort(diff)
Expand Down
38 changes: 38 additions & 0 deletions tests/generic_config_updater/files/patch_sorter_test_failure.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,43 @@
"expected_error_substrings": [
"There is no possible sorting"
]
},
"EMPTYING_A_CONFIGDB_TABLE_AND_CONFIG_HAS_NON_YANG_TABLE__FAILURE": {
"desc": [
"Emptying a configdb table fails because empty tables are not allowed in configdb.",
"User should remove whole table instead e.g. remove /ACL_TABLE in this case.",
"Also there is a table without YANG in the config, which the sorting logic will loop over.",
"The sorting logic should fail with GenericConfigUpdaterError: 'There is no possible sorting' and not KeyError: 'TABLE_WITHOUT_YANG'"
],
"current_config": {
"TABLE_WITHOUT_YANG": {
"key1": "value1",
"key2": "value2"
},
"ACL_TABLE": {
"EVERFLOW": {
"policy_desc": "EVERFLOW",
"ports": [
"Ethernet0"
],
"stage": "ingress",
"type": "MIRROR"
}
},
"PORT": {
"Ethernet0": {
"alias": "Eth1",
"lanes": "65, 66, 67, 68",
"description": "Ethernet0 100G link",
"speed": "100000"
}
}
},
"patch": [
{"op": "remove", "path": "/ACL_TABLE/EVERFLOW"}
],
"expected_error_substrings": [
"There is no possible sorting"
]
}
}
13 changes: 13 additions & 0 deletions tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import copy
import json
import jsonpatch
import sonic_yang
Expand Down Expand Up @@ -674,6 +675,18 @@ def test_find_ref_paths__whole_config_path__returns_all_refs(self):
# Assert
self.assertEqual(expected, actual)

def test_find_ref_paths__does_not_remove_tables_without_yang(self):
# Arrange
config = Files.CONFIG_DB_AS_JSON # This has a table without yang named 'TABLE_WITHOUT_YANG'
any_path = ""
expected_config = copy.deepcopy(config)

# Act
self.path_addressing.find_ref_paths(any_path, config)

# Assert
self.assertEqual(expected_config, config)

def test_convert_path_to_xpath(self):
def check(path, xpath, config=None):
if not config:
Expand Down
20 changes: 19 additions & 1 deletion tests/generic_config_updater/patch_sorter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1829,7 +1829,23 @@ def run_single_failure_case(self, data):
if notfound_substrings:
self.fail(f"Did not find the substrings {notfound_substrings} in the error: '{error}'")

def create_patch_sorter(self, config=None):
def test_sort__does_not_remove_tables_without_yang_unintentionally_if_generated_change_replaces_whole_config(self):
# Arrange
current_config = Files.CONFIG_DB_AS_JSON # has a table without yang named 'TABLE_WITHOUT_YANG'
any_patch = Files.SINGLE_OPERATION_CONFIG_DB_PATCH
target_config = any_patch.apply(current_config)
sort_algorithm = Mock()
sort_algorithm.sort = lambda diff: [ps.JsonMove(diff, OperationType.REPLACE, [], [])]
patch_sorter = self.create_patch_sorter(current_config, sort_algorithm)
expected = [JsonChange(jsonpatch.JsonPatch([OperationWrapper().create(OperationType.REPLACE, "", target_config)]))]

# Act
actual = patch_sorter.sort(any_patch)

# Assert
self.assertEqual(expected, actual)

def create_patch_sorter(self, config=None, sort_algorithm=None):
if config is None:
config=Files.CROPPED_CONFIG_DB_AS_JSON
config_wrapper = self.config_wrapper
Expand All @@ -1838,6 +1854,8 @@ def create_patch_sorter(self, config=None):
operation_wrapper = OperationWrapper()
path_addressing= ps.PathAddressing(config_wrapper)
sort_algorithm_factory = ps.SortAlgorithmFactory(operation_wrapper, config_wrapper, path_addressing)
if sort_algorithm:
sort_algorithm_factory.create = MagicMock(return_value=sort_algorithm)

return ps.PatchSorter(config_wrapper, patch_wrapper, sort_algorithm_factory)

Expand Down

0 comments on commit 35cb524

Please sign in to comment.