-
Notifications
You must be signed in to change notification settings - Fork 659
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
[CLI][PFCWD][Multi-ASIC] Added multi ASIC support to 'pfcwd' CLI command #1080
Conversation
Working on unit test code, raised the PR so review can start. |
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 add more detail to the PR title
show/main.py
Outdated
@click.option('--verbose', is_flag=True, help="Enable verbose output") | ||
def config(verbose): | ||
"""Show pfc watchdog config""" | ||
|
||
cmd = "pfcwd show config" | ||
cmd = "pfcwd show config -n {} -d {}".format(namespace, display) |
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.
will add check for namespace 'None'
show/main.py
Outdated
"""Show pfc watchdog stats""" | ||
|
||
cmd = "pfcwd show stats" | ||
cmd = "pfcwd show stats -n {} -d {}".format(namespace, display) |
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.
will add check for namespace 'None'
Unit tests are written only for 'show' commands. Unit tests for config commands is pending on mock config set implementation for multi ASIC environment. |
will wait for reviews from @arlakshm and @neethajohn and make suggested changes. |
This pull request introduces 1 alert when merging 22bfe46 into 8d6d871 - view on LGTM.com new alerts:
|
retest this please |
Just realized pytest needs file names to start or end with “test”. Will rename file. |
…sues with mock DB connection when running separatly
This passes @neethajohn recently added unit tests on single ASIC platform. |
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.
Looks good from my perspective. Please wait for other reviewers.
@smaheshm Create PR for 201911 |
- What I did
Added multi ASIC support to the following CLI command:
Added multi ASIC support to following script:
Commands:
big_red_switch Enable/disable BIG_RED_SWITCH mode
counter_poll Enable/disable counter polling
interval Set PFC watchdog counter polling interval
show Show PFC Watchdog information
start Start PFC watchdog on port(s).
start_default Start PFC WD by default configurations
stop Stop PFC watchdog on port(s)
- How I did it
Multi ASIC decorator is used to run on multi ASIC and single ASIC. Multi ASIC requires the CLI to be a class instance.
Converted CLIs to classes to that multi ASIC decorator can be used.
Main logic change in 'show' command is:
'show' commands have the option of passing asic instance and display filter, applicable to multi ASIC platforms.
For config changes,
Config commands don't have the option to pass the asic instance or the display filter. By default its all instances and all ports.
- How to verify it
Ran the commands CLI commands on both multi ASIC and single ASIC platforms, and unit tests.
pfcstat command