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 multi-asic behaviour for mmuconfig #3061

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

bktsim-arista
Copy link
Contributor

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.

@vmittal-msft
Copy link
Contributor

@bktsim-arista please re-base to latest

@vmittal-msft
Copy link
Contributor

@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.

Copy link
Contributor

@vmittal-msft vmittal-msft left a 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.

@rdjeric-arista
Copy link
Contributor

rdjeric-arista commented Apr 11, 2024

@vmittal-msft
Here's the output on a multiasic platform:

No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -l -n asic0
No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -p alpha_profile -a 2 -n asic0
root@cmp235-3:~# mmuconfig -l
No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -l -n asic0
No buffer pool information available
Profile: alpha_profile
----------  -
dynamic_th  2
----------  -

root@cmp235-3:~#

Also, we tested this on a ToR, and behavior was consistent on a single ASIC system.

@vmittal-msft
Copy link
Contributor

@vmittal-msft Here's the output on a multiasic platform:

No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -l -n asic0
No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -p alpha_profile -a 2 -n asic0
root@cmp235-3:~# mmuconfig -l
No buffer pool information available
No buffer profile information available
root@cmp235-3:~# mmuconfig -l -n asic0
No buffer pool information available
Profile: alpha_profile
----------  -
dynamic_th  2
----------  -

root@cmp235-3:~#

Also, we tested this on a ToR, and behavior was consistent on a single ASIC system.

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
---- ---------
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no hbm pool in voq chassis. Can you please have example showing something like this ?

image

Copy link
Contributor

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:

"BUFFER_POOL|ingress_lossless_pool_hbm": {

@kenneth-arista
Copy link
Contributor

@vmittal-msft @wenyiz2021 @arlakshm this PR is ready for review

@vmittal-msft
Copy link
Contributor

@bktsim-arista Please rebase

@arista-hpandya
Copy link
Contributor

@bktsim-arista Please rebase

@bktsim-arista is not at Arista anymore. You can redirect these requests to me or @kenneth-arista. Thank you!

bktsim-arista and others added 4 commits August 23, 2024 00:38
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
@arista-hpandya
Copy link
Contributor

@vmittal-msft Can we merge this change?

@vmittal-msft vmittal-msft merged commit 8f5e4b6 into sonic-net:master Sep 5, 2024
7 checks passed
@bingwang-ms
Copy link
Contributor

@bktsim-arista Please raise another PR to 202405 branch to address conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants