From 6d3aa1e79d853493f06174962f6e510dcf55d7df Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Fri, 1 Apr 2022 09:19:08 -0700 Subject: [PATCH] [GCU] Optimizing moves by adding generators for keys/tables (#2120) #### What I did Fixes #2099 Grouping all fields under tables/keys to be added/deleted in 1 JsonChange. **BEFORE** how did we generate moves to try? - Start with the low level differences, low level means differences that has no children - Then extend this low level by different extenders, the most used is the `UpperLevelMoveExtender` - Extended the already extended moves to go upper in level, and do that multiple times until there are no possible moves to extend The diagram below shows: - Generation (low level): ```json {"op":"replace", "path":"/ACL_RULE/SNMP_ACL|RULE_1/SRC_IP", "value": "1.1.1.1/21"} {"op":"remove", "path":"/ACL_RULE/SNMP_ACL|RULE_2/PRIORITY"} {"op":"remove", "path":"/ACL_RULE/SNMP_ACL|RULE_2/SRC_IP"} {"op":"remove", "path":"/ACL_RULE/SNMP_ACL|RULE_2/IP_PROTOCOL"} {"op":"remove", "path":"/ACL_RULE/SNMP_ACL|RULE_2/PACKET_ACTION"} {"op":"add", "path":"/ACL_RULE/SNMP_ACL|RULE_3/PRIORITY", "value": "9997"} {"op":"add", "path":"/ACL_RULE/SNMP_ACL|RULE_3/SRC_IP", "value": "3.3.3.3/20"} {"op":"add", "path":"/ACL_RULE/SNMP_ACL|RULE_3/IP_PROTOCOL", "value": "17"} {"op":"add", "path":"/ACL_RULE/SNMP_ACL|RULE_3/PACKET_ACTION", "value": "ACCEPT"} ``` - Extension - 1st level ```json {"op":"replace", "path":"/ACL_RULE/SNMP_ACL|RULE_1", "value": {"PRIORITY": "9999","SRC_IP": "2.2.2.2/21","SRC_IP": "1.1.1.1/21","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"}} {"op":"remove", "path":"/ACL_RULE/SNMP_ACL|RULE_2"} {"op":"add", "path":"/ACL_RULE/SNMP_ACL|RULE_3", "value": {"PRIORITY": "9997","SRC_IP": "3.3.3.3/20","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"}} ``` - Extension - 2nd level ```json {"op":"replace", "path":"/ACL_RULE", "value": {"SNMP_ACL|RULE_1": {"PRIORITY": "9999","SRC_IP": "1.1.1.1/21","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"},"SNMP_ACL|RULE_3": {"PRIORITY": "9997","SRC_IP": "3.3.3.3/20","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"},"SNMP_ACL|DEFAULT_RULE": {"PRIORITY": "1","ETHER_TYPE": "2048","PACKET_ACTION": "DROP"}}} ``` - Extension - 3rd level ```json {"op":"replace", "path":"", "value": {"ACL_RULE": {"SNMP_ACL|RULE_1": {"PRIORITY": "9999","SRC_IP": "1.1.1.1/21","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"},"SNMP_ACL|RULE_3": {"PRIORITY": "9997","SRC_IP": "3.3.3.3/20","IP_PROTOCOL": "17","PACKET_ACTION": "ACCEPT"},"SNMP_ACL|DEFAULT_RULE": {"PRIORITY": "1","ETHER_TYPE": "2048","PACKET_ACTION": "DROP"}},"ACL_TABLE": {"SNMP_ACL": {"type": "CTRLPLANE","policy_desc": "SNMP_ACL","services": ["SNMP"]}}}} ``` ![image](https://user-images.githubusercontent.com/3927651/160676723-29688656-3382-4247-8358-cb988cda5134.png) **AFTER** In this PR, we introduce a different kind of generator. The non-extendable generators i.e. generators that their moves do not get extended using the extenders. We added 2 generators: - KeyLevelGenerator: if a whole key to be deleted or added, do that directly. - TableLevelGenerator: if a whole table to be deleted or added, do that directly. Only **enabled** KeyLevelGenerator, to enable **TableLevelGenerator** we have to confirm with Renuka if it is possible to update a whole table at once in the `ChangeApplier` (instead of just keys like it is today). We have to be able to update a table as a whole because within the table there can be internal dependencies between the object, so we have to guarantee all objects are deleted together. For the keys it is guaranteed for the whole key to be updated at once according to the `ChangeApplier`. **Why non-extendable generators?** Calling the non-extendable generators precedes the extendable ones, it is like checking for the low hanging fruits. If we use the same extenders for these new generators we will always go up the config tree. Imaging a move that tries to update a key but fails, then we call the extenders on this move. What will happen is the extenders will go up the config tree to the table level, will first try to replace the table, then try to delete the table. Deleting the table is a problem because it affects multiple more than intended and thus violates the minimum disruption guarantee. We decided to leave the extenders only for the LowLevelGenerator thus making sure we try all possible moves from the lowest levels and go up from there. Another way to look at it is like this: - Non-extendable generators do top-down search: Check tables, keys in this order - Extendable generators and extenders do bottom-up approach: Check fields, keys, tables in this order #### How I did it Modifying the `MoveWrapper.Generate` method to allow for different type of move generators #### How to verify it Unit-test Please check `tests/generic_config_updater/files/patch_sorter_test_success.json` to see how the moved generated got much compacted. #### 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 | 116 ++- .../files/patch_sorter_test_success.json | 760 +++++++----------- .../patch_sorter_test.py | 240 +++++- 3 files changed, 624 insertions(+), 492 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index e89c542e73b1..97db21b24eba 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -290,29 +290,52 @@ def __hash__(self): return hash((self.op_type, self.path, json.dumps(self.value))) class MoveWrapper: - def __init__(self, move_generators, move_extenders, move_validators): + def __init__(self, move_generators, move_non_extendable_generators, move_extenders, move_validators): self.move_generators = move_generators + self.move_non_extendable_generators = move_non_extendable_generators self.move_extenders = move_extenders self.move_validators = move_validators def generate(self, diff): + """ + Generates all possible moves to help transform diff.current_config to diff.target_config. + + It starts by generating the non-extendable moves i.e. moves that will not extended to e.g. change its parent. + The non-extendable moves are mostly high level moves such as deleting/adding whole tables. + + After that it generates extendable moves i.e. moves that can be extended to e.g. change its parent. + The extendable moves are typically very low level moves that can achieve the minimum disruption guarantee. + + Lastly the moves are extended for example to try to replace the parent config instead, or by deleting + the dependencies of the config. + """ processed_moves = set() + extended_moves = set() moves = deque([]) + for move in self._generate_non_extendable_moves(diff): + if not(move in processed_moves): + processed_moves.add(move) + yield move + for move in self._generate_moves(diff): - if move in processed_moves: - continue - processed_moves.add(move) - yield move - moves.extend(self._extend_moves(move, diff)) + if not(move in processed_moves): + processed_moves.add(move) + yield move + + if not(move in extended_moves): + extended_moves.add(move) + moves.extend(self._extend_moves(move, diff)) while moves: move = moves.popleft() - if move in processed_moves: - continue - processed_moves.add(move) - yield move - moves.extend(self._extend_moves(move, diff)) + if not(move in processed_moves): + processed_moves.add(move) + yield move + + if not(move in extended_moves): + extended_moves.add(move) + moves.extend(self._extend_moves(move, diff)) def validate(self, move, diff): for validator in self.move_validators: @@ -328,6 +351,11 @@ def _generate_moves(self, diff): for move in generator.generate(diff): yield move + def _generate_non_extendable_moves(self, diff): + for generator in self.move_non_extendable_generators: + for move in generator.generate(diff): + yield move + def _extend_moves(self, move, diff): for extender in self.move_extenders: for newmove in extender.extend(move, diff): @@ -921,6 +949,68 @@ def validate(self, move, diff): return True +class TableLevelMoveGenerator: + """ + A class that key level moves. The item name at the root level of ConfigDB is called 'Table'. + + e.g. + { + "Table": ... + } + + This class will generate moves to remove tables if they are in current, but not target. It also add tables + if they are in target but not current configs. + """ + + def generate(self, diff): + # Removing tables in current but not target + for tokens in self._get_non_existing_tables_tokens(diff.current_config, diff.target_config): + yield JsonMove(diff, OperationType.REMOVE, tokens) + + # Adding tables in target but not current + for tokens in self._get_non_existing_tables_tokens(diff.target_config, diff.current_config): + yield JsonMove(diff, OperationType.ADD, tokens, tokens) + + def _get_non_existing_tables_tokens(self, config1, config2): + for table in config1: + if not(table in config2): + yield [table] + +class KeyLevelMoveGenerator: + """ + A class that key level moves. The item name at the root level of ConfigDB is called 'Table', the item + name in the Table level of ConfigDB is called key. + + e.g. + { + "Table": { + "Key": ... + } + } + + This class will generate moves to remove keys if they are in current, but not target. It also add keys + if they are in target but not current configs. + """ + def generate(self, diff): + # Removing keys in current but not target + for tokens in self._get_non_existing_keys_tokens(diff.current_config, diff.target_config): + table = tokens[0] + # if table has a single key, delete the whole table because empty tables are not allowed in ConfigDB + if len(diff.current_config[table]) == 1: + yield JsonMove(diff, OperationType.REMOVE, [table]) + else: + yield JsonMove(diff, OperationType.REMOVE, tokens) + + # Adding keys in target but not current + for tokens in self._get_non_existing_keys_tokens(diff.target_config, diff.current_config): + yield JsonMove(diff, OperationType.ADD, tokens, tokens) + + def _get_non_existing_keys_tokens(self, config1, config2): + for table in config1: + for key in config1[table]: + if not(table in config2) or not (key in config2[table]): + yield [table, key] + class LowLevelMoveGenerator: """ A class to generate the low level moves i.e. moves corresponding to differences between current/target config @@ -1407,6 +1497,8 @@ def __init__(self, operation_wrapper, config_wrapper, path_addressing): def create(self, algorithm=Algorithm.DFS): move_generators = [LowLevelMoveGenerator(self.path_addressing)] + # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time + move_non_extendable_generators = [KeyLevelMoveGenerator()] move_extenders = [RequiredValueMoveExtender(self.path_addressing, self.operation_wrapper), UpperLevelMoveExtender(), DeleteInsteadOfReplaceMoveExtender(), @@ -1419,7 +1511,7 @@ def create(self, algorithm=Algorithm.DFS): RequiredValueMoveValidator(self.path_addressing), NoEmptyTableMoveValidator(self.path_addressing)] - move_wrapper = MoveWrapper(move_generators, move_extenders, move_validators) + move_wrapper = MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators) if algorithm == Algorithm.DFS: sorter = DfsSorter(move_wrapper) 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 894f68896cba..c134b0b5e243 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -169,16 +169,10 @@ "op": "add", "path": "/INTERFACE/Ethernet8|10.0.0.1~130", "value": { - "family": "IPv4" + "family": "IPv4", + "scope": "global" } } - ], - [ - { - "op": "add", - "path": "/INTERFACE/Ethernet8|10.0.0.1~130/scope", - "value": "global" - } ] ] }, @@ -282,39 +276,15 @@ "op": "add", "path": "/ACL_TABLE/EVERFLOWV6", "value": { + "policy_desc": "EVERFLOWV6", + "ports": [ + "Ethernet4", + "Ethernet8" + ], + "stage": "ingress", "type": "MIRRORV6" } } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/policy_desc", - "value": "EVERFLOWV6" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports", - "value": [ - "Ethernet4" - ] - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports/1", - "value": "Ethernet8" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/stage", - "value": "ingress" - } ] ] }, @@ -419,129 +389,57 @@ "path": "/ACL_TABLE", "value": { "NO-NSW-PACL-V4": { - "type": "L3" + "type": "L3", + "policy_desc": "NO-NSW-PACL-V4", + "ports": [ + "Ethernet0" + ] } } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", - "value": "NO-NSW-PACL-V4" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", - "value": [ - "Ethernet0" - ] - } - ], [ { "op": "add", "path": "/ACL_TABLE/DATAACL", "value": { + "policy_desc": "DATAACL", + "ports": [ + "Ethernet4" + ], + "stage": "ingress", "type": "L3" } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/DATAACL/policy_desc", - "value": "DATAACL" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/DATAACL/ports", - "value": [ - "Ethernet4" - ] - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/DATAACL/stage", - "value": "ingress" - } - ], [ { "op": "add", "path": "/ACL_TABLE/EVERFLOW", "value": { + "policy_desc": "EVERFLOW", + "ports": [ + "Ethernet8" + ], + "stage": "ingress", "type": "MIRROR" } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOW/policy_desc", - "value": "EVERFLOW" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOW/ports", - "value": [ - "Ethernet8" - ] - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOW/stage", - "value": "ingress" - } - ], [ { "op": "add", "path": "/ACL_TABLE/EVERFLOWV6", "value": { + "policy_desc": "EVERFLOWV6", + "ports": [ + "Ethernet4", + "Ethernet8" + ], + "stage": "ingress", "type": "MIRRORV6" } } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/policy_desc", - "value": "EVERFLOWV6" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports", - "value": [ - "Ethernet4" - ] - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports/1", - "value": "Ethernet8" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/stage", - "value": "ingress" - } ] ] }, @@ -979,30 +877,34 @@ [ { "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", - "value": [ - "Ethernet0" - ] + "path": "/PORT/Ethernet3", + "value": { + "alias": "Eth1/4", + "lanes": "68", + "description": "", + "speed": "10000" + } } ], [ { "op": "add", - "path": "/VLAN_MEMBER", + "path": "/PORT/Ethernet2", "value": { - "Vlan100|Ethernet0": { - "tagging_mode": "untagged" - } + "alias": "Eth1/3", + "lanes": "67", + "description": "", + "speed": "10000" } } ], [ { "op": "add", - "path": "/PORT/Ethernet3", + "path": "/PORT/Ethernet1", "value": { - "alias": "Eth1/4", - "lanes": "68", + "alias": "Eth1/2", + "lanes": "66", "description": "", "speed": "10000" } @@ -1011,14 +913,18 @@ [ { "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1", - "value": "Ethernet3" + "path": "/VLAN_MEMBER", + "value": { + "Vlan100|Ethernet0": { + "tagging_mode": "untagged" + } + } } ], [ { "op": "add", - "path": "/VLAN_MEMBER/Vlan100|Ethernet3", + "path": "/VLAN_MEMBER/Vlan100|Ethernet1", "value": { "tagging_mode": "untagged" } @@ -1027,22 +933,12 @@ [ { "op": "add", - "path": "/PORT/Ethernet2", + "path": "/VLAN_MEMBER/Vlan100|Ethernet3", "value": { - "alias": "Eth1/3", - "lanes": "67", - "description": "", - "speed": "10000" + "tagging_mode": "untagged" } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/2", - "value": "Ethernet2" - } - ], [ { "op": "add", @@ -1055,13 +951,10 @@ [ { "op": "add", - "path": "/PORT/Ethernet1", - "value": { - "alias": "Eth1/2", - "lanes": "66", - "description": "", - "speed": "10000" - } + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] } ], [ @@ -1073,23 +966,16 @@ ], [ { - "op": "replace", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", - "value": [ - "Ethernet0", - "Ethernet1", - "Ethernet2", - "Ethernet3" - ] + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/2", + "value": "Ethernet2" } ], [ { "op": "add", - "path": "/VLAN_MEMBER/Vlan100|Ethernet1", - "value": { - "tagging_mode": "untagged" - } + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/3", + "value": "Ethernet3" } ] ] @@ -1220,6 +1106,24 @@ } ], "expected_changes": [ + [ + { + "op": "remove", + "path": "/VLAN_MEMBER/Vlan100|Ethernet1" + } + ], + [ + { + "op": "remove", + "path": "/VLAN_MEMBER/Vlan100|Ethernet2" + } + ], + [ + { + "op": "remove", + "path": "/VLAN_MEMBER/Vlan100|Ethernet3" + } + ], [ { "op": "replace", @@ -1283,24 +1187,6 @@ "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" - } - ], - [ - { - "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet1" - } - ], [ { "op": "remove", @@ -1310,7 +1196,7 @@ [ { "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet2" + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" } ], [ @@ -1322,7 +1208,7 @@ [ { "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet3" + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" } ], [ @@ -1358,6 +1244,17 @@ "path": "/ACL_TABLE" } ], + [ + { + "op": "add", + "path": "/VLAN_MEMBER", + "value": { + "Vlan100|Ethernet0": { + "tagging_mode": "untagged" + } + } + } + ], [ { "op": "add", @@ -1369,17 +1266,31 @@ } } ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] + } + ], [ { "op": "remove", - "path": "/PORT" + "path": "/VLAN_MEMBER" } ], [ { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", - "value": "NO-NSW-PACL-V4" + "op": "remove", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports" + } + ], + [ + { + "op": "remove", + "path": "/PORT" } ], [ @@ -1389,22 +1300,13 @@ "value": { "Ethernet0": { "alias": "Eth1", - "lanes": "65, 66, 67, 68", "description": "Ethernet0 100G link", + "lanes": "65, 66, 67, 68", "speed": "100000" } } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", - "value": [ - "Ethernet0" - ] - } - ], [ { "op": "add", @@ -1415,6 +1317,22 @@ } } } + ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", + "value": "NO-NSW-PACL-V4" + } + ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] + } ] ] }, @@ -2085,18 +2003,6 @@ } ], "expected_changes": [ - [ - { - "op": "remove", - "path": "/PORT/Ethernet0/alias" - } - ], - [ - { - "op": "remove", - "path": "/PORT/Ethernet0/description" - } - ], [ { "op": "remove", @@ -2154,18 +2060,6 @@ } ], "expected_changes": [ - [ - { - "op": "remove", - "path": "/INTERFACE/Ethernet8|10.0.0.1~130/family" - } - ], - [ - { - "op": "remove", - "path": "/INTERFACE/Ethernet8|10.0.0.1~130/scope" - } - ], [ { "op": "remove", @@ -2378,90 +2272,24 @@ } ], "expected_changes": [ - [ - { - "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/DATAACL/policy_desc" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/DATAACL/stage" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOW/policy_desc" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOW/stage" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/policy_desc" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/stage" - } - ], [ { "op": "remove", "path": "/ACL_TABLE/NO-NSW-PACL-V4" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/DATAACL/ports" - } - ], [ { "op": "remove", "path": "/ACL_TABLE/DATAACL" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOW/ports" - } - ], [ { "op": "remove", "path": "/ACL_TABLE/EVERFLOW" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/ports" - } - ], [ { "op": "remove", @@ -2752,6 +2580,17 @@ "path": "/ACL_TABLE" } ], + [ + { + "op": "add", + "path": "/VLAN_MEMBER", + "value": { + "Vlan100|Ethernet0": { + "tagging_mode": "untagged" + } + } + } + ], [ { "op": "add", @@ -2763,17 +2602,31 @@ } } ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] + } + ], [ { "op": "remove", - "path": "/PORT" + "path": "/VLAN_MEMBER" } ], [ { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", - "value": "NO-NSW-PACL-V4" + "op": "remove", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports" + } + ], + [ + { + "op": "remove", + "path": "/PORT" } ], [ @@ -2783,22 +2636,13 @@ "value": { "Ethernet0": { "alias": "Eth1", - "lanes": "67", "description": "Ethernet0 100G link", + "lanes": "67", "speed": "100000" } } } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", - "value": [ - "Ethernet0" - ] - } - ], [ { "op": "add", @@ -2809,6 +2653,22 @@ } } } + ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/policy_desc", + "value": "NO-NSW-PACL-V4" + } + ], + [ + { + "op": "add", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0" + ] + } ] ] }, @@ -4267,6 +4127,127 @@ } ], "expected_changes": [ + [ + { + "op": "add", + "path": "/BGP_NEIGHBOR/10.0.0.33", + "value": { + "admin_status": "up", + "asn": "64001", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.32", + "name": "ARISTA01T0", + "nhopself": "0", + "rrclient": "0" + } + } + ], + [ + { + "op": "add", + "path": "/BGP_NEIGHBOR/fc00::42", + "value": { + "admin_status": "up", + "asn": "64001", + "holdtime": "10", + "keepalive": "3", + "local_addr": "fc00::41", + "name": "ARISTA01T0", + "nhopself": "0", + "rrclient": "0" + } + } + ], + [ + { + "op": "add", + "path": "/BUFFER_PG/Ethernet64|3-4", + "value": { + "profile": "pg_lossless_40000_40m_profile" + } + } + ], + [ + { + "op": "add", + "path": "/BUFFER_PG/Ethernet64|0", + "value": { + "profile": "ingress_lossy_profile" + } + } + ], + [ + { + "op": "add", + "path": "/BUFFER_QUEUE/Ethernet64|0-2", + "value": { + "profile": "egress_lossy_profile" + } + } + ], + [ + { + "op": "add", + "path": "/BUFFER_QUEUE/Ethernet64|3-4", + "value": { + "profile": "egress_lossless_profile" + } + } + ], + [ + { + "op": "add", + "path": "/BUFFER_QUEUE/Ethernet64|5-6", + "value": { + "profile": "egress_lossy_profile" + } + } + ], + [ + { + "op": "add", + "path": "/DEVICE_NEIGHBOR/Ethernet64", + "value": { + "name": "ARISTA01T0", + "port": "Ethernet1" + } + } + ], + [ + { + "op": "add", + "path": "/INTERFACE/Ethernet64", + "value": {} + } + ], + [ + { + "op": "add", + "path": "/INTERFACE/Ethernet64|10.0.0.32~131", + "value": {} + } + ], + [ + { + "op": "add", + "path": "/INTERFACE/Ethernet64|FC00::41~1126", + "value": {} + } + ], + [ + { + "op": "add", + "path": "/PORT_QOS_MAP/Ethernet64", + "value": { + "dscp_to_tc_map": "AZURE", + "pfc_enable": "3,4", + "pfc_to_queue_map": "AZURE", + "tc_to_pg_map": "AZURE", + "tc_to_queue_map": "AZURE" + } + } + ], [ { "op": "add", @@ -4603,88 +4584,6 @@ "value": "up" } ], - [ - { - "op": "add", - "path": "/BUFFER_PG/Ethernet64|3-4", - "value": { - "profile": "pg_lossless_40000_40m_profile" - } - } - ], - [ - { - "op": "add", - "path": "/BUFFER_PG/Ethernet64|0", - "value": { - "profile": "ingress_lossy_profile" - } - } - ], - [ - { - "op": "add", - "path": "/BUFFER_QUEUE/Ethernet64|0-2", - "value": { - "profile": "egress_lossy_profile" - } - } - ], - [ - { - "op": "add", - "path": "/BUFFER_QUEUE/Ethernet64|3-4", - "value": { - "profile": "egress_lossless_profile" - } - } - ], - [ - { - "op": "add", - "path": "/BUFFER_QUEUE/Ethernet64|5-6", - "value": { - "profile": "egress_lossy_profile" - } - } - ], - [ - { - "op": "add", - "path": "/DEVICE_NEIGHBOR/Ethernet64", - "value": { - "name": "ARISTA01T0" - } - } - ], - [ - { - "op": "add", - "path": "/DEVICE_NEIGHBOR/Ethernet64/port", - "value": "Ethernet1" - } - ], - [ - { - "op": "add", - "path": "/INTERFACE/Ethernet64", - "value": {} - } - ], - [ - { - "op": "add", - "path": "/INTERFACE/Ethernet64|10.0.0.32~131", - "value": {} - } - ], - [ - { - "op": "add", - "path": "/INTERFACE/Ethernet64|FC00::41~1126", - "value": {} - } - ], [ { "op": "replace", @@ -4692,81 +4591,12 @@ "value": "ARISTA01T0:Ethernet1" } ], - [ - { - "op": "add", - "path": "/PORT_QOS_MAP/Ethernet64", - "value": { - "dscp_to_tc_map": "AZURE" - } - } - ], - [ - { - "op": "add", - "path": "/PORT_QOS_MAP/Ethernet64/pfc_enable", - "value": "3,4" - } - ], - [ - { - "op": "add", - "path": "/PORT_QOS_MAP/Ethernet64/pfc_to_queue_map", - "value": "AZURE" - } - ], - [ - { - "op": "add", - "path": "/PORT_QOS_MAP/Ethernet64/tc_to_pg_map", - "value": "AZURE" - } - ], - [ - { - "op": "add", - "path": "/PORT_QOS_MAP/Ethernet64/tc_to_queue_map", - "value": "AZURE" - } - ], [ { "op": "add", "path": "/PORT/Ethernet64/admin_status", "value": "up" } - ], - [ - { - "op": "add", - "path": "/BGP_NEIGHBOR/10.0.0.33", - "value": { - "admin_status": "up", - "asn": "64001", - "holdtime": "10", - "keepalive": "3", - "local_addr": "10.0.0.32", - "name": "ARISTA01T0", - "nhopself": "0", - "rrclient": "0" - } - } - ], - [ - { - "op": "add", - "path": "/BGP_NEIGHBOR/fc00::42", - "value": { - "admin_status": "up", - "asn": "64001", - "holdtime": "10", - "keepalive": "3", - "local_addr": "fc00::41", - "name": "ARISTA01T0", - "nhopself": "0", - "rrclient": "0" - } - } ] ] } diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 12d76c528399..5530de7cef9c 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -498,21 +498,23 @@ def setUp(self): def test_ctor__assigns_values_correctly(self): # Arrange move_generators = Mock() + move_non_extendable_generators = Mock() move_extenders = Mock() move_validators = Mock() # Act - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, move_validators) + move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators) # Assert self.assertIs(move_generators, move_wrapper.move_generators) + self.assertIs(move_non_extendable_generators, move_wrapper.move_non_extendable_generators) self.assertIs(move_extenders, move_wrapper.move_extenders) self.assertIs(move_validators, move_wrapper.move_validators) def test_generate__single_move_generator__single_move_returned(self): # Arrange move_generators = [self.single_move_generator] - move_wrapper = ps.MoveWrapper(move_generators, [], []) + move_wrapper = ps.MoveWrapper(move_generators, [], [], []) expected = [self.any_move] # Act @@ -524,7 +526,7 @@ def test_generate__single_move_generator__single_move_returned(self): def test_generate__multiple_move_generator__multiple_move_returned(self): # Arrange move_generators = [self.multiple_move_generator] - move_wrapper = ps.MoveWrapper(move_generators, [], []) + move_wrapper = ps.MoveWrapper(move_generators, [], [], []) expected = [self.any_move, self.any_other_move1, self.any_other_move2] # Act @@ -536,7 +538,7 @@ def test_generate__multiple_move_generator__multiple_move_returned(self): def test_generate__different_move_generators__different_moves_returned(self): # Arrange move_generators = [self.single_move_generator, self.another_single_move_generator] - move_wrapper = ps.MoveWrapper(move_generators, [], []) + move_wrapper = ps.MoveWrapper(move_generators, [], [], []) expected = [self.any_move, self.any_other_move1] # Act @@ -548,7 +550,44 @@ def test_generate__different_move_generators__different_moves_returned(self): def test_generate__duplicate_generated_moves__unique_moves_returned(self): # Arrange move_generators = [self.single_move_generator, self.single_move_generator] - move_wrapper = ps.MoveWrapper(move_generators, [], []) + move_wrapper = ps.MoveWrapper(move_generators, [], [], []) + expected = [self.any_move] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + + def test_generate__different_move_non_extendable_generators__different_moves_returned(self): + # Arrange + move_non_extendable_generators = [self.single_move_generator, self.another_single_move_generator] + move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, [], []) + expected = [self.any_move, self.any_other_move1] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + + def test_generate__duplicate_generated_non_extendable_moves__unique_moves_returned(self): + # Arrange + move_non_extendable_generators = [self.single_move_generator, self.single_move_generator] + move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, [], []) + expected = [self.any_move] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + + def test_generate__duplicate_move_between_extendable_and_non_extendable_generators__unique_move_returned(self): + # Arrange + move_generators = [self.single_move_generator] + move_non_extendable_generators = [self.single_move_generator] + move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, [], []) expected = [self.any_move] # Act @@ -561,7 +600,7 @@ def test_generate__single_move_extender__one_extended_move_returned(self): # Arrange move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender] - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, []) + move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) expected = [self.any_move, self.any_extended_move] # Act @@ -574,7 +613,7 @@ def test_generate__multiple_move_extender__multiple_extended_move_returned(self) # Arrange move_generators = [self.single_move_generator] move_extenders = [self.multiple_move_extender] - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, []) + move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) expected = [self.any_move, self.any_extended_move, self.any_other_extended_move1, self.any_other_extended_move2] # Act @@ -587,7 +626,7 @@ def test_generate__different_move_extenders__different_extended_moves_returned(s # Arrange move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender, self.another_single_move_extender] - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, []) + move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) expected = [self.any_move, self.any_extended_move, self.any_other_extended_move1] # Act @@ -600,7 +639,7 @@ def test_generate__duplicate_extended_moves__unique_moves_returned(self): # Arrange move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender, self.single_move_extender] - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, []) + move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) expected = [self.any_move, self.any_extended_move] # Act @@ -613,7 +652,7 @@ def test_generate__mixed_extended_moves__unique_moves_returned(self): # Arrange move_generators = [self.single_move_generator, self.another_single_move_generator] move_extenders = [self.mixed_move_extender] - move_wrapper = ps.MoveWrapper(move_generators, move_extenders, []) + move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) expected = [self.any_move, self.any_other_move1, self.any_extended_move, @@ -626,10 +665,54 @@ def test_generate__mixed_extended_moves__unique_moves_returned(self): # Assert self.assertListEqual(expected, actual) + def test_generate__multiple_non_extendable_moves__no_moves_extended(self): + # Arrange + move_non_extendable_generators = [self.single_move_generator, self.another_single_move_generator] + move_extenders = [self.mixed_move_extender] + move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, move_extenders, []) + expected = [self.any_move, self.any_other_move1] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + + def test_generate__mixed_extendable_non_extendable_moves__only_extendable_moves_extended(self): + # Arrange + move_generators = [self.another_single_move_generator] # generates: any_other_move1, extends: any_other_extended_move1 + move_non_extendable_generators = [self.single_move_generator] # generates: any_move + move_extenders = [self.mixed_move_extender] + move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, []) + expected = [self.any_move, + self.any_other_move1, + self.any_other_extended_move1] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + + def test_generate__move_is_extendable_and_non_extendable__move_is_extended(self): + # Arrange + move_generators = [self.single_move_generator] + move_non_extendable_generators = [self.single_move_generator] + move_extenders = [self.single_move_extender] + move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, []) + expected = [self.any_move, + self.any_extended_move] + + # Act + actual = list(move_wrapper.generate(self.any_diff)) + + # Assert + self.assertListEqual(expected, actual) + def test_validate__validation_fail__false_returned(self): # Arrange move_validators = [self.fail_move_validator] - move_wrapper = ps.MoveWrapper([], [], move_validators) + move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)) @@ -637,7 +720,7 @@ def test_validate__validation_fail__false_returned(self): def test_validate__validation_succeed__true_returned(self): # Arrange move_validators = [self.success_move_validator] - move_wrapper = ps.MoveWrapper([], [], move_validators) + move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert self.assertTrue(move_wrapper.validate(self.any_move, self.any_diff)) @@ -645,7 +728,7 @@ def test_validate__validation_succeed__true_returned(self): def test_validate__multiple_validators_last_fail___false_returned(self): # Arrange move_validators = [self.success_move_validator, self.success_move_validator, self.fail_move_validator] - move_wrapper = ps.MoveWrapper([], [], move_validators) + move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)) @@ -653,7 +736,7 @@ def test_validate__multiple_validators_last_fail___false_returned(self): def test_validate__multiple_validators_succeed___true_returned(self): # Arrange move_validators = [self.success_move_validator, self.success_move_validator, self.success_move_validator] - move_wrapper = ps.MoveWrapper([], [], move_validators) + move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert self.assertTrue(move_wrapper.validate(self.any_move, self.any_diff)) @@ -662,7 +745,7 @@ def test_simulate__applies_move(self): # Arrange diff = Mock() diff.apply_move.side_effect = create_side_effect_dict({(str(self.any_move), ): self.any_diff}) - move_wrapper = ps.MoveWrapper(None, None, None) + move_wrapper = ps.MoveWrapper(None, None, None, None) # Act actual = move_wrapper.simulate(self.any_move, diff) @@ -1862,6 +1945,130 @@ def _get_no_critical_port_change_test_cases(self): def _apply_operations(self, config, operations): return jsonpatch.JsonPatch(operations).apply(config) +class TestTableLevelMoveGenerator(unittest.TestCase): + def setUp(self): + self.generator = ps.TableLevelMoveGenerator() + + def test_generate__tables_in_current_but_not_target__tables_deleted_moves(self): + self.verify(current = {"ExistingTable": {}, "NonExistingTable1": {}, "NonExistingTable2": {}}, + target = {"ExistingTable": {}}, + ex_ops = [{"op": "remove", 'path': '/NonExistingTable1'}, + {"op": "remove", 'path': '/NonExistingTable2'}]) + + def test_generate__tables_in_target_but_not_current__tables_added_moves(self): + self.verify(current = {"ExistingTable": {}}, + target = {"ExistingTable": {}, "NonExistingTable1": {}, "NonExistingTable2": {}}, + ex_ops = [{"op": "add", 'path': '/NonExistingTable1', 'value': {}}, + {"op": "add", 'path': '/NonExistingTable2', 'value': {}}]) + + def test_generate__all_tables_exist__no_moves(self): + self.verify(current = {"ExistingTable1": { "Key1": "Value1" }, "ExistingTable2": {}}, + target = {"ExistingTable1": {}, "ExistingTable2": { "Key2": "Value2" }}, + ex_ops = []) + + def test_generate__multiple_cases__deletion_precedes_addition(self): + self.verify(current = {"CommonTable": { "Key1": "Value1" }, "CurrentTable": {}}, + target = {"CommonTable": {}, "TargetTable": { "Key2": "Value2" }}, + ex_ops = [{"op": "remove", 'path': '/CurrentTable'}, + {"op": "add", 'path': '/TargetTable', 'value': { "Key2": "Value2" }}]) + + def verify(self, current, target, ex_ops): + # Arrange + diff = ps.Diff(current, target) + + # Act + moves = self.generator.generate(diff) + + # Assert + self.verify_moves(ex_ops, + moves) + + def verify_moves(self, ops, moves): + moves_ops = [list(move.patch)[0] for move in moves] + self.assertCountEqual(ops, moves_ops) + +class TestKeyLevelMoveGenerator(unittest.TestCase): + def setUp(self): + self.generator = ps.KeyLevelMoveGenerator() + + def test_generate__keys_in_current_but_not_target__keys_deleted_moves(self): + self.verify(current = { + "ExistingTable1": { + "ExistingKey11": "Value11", + "NonExistingKey12" : "Value12", + "NonExistingKey13" : "Value13" + }, + "NonExistingTable2": + { + "NonExistingKey21" : "Value21", + "NonExistingKey22" : "Value22" + } + }, + target = { + "ExistingTable1": { + "ExistingKey11": "Value11" + } + }, + ex_ops = [{"op": "remove", 'path': '/ExistingTable1/NonExistingKey12'}, + {"op": "remove", 'path': '/ExistingTable1/NonExistingKey13'}, + {"op": "remove", 'path': '/NonExistingTable2/NonExistingKey21'}, + {"op": "remove", 'path': '/NonExistingTable2/NonExistingKey22'}]) + + + def test_generate__single_key_in_current_but_not_target__whole_table_deleted(self): + self.verify(current = { "ExistingTable1": { "NonExistingKey11" : "Value11" }}, + target = {}, + ex_ops = [{"op": "remove", 'path': '/ExistingTable1'}]) + + def test_generate__keys_in_target_but_not_current__keys_added_moves(self): + self.verify(current = { + "ExistingTable1": { + "ExistingKey11": "Value11" + } + }, + target = { + "ExistingTable1": { + "ExistingKey11": "Value11", + "NonExistingKey12" : "Value12", + "NonExistingKey13" : "Value13" + }, + "NonExistingTable2": + { + "NonExistingKey21" : "Value21", + "NonExistingKey22" : "Value22" + } + }, + ex_ops = [{"op": "add", 'path': '/ExistingTable1/NonExistingKey12', "value": "Value12"}, + {"op": "add", 'path': '/ExistingTable1/NonExistingKey13', "value": "Value13"}, + {"op": "add", 'path': '/NonExistingTable2', "value": { "NonExistingKey21": "Value21" }}, + {"op": "add", 'path': '/NonExistingTable2', "value": { "NonExistingKey22": "Value22" }}]) + + def test_generate__all_keys_exist__no_moves(self): + self.verify(current = {"ExistingTable1": { "Key1": "Value1Current" }, "ExistingTable2": { "Key2": "Value2" }}, + target = {"ExistingTable1": { "Key1": "Value1Target" }, "ExistingTable2": { "Key2": {} } }, + ex_ops = []) + + def test_generate__multiple_cases__deletion_precedes_addition(self): + self.verify(current = {"AnyTable": { "CommonKey": "CurrentValue1", "CurrentKey": "CurrentValue2" }}, + target = {"AnyTable": { "CommonKey": "TargetValue1", "TargetKey": "TargetValue2" }}, + ex_ops = [{"op": "remove", 'path': '/AnyTable/CurrentKey'}, + {"op": "add", 'path': '/AnyTable/TargetKey', 'value': "TargetValue2"}]) + + def verify(self, current, target, ex_ops): + # Arrange + diff = ps.Diff(current, target) + + # Act + moves = self.generator.generate(diff) + + # Assert + self.verify_moves(ex_ops, + moves) + + def verify_moves(self, ops, moves): + moves_ops = [list(move.patch)[0] for move in moves] + self.assertCountEqual(ops, moves_ops) + class TestLowLevelMoveGenerator(unittest.TestCase): def setUp(self): path_addressing = PathAddressing() @@ -2823,6 +3030,7 @@ def verify(self, algo, algo_class): config_wrapper = ConfigWrapper() factory = ps.SortAlgorithmFactory(OperationWrapper(), config_wrapper, PathAddressing(config_wrapper)) expected_generators = [ps.LowLevelMoveGenerator] + expected_non_extendable_generators = [ps.KeyLevelMoveGenerator] expected_extenders = [ps.RequiredValueMoveExtender, ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, @@ -2838,12 +3046,14 @@ def verify(self, algo, algo_class): # Act sorter = factory.create(algo) actual_generators = [type(item) for item in sorter.move_wrapper.move_generators] + actual_non_extendable_generators = [type(item) for item in sorter.move_wrapper.move_non_extendable_generators] actual_extenders = [type(item) for item in sorter.move_wrapper.move_extenders] actual_validators = [type(item) for item in sorter.move_wrapper.move_validators] # Assert self.assertIsInstance(sorter, algo_class) self.assertCountEqual(expected_generators, actual_generators) + self.assertCountEqual(expected_non_extendable_generators, actual_non_extendable_generators) self.assertCountEqual(expected_extenders, actual_extenders) self.assertCountEqual(expected_validator, actual_validators)