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.

Sign up for GitHub

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

Jump to bottom

[PFCWD] Fix 'start' pfcwd command #1345

Merged
merged 3 commits into from
Jan 17, 2021
Merged

[PFCWD] Fix 'start' pfcwd command #1345

merged 3 commits into from
Jan 17, 2021

Conversation

shlomibitton
Copy link
Contributor

@shlomibitton shlomibitton commented Jan 3, 2021 •
edited
Loading

- What I did
The 'config' method is getting arguments from the user CLI and build a proper command for the pfcwd script in a wrong way, causing this issue.

- How I did it
Fix the command structure to align with pfcwd.main script.

- How to verify it
Run "config pfcwd start --action drop ports all detection-time 400 --restoration-time 400"

- Previous command output (if the output of a command-line utility has changed)
config pfcwd start --action drop ports all detection-time 400 --restoration-time 400
Failed to run command, invalid options:
ports
detection-time

- New command output (if the output of a command-line utility has changed)
Command succeeded.

Fix PFCWD 'start' command
8a034a0 
Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
@liat-grozovik
Copy link
Collaborator

retest this please

@abdosi abdosi requested a review from smaheshm January 3, 2021 20:09
@abdosi
Copy link
Contributor

abdosi commented Jan 3, 2021

retest this please

@liat-grozovik liat-grozovik mentioned this pull request Jan 4, 2021
Merged
@smaheshm
Copy link
Contributor

smaheshm commented Jan 4, 2021 •
edited
Loading

"ports" and "detection-time" have been added as click arguments. This code hasn't changed.

@click.option('--action', '-a', type=click.Choice(['drop', 'forward', 'alert']))
@click.option('--restoration-time', '-r', type=click.IntRange(100, 60000))
@click.argument('ports', nargs=-1)
@click.argument('detection-time', type=click.IntRange(100, 5000))

The right syntax would be:

config pfcwd start --action drop --restoration-time 400 all 400

Looks like the documentation shows the same options that you used. Either the documentation should be changed, or the "port", "detection-time" should be changed to click options.

@smaheshm
Copy link
Contributor

smaheshm commented Jan 4, 2021

you will have to update config/main.py

@shlomibitton
Copy link
Contributor Author

shlomibitton commented Jan 5, 2021 •
edited
Loading

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

@smaheshm
Copy link
Contributor

smaheshm commented Jan 5, 2021

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

That's right. Not sure why pfcwd wasn't added as a module. Can you add a unit test that uses 'config pfcwd' CLI.

@shlomibitton
Copy link
Contributor Author

@smaheshm can you please review the change?
I have simply modified the wrapper from config.main to align with the desired command format on pfcwd.main.
Looks like on the unit-tests we are calling these function directly from pfcwd.main script and not from the config.main script so the test didn't catch this bug.

That's right. Not sure why pfcwd wasn't added as a module. Can you add a unit test that uses 'config pfcwd' CLI.

@smaheshm calling the 'config' script to start pfcwd is failing on the unit-test environment, need to further investigate it.
I suggest to merge this PR first to fix this issue and I will handle the 'config' test adaptation on a different PR.
what do you think?

@shlomibitton
Copy link
Contributor Author

retest this please

@liat-grozovik
Copy link
Collaborator

@smaheshm i agree that testing is always good to have. but in this case it is not the scopt. we are missing as a community various of test infra to add tests for bugs fixes.
as this bug is critical as one cannot configure specific pfc wd as before i suggest first to merge the fix and add to the next release backlog the infra for pfc wd testing on vs.

smaheshm
smaheshm approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

:shipit:

