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.

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

SONIC broadcast, unknown-unicast and unknown-multicast (BUM) storm-control HLD #441

Merged
merged 6 commits into from
Nov 9, 2021

Conversation

mohan-selvaraj
Copy link
Contributor

@mohan-selvaraj mohan-selvaraj commented Aug 16, 2019

…oadcast, unknown-unicast and unknown-multicast storm-control feature in SONiC

PR Tracking Table

RepoPR titleState
sonic-swssOrchestration Agent changes for BUM Storm-ControlGitHub issue/pull request detail
sonic-utilitiesCLI changes for BUM Storm-ControlGitHub issue/pull request detail
sonic-swss-commonCONFIG_DB Table changes for BUM Storm-ControlGitHub issue/pull request detail
sonic-swss-commonSTATE_DB Table changes for BUM Storm-ControlGitHub issue/pull request detail

…oadcast, unknown-unicast and unknown-multicast storm-control feature in SONiC
@msftclas
Copy link

msftclas commented Aug 16, 2019

CLA assistant check
All CLA requirements met.

@mohan-selvaraj mohan-selvaraj changed the title This document describes the functionality and high level desigh of br… SONIC broadcast, unknown-unicast and unknown-multicast storm-control HLD Aug 19, 2019
@mohan-selvaraj mohan-selvaraj changed the title SONIC broadcast, unknown-unicast and unknown-multicast storm-control HLD SONIC broadcast, unknown-unicast and unknown-multicast (BUM) storm-control HLD Aug 19, 2019

### 3.2.1 CONFIG_DB
A new table CFG_PORT_STORM_CONTROL_TABLE is introduced in the configuration database for the purpose of storing storm-control configuration parameters. This table is filled by the management framework.
#### CFG_PORT_STORM_CONTROL_TABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

missing yang model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please correct the table name as PORT_STORM_CONTROL and use "|" has key separators in config-DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated Table Name.

@lguohan
Copy link
Contributor

lguohan commented May 12, 2021

@prsunny to review

BUM storm control
- Configuration is not supported on VLAN and port-channel interfaces. User can configure on physical port which is part of a VLAN / port-channel.
- Statistics is not supported.
- User is expected to remove all storm-control configurations from interface before doing a breakout from Click CLI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, we do force delete configs during breakout..right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. will remove this comment. breakout to be handled in code.


__Figure 1: Storm Control High Level Architecture__

1) Storm-control configurations are parsed and stored in CFG_PORT_STORM_CONTROL_TABLE in Configuration database by the Management Framework.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this section to capture SONiC CLI update as well.

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 flow from CONFIG_DB update to backend is already captured.

This section lists down the Click commands.
BUM storm-control can be configured only on physical interfaces.
**switch# config interface storm-control {broadcast | unknown-unicast | unknown-multicast} {add|del} \<interface_name\> {\<kilo_bits_per_second\>}**

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to change the SONiC CLI command as follows,
config interface storm-control { add | del } {broadcast | unknown-unicast | unknown-multicast} <kilo_bits_per_second>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

storm-control policer types are keywords. add/del are operations. Keywords are kept in the beginning of the command.
user might want to know the type before performing the operation.

### 3.5.3 Show Commands
The following show command displays storm-control configurations.

**show storm-control {all | interface \<interface_name\>}**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to use the following format,
show storm-control [interface <interface_name>]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@zhangyanzhao
Copy link
Collaborator

zhangyanzhao commented Oct 12, 2021

Can we add the code PRs by referring to #806 ?
@mohan-selvaraj


### 1.1.1 Functional Requirements
1. Support configuration of Broadcast, Unknown-unicast and unknown-Multicast storm-control independently on physical interfaces.
2. Support threshold rate configuration in kilo bits per second (kbps) in the range of 0 kbps to 100,000,000 kbps (100Gbps).

Choose a reason for hiding this comment

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

Could unit packets per second (pps) also supported along with kilo bits per second (kbps).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try to accomodate the change in the future.

@zhangyanzhao
Copy link
Collaborator

@mohan-selvaraj would you please update the code PRs by following template of #806 ? Thanks.

@adyeung
Copy link
Collaborator

adyeung commented Nov 4, 2021

@zhangyanzhao Review is done, pls help push for merge

@zhangyanzhao zhangyanzhao merged commit decc8eb into sonic-net:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants