Fix sFlow sampling-rate and admin-state (sonic-net#1728)

venkatmahalingam · raphaelt-nvidia · commit 8f827863784e · 2021-10-05T06:55:30.000Z
* Fixed incorrect interface admin-status when 'all' interface is disabled but there are local sampling-rate configurations.
Signed-off-by: Venkatesan Mahalingam &lt;venkatesan_mahalinga@dell.com&gt;
diff --git a/cfgmgr/sflowmgr.cpp b/cfgmgr/sflowmgr.cpp
@@ -83,7 +83,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
             if (sflowPortConf == m_sflowPortConfMap.end())
             {
                 new_port = true;
-                port_info.local_conf = false;
+                port_info.local_rate_cfg = false;
+                port_info.local_admin_cfg = false;
                 port_info.speed = SFLOW_ERROR_SPEED_STR;
                 port_info.rate = "";
                 port_info.admin = "";
@@ -107,8 +108,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
 
             if (m_gEnable && m_intfAllConf)
             {
-                // If the Local Conf is already present, dont't override it even though the speed is changed
-                if (new_port || (speed_change && !m_sflowPortConfMap[key].local_conf))
+                // If the Local rate Conf is already present, dont't override it even though the speed is changed
+                if (new_port || (speed_change && !m_sflowPortConfMap[key].local_rate_cfg))
                 {
                     vector<FieldValueTuple> fvs;
                     sflowGetGlobalInfo(fvs, m_sflowPortConfMap[key].speed);
@@ -121,7 +122,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
             auto sflowPortConf = m_sflowPortConfMap.find(key);
             if (sflowPortConf != m_sflowPortConfMap.end())
             {
-                bool local_cfg = m_sflowPortConfMap[key].local_conf;
+                bool local_cfg = m_sflowPortConfMap[key].local_rate_cfg ||
+                                 m_sflowPortConfMap[key].local_admin_cfg;
 
                 m_sflowPortConfMap.erase(key);
                 if ((m_intfAllConf && m_gEnable) || local_cfg)
@@ -138,18 +140,27 @@ void SflowMgr::sflowHandleSessionAll(bool enable)
 {
     for (auto it: m_sflowPortConfMap)
     {
-        if (!it.second.local_conf)
+        if (enable)
         {
             vector<FieldValueTuple> fvs;
-            sflowGetGlobalInfo(fvs, it.second.speed);
-            if (enable)
+            if (it.second.local_rate_cfg || it.second.local_admin_cfg)
             {
-                m_appSflowSessionTable.set(it.first, fvs);
+                sflowGetPortInfo(fvs, it.second);
+                /* Use global admin state if there is not a local one */
+                if (!it.second.local_admin_cfg) {
+                    FieldValueTuple fv1("admin_state", "up");
+                    fvs.push_back(fv1);
+                }
             }
             else
             {
-                m_appSflowSessionTable.del(it.first);
+                sflowGetGlobalInfo(fvs, it.second.speed);
             }
+            m_appSflowSessionTable.set(it.first, fvs);
+        }
+        else if (!it.second.local_admin_cfg)
+        {
+            m_appSflowSessionTable.del(it.first);
         }
     }
 }
@@ -158,7 +169,7 @@ void SflowMgr::sflowHandleSessionLocal(bool enable)
 {
     for (auto it: m_sflowPortConfMap)
     {
-        if (it.second.local_conf)
+        if (it.second.local_admin_cfg || it.second.local_rate_cfg)
         {
             vector<FieldValueTuple> fvs;
             sflowGetPortInfo(fvs, it.second);
@@ -194,7 +205,7 @@ void SflowMgr::sflowGetGlobalInfo(vector<FieldValueTuple> &fvs, string speed)
 
 void SflowMgr::sflowGetPortInfo(vector<FieldValueTuple> &fvs, SflowPortInfo &local_info)
 {
-    if (local_info.admin.length() > 0)
+    if (local_info.local_admin_cfg)
     {
         FieldValueTuple fv1("admin_state", local_info.admin);
         fvs.push_back(fv1);
@@ -217,13 +228,15 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va
         {
             rate_present = true;
             m_sflowPortConfMap[alias].rate = fvValue(i);
+            m_sflowPortConfMap[alias].local_rate_cfg = true;
             FieldValueTuple fv(fvField(i), fvValue(i));
             fvs.push_back(fv);
         }
         if (fvField(i) == "admin_state")
         {
             admin_present = true;
             m_sflowPortConfMap[alias].admin = fvValue(i);
+            m_sflowPortConfMap[alias].local_admin_cfg = true;
             FieldValueTuple fv(fvField(i), fvValue(i));
             fvs.push_back(fv);
         }
@@ -235,7 +248,11 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va
 
     if (!rate_present)
     {
-        if (m_sflowPortConfMap[alias].rate == "")
+        /* Go back to default sample-rate if there is not existing rate OR
+         * if a local config has been done but the rate has been removed
+         */
+        if (m_sflowPortConfMap[alias].rate == "" ||
+            m_sflowPortConfMap[alias].local_rate_cfg)
         {
             string speed = m_sflowPortConfMap[alias].speed;
 
@@ -249,6 +266,7 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va
             }
             m_sflowPortConfMap[alias].rate = rate;
         }
+        m_sflowPortConfMap[alias].local_rate_cfg = false;
         FieldValueTuple fv("sample_rate", m_sflowPortConfMap[alias].rate);
         fvs.push_back(fv);
     }
@@ -257,9 +275,10 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va
     {
         if (m_sflowPortConfMap[alias].admin == "")
         {
-            /* By default admin state is enable if not set explicitly */
+            /* By default admin state is enabled if not set explicitly */
             m_sflowPortConfMap[alias].admin = "up";
         }
+        m_sflowPortConfMap[alias].local_admin_cfg = false;
         FieldValueTuple fv("admin_state", m_sflowPortConfMap[alias].admin);
         fvs.push_back(fv);
     }
@@ -347,7 +366,6 @@ void SflowMgr::doTask(Consumer &consumer)
                     }
                     vector<FieldValueTuple> fvs;
                     sflowCheckAndFillValues(key, values, fvs);
-                    m_sflowPortConfMap[key].local_conf = true;
                     if (m_gEnable)
                     {
                         m_appSflowSessionTable.set(key, fvs);
@@ -384,7 +402,8 @@ void SflowMgr::doTask(Consumer &consumer)
                 else
                 {
                     m_appSflowSessionTable.del(key);
-                    m_sflowPortConfMap[key].local_conf = false;
+                    m_sflowPortConfMap[key].local_rate_cfg = false;
+                    m_sflowPortConfMap[key].local_admin_cfg = false;
                     m_sflowPortConfMap[key].rate = "";
                     m_sflowPortConfMap[key].admin = "";
 
diff --git a/cfgmgr/sflowmgr.h b/cfgmgr/sflowmgr.h
@@ -32,7 +32,8 @@ namespace swss {
 
 struct SflowPortInfo
 {
-    bool        local_conf;
+    bool        local_rate_cfg;
+    bool        local_admin_cfg;
     std::string speed;
     std::string rate;
     std::string admin;
diff --git a/tests/test_sflow.py b/tests/test_sflow.py
@@ -1,4 +1,5 @@
 import time
+from swsscommon import swsscommon
 
 class TestSflow:
     speed_rate_table = {
@@ -175,7 +176,78 @@ def test_SamplingRateManualUpdate(self, dvs, testlog):
         time.sleep(1) 
         appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet4", {"sample_rate": "256"})
     
+    def test_InterfaceDisableAllUpdate(self, dvs, testlog):
+        '''  
+        This test checks if the SflowMgr updates sflow session table in APPL_DB when user has not configured the admin_status.
+        Steps to verify the issue:
+        1) Enable sflow globally
+        2) Configure sample rate for Ethernet0
+        3) Configure sample rate for Ethernet4
+        4) verify whether sample rates are reflected in the Ethernet0 & Ethernet4 interfaces
+        5) Execute sflow disable all command to disable all interfaces
+        6) Verify whether all interfaces are disabled (without the fix, interfaces were shown with admin up with configured rate, 
+           this is not expected).
+        '''
+        self.setup_sflow(dvs)
+
+        appldb = dvs.get_app_db()
+        # create the interface session
+        session_params = {"sample_rate": "256"}
+        self.cdb.create_entry("SFLOW_SESSION", "Ethernet0", session_params)
+        expected_fields = {"sample_rate": "256"}
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet0", expected_fields)
+        
+        session_params = {"sample_rate": "512"}
+        self.cdb.create_entry("SFLOW_SESSION", "Ethernet4", session_params)
+        expected_fields = {"sample_rate": "512"}
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet4", expected_fields)
 
+        session_params = {"admin_state": "down"}
+        self.cdb.create_entry("SFLOW_SESSION", "all", session_params)
+        # Wait for the APPL_DB from sflowmgrd
+        expected_fields = {}
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet0", expected_fields)
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet4", expected_fields)
+
+        self.cdb.delete_entry("SFLOW_SESSION", "all")
+        self.cdb.delete_entry("SFLOW_SESSION", "Ethernet0")
+        self.cdb.delete_entry("SFLOW_SESSION", "Ethernet4")
+
+    def test_InterfaceDefaultSampleRate(self, dvs, testlog):
+        '''
+        This test checks if the SflowMgr updates sflow session table in APPL_DB with default rate.
+        Steps to verify the issue:
+        1) Enable sflow globally
+        2) Configure sample rate for Ethernet0
+        3) Verify whether sample rate is reflected in the Ethernet0 interfaces
+        4) Remove sample rate for Ethernet0
+        5) Verify whether sample rate of Ethernet0 interface moved to default sample rate
+        '''
+        self.setup_sflow(dvs)
+    
+        port_oid = self.adb.port_name_map["Ethernet0"]
+        expected_fields = {"SAI_PORT_ATTR_INGRESS_SAMPLEPACKET_ENABLE": "oid:0x0"}
+        fvs = self.adb.wait_for_field_negative_match("ASIC_STATE:SAI_OBJECT_TYPE_PORT", port_oid, expected_fields)
+        speed = fvs["SAI_PORT_ATTR_SPEED"]
+        rate = self.speed_rate_table.get(speed, None)
+        assert rate
+    
+        appldb = dvs.get_app_db()
+        # create the interface session
+        session_params = {"sample_rate": "256"}
+        self.cdb.create_entry("SFLOW_SESSION", "Ethernet0", session_params)
+        expected_fields = {"admin_state": "up", "sample_rate": "256"}
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet0", expected_fields)
+   
+        cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0)
+        tbl = swsscommon.Table(cdb, "SFLOW_SESSION")
+        tbl.hdel("Ethernet0","sample_rate")
+        
+        expected_fields = {"admin_state": "up", "sample_rate": rate}
+        appldb.wait_for_field_match("SFLOW_SESSION_TABLE", "Ethernet0", expected_fields)
+    
+        self.cdb.delete_entry("SFLOW_SESSION", "Ethernet0")
+    
     def test_Teardown(self, dvs, testlog):
         self.setup_sflow(dvs)
 

Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)`
`83`	`83`	`if (sflowPortConf == m_sflowPortConfMap.end())`
`84`	`84`	`{`
`85`	`85`	`new_port = true;`
`86`		`- port_info.local_conf = false;`
	`86`	`+ port_info.local_rate_cfg = false;`
	`87`	`+ port_info.local_admin_cfg = false;`
`87`	`88`	`port_info.speed = SFLOW_ERROR_SPEED_STR;`
`88`	`89`	`port_info.rate = "";`
`89`	`90`	`port_info.admin = "";`
`@@ -107,8 +108,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)`
`107`	`108`
`108`	`109`	`if (m_gEnable && m_intfAllConf)`
`109`	`110`	`{`
`110`		`- // If the Local Conf is already present, dont't override it even though the speed is changed`
`111`		`- if (new_port \|\| (speed_change && !m_sflowPortConfMap[key].local_conf))`
	`111`	`+ // If the Local rate Conf is already present, dont't override it even though the speed is changed`
	`112`	`+ if (new_port \|\| (speed_change && !m_sflowPortConfMap[key].local_rate_cfg))`
`112`	`113`	`{`
`113`	`114`	`vector<FieldValueTuple> fvs;`
`114`	`115`	`sflowGetGlobalInfo(fvs, m_sflowPortConfMap[key].speed);`
`@@ -121,7 +122,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)`
`121`	`122`	`auto sflowPortConf = m_sflowPortConfMap.find(key);`
`122`	`123`	`if (sflowPortConf != m_sflowPortConfMap.end())`
`123`	`124`	`{`
`124`		`- bool local_cfg = m_sflowPortConfMap[key].local_conf;`
	`125`	`+ bool local_cfg = m_sflowPortConfMap[key].local_rate_cfg \|\|`
	`126`	`+ m_sflowPortConfMap[key].local_admin_cfg;`
`125`	`127`
`126`	`128`	`m_sflowPortConfMap.erase(key);`
`127`	`129`	`if ((m_intfAllConf && m_gEnable) \|\| local_cfg)`
`@@ -138,18 +140,27 @@ void SflowMgr::sflowHandleSessionAll(bool enable)`
`138`	`140`	`{`
`139`	`141`	`for (auto it: m_sflowPortConfMap)`
`140`	`142`	`{`
`141`		`- if (!it.second.local_conf)`
	`143`	`+ if (enable)`
`142`	`144`	`{`
`143`	`145`	`vector<FieldValueTuple> fvs;`
`144`		`- sflowGetGlobalInfo(fvs, it.second.speed);`
`145`		`- if (enable)`
	`146`	`+ if (it.second.local_rate_cfg \|\| it.second.local_admin_cfg)`
`146`	`147`	`{`
`147`		`- m_appSflowSessionTable.set(it.first, fvs);`
	`148`	`+ sflowGetPortInfo(fvs, it.second);`
	`149`	`+ /* Use global admin state if there is not a local one */`
	`150`	`+ if (!it.second.local_admin_cfg) {`
	`151`	`+ FieldValueTuple fv1("admin_state", "up");`
	`152`	`+ fvs.push_back(fv1);`
	`153`	`+ }`
`148`	`154`	`}`
`149`	`155`	`else`
`150`	`156`	`{`
`151`		`- m_appSflowSessionTable.del(it.first);`
	`157`	`+ sflowGetGlobalInfo(fvs, it.second.speed);`
`152`	`158`	`}`
	`159`	`+ m_appSflowSessionTable.set(it.first, fvs);`
	`160`	`+ }`
	`161`	`+ else if (!it.second.local_admin_cfg)`
	`162`	`+ {`
	`163`	`+ m_appSflowSessionTable.del(it.first);`
`153`	`164`	`}`
`154`	`165`	`}`
`155`	`166`	`}`
`@@ -158,7 +169,7 @@ void SflowMgr::sflowHandleSessionLocal(bool enable)`
`158`	`169`	`{`
`159`	`170`	`for (auto it: m_sflowPortConfMap)`
`160`	`171`	`{`
`161`		`- if (it.second.local_conf)`
	`172`	`+ if (it.second.local_admin_cfg \|\| it.second.local_rate_cfg)`
`162`	`173`	`{`
`163`	`174`	`vector<FieldValueTuple> fvs;`
`164`	`175`	`sflowGetPortInfo(fvs, it.second);`
`@@ -194,7 +205,7 @@ void SflowMgr::sflowGetGlobalInfo(vector<FieldValueTuple> &fvs, string speed)`
`194`	`205`
`195`	`206`	`void SflowMgr::sflowGetPortInfo(vector<FieldValueTuple> &fvs, SflowPortInfo &local_info)`
`196`	`207`	`{`
`197`		`- if (local_info.admin.length() > 0)`
	`208`	`+ if (local_info.local_admin_cfg)`
`198`	`209`	`{`
`199`	`210`	`FieldValueTuple fv1("admin_state", local_info.admin);`
`200`	`211`	`fvs.push_back(fv1);`
`@@ -217,13 +228,15 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va`
`217`	`228`	`{`
`218`	`229`	`rate_present = true;`
`219`	`230`	`m_sflowPortConfMap[alias].rate = fvValue(i);`
	`231`	`+ m_sflowPortConfMap[alias].local_rate_cfg = true;`
`220`	`232`	`FieldValueTuple fv(fvField(i), fvValue(i));`
`221`	`233`	`fvs.push_back(fv);`
`222`	`234`	`}`
`223`	`235`	`if (fvField(i) == "admin_state")`
`224`	`236`	`{`
`225`	`237`	`admin_present = true;`
`226`	`238`	`m_sflowPortConfMap[alias].admin = fvValue(i);`
	`239`	`+ m_sflowPortConfMap[alias].local_admin_cfg = true;`
`227`	`240`	`FieldValueTuple fv(fvField(i), fvValue(i));`
`228`	`241`	`fvs.push_back(fv);`
`229`	`242`	`}`
`@@ -235,7 +248,11 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va`
`235`	`248`
`236`	`249`	`if (!rate_present)`
`237`	`250`	`{`
`238`		`- if (m_sflowPortConfMap[alias].rate == "")`
	`251`	`+ /* Go back to default sample-rate if there is not existing rate OR`
	`252`	`+ * if a local config has been done but the rate has been removed`
	`253`	`+ */`
	`254`	`+ if (m_sflowPortConfMap[alias].rate == "" \|\|`
	`255`	`+ m_sflowPortConfMap[alias].local_rate_cfg)`
`239`	`256`	`{`
`240`	`257`	`string speed = m_sflowPortConfMap[alias].speed;`
`241`	`258`
`@@ -249,6 +266,7 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va`
`249`	`266`	`}`
`250`	`267`	`m_sflowPortConfMap[alias].rate = rate;`
`251`	`268`	`}`
	`269`	`+ m_sflowPortConfMap[alias].local_rate_cfg = false;`
`252`	`270`	`FieldValueTuple fv("sample_rate", m_sflowPortConfMap[alias].rate);`
`253`	`271`	`fvs.push_back(fv);`
`254`	`272`	`}`
`@@ -257,9 +275,10 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &va`
`257`	`275`	`{`
`258`	`276`	`if (m_sflowPortConfMap[alias].admin == "")`
`259`	`277`	`{`
`260`		`- /* By default admin state is enable if not set explicitly */`
	`278`	`+ /* By default admin state is enabled if not set explicitly */`
`261`	`279`	`m_sflowPortConfMap[alias].admin = "up";`
`262`	`280`	`}`
	`281`	`+ m_sflowPortConfMap[alias].local_admin_cfg = false;`
`263`	`282`	`FieldValueTuple fv("admin_state", m_sflowPortConfMap[alias].admin);`
`264`	`283`	`fvs.push_back(fv);`
`265`	`284`	`}`
`@@ -347,7 +366,6 @@ void SflowMgr::doTask(Consumer &consumer)`
`347`	`366`	`}`
`348`	`367`	`vector<FieldValueTuple> fvs;`
`349`	`368`	`sflowCheckAndFillValues(key, values, fvs);`
`350`		`- m_sflowPortConfMap[key].local_conf = true;`
`351`	`369`	`if (m_gEnable)`
`352`	`370`	`{`
`353`	`371`	`m_appSflowSessionTable.set(key, fvs);`
`@@ -384,7 +402,8 @@ void SflowMgr::doTask(Consumer &consumer)`
`384`	`402`	`else`
`385`	`403`	`{`
`386`	`404`	`m_appSflowSessionTable.del(key);`
`387`		`- m_sflowPortConfMap[key].local_conf = false;`
	`405`	`+ m_sflowPortConfMap[key].local_rate_cfg = false;`
	`406`	`+ m_sflowPortConfMap[key].local_admin_cfg = false;`
`388`	`407`	`m_sflowPortConfMap[key].rate = "";`
`389`	`408`	`m_sflowPortConfMap[key].admin = "";`
`390`	`409`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,8 @@ namespace swss {`
`32`	`32`
`33`	`33`	`struct SflowPortInfo`
`34`	`34`	`{`
`35`		`- bool local_conf;`
	`35`	`+ bool local_rate_cfg;`
	`36`	`+ bool local_admin_cfg;`
`36`	`37`	`std::string speed;`
`37`	`38`	`std::string rate;`
`38`	`39`	`std::string admin;`