@liat-grozovik liat-grozovik merged commit 2cbeccb into sonic-net:master Jan 17, 2021
@shlomibitton shlomibitton mentioned this pull request Jan 18, 2021
Merged
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 20, 2021
@shlomibitton
[submodule]: update sonic-utilities (#6485)
abb0139 
- [route_check.py] - update includes checks on subscriptions (sonic-net/sonic-utilities#1344)
- Validations checks while adding a member to PortChannel and removing a member from a Portchannel (sonic-net/sonic-utilities#1328)
- [show] Add subcommand to show midplane status for modular chassis (sonic-net/sonic-utilities#1267)
- [pytest][qos][config] Added pytests for "config qos reload" commands" (sonic-net/sonic-utilities#1346)
- Drop explict 3 seconds pause between two object updates/deletes. (sonic-net/sonic-utilities#1359)
- [show]fix for show muxcable status by replacing "hostname" to "peer_switch" for deriving tor ipv4_address (sonic-net/sonic-utilities#1360)
- [PFCWD] Fix 'start' pfcwd command (sonic-net/sonic-utilities#1345)

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 21, 2021
@shlomibitton @lguohan
[submodule]: update sonic-utilities (#6485)
deea124 
- [route_check.py] - update includes checks on subscriptions (sonic-net/sonic-utilities#1344)
- Validations checks while adding a member to PortChannel and removing a member from a Portchannel (sonic-net/sonic-utilities#1328)
- [show] Add subcommand to show midplane status for modular chassis (sonic-net/sonic-utilities#1267)
- [pytest][qos][config] Added pytests for "config qos reload" commands" (sonic-net/sonic-utilities#1346)
- Drop explict 3 seconds pause between two object updates/deletes. (sonic-net/sonic-utilities#1359)
- [show]fix for show muxcable status by replacing "hostname" to "peer_switch" for deriving tor ipv4_address (sonic-net/sonic-utilities#1360)
- [PFCWD] Fix 'start' pfcwd command (sonic-net/sonic-utilities#1345)

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
abdosi pushed a commit that referenced this pull request Jan 28, 2021
@shlomibitton @abdosi
[PFCWD] Fix 'start' pfcwd command (#1345)
d324eae 
- What I did
The 'config' method is getting arguments from the user CLI and build a proper command for the pfcwd script in a wrong way, causing this issue.

- How I did it
Fix the command structure to align with pfcwd.main script.

- How to verify it
Run "config pfcwd start --action drop ports all detection-time 400 --restoration-time 400"

- Previous command output (if the output of a command-line utility has changed)
config pfcwd start --action drop ports all detection-time 400 --restoration-time 400
Failed to run command, invalid options:
ports
detection-time
anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
@shlomibitton @anand-kumar-subramanian
[PFCWD] Fix 'start' pfcwd command (sonic-net#1345)
9a965e4 
- What I did
The 'config' method is getting arguments from the user CLI and build a proper command for the pfcwd script in a wrong way, causing this issue.

- How I did it
Fix the command structure to align with pfcwd.main script.

- How to verify it
Run "config pfcwd start --action drop ports all detection-time 400 --restoration-time 400"

- Previous command output (if the output of a command-line utility has changed)
config pfcwd start --action drop ports all detection-time 400 --restoration-time 400
Failed to run command, invalid options:
ports
detection-time
@shlomibitton shlomibitton deleted the shlomi_pfcwd_fix branch March 24, 2021 20:41
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
@abdosi
[submodule update] sonic-utilities
8606d78 
d324eae (HEAD -> 201911, origin/201911) [PFCWD] Fix 'start' pfcwd command (sonic-net#1345)
235c61c [ecnconfig] handle backend port names when extracting port I/F ID from the port name (sonic-net#1361)
7f5c3b4 Drop explict 3 seconds pause between two object updates/deletes. (sonic-net#1359)
12c8992 add vlan_intf_object only if there are ipv4 or ipv6 mappings (sonic-net#1377)
52ce2c3 Add  subcommand description to interfaces counters (sonic-net#1373)
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaheshm smaheshm smaheshm approved these changes

@liat-grozovik liat-grozovik liat-grozovik approved these changes

Assignees
No one assigned
Labels
Bug Included in 201911 Branch Request for 201911 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shlomibitton @liat-grozovik @abdosi @smaheshm