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

Fix call for spanning-tree commands in dump script #3723

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

DavidZagury
Copy link
Contributor

What I did

Fix the calls for spanning-tree commands in dump script.
During call to generate techsupport, we can see that the spanning tree commands fail:

.
Error: No such command "spanning_tree".
timeout --foreground 5m bash -c "dummy_cleanup_method ()
.

How I did it

Change from show spanning_tree to the actual command show spanning-tree

How to verify it

Call show techsupport

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)

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DavidZagury DavidZagury marked this pull request as ready for review January 22, 2025 12:58
Copy link
Contributor

@FengPan-Frank FengPan-Frank left a comment

Choose a reason for hiding this comment

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

Seems we'll need some test for generate_dump flow.

stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Jan 27, 2025
@qiluo-msft
Copy link
Contributor

@DavidZagury could you add a sonic-mgmt or unit test to prevent regression like this?

@liat-grozovik
Copy link
Collaborator

@DavidZagury i wonder what was the PRs that changed the spanning tree and didnt catch it, can you share it?
this is a degradation which should be avoided in the PR review and the PR checkers of the original one. Maybe we are missing a unit test or a test?

@DavidZagury
Copy link
Contributor Author

DavidZagury commented Feb 2, 2025

@DavidZagury i wonder what was the PRs that changed the spanning tree and didnt catch it, can you share it? this is a degradation which should be avoided in the PR review and the PR checkers of the original one. Maybe we are missing a unit test or a test?

@liat-grozovik the PR the introduced this change in sonic-utilities was #3567 by @divyachandralekha
There are no unit test for this area in the code.
There is a sonic-mgmt test, that touch this area, but it only check that a specific set of commands run successfully, and does not check if all commands that run during techsupport has passed

@DavidZagury
Copy link
Contributor Author

@DavidZagury could you add a sonic-mgmt or unit test to prevent regression like this?

@qiluo-msft
At the moment we don't have capacity to handle such request. This bug was created from a PR from broadcom ( #3567 ) maybe they could create a UT in this area

@qiluo-msft qiluo-msft merged commit 9d273f1 into sonic-net:master Feb 3, 2025
7 checks passed
@wajahatrazi
Copy link
Contributor

Hi @DavidZagury ,

Just wondering the steps you took and why regarding this error. Can you share the process? Would be helpful in upcoming PRs.

@DavidZagury
Copy link
Contributor Author

Hi @DavidZagury ,

Just wondering the steps you took and why regarding this error. Can you share the process? Would be helpful in upcoming PRs.

Hi @wajahatrazi , after I saw that when running show techsupport there is the error which I shared in the PR:

.
Error: No such command "spanning_tree".
timeout --foreground 5m bash -c "dummy_cleanup_method ()
.

I check the generate dump script, saw that the relevant CLIs are:

show spanning_tree
show spanning_tree statistics
show spanning_tree bpdu_guard
show spanning_tree root_guard

I tried them manually on a switch, and saw that indead this CLIs don't work.
To understand why it was added, when and by who, I have check the PR which added these lines, and saw that it also created the show command.

The show command has been defined:

sonic-utilities/show/main.py

Lines 1998 to 2000 in 9d273f1

@runningconfiguration.command()
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def spanning_tree(verbose):

In SONiC we use click to create the CLIs, and I know that when click see a command with underscore as definition, the created CLI is with dash instread, as we can see in the release note from version 7.0.0:

Subcommands that are named by the function now automatically have the underscore replaced with a dash. If you register a function named my_command it becomes my-command in the command line interface.
https://click.palletsprojects.com/en/stable/changes/#version-7-0

I have check running the CLIs with the correct name, and the error did not appear.
I have updated the generate dump script, and made sure there are no error.

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202411: #3752

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.

8 participants