sonic-utilities
sonic-utilities copied to clipboard
sonic-net
Fix multi-asic behaviour for ecnconfig
What I did
Previously, ecnconfig -l was not behaving correctly on multi-asic devices, as the '-n' namespace option was not available, and correct namespaces were not traversed on multi-asic devices to retrieve correct results for ecnconfig. This change fixes multi-asic behaviour of CLI commands that rely on ecnconfig.
This is a part of the set of changes being pushed for https://github.com/sonic-net/sonic-buildimage/issues/15148 This change relies on helpers added in https://github.com/sonic-net/sonic-buildimage/pull/17243
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 ecnconfig commands with option -n.
@bktsim-arista 1.Is this verified on pizzabox as well ? 2. If no namespace is given, will it show for all ?
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
@arlakshm @vmittal-msft @wenyiz2021 this PR is ready for review. All tests have passed with the latest updates. It will be easier to review with whitespace difference disabled because the new static checks requires reformatting (e.g. spacing) of touched Python files.
please share command and it's output from multi asic platform. Also, please make sure it still works for ToR.
Here are the snapshots obtained after running it on a multi-asic devise:
ecnconfig -l -n asic0
ecnconfig -l -n asic1
ecnconfig -l
Behaviour of queue on mutli-asic platform
Behaviour of set on multi-asic platform when no namespace is specified
@vmittal-msft 1.Is this verified on pizzabox as well ? 2. If no namespace is given, will it show for all ?
Yes, behaviour on single asic devices is preserved and when no namespace is given, it will show/set for all
I am assuming this is verified on T0/T1 device as well to make sure it is fine.
No, it has not been tested on T0/T1 devices yet. I will update the PR once I have tested it on the same.
I am assuming this is verified on T0/T1 device as well to make sure it is fine.
The changes have been verified on T0 and T1 devices, the snapshots of the same are attached below. TL;DR: It should be safe to merge
T0
T1