Skip to content

Commit a98858d

Browse files
ghoooghooo
authored
[GCU] Different apply-patch runs should produce same sorted steps (sonic-net#1988)
#### What I did Fixes sonic-net#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)
1 parent 8c81ae3 commit a98858d

File tree

4 files changed

+69
-32
lines changed

4 files changed

+69
-32
lines changed

‎generic_config_updater/gu_common.py

+5-2
Original file line numberDiff line numberDiff line change
@@ -414,11 +414,14 @@ def _find_leafref_paths(self, path, config):
414414
ref_xpaths.extend(sy.find_data_dependencies(xpath))
415415

416416
ref_paths = []
417+
ref_paths_set = set()
417418
for ref_xpath in ref_xpaths:
418419
ref_path = self.convert_xpath_to_path(ref_xpath, config, sy)
419-
ref_paths.append(ref_path)
420+
if ref_path not in ref_paths_set:
421+
ref_paths.append(ref_path)
422+
ref_paths_set.add(ref_path)
420423

421-
return set(ref_paths)
424+
return ref_paths
422425

423426
def _get_inner_leaf_xpaths(self, xpath, sy):
424427
if xpath == "/": # Point to Root element which contains all xpaths

‎tests/generic_config_updater/files/patch_sorter_test_success.json

+42-9
Original file line numberDiff line numberDiff line change
@@ -698,7 +698,7 @@
698698
[
699699
{
700700
"op": "remove",
701-
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128"
701+
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132"
702702
}
703703
],
704704
[
@@ -752,7 +752,7 @@
752752
[
753753
{
754754
"op": "add",
755-
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128",
755+
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132",
756756
"value": {}
757757
}
758758
]
@@ -1331,16 +1331,42 @@
13311331
"path": "/PORT/Ethernet3"
13321332
}
13331333
],
1334+
[
1335+
{
1336+
"op": "remove",
1337+
"path": "/ACL_TABLE/NO-NSW-PACL-V4/ports"
1338+
}
1339+
],
13341340
[
13351341
{
13361342
"op": "remove",
13371343
"path": "/VLAN_MEMBER"
13381344
}
13391345
],
1346+
[
1347+
{
1348+
"op": "add",
1349+
"path": "/ACL_TABLE/NO-NSW-PACL-V4/ports",
1350+
"value": [
1351+
"Ethernet0"
1352+
]
1353+
}
1354+
],
13401355
[
13411356
{
13421357
"op": "remove",
1343-
"path": "/ACL_TABLE/NO-NSW-PACL-V4/ports"
1358+
"path": "/ACL_TABLE"
1359+
}
1360+
],
1361+
[
1362+
{
1363+
"op": "add",
1364+
"path": "/ACL_TABLE",
1365+
"value": {
1366+
"NO-NSW-PACL-V4": {
1367+
"type": "L3"
1368+
}
1369+
}
13441370
}
13451371
],
13461372
[
@@ -1349,6 +1375,13 @@
13491375
"path": "/PORT"
13501376
}
13511377
],
1378+
[
1379+
{
1380+
"op": "add",
1381+
"path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc",
1382+
"value": "NO-NSW-PACL-V4"
1383+
}
1384+
],
13521385
[
13531386
{
13541387
"op": "add",
@@ -2465,19 +2498,19 @@
24652498
[
24662499
{
24672500
"op": "remove",
2468-
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128"
2501+
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132"
24692502
}
24702503
],
24712504
[
24722505
{
24732506
"op": "remove",
2474-
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132"
2507+
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128"
24752508
}
24762509
],
24772510
[
24782511
{
24792512
"op": "add",
2480-
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128",
2513+
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132",
24812514
"value": {}
24822515
}
24832516
],
@@ -2490,20 +2523,20 @@
24902523
[
24912524
{
24922525
"op": "add",
2493-
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132",
2526+
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128",
24942527
"value": {}
24952528
}
24962529
],
24972530
[
24982531
{
24992532
"op": "remove",
2500-
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128"
2533+
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132"
25012534
}
25022535
],
25032536
[
25042537
{
25052538
"op": "remove",
2506-
"path": "/LOOPBACK_INTERFACE/Loopback1|20.2.0.32~132"
2539+
"path": "/LOOPBACK_INTERFACE/Loopback1|2200:2::32~1128"
25072540
}
25082541
],
25092542
[

‎tests/generic_config_updater/gu_common_test.py

+16-16
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,7 @@ def test_find_ref_paths__ref_is_the_whole_key__returns_ref_paths(self):
590590
actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON)
591591

592592
# Assert
593-
self.assertCountEqual(expected, actual)
593+
self.assertEqual(expected, actual)
594594

595595
def test_find_ref_paths__ref_is_a_part_of_key__returns_ref_paths(self):
596596
# Arrange
@@ -605,7 +605,7 @@ def test_find_ref_paths__ref_is_a_part_of_key__returns_ref_paths(self):
605605
actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON)
606606

607607
# Assert
608-
self.assertCountEqual(expected, actual)
608+
self.assertEqual(expected, actual)
609609

610610
def test_find_ref_paths__ref_is_in_multilist__returns_ref_paths(self):
611611
# Arrange
@@ -619,7 +619,7 @@ def test_find_ref_paths__ref_is_in_multilist__returns_ref_paths(self):
619619
actual = self.path_addressing.find_ref_paths(path, Files.CONFIG_DB_WITH_INTERFACE)
620620

621621
# Assert
622-
self.assertCountEqual(expected, actual)
622+
self.assertEqual(expected, actual)
623623

624624
def test_find_ref_paths__ref_is_in_leafref_union__returns_ref_paths(self):
625625
# Arrange
@@ -632,47 +632,47 @@ def test_find_ref_paths__ref_is_in_leafref_union__returns_ref_paths(self):
632632
actual = self.path_addressing.find_ref_paths(path, Files.CONFIG_DB_WITH_PORTCHANNEL_AND_ACL)
633633

634634
# Assert
635-
self.assertCountEqual(expected, actual)
635+
self.assertEqual(expected, actual)
636636

637637
def test_find_ref_paths__path_is_table__returns_ref_paths(self):
638638
# Arrange
639639
path = "/PORT"
640640
expected = [
641-
"/ACL_TABLE/DATAACL/ports/0",
642-
"/ACL_TABLE/EVERFLOW/ports/0",
643-
"/ACL_TABLE/EVERFLOWV6/ports/0",
644-
"/ACL_TABLE/EVERFLOWV6/ports/1",
645641
"/ACL_TABLE/NO-NSW-PACL-V4/ports/0",
646642
"/VLAN_MEMBER/Vlan1000|Ethernet0",
643+
"/ACL_TABLE/DATAACL/ports/0",
644+
"/ACL_TABLE/EVERFLOWV6/ports/0",
647645
"/VLAN_MEMBER/Vlan1000|Ethernet4",
648-
"/VLAN_MEMBER/Vlan1000|Ethernet8",
646+
"/ACL_TABLE/EVERFLOW/ports/0",
647+
"/ACL_TABLE/EVERFLOWV6/ports/1",
648+
"/VLAN_MEMBER/Vlan1000|Ethernet8"
649649
]
650650

651651
# Act
652652
actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON)
653653

