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

Add CLI support for configurable drop counters #688

Merged
merged 12 commits into from
Nov 19, 2019
Merged

Add CLI support for configurable drop counters #688

merged 12 commits into from
Nov 19, 2019

Conversation

daall
Copy link
Contributor

@daall daall commented Oct 1, 2019 •
edited
Loading

Add CLI support for configurable drop counters

  • Adds dropconfig script to configure drop counters
  • Adds dropstat script to show and clear drop counters
  • Adds handlers for drop counters to show, config, and sonic-clear

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

  • A command reference update has been provided

Details if related
Depends on:

Signed-off-by: Danny Allen daall@microsoft.com

@daall
Copy link
Contributor Author

daall commented Oct 2, 2019 •
edited
Loading

I got some feedback on the behavior of the scripts that I wanted to note here:

  • It would be helpful to print the descriptions for the counters in the header of show drops counts so that users know what they're looking at

Because drop reasons are very platform specific it would probably be good to include the check for supported counters and drop reasons at this stage and work with the platform vendors to support the query APIs along with the drop counters (addressed below)

@daall daall marked this pull request as ready for review October 3, 2019 22:08
dev-aviz
dev-aviz reviewed Oct 16, 2019
View reviewed changes
scripts/dropconfig
self.config_db.set_entry(DEBUG_COUNTER_CONFIG_TABLE, counter_name, None)
self.config_db.delete_table(self.config_db.serialize_key((DROP_REASON_TABLE_PREFIX, counter_name)))

def add_reasons(self, counter_name, reasons):
Copy link

Choose a reason for hiding this comment

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

Do user need to reset the drop counter explicitly when drop_reasons are added/removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I don't believe the counter gets cleared when drop reasons are added and removed from the counter but I need to verify.

Copy link
Contributor Author

@daall daall Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

They don't appear to be automatically cleared when drop reasons are added/removed. Do you have any concerns with this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The counters are cleared when drop reasons are added and removed.

@lguohan
Copy link
Contributor

lguohan commented Oct 18, 2019

can you follow example here to add unit test for your show command.

https://github.com/Azure/sonic-utilities/tree/master/sonic-utilities-tests

lguohan
lguohan requested changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

need unit test for show commands

@daall daall requested a review from jleveque October 29, 2019 18:35
jleveque
jleveque suggested changes Oct 29, 2019
View reviewed changes
show/main.py Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft reviewed Oct 31, 2019
View reviewed changes
config/main.py Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft reviewed Oct 31, 2019
View reviewed changes
scripts/dropconfig Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft reviewed Oct 31, 2019
View reviewed changes
scripts/dropconfig Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft reviewed Oct 31, 2019
View reviewed changes
scripts/dropstat Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft reviewed Oct 31, 2019
View reviewed changes
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

commented

@daall
Copy link
Contributor Author

daall commented Nov 5, 2019

need unit test for show commands

added!

@daall
Add CLI support for configurable drop counters
61aa070 
- Adds dropconfig script to configure drop counters
- Adds dropstat script to show and clear drop counters
- Adds handlers for drop counters to show, config, and sonic-clear

Signed-off-by: Danny Allen <daall@microsoft.com>
@daall
Update command naming
3b8d26d
@daall
Add safety check for unsupported drop counters/reasons
3fcb0a8
@daall
Make drop reasons more easily readable
0cdda0d
@daall
Respond to community feedback
3597154
@daall
Copy link
Contributor Author

daall commented Nov 5, 2019

retest this please

qiluo-msft
qiluo-msft reviewed Nov 5, 2019
View reviewed changes
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please check other review comments.
In future, please minimize force-push, because we are using internal tool to compare between iterations, and force push will lose comment context.

@daall
Copy link
Contributor Author

daall commented Nov 5, 2019

Looks good to me. Please check other review comments.
In future, please minimize force-push, because we are using internal tool to compare between iterations, and force push will lose comment context.

Will do, thanks for the heads up! And thanks for the review! :)

@daall daall requested a review from yxieca November 11, 2019 23:08
jleveque
jleveque reviewed Nov 15, 2019
View reviewed changes
clear/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@yxieca yxieca merged commit 587e630 into sonic-net:master Nov 19, 2019
@daall daall deleted the drop_counter_cli branch January 16, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft left review comments

@dev-aviz dev-aviz dev-aviz left review comments

@jleveque jleveque jleveque approved these changes

@yxieca yxieca Awaiting requested review from yxieca

@lguohan lguohan Awaiting requested review from lguohan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@daall @lguohan @jleveque @qiluo-msft @dev-aviz @yxieca