From fc0ecb6b89ebb4118380957e6f33d1d5cb7362cd Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Thu, 13 Jan 2022 11:35:14 -0800 Subject: [PATCH] [GCU] Disallowing DeleteInsteadOfReplaceMoveExtender from generating delete whole config move (#2006) #### What I did Not generating delete whole config move because it is not allowed by JsonPatch library and it is not possible for ConfigDb to be equal to NULL. This solves the first problem in issue https://github.com/Azure/sonic-utilities/issues/2000#issuecomment-1011537050 #### How I did it - Verified UpperLevelMoveExtender does not generate delete whole config move - Modified DeleteInsteadOfReplaceMoveExtender to not generate delete whole config move - DeleteRefsMoveExtender cannot generate delete whole config move, because it deletes the referrer leafs #### How to verify it unit-test #### 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/patch_sorter.py | 4 + .../files/patch_sorter_test_success.json | 83 +++++++++++++++++++ .../patch_sorter_test.py | 12 ++- 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index ca750723187d..daf546424501 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -936,6 +936,10 @@ def extend(self, move, diff): if operation_type != OperationType.REPLACE: return + # Cannot delete the whole config, JsonPatch lib does not support it + if not move.current_config_tokens: + return + new_move = JsonMove(diff, OperationType.REMOVE, move.current_config_tokens) yield new_move 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 14506503c7d5..ef863170defe 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -2811,5 +2811,88 @@ } ] ] + }, + "ADDING_LOOPBACK0_VRF_NAME__DELETES_LOOPBACK0_AND_IPS_DOES_NOT_AFFECT_OTHER_TABLES": { + "desc": ["Adding loopback vrf name, deletes loopback0 and the associated ips. ", + "It does not affect other tables."], + "current_config": { + "CABLE_LENGTH": { + "AZURE": { + "Ethernet0": "0m", + "Ethernet100": "0m" + } + }, + "LOOPBACK_INTERFACE": { + "Loopback0": {}, + "Loopback0|10.1.0.32/32": {}, + "Loopback0|1100:1::32/128": {} + }, + "VRF": { + "Vrf_01": {}, + "Vrf_02": {} + }, + "PORT": { + "Ethernet0": { + "lanes": "25,26,27,28", + "speed": "10000" + }, + "Ethernet100": { + "lanes": "121,122,123,124", + "speed": "10000" + } + } + }, + "patch": [ + { + "op": "add", + "path": "/LOOPBACK_INTERFACE/Loopback0/vrf_name", + "value": "Vrf_01" + } + ], + "expected_changes": [ + [ + { + "op": "remove", + "path": "/LOOPBACK_INTERFACE/Loopback0|10.1.0.32~132" + } + ], + [ + { + "op": "remove", + "path": "/LOOPBACK_INTERFACE/Loopback0|1100:1::32~1128" + } + ], + [ + { + "op": "remove", + "path": "/LOOPBACK_INTERFACE" + } + ], + [ + { + "op": "add", + "path": "/LOOPBACK_INTERFACE", + "value": { + "Loopback0":{ + "vrf_name": "Vrf_01" + } + } + } + ], + [ + { + "op": "add", + "path": "/LOOPBACK_INTERFACE/Loopback0|10.1.0.32~132", + "value": {} + } + ], + [ + { + "op": "add", + "path": "/LOOPBACK_INTERFACE/Loopback0|1100:1::32~1128", + "value": {} + } + ] + ] } } \ No newline at end of file diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index cd9786fdf1fe..29c1b6562ef2 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1591,6 +1591,14 @@ def test_extend__replace_table__replace_whole_config(self): cc_ops=[{'op':'replace', 'path':'/VLAN/Vlan1000/dhcp_servers/1', 'value':'192.0.0.7'}], ex_ops=[{'op':'replace', 'path':'', 'value':Files.CROPPED_CONFIG_DB_AS_JSON}]) + def test_extend__remove_table_while_config_has_only_that_table__replace_whole_config_with_empty_config(self): + self.verify(OperationType.REMOVE, + ["VLAN"], + ["VLAN"], + cc_ops=[{'op':'replace', 'path':'', 'value':{'VLAN':{}}}], + tc_ops=[{'op':'replace', 'path':'', 'value':{}}], + ex_ops=[{'op':'replace', 'path':'', 'value':{}}]) + def verify(self, op_type, ctokens, ttokens=None, cc_ops=[], tc_ops=[], ex_ops=[]): """ cc_ops, tc_ops are used to build the diff object. @@ -1649,12 +1657,12 @@ def test_extend__replace_table__delete_table(self): cc_ops=[{'op':'replace', 'path':'/ACL_TABLE/EVERFLOW/policy_desc', 'value':'old_desc'}], ex_ops=[{'op':'remove', 'path':'/ACL_TABLE'}]) - def test_extend__replace_whole_config__delete_whole_config(self): + def test_extend__replace_whole_config__no_moves(self): self.verify(OperationType.REPLACE, [], [], cc_ops=[{'op':'replace', 'path':'/ACL_TABLE/EVERFLOW/policy_desc', 'value':'old_desc'}], - ex_ops=[{'op':'remove', 'path':''}]) + ex_ops=[]) def verify(self, op_type, ctokens, ttokens=None, cc_ops=[], tc_ops=[], ex_ops=[]): """