Fix multi-asic behaviour for ecnconfig #3062
Conversation
bktsim-arista
force-pushed
the
multi-asic-ecnconfig
branch
from
51125da
to
32b5505
Compare
|
please re-base the PR to latest |
| @@ -197,5 +197,16 @@ | |||
| "holdtime": "0", | |||
| "asn": "65100", | |||
| "keepalive": "0" | |||
There was a problem hiding this comment.
this profile should be for lossless not lossy
There was a problem hiding this comment.
The comment is outdated, but to address it, the test vectors are added as a dummy to validate the functionality.
| yellow_drop_probability 5 | ||
| red_drop_probability 5 | ||
| ----------------------- ------- | ||
|
|
There was a problem hiding this comment.
there shouldn't be wred on lossy. please fix this.
There was a problem hiding this comment.
@vmittal-msft WRED has been removed from lossy.
arista-nwolfe
force-pushed
the
multi-asic-ecnconfig
branch
from
32b5505
to
acfd7f4
Compare
arista-nwolfe
force-pushed
the
multi-asic-ecnconfig
branch
from
acfd7f4
to
067731e
Compare
| "wred_yellow_enable":"true", | ||
| "red_max_threshold":"32760", | ||
| "red_min_threshold":"4095", | ||
| "yellow_max_threshold":"32760", |
There was a problem hiding this comment.
please remove wred on lossy
There was a problem hiding this comment.
@vmittal-msft WRED has been removed from lossy.
|
@bktsim-arista |
|
/Azp run Azure.sonic-utilities |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
kenneth-arista
force-pushed
the
multi-asic-ecnconfig
branch
2 times, most recently
from
693686c
to
f59a2c8
Compare
- Removed unix socket and fixed failing unit test - Replace argparse with click - Add multi-asic support for get and set queue - Add multi-asic support for set threshold and prob - Modify test framework to support multi-asic - Use multi_asic decorators - Resolve precommit errors
kenneth-arista
force-pushed
the
multi-asic-ecnconfig
branch
from
f59a2c8
to
f995b3b
Compare
|
@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. |
Here are the snapshots obtained after running it on a multi-asic devise:
|
Yes, behaviour on single asic devices is preserved and when no namespace is given, it will show/set for all |
No, it has not been tested on T0/T1 devices yet. I will update the PR once I have tested it on the same. |
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
|
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 sonic-net/sonic-buildimage#15148
This change relies on helpers added in sonic-net/sonic-buildimage#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.