Sflow orchagent changes #1012

dgsudharsan · 2019-07-31T22:06:09Z

What I did
Added support in swss for SFLOW. It includes sfloworch and sflowmgr changes.

Why I did it
To support SFLOW in SONIC

How I verified it
The verification is done using the vs tests which are part of these pull request.

Details if related
sflow_swss.txt

Mgr changes Mgr changes 2 Test cases Changing Global to global optimizations Reverting copp changes clean up UT Fixes Addressing code review comments Updating UT Build fix Adding mgr changes

prsunny · 2019-09-03T22:32:04Z

cfgmgr/sflowmgr.cpp

+{
+    vector<FieldValueTuple> fieldValues;
+
+    fieldValues.emplace_back(SFLOW_SAMPLE_RATE_KEY_400G, SFLOW_SAMPLE_RATE_VALUE_400G);


Can you create a map with enum values. You can refer vxlanorch.cpp/crmorch.cpp.

prsunny · 2019-09-03T22:34:39Z

cfgmgr/sflowmgr.cpp

+        }
+        else if(op == DEL_COMMAND)
+        {
+            m_appSflowTable.del(key);


Do we need to stop hsflowd if it is running during delete case?

prsunny · 2019-09-03T22:36:42Z

cfgmgr/sflowmgr.cpp

+
+    auto table = consumer.getTableName();
+
+    if(table == CFG_SFLOW_TABLE_NAME)


space after if

cfgmgr/sflowmgr.cpp

prsunny · 2019-09-03T22:49:13Z

cfgmgr/sflowmgr.h

+private:
+    Table m_cfgSflowTable;
+    Table m_cfgSflowSessionTable;
+    Table m_appSflowSpeedRateTable;


Why is this a Table?. It is m_app and must be ProducerStateTable . If it is not used, please delete this and the references.

prsunny · 2019-09-03T23:53:37Z

orchagent/sfloworch.cpp

+    Orch(db, tableNames)
+{
+    SWSS_LOG_ENTER();
+    m_sflowSampleRateTable = unique_ptr<Table>(new Table(db, APP_SFLOW_SAMPLE_RATE_TABLE_NAME));


i'm not fully understanding this sflowSampleRateTable . Does orchagent subscribe to this and get the info? Why to query the table directly?

prsunny · 2019-09-03T23:57:09Z

orchagent/sfloworch.cpp

+    sai_attribute_t attr;
+    sai_status_t    sai_rc;
+
+


Please remove extra line

prsunny · 2019-09-03T23:58:23Z

orchagent/sfloworch.cpp

+    for(auto& pair: gPortsOrch->getAllPorts())
+    {
+        auto& port = pair.second;
+        if (port.m_type != Port::PHY) continue;


Please use braces {

prsunny · 2019-09-03T23:58:59Z

orchagent/sfloworch.cpp

+        auto sflowInfo = m_sflowPortSessionMap.find(port.m_port_id);
+        if(sflowInfo == m_sflowPortSessionMap.end())
+        {
+            if(!enable)


This could be checked above and returned - Why to do inside loop?

This is inside another if condition inside for loop. Only when there is no session available for port and when global is enabled, the session needs to be created. When enable is set to false when there is no session on port already it can be skipped.

prsunny · 2019-09-04T00:00:07Z

orchagent/sfloworch.cpp

+        if (port.m_type != Port::PHY) continue;
+
+        auto sflowInfo = m_sflowPortSessionMap.find(port.m_port_id);
+        if(sflowInfo == m_sflowPortSessionMap.end())


Extra space after if - Please check other places as well

prsunny · 2019-09-05T22:31:33Z

Please provide a control flow diagram in the sFlow HLD for orchagent code flow.

prsunny

@dgsudharsan , Can we schedule a meeting to go over the implementation. The design is simple but the implementation has been found to be complex and I think we can find some areas to optimize.

cfgmgr/sflowmgr.cpp

prsunny · 2019-09-26T17:40:30Z

orchagent/sfloworch.cpp

+    while (it != consumer.m_toSync.end())
+    {
+        auto tuple = it->second;
+        string op = kfvOp(tuple);


op is not used

prsunny · 2019-09-26T18:25:23Z

orchagent/sfloworch.cpp

+            SflowSession session;
+            uint32_t     rate = 0;
+
+            sflowGetDefaultSampleRate(port, rate);


Shouldn't we handle the error case here?

prsunny · 2019-09-26T18:35:46Z

orchagent/sfloworch.cpp

+                }
+            }
+        }
+        else


The if and else is long that its hard to find which is the matching if. Can you add a one line comment here?

prsunny · 2019-09-26T18:39:21Z

orchagent/sfloworch.cpp

+    }
+
+    auto it = consumer.m_toSync.begin();
+    while (it != consumer.m_toSync.end())


This has become so long. Can you revisit this?

dgsudharsan · 2019-10-08T05:12:11Z

retest this please

prsunny · 2019-10-10T00:56:22Z

cfgmgr/sflowmgr.h

+    ProducerStateTable     m_appSflowTable;
+    ProducerStateTable     m_appSflowSessionTable;
+    SflowPortLocalConfMap  m_sflowPortLocalConfMap;
+    bool                   intf_all_conf;


please follow the same coding style - m_intfAllConf, m_gEnable

prsunny · 2019-10-10T00:57:48Z

cfgmgr/sflowmgr.h

+{
+    bool        local_conf;
+    std::string speed;
+    std::string local_rate;


Suggest to use just rate instead of local_rate, same for other params

prsunny · 2019-10-10T00:59:20Z

cfgmgr/sflowmgr.h

+    void sflowHandleSessionAll(bool enable);
+    void sflowHandleSessionLocal(bool enable);
+    void sflowCheckAndFillRate(std::string alias, std::vector<FieldValueTuple> &fvs);
+    void sflowGetLocalFvs(std::vector<FieldValueTuple> &fvs, SflowLocalPortInfo &local_info);


sflowGetLocalPortInfo, similar to sflowUpdateLocalPortInfo

prsunny · 2019-10-10T00:59:44Z

cfgmgr/sflowmgr.h

+    void sflowHandleSessionLocal(bool enable);
+    void sflowCheckAndFillRate(std::string alias, std::vector<FieldValueTuple> &fvs);
+    void sflowGetLocalFvs(std::vector<FieldValueTuple> &fvs, SflowLocalPortInfo &local_info);
+    void sflowGetGlobalFvs(std::vector<FieldValueTuple> &fvs, std::string speed);


sflowGetGlobalInfo?

prsunny · 2019-10-10T01:03:47Z

cfgmgr/sflowmgr.cpp

+    }
+    else
+    {
+        SWSS_LOG_INFO("Command '%s' succeeded", cmd.str().c_str());


You may want to put is as NOTICE stating "Starting hsflowd service"

orchagent/sfloworch.h

prsunny · 2019-10-10T01:48:22Z

orchagent/sfloworch.h

+private:
+    SflowPortInfoMap    m_sflowPortInfoMap;
+    SflowRateSampleMap  m_sflowRateSampleMap;
+    bool                sflowStatus;


pls rename to m_sflowStatus

prsunny · 2019-10-10T01:51:08Z

orchagent/sfloworch.cpp

+    {
+        if (!sflowDestroySession(m_sflowRateSampleMap[old_rate]))
+        {
+            SWSS_LOG_ERROR("Failed to clean old session %lx",


Can you please print the old_rate value also?

prsunny · 2019-10-10T01:52:10Z

orchagent/sfloworch.cpp

+
+        if (op == SET_COMMAND)
+        {
+            sflowExtractFvs (kfvFieldsValues(tuple), sflowStatus, rate);


Redundant space before (. Set the rate param to default as 0 in declaration so that you don't have to specify here.

prsunny · 2019-10-10T02:04:32Z

orchagent/sfloworch.cpp

+                admin_state = sflowInfo->second.admin_state;
+            }
+
+            sflowExtractFvs(kfvFieldsValues(tuple), admin_state, rate);


So it just overwrites the admin_state and rate retrieved from above. I'm not fully understanding the case here. Do you intend else here?

If there is an already existing session, then the current admin and rate are initialized from it. Then based on key value pair parsing, we fill the admin_state and rate and override it.
In the logic below, the existing session admin state and rate are compared with the above values. If there is difference, then the session is updated, else it is ignored.
if (admin_state != sflowInfo->second.admin_state)

if (rate != sflowSessionGetRate(sflowInfo->second.m_sample_id))
For e.g current session has admin=true and rate=100
User configured new rate as 200
The in above logic first the admin is initialized to "true" and rate to 100
in key value parsing, admin is not re-initialized while rate is set to 200
In the checks below, admin state is not changed so no reprogramming is done, while rate is changed it is updated.

dgsudharsan · 2019-10-17T01:25:10Z

retest this please

* Change admin_state from enable/disable to up/down to match SONiC YANG. Signed-off-by: Garrick He <garrick_he@dell.com>

prsunny · 2019-10-22T01:49:23Z

retest this please

…oworch

prsunny · 2019-10-22T18:55:36Z

retest this please

prsunny · 2019-10-22T22:33:30Z

retest this please

dgsudharsan · 2019-10-23T18:12:06Z

retest this please

prsunny · 2019-10-23T23:43:07Z

retest this please

dgsudharsan · 2019-10-24T02:33:22Z

retest this please

wendani · 2020-08-12T05:08:43Z

cfgmgr/sflowmgr.cpp

+                            sflowHandleSessionAll(enable);
+                        }
+                        sflowHandleSessionLocal(enable);


Typical practice for admin_state toggle is just to propagate the field-value tuple change to APPL_DB. Here, when looking into these two function internals, admin_state: down triggers deleting the entire key, say Ethernet1, in APPL_DB SFLOW_SESSION table instead of an update to field admin_state under the key. This is consistent with the HLD. But quite curious about the design thought behind it.

@wendani : Sflow has the following commands

config sflow enable ------------> Enable/disable feature as a whole. When this is disabled, it means all sflow configurations need to be removed (Feature level disable).

config sflow interface all enable/disable ----> Global level sflow command to control the enabling/disabling sflow globally. By default when sflow feature is enabled, all ports will have flow enabled. The main usage of this command is in 'disable' configuration. When users need to enable sflow on only one interface or very few interfaces, without global interface disable command, the user has to configure individual interface disable on the rest of the interfaces except for the interfaces of interest. Thus to avoid this, users can have sflow global interface all disable and then do individual sflow enable on interested interfaces which would override the global setting.

config sflow interface Ethernetx enable/disable ------> Local interface level command which would override global interface all command when configured on specific interfaces

wendani · 2020-08-29T05:18:18Z

cfgmgr/sflowmgr.cpp

+                if (m_gEnable)
+                {
+                    sflowHandleService(false);
+                    sflowHandleSessionAll(false);


Also call sflowHandleSessionLocal(false); here to disable individually configured port(s); otherwise redis-cli -n 4 del "SFLOW|global" command will not remove APPL_DB SFLOW_SESSION_TABLE:EthernetX for individually configured port EthernetX.

Agreed. This needs to be fixed.

wendani · 2020-08-29T05:31:43Z

cfgmgr/sflowmgr.cpp

+                {
+                    if (!intf_all_conf)
+                    {
+                        sflowHandleSessionAll(true);


Should feed m_gEnable instead of true. Consider the following sequence of CLIs:

$ sudo config sflow interface disable all m_intfAllConf is false $ redis-cli -n 4 del "SFLOW|global" m_gEnable is false $ redis-cli -n 4 del "SFLOW_SESSION|all"

APPL_DB SFLOW_SESSION_TABLE:EthernetX will show up for non-individually configured port EthernetX under the condition that m_gEnable is false

prsunny · 2020-08-31T21:44:43Z

@dgsudharsan , @padmanarayana, can you please check?

…ng() (sonic-net#1012) Fix typo and enable test for tablesWithOutYang() Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com

GarrickHe mentioned this pull request Jul 31, 2019

sonic-utilities: Add support for sFlow sonic-net/sonic-utilities#592

Merged

stcheng added the Enhancement ➕ label Aug 5, 2019

dgsudharsan force-pushed the sfloworch branch from 8208fac to 3f264cb Compare August 7, 2019 21:08

sonic-net deleted a comment from dgsudharsan Aug 7, 2019

dgsudharsan force-pushed the sfloworch branch 3 times, most recently from d45786b to b8f1a9b Compare August 16, 2019 18:08

Sflow orchagent changes

9974a14

Mgr changes Mgr changes 2 Test cases Changing Global to global optimizations Reverting copp changes clean up UT Fixes Addressing code review comments Updating UT Build fix Adding mgr changes

dgsudharsan force-pushed the sfloworch branch from b8f1a9b to 9974a14 Compare August 21, 2019 17:17

lguohan assigned prsunny Sep 3, 2019

prsunny reviewed Sep 4, 2019

View reviewed changes

Addressing review comments

1714af3

prsunny reviewed Sep 26, 2019

View reviewed changes

dgsudharsan added 3 commits October 3, 2019 22:38

Addressig review comments

5d20b8e

Fixing build

0a784d8

Fixing build

d4a2b52

prsunny reviewed Oct 10, 2019

View reviewed changes

Addressing code review comments

7674af7

dgsudharsan mentioned this pull request Oct 12, 2019

Enable sflowmgrd in docker-sonic-vs sonic-net/sonic-buildimage#3595

Merged

dgsudharsan and others added 2 commits October 11, 2019 22:27

Renaming variables

d31ccd1

Merge branch 'master' into sfloworch

082232d

Change admin_state

b34b48e

* Change admin_state from enable/disable to up/down to match SONiC YANG. Signed-off-by: Garrick He <garrick_he@dell.com>

prsunny approved these changes Oct 22, 2019

View reviewed changes

Merge branch 'master' of https://github.com/Azure/sonic-swss into sfl…

ef110c3

…oworch

prsunny merged commit 20747fa into sonic-net:master Oct 28, 2019

wendani reviewed Aug 12, 2020

View reviewed changes

wendani reviewed Aug 29, 2020

View reviewed changes

dgsudharsan mentioned this pull request Sep 4, 2020

Sflow fixes during DEL processing #1427

Merged

dgsudharsan deleted the sfloworch branch March 9, 2023 02:00


		auto table = consumer.getTableName();

		if(table == CFG_SFLOW_TABLE_NAME)

Sflow orchagent changes #1012

Sflow orchagent changes #1012

Conversation

dgsudharsan commented Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Sep 5, 2019

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgsudharsan commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgsudharsan commented Oct 17, 2019

prsunny commented Oct 22, 2019

prsunny commented Oct 22, 2019

prsunny commented Oct 22, 2019

dgsudharsan commented Oct 23, 2019

prsunny commented Oct 23, 2019

dgsudharsan commented Oct 24, 2019

Choose a reason for hiding this comment

dgsudharsan Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Aug 31, 2020

dgsudharsan commented Jul 31, 2019 •

edited

Loading

dgsudharsan Sep 1, 2020 •

edited

Loading