Fix sFlow sampling-rate and admin-state

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 = "";
 

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;

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)