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

Broadcast Unknown-multicast and Unknown-unicast Storm-control #928

Merged
merged 39 commits into from
May 18, 2022

Conversation

mohan-selvaraj
Copy link
Contributor

@mohan-selvaraj mohan-selvaraj commented Jun 1, 2020

Broadcast, Unknown-multicast and Unknown-unicast storm-control on Ethernet interfaces.

configuration commands

config interface storm-control broadcast add Ethernet0 10000
config interface storm-control unknown-multicast add Ethernet0 10000
config interface storm-control unknown-unicast add Ethernet0 10000

config interface storm-control broadcast del Ethernet0
config interface storm-control unknown-multicast del Ethernet0
config interface storm-control unknown-unicast del Ethernet0

show commands

show storm-control all
show storm-control interface Ethernet0

Sample output

show storm-control interface Ethernet0
+------------------+-------------------+---------------+
| Interface Name | Storm Type | Rate (kbps) |
+==================+===================+===============+
| Ethernet0 | broadcast | 10000 |
+------------------+-------------------+---------------+
| Ethernet0 | unknown-unicast | 10000 |
+------------------+-------------------+---------------+
| Ethernet0 | unknown-multicast | 10000 |
+------------------+-------------------+---------------+

- What I did
Added configuration and show commands for BUM Storm-control feature.
- How I did it
modified the config and show scripts to add the commands.
- How to verify it
pytest script test_storm_control.py
- Previous command output (if the output of a command-line utility has changed)
New commands are added. Existing commands unaffected.
- New command output (if the output of a command-line utility has changed)
New commands are added. Existing commands unaffected.

…ture.

configuration commands
----------------------
config interface storm-control broadcast add Ethernet0 10000
config interface storm-control unknown-multicast add Ethernet0 10000
config interface storm-control unknown-unicast add Ethernet0 10000

config interface storm-control broadcast del Ethernet0
config interface storm-control unknown-multicast del Ethernet0
config interface storm-control unknown-unicast del Ethernet0

show commands
-------------
show storm-control all
show storm-control interface Ethernet0

Sample output
-------------
show storm-control interface Ethernet0
+------------------+-------------------+---------------+
| Interface Name   | Storm Type        |   Rate (kbps) |
+==================+===================+===============+
| Ethernet0        | broadcast         |         10000 |
+------------------+-------------------+---------------+
| Ethernet0        | unknown-unicast   |         10000 |
+------------------+-------------------+---------------+
| Ethernet0        | unknown-multicast |         10000 |
+------------------+-------------------+---------------+
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
show/main.py Outdated Show resolved Hide resolved
@neethajohn
Copy link
Contributor

@mohan-selvaraj, please resolve the merge conflicts as well

@neethajohn
Copy link
Contributor

@mohan-selvaraj, can you address the review comments?

