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

fixing required value validator, and adding replacelane generator #2273

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented Jul 21, 2022

What I did

Fixes #2263

What is the problem?

  • Lanes is create-only field, so the parent /PORT/Ethernet124 for example needs to be deleted then added.
  • Deletion of the /PORT/Ethernet124 requires deletion of all of its dependencies.
  • In the DUT there are around 18 different dependencies
  • The search algorithm to find the moves to apply is not very efficient in this case, what it does is find the first dependency delete it. Then it will try to add the dependency back because it thinks this will bring us closer to the final goal config. But then it realizes adding the config back, will bring us back to a visited state. So we move on and try to delete another dependency, after that the search algorithm will try to add the first dependency, but this time the config state was not visited before because that's a new state.

Check the following to better understand:
initial state:

port ethernet124
dep1
dep2
dep3

Deletion dep1

port ethernet124
dep2
dep3

Adding dep1 back will rejected, because it will move us back to the initial state
Try and delete dep2

port ethernet124
dep3

Try and add dep1

port ethernet124
dep1
dep3

Now the above is a valid state, but it is useless. If you can expand that to 18 dependencies we end up with a lot of tries that are useless.

Solution is to add a custom validator that understand that once a dependency is deleted, do not add it back until we delete the /PORT/Ethernet124

Also add a generator for the deletion to make it faster to pick the dependencies to remove.

How I did it

Added new validator/generator for handling the lane replacement case. The validator/generator understand that for lane replacement the port and its dependencies need to be removed.

How to verify it

UTs TBA

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)

@@ -399,10 +399,10 @@ def _get_paths_recursive(self, config, pattern_tokens, matching_tokens, idx, com
if token == "*":
matching_keys = config.keys()
elif token.startswith("*|"):
suffix = token[2:]
suffix = token[1:] # the suffix will be `|...`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change the pattern Ethernet12|* would match Ethernet124|0 and Ethernet12|0

matching_keys = [key for key in config.keys() if key.endswith(suffix)]
elif token.endswith("|*"):
prefix = token[:-2]
prefix = token[:-1] # the prefix will be `...|`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change the pattern *|10 would match Ethernet1|10 and Ethernet1|11110

@@ -921,6 +981,8 @@ def validate(self, move, diff):
for required_path, required_value in data[path]:
current_value = self.identifier.get_value_or_default(current_config, required_path)
simulated_value = self.identifier.get_value_or_default(simulated_config, required_path)
if simulated_value == None: # Simulated config does not have this value at all.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If simulated config does not have the value, it means the move is deleting its parent. in such case skip and assume the mvoe is valid. Let other validators decide.

@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2022

This pull request introduces 3 alerts when merging 63278a8 into 27667cf - view on LGTM.com

new alerts:

  • 3 for Testing equality to None

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] Ethernet lane update fails, hung on patch sorting step
1 participant