diff --git a/cfgmgr/sflowmgr.cpp b/cfgmgr/sflowmgr.cpp index 0e328122677..bb732e83d53 100644 --- 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 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 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 fvs; sflowGetPortInfo(fvs, it.second); @@ -194,7 +205,7 @@ void SflowMgr::sflowGetGlobalInfo(vector &fvs, string speed) void SflowMgr::sflowGetPortInfo(vector &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,6 +228,7 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector &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); } @@ -224,6 +236,7 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector &va { 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 &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 &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 &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 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 index f83d9a103ac..eb35ec21259 100644 --- 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 index f70880fce66..50d68979fde 100644 --- 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)