swss: Fix sFlow sampling-rate and admin-state#1224
swss: Fix sFlow sampling-rate and admin-state#1224GarrickHe wants to merge 1 commit intosonic-net:masterfrom GarrickHe:sflow-fix
Conversation
* Fixing Add logic to allow user to revert to
default sFlow sampling-rate on
interface.
* Fixing Fixed incorrect interface admin-status
when 'all' interface is disabled but
there are local sampling-rate configurations.
Signed-off-by: Garrick He <[email protected]>
|
@dgsudharsan , @padmanarayana, can you review? |
|
PR for CLICK CLI script update: |
|
retest this please |
|
is there a test to cover this? |
|
@lguohan can we merge this fix first and put the test-coverage in a separate PR? |
|
@GarrickHe , it is better to have this tested as part of |
| { | ||
| for (auto it: m_sflowPortConfMap) | ||
| { | ||
| if (!it.second.local_conf) |
There was a problem hiding this comment.
In configuring admin_state field of CONFIG_DB SFLOW|global table, both sflowHandleSessionAll and sflowHandleSessionLocal will be called if m_intfAllConf is true (default value). Before your change, it.second.local_conf, to my understanding, controls the mutual exclusion that an individual port is set in either sflowHandleSessionAll or sflowHandleSessionLocal, depending on whether it has its own configuration or not. Now you remove this variable, a port having its own config can be set twice to APPL_DB by both sflowHandleSessionAll and sflowHandleSessionLocal, calling m_appSflowSessionTable.set(it.first, fvs).
There was a problem hiding this comment.
@wangxin - Are you asking a question or suggesting a change?
I replaced port_info.local_conf with two variables port_info.local_rate_cfg and port_info.local_admin_cfg to differeniate between a local change to the port's admin-state and a local change to the port's sampling-rate. This change is needed to address the issue at hand. Without it we will hit a case where the user configured a port's sampling-rate, but the port will not obey the global admin-status (because there is a local config), as the unfixed UT demonstrates.
Now you remove this variable, a port having its own config can be set twice to APPL_DB by both sflowHandleSessionAll and sflowHandleSessionLocal, calling m_appSflowSessionTable.set(it.first, fvs).
Not sure what you mean here. If a port has a local config (either sample-rate, admin-status, or both), APPL_DB will be set when either sflowHandleSessionAll or sFlowHandleSessionLocal is called. This behavior is exactly the same as when there was only one variable port_info.local_config, because we are doing a OR (||) check:
https://github.com/Azure/sonic-swss/pull/1224/files#diff-e3e7c0a4d48317ec70d5339d6fd3c6d1R136
https://github.com/Azure/sonic-swss/pull/1224/files#diff-e3e7c0a4d48317ec70d5339d6fd3c6d1R162
Each of those function can configure ApplDB once as needed.
There was a problem hiding this comment.
As you said in reply "APPL_DB will be set when either sflowHandleSessionAll or sFlowHandleSessionLocal is called", APPL_DB will be set twice here https://github.com/Azure/sonic-swss/blob/master/cfgmgr/sflowmgr.cpp#L289
There was a problem hiding this comment.
Yes, once per function call and only if m_intfallConf evaluates to true. This is needed. I'm still not sure if you're trying to make a suggestion or asking a question...
|
The problem described is an expected behavior from HLD 6.2 Configuration and control flow In control flow https://github.com/Azure/SONiC/blob/master/images/sflow/sflow_intf_disable_all.png |
We need to consider that there are two properties in a port that can be globally or locally configured, the sample-rate and admin-state. The issue here is, if the user configures JUST the sample-rate, the admin-status SHOULD follow what is global. Therefore, if I confiigure just the sample-rate on EthernetX then I disable sFlow on all interface, EthernetX should also be disabled. The admin-state only becomes locally configured if the user explicitly enable/disable it. @padmanarayana @dgsudharsan, what do you think? I believe the HLD should be updated to reflect this. |
|
@GarrickHe and @wendani please note that we need this fix for 202012 where sflow should be qualified as a new feature. For your information the problem encountered by Nvidia on 202012 is as following "keyword 'all' is ignored when per-interface sflow is enabled/disabled " . Could it be that this PR is expected to fix the problem? |
@liat-grozovik - I'll take care of this. the PR has been reviewed and approved. the only thing missing is test-cases to cover this fix. Sorry this PR slipped under my radar. I'll work on it. Thanks. |
|
Thank you very much
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Garrick He <[email protected]>
Sent: Friday, February 26, 2021 12:04:35 AM
To: Azure/sonic-swss <[email protected]>
Cc: Liat Grozovik <[email protected]>; Mention <[email protected]>
Subject: Re: [Azure/sonic-swss] swss: Fix sFlow sampling-rate and admin-state (#1224)
@GarrickHe<https://github.com/GarrickHe> and @wendani<https://github.com/wendani> please note that we need this fix for 202012 where sflow should be qualified as a new feature.
if possible please make sure it can be cherry picked and the label of 'required for 202012' is added.
as this is a 6 month old PR i strongly suggest to move forward with it. any objection?
For your information the problem encountered by Nvidia on 202012 is as following "keyword 'all' is ignored when per-interface sflow is enabled/disabled " . Could it be that this PR is expected to fix the problem?
@liat-grozovik<https://github.com/liat-grozovik> - I'll take care of this. the PR has been reviewed and approved. the only thing missing is test-cases to cover this fix. Sorry this PR slipped under my radar. I'll work on it. Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1224 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKTABAZRHDFOXY4UOJJL3Y3TA3CPHANCNFSM4LOZ3ZZQ>.
|
|
#1728 has been created to progress on this PR, please follow the new PR. |
|
Closing this in favor of #1728 |
…ble (sonic-net#1224) - What I did Updates to bgp config and show commands with the movement of internal bgp sessions to a new table BGP_INTERNAL_NEIGHBOR table. With the introduction of BGP_INERNAL_NEIGHBOR table, the user cannot start/stop/remove internal neighbors which I feel is not needed as these internal bgp sessions which are supposed to be UP always. - How I did it Updated the show/bgp util so that it gets the internal bgp sessions from BGP_INTERNAL_NEIGHBOR table. Removed the API's to get the local_as in config/bgp which is not required now. - How to verify it Made sure that the output remain same with the new internal bgp neighbor table.
7 participants
default sFlow sampling-rate on
interface.
when 'all' interface is disabled but
there are local sampling-rate configurations.
Signed-off-by: Garrick He [email protected]
What I did
Fixed a bug where the interface sFlow admin-status is not correct if the user configures a custom sFlow sampling-rate. (UT performed here)
As a side-effect of this fix, users can now restore the default sFlow sampling-rate with the
defaultoption using:config sflow interface sampling-rate Ethernet4 defaultThis is not tested here but will be tested when the config script update PR is submitted to sonic-utilities. The sFlow corresponding HLD and command-line reference will also be updated.
Why I did it
If the user ONLY configures a custom (local) sFlow sampling-rate for a interface, the interface's admin-status should still adhere to the global interface admin-status.
How I verified it
Ran unit-test before and after the fix. See 'details' section below.
Details if related