Skip to content

Commit 6d3aa1e

Browse files
authored
[GCU] Optimizing moves by adding generators for keys/tables (sonic-net#2120)
#### What I did Fixes sonic-net#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)
1 parent 65a5a6b commit 6d3aa1e

File tree

3 files changed

+624
-492
lines changed

3 files changed

+624
-492
lines changed

generic_config_updater/patch_sorter.py

+104-12
Original file line numberDiff line numberDiff line change
@@ -290,29 +290,52 @@ def __hash__(self):
290290
return hash((self.op_type, self.path, json.dumps(self.value)))
291291

292292
class MoveWrapper:
293-
def __init__(self, move_generators, move_extenders, move_validators):
293+
def __init__(self, move_generators, move_non_extendable_generators, move_extenders, move_validators):
294294
self.move_generators = move_generators
295+
self.move_non_extendable_generators = move_non_extendable_generators
295296
self.move_extenders = move_extenders
296297
self.move_validators = move_validators
297298

298299
def generate(self, diff):
300+
"""
301+
Generates all possible moves to help transform diff.current_config to diff.target_config.
302+
303+
It starts by generating the non-extendable moves i.e. moves that will not extended to e.g. change its parent.
304+
The non-extendable moves are mostly high level moves such as deleting/adding whole tables.
305+
306+
After that it generates extendable moves i.e. moves that can be extended to e.g. change its parent.
307+
The extendable moves are typically very low level moves that can achieve the minimum disruption guarantee.
308+
309+
Lastly the moves are extended for example to try to replace the parent config instead, or by deleting
310+
the dependencies of the config.
311+
"""
299312
processed_moves = set()
313+
extended_moves = set()
300314
moves = deque([])
301315

316+
for move in self._generate_non_extendable_moves(diff):
317+
if not(move in processed_moves):
318+
processed_moves.add(move)
319+
yield move
320+
302321
for move in self._generate_moves(diff):
303-
if move in processed_moves:
304-
continue
305-
processed_moves.add(move)
306-
yield move
307-
moves.extend(self._extend_moves(move, diff))
322+
if not(move in processed_moves):
323+
processed_moves.add(move)
324+
yield move
325+
326+
if not(move in extended_moves):
327+
extended_moves.add(move)
328+
moves.extend(self._extend_moves(move, diff))
308329

309330
while moves:
310331
move = moves.popleft()
311-
if move in processed_moves:
312-
continue
313-
processed_moves.add(move)
314-
yield move
315-
moves.extend(self._extend_moves(move, diff))
332+
if not(move in processed_moves):
333+
processed_moves.add(move)
334+
yield move
335+
336+
if not(move in extended_moves):
337+
extended_moves.add(move)
338+
moves.extend(self._extend_moves(move, diff))
316339

317340
def validate(self, move, diff):
318341
for validator in self.move_validators:
@@ -328,6 +351,11 @@ def _generate_moves(self, diff):
328351
for move in generator.generate(diff):
329352
yield move
330353

354+
def _generate_non_extendable_moves(self, diff):
355+
for generator in self.move_non_extendable_generators:
356+
for move in generator.generate(diff):
357+
yield move
358+
331359
def _extend_moves(self, move, diff):
332360
for extender in self.move_extenders:
333361
for newmove in extender.extend(move, diff):
@@ -921,6 +949,68 @@ def validate(self, move, diff):
921949

922950
return True
923951

952+
class TableLevelMoveGenerator:
953+
"""
954+
A class that key level moves. The item name at the root level of ConfigDB is called 'Table'.
955+
956+
e.g.
957+
{
958+
"Table": ...
959+
}
960+
961+
This class will generate moves to remove tables if they are in current, but not target. It also add tables
962+
if they are in target but not current configs.
963+
"""
964+
965+
def generate(self, diff):
966+
# Removing tables in current but not target
967+
for tokens in self._get_non_existing_tables_tokens(diff.current_config, diff.target_config):
968+
yield JsonMove(diff, OperationType.REMOVE, tokens)
969+
970+
# Adding tables in target but not current
971+
for tokens in self._get_non_existing_tables_tokens(diff.target_config, diff.current_config):
972+
yield JsonMove(diff, OperationType.ADD, tokens, tokens)
973+
974+
def _get_non_existing_tables_tokens(self, config1, config2):
975+
for table in config1:
976+
if not(table in config2):
977+
yield [table]
978+
979+
class KeyLevelMoveGenerator:
980+
"""
981+
A class that key level moves. The item name at the root level of ConfigDB is called 'Table', the item
982+
name in the Table level of ConfigDB is called key.
983+
984+
e.g.
985+
{
986+
"Table": {
987+
"Key": ...
988+
}
989+
}
990+
991+
This class will generate moves to remove keys if they are in current, but not target. It also add keys
992+
if they are in target but not current configs.
993+
"""
994+
def generate(self, diff):
995+
# Removing keys in current but not target
996+
for tokens in self._get_non_existing_keys_tokens(diff.current_config, diff.target_config):
997+
table = tokens[0]
998+
# if table has a single key, delete the whole table because empty tables are not allowed in ConfigDB
999+
if len(diff.current_config[table]) == 1:
1000+
yield JsonMove(diff, OperationType.REMOVE, [table])
1001+
else:
1002+
yield JsonMove(diff, OperationType.REMOVE, tokens)
1003+
1004+
# Adding keys in target but not current
1005+
for tokens in self._get_non_existing_keys_tokens(diff.target_config, diff.current_config):
1006+
yield JsonMove(diff, OperationType.ADD, tokens, tokens)
1007+
1008+
def _get_non_existing_keys_tokens(self, config1, config2):
1009+
for table in config1:
1010+
for key in config1[table]:
1011+
if not(table in config2) or not (key in config2[table]):
1012+
yield [table, key]
1013+
9241014
class LowLevelMoveGenerator:
9251015
"""
9261016
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):
14071497

14081498
def create(self, algorithm=Algorithm.DFS):
14091499
move_generators = [LowLevelMoveGenerator(self.path_addressing)]
1500+
# TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time
1501+
move_non_extendable_generators = [KeyLevelMoveGenerator()]
14101502
move_extenders = [RequiredValueMoveExtender(self.path_addressing, self.operation_wrapper),
14111503
UpperLevelMoveExtender(),
14121504
DeleteInsteadOfReplaceMoveExtender(),
@@ -1419,7 +1511,7 @@ def create(self, algorithm=Algorithm.DFS):
14191511
RequiredValueMoveValidator(self.path_addressing),
14201512
NoEmptyTableMoveValidator(self.path_addressing)]
14211513

1422-
move_wrapper = MoveWrapper(move_generators, move_extenders, move_validators)
1514+
move_wrapper = MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators)
14231515

14241516
if algorithm == Algorithm.DFS:
14251517
sorter = DfsSorter(move_wrapper)

0 commit comments

Comments
 (0)