-
Notifications
You must be signed in to change notification settings - Fork 670
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 multi-asic behaviour for queuestat #3554
Fix multi-asic behaviour for queuestat #3554
Conversation
- Added support for iterating over all namespaces (ns) when none specified - Added a test case to verify all ns behaviour - Introduced a wrapper class to handle the mutli-asic functionality - Replaced argparse with click for better argument checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result LGTM, haven't check code details
@wenyiz2021 Can we add the backport template in the PR description similar to sonic-mgmt? This will help to remind us to mention the branches the change should be cherry-picked in when opening a PR in the future. Back port request
|
LGTM. Did we verify single asic box to make sure it is still good ? If so, we can merge. |
Yes, single asic snapshots are attached in the PR description. |
- Added support for iterating over all namespaces (ns) when none specified - Added a test case to verify all ns behaviour - Introduced a wrapper class to handle the mutli-asic functionality - Replaced argparse with click for better argument checks
Cherry-pick PR to 202405: #3635 |
- Added support for iterating over all namespaces (ns) when none specified - Added a test case to verify all ns behaviour - Introduced a wrapper class to handle the mutli-asic functionality - Replaced argparse with click for better argument checks
Hi @vmittal-msft @arista-hpandya, we suspect that this PR could potentially led to the issue mentioned here: Could you please advise if this is the expected fix. Thanks |
@auspham This PR was built on the philosophy that when no namespace is mentioned, all namespaces are considered. This behaviour also sustains if you use For clarity:
Note above how we got the expected output on asic 0 but since the interface was absent on asic1 it raised an error. Additionally the sudo ip netns command to choose asic1 was ignored because we no longer support it. For completeness, the ideal way to. use this command would be:
CMD:
|
Thanks for the clear and detail response @arista-hpandya . @yejianquan for viz |
What I did
This is in accordance to the issue sonic-net/sonic-buildimage#15148 which tracks the multi-asic support for scripts in sonic-utilities.
How I did it
Modified the queuestat script and associated test files.
How to verify it
The changes were verified by:
Previous command output (if the output of a command-line utility has changed)
Cmd:
queuestat
Result: Empty
Reason: When the namespace (
-n
/--namespace
) argument is not specified on a multi asic device the scripts will return nothing.New command output (if the output of a command-line utility has changed)
Cmd:
queuestat
Result: Runs on all asics present on the device.
Reason: To be consistent with the other PRs in #15148, we iterate over all namespaces when none is specified on a multi-asic device.
Test snapshots provided below:
Specifying invalid namespace on single asic devices:
queuestat -n asic0
Single asic behaviour is preserved:
queuestat -p Ethernet152
Multi asic behaviour when namespace is specified:
queuestat -n asic1 -p Ethernet-Rec1
Multi asic behaviour when no namespace is specified:
queuestat
.
.
.
.
.
.