CharlieChenEC pushed a commit to CharlieChenEC/sonic-utilities that referenced this pull request May 14, 2021
…m-control (sonic-net#928)

CLICK CLI - Configuration and show commands for BUM Storm-control feature.

configuration commands
----------------------
config interface storm-control broadcast add Ethernet0 10000
config interface storm-control unknown-multicast add Ethernet0 10000
config interface storm-control unknown-unicast add Ethernet0 10000

config interface storm-control broadcast del Ethernet0
config interface storm-control unknown-multicast del Ethernet0
config interface storm-control unknown-unicast del Ethernet0

show commands
-------------
show storm-control all
show storm-control interface Ethernet0

Sample output
-------------
show storm-control interface Ethernet0
+------------------+-------------------+---------------+
| Interface Name   | Storm Type        |   Rate (kbps) |
+==================+===================+===============+
| Ethernet0        | broadcast         |         10000 |
+------------------+-------------------+---------------+
| Ethernet0        | unknown-unicast   |         10000 |
+------------------+-------------------+---------------+
| Ethernet0        | unknown-multicast |         10000 |
+------------------+-------------------+---------------+
@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2021

This pull request introduces 2 alerts when merging 6be166a into 474bdbf - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@srj102
Copy link
Contributor

srj102 commented Nov 4, 2021

/azpw run

1 similar comment
@mohan-selvaraj
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2021

This pull request introduces 7 alerts when merging f5b354e into 63a5257 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 1 for Unused local variable
  • 1 for Implicit string concatenation in a list

@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2021

This pull request introduces 6 alerts when merging 55768bd into 63a5257 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2021

This pull request introduces 1 alert when merging 7542172 into 63a5257 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2021

This pull request introduces 1 alert when merging 4c9945f into 00b6045 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2021

This pull request introduces 1 alert when merging 8147999 into e7535ae - view on LGTM.com

new alerts:

  • 1 for Unused import

@venkatmahalingam
Copy link
Contributor

Can we have the CLI commands like below?

OLD
config interface storm-control broadcast add/del Ethernet0 10000
config interface storm-control unknown-multicast add/del Ethernet0 10000
config interface storm-control unknown-unicast add/del Ethernet0 10000

NEW
config interface storm-control add/del Ethernet0 broadcast 10000
config interface storm-control add/del Ethernet0 unknown-multicast 10000
config interface storm-control add/del Ethernet0 unknown-unicast 10000

@venkatmahalingam
Copy link
Contributor

  • What I did

  • How I did it

  • How to verify it
    The above sections are not filled in the description.

config/main.py Outdated
pass

@broadcast.command('add')
@click.argument('port_name', metavar='<port_name>', required=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also include support for Multi-ASIC platform. ASIC name space support is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please point me to an example on how to include ASIC name space.

Copy link
Contributor

Choose a reason for hiding this comment

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

look into the following PR which supports multi-asic:
#1574

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gechiang Please review

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging 991210c into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Conflicts:
	tests/mock_tables/config_db.json
…ic-utilities into storm_control

Conflicts:
	tests/mock_tables/config_db.json
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging 93c01c3 into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging d288b08 into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging 9a85aad into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging 5b63779 into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging f1bb1cb into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2022

This pull request introduces 2 alerts when merging e4389e5 into 1143869 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 9, 2022

This pull request introduces 2 alerts when merging afe1122 into 6ab1c51 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2022

This pull request introduces 2 alerts when merging f2c32ca into 288c2d8 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@mohan-selvaraj mohan-selvaraj requested a review from gechiang May 13, 2022 10:54
@prsunny
Copy link
Contributor

prsunny commented May 17, 2022

@venkatmahalingam , @neethajohn , can you please uncheck "change requested" and signoff?

@prsunny prsunny merged commit 9881f3e into sonic-net:master May 18, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request May 25, 2022
Update sonic-utilities submodule pointer to include the following:
* [GCU] Handling type1 lists ([sonic-net#2171](sonic-net/sonic-utilities#2171))
* [yang] extend ConfigMgmt constructor to pass YANG options ([sonic-net#2118](sonic-net/sonic-utilities#2118))
* [dump] implement ACL modules ([sonic-net#2153](sonic-net/sonic-utilities#2153))
* show commands for SYSTEM READY ([sonic-net#1851](sonic-net/sonic-utilities#1851))
* [GCU] Handling non-compliant leaf-list with string values ([sonic-net#2174](sonic-net/sonic-utilities#2174))
* Add sonic-delayed.target to Application Extension .timer file generator ([sonic-net#2176](sonic-net/sonic-utilities#2176))
* [portconfig] Allow to configure interface mtu for physical ports ([#l](https://github.com/Azure/sonic-utilities/pull/l))
* Broadcast Unknown-multicast and Unknown-unicast Storm-control  ([sonic-net#928](sonic-net/sonic-utilities#928))
* sonic-utils: initial support for link-training ([sonic-net#2071](sonic-net/sonic-utilities#2071))
* [portchannel] Added ACL/PBH binding checks to the port before getting added to portchannel ([sonic-net#2151](sonic-net/sonic-utilities#2151))
* Modify override testcase to cover PORT admin_status ([sonic-net#2165](sonic-net/sonic-utilities#2165))
* [GCU] Validate peer_group_range ip_range are correct ([sonic-net#2145](sonic-net/sonic-utilities#2145))
* [auto-ts] add memory check ([sonic-net#2116](sonic-net/sonic-utilities#2116))
* support new interface types CR8/SR8/KR8/LR8 which are brougnt by SAI V.1.10.2 ([sonic-net#2167](sonic-net/sonic-utilities#2167))
* [scripts/fast-reboot] Add option to include ssd-upgrader-part boot option with SONiC partition ([sonic-net#2150](sonic-net/sonic-utilities#2150))
* [config reload] Fix invalid rstrip. ([sonic-net#2157](sonic-net/sonic-utilities#2157))
* Accept 0 for queue and dscp ([sonic-net#2162](sonic-net/sonic-utilities#2162))

Signed-off-by: dprital <drorp@nvidia.com>
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.

7 participants