Broadcast Unknown-multicast and Unknown-unicast Storm-control#1306
Broadcast Unknown-multicast and Unknown-unicast Storm-control#1306prsunny merged 86 commits intosonic-net:masterfrom
Conversation
Segregate POLICER table and PORT_STORM_CONTROL table handling
|
Please link the associated HLD and all other PR's in the various repositories here |
|
@mohan-selvaraj, could you please clarify on the design? Why do we need to create policers for every interface and storm type vs creating different policers for different cir rates and then binding them to the port based on the storm type? |
|
HLD - sonic-net/SONiC#441 |
- update key value to index the policer map in case of policer update. - use tokenize to split interface and storm-type. - updated pytest script to use common api for storm-control configuration and validation
orchagent/policerorch.cpp
Outdated
| SWSS_LOG_ERROR("Failed to remove policer %s, rv:%d", | ||
| storm_policer_name.c_str(), status); | ||
| /*TODO: Just doing a syslog. */ | ||
| } |
There was a problem hiding this comment.
use 'continue' to move on to the next set?. In the current flow, it proceeds to print 'policer removal success' as well
There was a problem hiding this comment.
continue can result in stale policer information which remains in m_syncdPolicers. Hence not using continue.
orchagent/policerorch.cpp
Outdated
| SWSS_LOG_NOTICE("Failed to delete policer %s created for port %s", | ||
| storm_policer_name.c_str(), port.m_alias.c_str()); | ||
| } |
There was a problem hiding this comment.
use 'continue' after the error
There was a problem hiding this comment.
continue can result in stale policer information which remains in m_syncdPolicers. Hence not using continue.
| SWSS_LOG_INFO("CIR %s",value.c_str()); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
The HLD also mentions 'enabled' field used in the STORM_CONTROL_TABLE for each interface, storm type. Didn't see that field handled in this PR
There was a problem hiding this comment.
since the default behavior is "enabled" didn't implement the field here. Can be introduced in a scenario where the CONFIG_DB table contains the configuration but it isn't programmed in the hardware possibly because of hardware limitation.
tests/test_storm_control.py
Outdated
| self.add_storm_control_on_interface(dvs,if_name,storm_type,kbps_value) | ||
|
|
||
| def test_del_bcast_storm(self,dvs,testlog): | ||
| #Proceeding with assumption that storm-control is already enabled on interface |
There was a problem hiding this comment.
Each test should run independently.
There was a problem hiding this comment.
Will update the test code.
|
@mohan-selvaraj, can you address the review comments? |
…ress other minor comments.
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 1306 in repo Azure/sonic-swss |
|
/AzurePipelines run |
|
Commenter does not have sufficient privileges for PR 1306 in repo Azure/sonic-swss |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@neethajohn , could you please sign-off? |
…net#1306) * Handle BUM Storm-control CONFIG_DB update. * Segregate POLICER table and PORT_STORM_CONTROL table handling * Broadcast, Unknown-multicast and Unknown-unicast storm-control on Ethernet interfaces.
…net#1306) * Handle BUM Storm-control CONFIG_DB update. * Segregate POLICER table and PORT_STORM_CONTROL table handling * Broadcast, Unknown-multicast and Unknown-unicast storm-control on Ethernet interfaces.
6 participants
Handle BUM Storm-control CONFIG_DB update.
Segregate POLICER table and PORT_STORM_CONTROL table handling
Broadcast, Unknown-multicast and Unknown-unicast storm-control on Ethernet interfaces.
configuration commands
show commands
Sample output
What I did
Why I did it
How I verified it
Details if related