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] Add RemoveCreateOnlyDependency Validator/Generator #2500

Merged
merged 3 commits into from
Dec 2, 2022

Conversation

wen587
Copy link
Contributor

@wen587 wen587 commented Nov 15, 2022

What I did

Refer #2273

Efficiency Improvement:

  • Add RemoveCreateOnlyDependency Generator and Validator.

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 understands that for which create-only fields and their dependencies need to be removed.

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)

@wen587 wen587 marked this pull request as ready for review November 16, 2022 02:33
1. Make LaneReplacementValidator into a general Validator.
2. Don't change to sonic-cfggen and submit issue for show command slow execution.
@wen587 wen587 changed the title [GCU] Add LaneReplacement Generator [GCU] Add RemoveCreateOnlyDependency Validator/Generator Nov 25, 2022
if simulated_member is None:
return True

if table_to_check == "PORT":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft , still need to confirm admin_status need to be set with "down" for PORT["lanes"] change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. PORT["lanes"] are created only field. If the user require to change this field, we need to delete port and create new port. It is already a disruptive change, set admin_status to down will make it super safe on all platform, and does not make things worse.

@wen587 wen587 merged commit a5e1e2b into sonic-net:master Dec 2, 2022
@wen587
Copy link
Contributor Author

wen587 commented Dec 2, 2022

Should also add to 202211.

preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Dec 6, 2022
)

What I did
Add RemoveCreateOnlyDependency Generator and Validator.

How I did it
Added new validator/generator for handling the lane replacement case. The validator/generator understands that for which create-only fields and their dependencies need to be removed.

How to verify it
Unit Test.
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Dec 6, 2022
Update sonic-utilities submodule pointer to include the following:
* ca9a020 [generate_dump] [Mellanox] Fix the duplicate dfw dump collection problem by adding symlinks ([sonic-net#2536](sonic-net/sonic-utilities#2536))
* 92c7001 [config] Add check in config interface ip command to block if the interface is portchannel member ([sonic-net#2539](sonic-net/sonic-utilities#2539))
* e8130f5 [system-health] Improve code structure of system health CLIs ([sonic-net#2453](sonic-net/sonic-utilities#2453))
* 00c01b3 Transceiver eeprom dom CLI modification to show output from TRANSCEIVER_DOM_THRESHOLD table ([sonic-net#2535](sonic-net/sonic-utilities#2535))
* 42f51c2 sonic-utilities: Update config reload() to verify formatting of an input file ([sonic-net#2529](sonic-net/sonic-utilities#2529))
* a5e1e2b [GCU] Add RemoveCreateOnlyDependency Validator/Generator ([sonic-net#2500](sonic-net/sonic-utilities#2500))
* 6411b52 [QoS] Introduce delay to the qos reload flow ([sonic-net#2503](sonic-net/sonic-utilities#2503))
* fce7ec3 Use github code scanning instead of LGTM ([sonic-net#2530](sonic-net/sonic-utilities#2530))
* 91bd6de Change show kube command default value of insecure key to True ([sonic-net#2517](sonic-net/sonic-utilities#2517))
* c44c584 Add db_migrator_constants.py script to setup.py ([sonic-net#2534](sonic-net/sonic-utilities#2534))
* 6a3238e [drop counters] Fix CLI script for unconfigured PGs ([sonic-net#2518](sonic-net/sonic-utilities#2518))
* 263810b Update vrf add, del commands for duplicate/non-existing VRFs ([sonic-net#2467](sonic-net/sonic-utilities#2467))
* addae73 Port 202012 DB migration changes to newer branches ([sonic-net#2515](sonic-net/sonic-utilities#2515))
* 2af8cfa [VXLAN]Fixing traceback in show remotemac when mac moves during command execution ([sonic-net#2506](sonic-net/sonic-utilities#2506))

Signed-off-by: dprital <[email protected]>
yxieca pushed a commit that referenced this pull request Dec 7, 2022
What I did
Add RemoveCreateOnlyDependency Generator and Validator.

How I did it
Added new validator/generator for handling the lane replacement case. The validator/generator understands that for which create-only fields and their dependencies need to be removed.

How to verify it
Unit Test.
StormLiangMS pushed a commit that referenced this pull request Dec 30, 2022
What I did
Add RemoveCreateOnlyDependency Generator and Validator.

How I did it
Added new validator/generator for handling the lane replacement case. The validator/generator understands that for which create-only fields and their dependencies need to be removed.

How to verify it
Unit Test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants