Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCU] Optimizing moves by adding generators for keys/tables #2120

Merged
merged 4 commits into from
Apr 1, 2022

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented Mar 28, 2022

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):
{"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
{"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
{"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
{"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

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)

@ghooo ghooo marked this pull request as ready for review March 29, 2022 19:05
@wen587
Copy link
Contributor

wen587 commented Mar 31, 2022

LGTM

@wen587
Copy link
Contributor

wen587 commented Mar 31, 2022

This PR fix the log anaylyzer ERR for ACL_RULE with prompting invalid configuration when apply-patch before optimization. It also greatly reduces the trial numbers of Yang model validation.

moves = deque([])

for move in self._generate_non_extendable_moves(diff):
if not(move in processed_moves):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not(move in processed_moves

Minor suggestion:

move not in processed_moves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another PR to to hide log printing from yanglib, will include this fix as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GCU] Optimize full key deletion/addition, to be done in a single step rather than multiple
3 participants