654654
# Assert
655-
self.assertCountEqual(expected, actual)
655+
self.assertEqual(expected, actual)
656656

657657
def test_find_ref_paths__whole_config_path__returns_all_refs(self):
658658
# Arrange
659659
path = ""
660660
expected = [
661-
"/ACL_TABLE/DATAACL/ports/0",
662-
"/ACL_TABLE/EVERFLOW/ports/0",
663-
"/ACL_TABLE/EVERFLOWV6/ports/0",
664-
"/ACL_TABLE/EVERFLOWV6/ports/1",
665-
"/ACL_TABLE/NO-NSW-PACL-V4/ports/0",
666661
"/VLAN_MEMBER/Vlan1000|Ethernet0",
667662
"/VLAN_MEMBER/Vlan1000|Ethernet4",
668663
"/VLAN_MEMBER/Vlan1000|Ethernet8",
664+
"/ACL_TABLE/NO-NSW-PACL-V4/ports/0",
665+
"/ACL_TABLE/DATAACL/ports/0",
666+
"/ACL_TABLE/EVERFLOWV6/ports/0",
667+
"/ACL_TABLE/EVERFLOW/ports/0",
668+
"/ACL_TABLE/EVERFLOWV6/ports/1",
669669
]
670670

671671
# Act
672672
actual = self.path_addressing.find_ref_paths(path, Files.CROPPED_CONFIG_DB_AS_JSON)
673673

674674
# Assert
675-
self.assertCountEqual(expected, actual)
675+
self.assertEqual(expected, actual)
676676

677677
def test_convert_path_to_xpath(self):
678678
def check(path, xpath, config=None):

‎tests/generic_config_updater/patch_sorter_test.py

+6-5
Original file line numberDiff line numberDiff line change
@@ -1747,6 +1747,9 @@ def verify(self, algo, algo_class):
17471747
self.assertCountEqual(expected_validator, actual_validators)
17481748

17491749
class TestPatchSorter(unittest.TestCase):
1750+
def setUp(self):
1751+
self.config_wrapper = ConfigWrapper()
1752+
17501753
def test_patch_sorter_success(self):
17511754
# Format of the JSON file containing the test-cases:
17521755
#
@@ -1762,9 +1765,7 @@ def test_patch_sorter_success(self):
17621765
# .
17631766
# }
17641767
data = Files.PATCH_SORTER_TEST_SUCCESS
1765-
# TODO: Investigate issue where different runs of patch-sorter generated different but correct steps
1766-
# Once investigation is complete remove the flag 'skip_exact_change_list_match'
1767-
skip_exact_change_list_match = True
1768+
skip_exact_change_list_match = False
17681769
for test_case_name in data:
17691770
with self.subTest(name=test_case_name):
17701771
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):
17871788
simulated_config = current_config
17881789
for change in actual_changes:
17891790
simulated_config = change.apply(simulated_config)
1790-
self.assertTrue(ConfigWrapper().validate_config_db_config(simulated_config))
1791+
self.assertTrue(self.config_wrapper.validate_config_db_config(simulated_config))
17911792
self.assertEqual(target_config, simulated_config)
17921793

17931794
def test_patch_sorter_failure(self):
@@ -1831,7 +1832,7 @@ def run_single_failure_case(self, data):
18311832
def create_patch_sorter(self, config=None):
18321833
if config is None:
18331834
config=Files.CROPPED_CONFIG_DB_AS_JSON
1834-
config_wrapper = ConfigWrapper()
1835+
config_wrapper = self.config_wrapper
18351836
config_wrapper.get_config_db_as_json = MagicMock(return_value=config)
18361837
patch_wrapper = PatchWrapper(config_wrapper)
18371838
operation_wrapper = OperationWrapper()

0 commit comments

Comments
 (0)