[sflow] Added sflow dropped packet notification support #4116
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for sFlow drop notification monitoring with rate limiting. It introduces a new CLI command to enable/disable drop monitoring and configure the maximum rate of drop notifications.
- New
drop-monitor-limitcommand to configure drop notification rate limiting (0-500 packets/sec) - Updated display formatting with consistent column alignment (ljust_len = 35)
- Added test coverage for the new drop-monitor-limit functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/main.py | Added drop_monitor_limit command and is_sflow_mirror_on_drop_supported() helper function to configure drop notification rate limiting |
| show/sflow.py | Updated display formatting for consistent alignment and added drop notification limit to output |
| tests/sflow_test.py | Added test for drop-monitor-limit command and updated expected output strings to match new formatting, fixed import path for YANG validation mocks |
| doc/Command-Reference.md | Added documentation for the new config sflow drop-monitor-limit command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
anders-nexthop
left a comment
anders-nexthop
left a comment
There was a problem hiding this comment.
A couple of minor comments but otherwise it looks good.
@qiluo-msft @StormLiangMS can we find someone with review permissions to review?
| assert result.exit_code == 0 | ||
| assert result.output == show_sflow_output_drop_monitor_enable | ||
|
|
||
| return |
There was a problem hiding this comment.
nit: you don't need a return here
| # drop-monitor-limit : Invalid | ||
| result = runner.invoke(config.config.commands["sflow"].commands["drop-monitor-limit"], ["NA"], obj=obj) | ||
| print(result.output) | ||
| expected = "Error: Invalid value for \"<packet_per_second>\": NA is not a valid integer" |
There was a problem hiding this comment.
you could add another error case for integers that are outside the range as well (like -1 or 501)
| return | ||
|
|
||
| @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) | ||
| @patch("config.validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) |
There was a problem hiding this comment.
this seems unrelated. was it not working before? why is this being added only now?
2 participants
What I did
Added sflow dropped packet notification support, HLD : sonic-net/SONiC#1786
How I did it
Enhancement
How to verify it
Added UT to verify the commands.
Previous command output (if the output of a command-line utility has changed)
show sflow
New command output (if the output of a command-line utility has changed)
show sflow