From a98858d021d92c9473e47c8c9b49fbddac1881e9 Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Mon, 27 Dec 2021 12:38:22 -0800 Subject: [PATCH] [GCU] Different apply-patch runs should produce same sorted steps (#1988) #### What I did Fixes #1976 Different runs of `apply-patch` would produce different sorted steps. It is better to be consistent while producing the sorted steps to make it easier to debug issues in the future. The issue is with using `set(list)` to remove duplicates from the list. Looping over the set elements does not guarantee same order every time we iterate. To reproduce this do the following: 1. Create a file named `test.py` with the content ```python x = ["XYZ", "ABC", "DEF"] y = set(x) print(y) ``` 2. Run `python3 test.py` 3. Again `python3 test.py` 4. Again `python3 test.py` The output of these runs will be different each time. #### How I did it Instead of returning a `set`, return a list of `ref_paths` which does not have duplicated elements inserted to begin with. #### 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) --- generic_config_updater/gu_common.py | 7 ++- .../files/patch_sorter_test_success.json | 51 +++++++++++++++---- .../generic_config_updater/gu_common_test.py | 32 ++++++------ .../patch_sorter_test.py | 11 ++-- 4 files changed, 69 insertions(+), 32 deletions(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index d7eef0f1b141..7fd45f618e38 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -414,11 +414,14 @@ def _find_leafref_paths(self, path, config): ref_xpaths.extend(sy.find_data_dependencies(xpath)) ref_paths = [] + ref_paths_set = set() for ref_xpath in ref_xpaths: ref_path = self.convert_xpath_to_path(ref_xpath, config, sy) - ref_paths.append(ref_path) + if ref_path not in ref_paths_set: + ref_paths.append(ref_path) + ref_paths_set.add(ref_path) - return set(ref_paths) + return ref_paths def _get_inner_leaf_xpaths(self, xpath, sy): if xpath == "/": # Point to Root element which contains all xpaths diff --git a/tests/generic_config_updater/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index 8d7b92d32fb7..14506503c7d5 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -698,7 +698,7 @@ [ { "op": "remove", - "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128" + "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132" } ], [ @@ -752,7 +752,7 @@ [ { "op": "add", - "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128", + "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132", "value": {} } ] @@ -1331,16 +1331,42 @@ "path": "/PORT/Ethernet3" } ], + [ + { + "op": "remove", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports" + } + ], [ { "op": "remove", "path": "/VLAN_MEMBER" } ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] + } + ], [ { "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports" + "path": "/ACL_TABLE" + } + ], + [ + { + "op": "add", + "path": "/ACL_TABLE", + "value": { + "NO-NSW-PACL-V4": { + "type": "L3" + } + } } ], [ @@ -1349,6 +1375,13 @@ "path": "/PORT" } ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", + "value": "NO-NSW-PACL-V4" + } + ], [ { "op": "add", @@ -2465,19 +2498,19 @@ [ { "op": "remove", - "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128" + "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132" } ], [ { "op": "remove", - "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132" + "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128" } ], [ { "op": "add", - "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128", + "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132", "value": {} } ], @@ -2490,20 +2523,20 @@ [ { "op": "add", - "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132", + "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128", "value": {} } ], [ { "op": "remove", - "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128" + "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132" } ], [ { "op": "remove", - "path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132" + "path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128" } ], [ diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 52cf9a818de1..083546c98012 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -590,7 +590,7 @@ def test_find_ref_paths__ref_is_the_whole_key__returns_ref_paths(self): actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_find_ref_paths__ref_is_a_part_of_key__returns_ref_paths(self): # Arrange @@ -605,7 +605,7 @@ def test_find_ref_paths__ref_is_a_part_of_key__returns_ref_paths(self): actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_find_ref_paths__ref_is_in_multilist__returns_ref_paths(self): # Arrange @@ -619,7 +619,7 @@ def test_find_ref_paths__ref_is_in_multilist__returns_ref_paths(self): actual = self.path_addressing.find_ref_paths(path, Files.CONFIG_DB_WITH_INTERFACE) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_find_ref_paths__ref_is_in_leafref_union__returns_ref_paths(self): # Arrange @@ -632,47 +632,47 @@ def test_find_ref_paths__ref_is_in_leafref_union__returns_ref_paths(self): actual = self.path_addressing.find_ref_paths(path, Files.CONFIG_DB_WITH_PORTCHANNEL_AND_ACL) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_find_ref_paths__path_is_table__returns_ref_paths(self): # Arrange path = "/PORT" expected = [ - "/ACL_TABLE/DATAACL/ports/0", - "/ACL_TABLE/EVERFLOW/ports/0", - "/ACL_TABLE/EVERFLOWV6/ports/0", - "/ACL_TABLE/EVERFLOWV6/ports/1", "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", "/VLAN_MEMBER/Vlan1000|Ethernet0", + "/ACL_TABLE/DATAACL/ports/0", + "/ACL_TABLE/EVERFLOWV6/ports/0", "/VLAN_MEMBER/Vlan1000|Ethernet4", - "/VLAN_MEMBER/Vlan1000|Ethernet8", + "/ACL_TABLE/EVERFLOW/ports/0", + "/ACL_TABLE/EVERFLOWV6/ports/1", + "/VLAN_MEMBER/Vlan1000|Ethernet8" ] # Act actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_find_ref_paths__whole_config_path__returns_all_refs(self): # Arrange path = "" expected = [ - "/ACL_TABLE/DATAACL/ports/0", - "/ACL_TABLE/EVERFLOW/ports/0", - "/ACL_TABLE/EVERFLOWV6/ports/0", - "/ACL_TABLE/EVERFLOWV6/ports/1", - "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", "/VLAN_MEMBER/Vlan1000|Ethernet0", "/VLAN_MEMBER/Vlan1000|Ethernet4", "/VLAN_MEMBER/Vlan1000|Ethernet8", + "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", + "/ACL_TABLE/DATAACL/ports/0", + "/ACL_TABLE/EVERFLOWV6/ports/0", + "/ACL_TABLE/EVERFLOW/ports/0", + "/ACL_TABLE/EVERFLOWV6/ports/1", ] # Act actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON) # Assert - self.assertCountEqual(expected, actual) + self.assertEqual(expected, actual) def test_convert_path_to_xpath(self): def check(path, xpath, config=None): diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 710de1860d3e..e79d18163bd2 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1747,6 +1747,9 @@ def verify(self, algo, algo_class): self.assertCountEqual(expected_validator, actual_validators) class TestPatchSorter(unittest.TestCase): + def setUp(self): + self.config_wrapper = ConfigWrapper() + def test_patch_sorter_success(self): # Format of the JSON file containing the test-cases: # @@ -1762,9 +1765,7 @@ def test_patch_sorter_success(self): # . # } data = Files.PATCH_SORTER_TEST_SUCCESS - # TODO: Investigate issue where different runs of patch-sorter generated different but correct steps - # Once investigation is complete remove the flag 'skip_exact_change_list_match' - skip_exact_change_list_match = True + skip_exact_change_list_match = False for test_case_name in data: with self.subTest(name=test_case_name): self.run_single_success_case(data[test_case_name], skip_exact_change_list_match) @@ -1787,7 +1788,7 @@ def run_single_success_case(self, data, skip_exact_change_list_match): simulated_config = current_config for change in actual_changes: simulated_config = change.apply(simulated_config) - self.assertTrue(ConfigWrapper().validate_config_db_config(simulated_config)) + self.assertTrue(self.config_wrapper.validate_config_db_config(simulated_config)) self.assertEqual(target_config, simulated_config) def test_patch_sorter_failure(self): @@ -1831,7 +1832,7 @@ def run_single_failure_case(self, data): def create_patch_sorter(self, config=None): if config is None: config=Files.CROPPED_CONFIG_DB_AS_JSON - config_wrapper = ConfigWrapper() + config_wrapper = self.config_wrapper config_wrapper.get_config_db_as_json = MagicMock(return_value=config) patch_wrapper = PatchWrapper(config_wrapper) operation_wrapper = OperationWrapper()