-
Notifications
You must be signed in to change notification settings - Fork 664
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 mmuconfig #3061
Fix multi-asic behaviour for mmuconfig #3061
Conversation
@bktsim-arista please re-base to latest |
@bktsim-arista please share command and it's output from multi-asic platform. Also, did we verify to make sure, it still works for ToR. |
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.
Please check above comments.
cbac167
to
cd38bc3
Compare
@vmittal-msft
Also, we tested this on a ToR, and behavior was consistent on a single ASIC system. |
cd38bc3
to
bb8f464
Compare
Thanks. @rdjeric-arista @bktsim-arista Can you please share the o/p with some config present ? If no namespace is given, will it show all namespaces ? |
---- ------- | ||
|
||
Pool: ingress_lossless_pool_hbm | ||
---- --------- |
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.
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.
@vmittal-msft the unit tests uses canned data in the mock_tables. The CLI show/config commands operate on CONFIG_DB and does not involve hardware programming. As such, it should be irrelevant whether or not the pool is HBM or not. Here's a code reference to the mock_tables:
sonic-utilities/tests/mock_tables/config_db.json
Line 1958 in d1ca905
"BUFFER_POOL|ingress_lossless_pool_hbm": { |
e608e39
to
ffaace8
Compare
@vmittal-msft @wenyiz2021 @arlakshm this PR is ready for review |
@bktsim-arista Please rebase |
@bktsim-arista is not at Arista anymore. You can redirect these requests to me or @kenneth-arista. Thank you! |
Previously, mmuconfig did not function correctly on multi-asic devices as it did not traverse through the correct namespaces, as required for multi-asic devices. This change adds multi-asic support to mmuconfig with the use of multi-asic helper libraries, ensuring that mmuconfig searches throuugh the correct namespaces when used on multi-asic devices, and adds the '-n' optional argument for users to specify namespaces on multi-asic devices.
- Resolve pre-commit errors - Remove use_unix_socket_path argument from DB connectors - Support multiple namespace when none specified - Refactor tests to use the testData dict - Delete single_asic_mmuconfig_test.py - Replace argparse with click in mmuconfig - Add support for namespace in show and config - Modified multi-asic tests to use show/config cli
ffaace8
to
febac77
Compare
@vmittal-msft Can we merge this change? |
@bktsim-arista Please raise another PR to 202405 branch to address conflict. |
What I did
Previously, mmuconfig did not function correctly on multi-asic devices as it did not traverse through the correct namespaces, as required for multi-asic devices. This change adds multi-asic support to mmuconfig with the use of multi-asic helper libraries, ensuring that mmuconfig searches throuugh the correct namespaces when used on multi-asic devices, and adds the '-n' optional argument for users to specify namespaces on multi-asic devices.
This is a part of the set of changes being pushed for sonic-net/sonic-buildimage#15148
How I did it
Added namespace option -n and used multi_asic library to implement multi_asic behaviour. Added relevant unit tests to ensure functionality.
How to verify it
Run unit tests, or mmuconfig with
-n
option.