From 53c630b82d429db892bf288dd9f323c5a8370cd5 Mon Sep 17 00:00:00 2001 From: noaOrMlnx <58519608+noaOrMlnx@users.noreply.github.com> Date: Wed, 26 Jan 2022 19:44:05 +0200 Subject: [PATCH] [CoPP] Add always_enabled field to coppmgr logic (#2034) - What I did Add the new design for copp manager - always_enabled field in copp_cfg.json file. - Why I did it A change was done for traps needs to be installed but doesn't have feature (arp, udld, lacp, ip2me), in the new implementation, coppmgr will not automatically install traps which has no entry in features table, but will check first if the trap has "always_enabled":"true" field. - How I verified it run tests for making sure traps are installed and not when expecting them to. - Details if related Related to sonic-buildimage change - Azure/sonic-buildimage#9302 SONiC mgmt test plan can be found in Azure/SONiC#903. --- cfgmgr/coppmgr.cpp | 308 ++++++++++++++++++++++++++++++++++----------- cfgmgr/coppmgr.h | 11 +- tests/test_copp.py | 183 +++++++++++++++++++++------ 3 files changed, 387 insertions(+), 115 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 834b2c5ff0b2..1721cc8593f8 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -78,31 +78,42 @@ bool CoppMgr::checkTrapGroupPending(string trap_group_name) /* Feature name and CoPP Trap table name must match */ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) { - bool disabled_trap = (m_coppDisabledTraps.find(feature) != m_coppDisabledTraps.end()); - - if ((enable && !disabled_trap) || (!enable && disabled_trap)) + bool disabled_trap {true}; + string always_enabled; + if (m_coppTrapConfMap.find(feature) != m_coppTrapConfMap.end()) { - return; + always_enabled = m_coppTrapConfMap[feature].is_always_enabled; + } + if (always_enabled == "true" || isFeatureEnabled(feature)) + { + disabled_trap = false; } - if (m_coppTrapConfMap.find(feature) == m_coppTrapConfMap.end()) + if ((enable && !disabled_trap) || (!enable && disabled_trap)) { - if (!enable) - { - m_coppDisabledTraps.insert(feature); - } return; } + string trap_group = m_coppTrapConfMap[feature].trap_group; bool prev_group_state = checkTrapGroupPending(trap_group); - if (!enable) + // update features cache + auto state = "disabled"; + if (enable) { - m_coppDisabledTraps.insert(feature); + state = "enabled"; } - else + if (m_featuresCfgTable.find(feature) != m_featuresCfgTable.end()) { - m_coppDisabledTraps.erase(feature); + auto vect = m_featuresCfgTable[feature]; + for (long unsigned int i=0; i < vect.size(); i++) + { + if (vect[i].first == "state") + { + vect[i].second = state; + } + } + m_featuresCfgTable.at(feature) = vect; } /* Trap group moved to pending state when feature is disabled. Remove trap group @@ -140,25 +151,46 @@ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) } } -bool CoppMgr::isTrapIdDisabled(string trap_id) +bool CoppMgr::isFeatureEnabled(std::string feature) { - for (auto &m: m_coppDisabledTraps) + if (m_featuresCfgTable.find(feature) != m_featuresCfgTable.end()) { - if (m_coppTrapConfMap.find(m) == m_coppTrapConfMap.end()) + std::vector feature_fvs = m_featuresCfgTable[feature]; + for (auto i: feature_fvs) { - continue; + if (fvField(i) == "state" && (fvValue(i) == "enabled" || fvValue(i) == "always_enabled")) + { + return true; + } } - vector trap_id_list; + } + return false; +} - trap_id_list = tokenize(m_coppTrapConfMap[m].trap_ids, list_item_delimiter); - if(std::find(trap_id_list.begin(), trap_id_list.end(), trap_id) != trap_id_list.end()) +bool CoppMgr::isTrapIdDisabled(string trap_id) +{ + // check if trap is always_enabled + string trap_name; + for (auto &t: m_coppTrapConfMap) + { + if (m_coppTrapConfMap[t.first].trap_ids.find(trap_id) != string::npos) { - return true; + trap_name = t.first; + if (m_coppTrapConfMap[t.first].is_always_enabled == "true") + { + return false; + } + break; } + } + if (isFeatureEnabled(trap_name)) + { + return false; } - return false; + return true; } + void CoppMgr::mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &cfg_keys, Table &cfgTable) { /* Read the init configuration first. If the same key is present in @@ -254,14 +286,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { std::vector feature_fvs; m_cfgFeatureTable.get(i, feature_fvs); - - for (auto j: feature_fvs) - { - if (fvField(j) == "state" && fvValue(j) == "disabled") - { - m_coppDisabledTraps.insert(i); - } - } + m_featuresCfgTable.emplace(i, feature_fvs); } mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable); @@ -270,6 +295,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { string trap_group; string trap_ids; + string is_always_enabled = "false"; std::vector trap_fvs = i.second; for (auto j: trap_fvs) @@ -282,13 +308,22 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { trap_group = fvValue(j); } + else if (fvField(j) == COPP_ALWAYS_ENABLED_FIELD) + { + is_always_enabled = fvValue(j); + } } + if (!trap_group.empty() && !trap_ids.empty()) { addTrapIdsToTrapGroup(trap_group, trap_ids); m_coppTrapConfMap[i.first].trap_group = trap_group; m_coppTrapConfMap[i.first].trap_ids = trap_ids; - setCoppTrapStateOk(i.first); + m_coppTrapConfMap[i.first].is_always_enabled = is_always_enabled; + if (is_always_enabled == "true" || isFeatureEnabled(i.first)) + { + setCoppTrapStateOk(i.first); + } } } @@ -384,7 +419,6 @@ void CoppMgr::removeTrapIdsFromTrapGroup(string trap_group, string trap_ids) void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) { - trap_ids.clear(); for (auto it: m_coppTrapIdTrapGroupMap) { @@ -406,6 +440,36 @@ void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) } } +void CoppMgr::removeTrap(string key) +{ + string trap_ids; + std::vector fvs; + removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); + getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) + { + m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); + } +} + +void CoppMgr::addTrap(string trap_ids, string trap_group) +{ + string trap_group_trap_ids; + std::vector fvs; + addTrapIdsToTrapGroup(trap_group, trap_ids); + getTrapGroupTrapIds(trap_group, trap_group_trap_ids); + FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); + fvs.push_back(fv1); + if (!checkTrapGroupPending(trap_group)) + { + m_appCoppTable.set(trap_group, fvs); + setCoppGroupStateOk(trap_group); + } +} + void CoppMgr::doCoppTrapTask(Consumer &consumer) { auto it = consumer.m_toSync.begin(); @@ -418,12 +482,21 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) vector fvs; string trap_ids = ""; string trap_group = ""; + string is_always_enabled = ""; bool conf_present = false; if (m_coppTrapConfMap.find(key) != m_coppTrapConfMap.end()) { trap_ids = m_coppTrapConfMap[key].trap_ids; trap_group = m_coppTrapConfMap[key].trap_group; + if (m_coppTrapConfMap[key].is_always_enabled.empty()) + { + is_always_enabled = "false"; + } + else + { + is_always_enabled = m_coppTrapConfMap[key].is_always_enabled; + } conf_present = true; } @@ -441,6 +514,10 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { trap_ids = fvValue(i); } + else if (fvField(i) == COPP_ALWAYS_ENABLED_FIELD) + { + is_always_enabled = fvValue(i); + } else if (fvField(i) == "NULL") { null_cfg = true; @@ -450,20 +527,9 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { if (conf_present) { - SWSS_LOG_DEBUG("Deleting trap key %s", key.c_str()); - removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, - m_coppTrapConfMap[key].trap_ids); - trap_ids.clear(); + removeTrap(key); setCoppTrapStateOk(key); - getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); - fvs.clear(); - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); - fvs.push_back(fv); - if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) - { - m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); - setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); - } + m_coppTrapConfMap.erase(key); } it = consumer.m_toSync.erase(it); @@ -472,38 +538,126 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) /*Duplicate check*/ if (conf_present && (trap_group == m_coppTrapConfMap[key].trap_group) && - (trap_ids == m_coppTrapConfMap[key].trap_ids)) + (trap_ids == m_coppTrapConfMap[key].trap_ids) && + (is_always_enabled == m_coppTrapConfMap[key].is_always_enabled)) { it = consumer.m_toSync.erase(it); continue; } + /* Incomplete configuration. Do not process until both trap group * and trap_ids are available */ if (trap_group.empty() || trap_ids.empty()) { + if (is_always_enabled.empty()) + { + it = consumer.m_toSync.erase(it); + continue; + } + + if (is_always_enabled != m_coppTrapConfMap[key].is_always_enabled) + { + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + if (is_always_enabled == "true") + { + if (m_coppTrapConfMap.find(key) != m_coppTrapConfMap.end()) + { + addTrap(m_coppTrapConfMap[key].trap_ids, m_coppTrapConfMap[key].trap_group); + } + // else if it has info in the init cfg map + else if (m_coppTrapInitCfg.find(key) != m_coppTrapInitCfg.end()) + { + auto fvs = m_coppTrapInitCfg[key]; + string init_trap_ids = ""; + string init_trap_group = ""; + for (auto i: fvs) + { + if (fvField(i) == COPP_TRAP_GROUP_FIELD) + { + init_trap_group = fvValue(i); + } + else if (fvField(i) == COPP_TRAP_ID_LIST_FIELD) + { + init_trap_ids = fvValue(i); + } + } + addTrap(init_trap_ids, init_trap_group); + } + } + else + { + /* if the value was changed from true to false, + check if there is a feature enabled. + if no, remove the trap. is yes, do nothing. */ + + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + if (isFeatureEnabled(key)) + { + it = consumer.m_toSync.erase(it); + continue; + } + + removeTrap(key); + delCoppTrapStateOk(key); + } + it = consumer.m_toSync.erase(it); + continue; + } + } + /* if always_enabled field has been changed */ + if (conf_present && + (trap_group == m_coppTrapConfMap[key].trap_group) && + (trap_ids == m_coppTrapConfMap[key].trap_ids)) + { + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + if (is_always_enabled == "true") + { + /* if the value was changed from false to true, + if the trap is not installed, install it. + otherwise, do nothing. */ + + // if the feature was not enabled, install the trap + if (!isFeatureEnabled(key)) + { + addTrap(trap_ids, trap_group); + } + + it = consumer.m_toSync.erase(it); + continue; + } + else + { + /* if the value was changed from true to false, + check if there is a feature enabled. + if no, remove the trap. is yes, do nothing. */ + + if (isFeatureEnabled(key)) + { + it = consumer.m_toSync.erase(it); + continue; + } + + removeTrap(key); + delCoppTrapStateOk(key); + } it = consumer.m_toSync.erase(it); continue; } + /* Remove the current trap IDs and add the new trap IDS to recompute the - * trap IDs for the trap group + * trap IDs for the trap group */ if (conf_present) { removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); } - fvs.clear(); - string trap_group_trap_ids; - addTrapIdsToTrapGroup(trap_group, trap_ids); - getTrapGroupTrapIds(trap_group, trap_group_trap_ids); - FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); - fvs.push_back(fv1); - if (!checkTrapGroupPending(trap_group)) - { - m_appCoppTable.set(trap_group, fvs); - setCoppGroupStateOk(trap_group); - } + + m_coppTrapConfMap[key].trap_group = trap_group; + m_coppTrapConfMap[key].trap_ids = trap_ids; + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + addTrap(trap_ids, trap_group); /* When the trap table's trap group is changed, the old trap group * should also be reprogrammed as some of its associated traps got @@ -511,7 +665,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) */ if (conf_present && (trap_group != m_coppTrapConfMap[key].trap_group)) { - trap_group_trap_ids.clear(); + string trap_group_trap_ids; fvs.clear(); getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_group_trap_ids); FieldValueTuple fv2(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); @@ -524,6 +678,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } m_coppTrapConfMap[key].trap_group = trap_group; m_coppTrapConfMap[key].trap_ids = trap_ids; + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; setCoppTrapStateOk(key); } else if (op == DEL_COMMAND) @@ -546,8 +701,9 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } } - if (conf_present) + if (conf_present && !m_coppTrapConfMap[key].trap_group.empty() && !m_coppTrapConfMap[key].trap_ids.empty()) { + m_coppTrapConfMap.erase(key); } delCoppTrapStateOk(key); @@ -559,6 +715,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (m_coppTrapInitCfg.find(key) != m_coppTrapInitCfg.end()) { auto fvs = m_coppTrapInitCfg[key]; + is_always_enabled.clear(); for (auto i: fvs) { if (fvField(i) == COPP_TRAP_GROUP_FIELD) @@ -569,21 +726,24 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { trap_ids = fvValue(i); } + else if (fvField(i) == COPP_ALWAYS_ENABLED_FIELD) + { + is_always_enabled = fvValue(i); + } } - vector g_fvs; - string trap_group_trap_ids; - addTrapIdsToTrapGroup(trap_group, trap_ids); - getTrapGroupTrapIds(trap_group, trap_group_trap_ids); - FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); - g_fvs.push_back(fv1); - if (!checkTrapGroupPending(trap_group)) + if (is_always_enabled.empty()) { - m_appCoppTable.set(trap_group, g_fvs); - setCoppGroupStateOk(trap_group); + is_always_enabled = "false"; } + m_coppTrapConfMap[key].trap_group = trap_group; m_coppTrapConfMap[key].trap_ids = trap_ids; - setCoppTrapStateOk(key); + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + if (is_always_enabled == "true" || isFeatureEnabled(key)) + { + addTrap(trap_ids, trap_group); + setCoppTrapStateOk(key); + } } } it = consumer.m_toSync.erase(it); @@ -706,6 +866,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) } } + void CoppMgr::doFeatureTask(Consumer &consumer) { auto it = consumer.m_toSync.begin(); @@ -715,17 +876,20 @@ void CoppMgr::doFeatureTask(Consumer &consumer) string key = kfvKey(t); string op = kfvOp(t); - vector fvs; string trap_ids; if (op == SET_COMMAND) { + if (m_featuresCfgTable.find(key) == m_featuresCfgTable.end()) + { + m_featuresCfgTable.emplace(key, kfvFieldsValues(t)); + } for (auto i : kfvFieldsValues(t)) { if (fvField(i) == "state") { bool status = false; - if (fvValue(i) == "enabled") + if (fvValue(i) == "enabled" || fvValue(i) == "always_enabled") { status = true; } diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index b010489f2eee..1d53756fceba 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -14,6 +14,7 @@ namespace swss { /* COPP Trap Table Fields */ #define COPP_TRAP_ID_LIST_FIELD "trap_ids" #define COPP_TRAP_GROUP_FIELD "trap_group" +#define COPP_ALWAYS_ENABLED_FIELD "always_enabled" /* COPP Group Table Fields */ #define COPP_GROUP_QUEUE_FIELD "queue" @@ -42,6 +43,7 @@ struct CoppTrapConf { std::string trap_ids; std::string trap_group; + std::string is_always_enabled; }; /* TrapName to TrapConf map */ @@ -70,10 +72,10 @@ class CoppMgr : public Orch CoppTrapConfMap m_coppTrapConfMap; CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; CoppGroupFvs m_coppGroupFvs; - std::set m_coppDisabledTraps; CoppCfg m_coppGroupInitCfg; CoppCfg m_coppTrapInitCfg; - + CoppCfg m_featuresCfgTable; + void doTask(Consumer &consumer); void doCoppGroupTask(Consumer &consumer); @@ -96,8 +98,13 @@ class CoppMgr : public Orch std::vector &modified_fvs); void parseInitFile(void); bool isTrapGroupInstalled(std::string key); + bool isFeatureEnabled(std::string feature); void mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &cfg_keys, Table &cfgTable); + void removeTrap(std::string key); + void addTrap(std::string trap_ids, std::string trap_group); + + }; } diff --git a/tests/test_copp.py b/tests/test_copp.py index 19faac954f79..5885a489b52e 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -151,17 +151,18 @@ "trap_action": "trap", "trap_priority": "5" } + copp_trap = { - "bgp,bgpv6": copp_group_queue4_group1, - "lacp": copp_group_queue4_group1, - "arp_req,arp_resp,neigh_discovery":copp_group_queue4_group2, - "lldp":copp_group_queue4_group3, - "dhcp,dhcpv6":copp_group_queue4_group3, - "udld":copp_group_queue4_group3, - "ip2me":copp_group_queue1_group1, - "src_nat_miss,dest_nat_miss": copp_group_queue1_group2, - "sample_packet": copp_group_queue2_group1, - "ttl_error": copp_group_default + "bgp": ["bgp;bgpv6", copp_group_queue4_group1], + "lacp": ["lacp", copp_group_queue4_group1, "always_enabled"], + "arp": ["arp_req;arp_resp;neigh_discovery", copp_group_queue4_group2, "always_enabled"], + "lldp": ["lldp", copp_group_queue4_group3], + "dhcp": ["dhcp;dhcpv6", copp_group_queue4_group3], + "udld": ["udld", copp_group_queue4_group3, "always_enabled"], + "ip2me": ["ip2me", copp_group_queue1_group1, "always_enabled"], + "nat": ["src_nat_miss;dest_nat_miss", copp_group_queue1_group2], + "sflow": ["sample_packet", copp_group_queue2_group1], + "ttl": ["ttl_error", copp_group_default] } disabled_traps = ["sample_packet"] @@ -201,7 +202,7 @@ def setup_copp(self, dvs): self.trap_ctbl = swsscommon.Table(self.cdb, "COPP_TRAP") self.trap_group_ctbl = swsscommon.Table(self.cdb, "COPP_GROUP") self.feature_tbl = swsscommon.Table(self.cdb, "FEATURE") - fvs = swsscommon.FieldValuePairs([("state", "disbled")]) + fvs = swsscommon.FieldValuePairs([("state", "disabled")]) self.feature_tbl.set("sflow", fvs) time.sleep(2) @@ -306,8 +307,12 @@ def test_defaults(self, dvs, testlog): self.setup_copp(dvs) trap_keys = self.trap_atbl.getKeys() for traps in copp_trap: - trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_info = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] + always_enabled = False + if len(trap_info) > 2: + always_enabled = True for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -325,6 +330,7 @@ def test_defaults(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + def test_restricted_trap_sflow(self, dvs, testlog): self.setup_copp(dvs) fvs = swsscommon.FieldValuePairs([("state", "enabled")]) @@ -334,10 +340,14 @@ def test_restricted_trap_sflow(self, dvs, testlog): trap_keys = self.trap_atbl.getKeys() for traps in copp_trap: - trap_ids = traps.split(",") + trap_info = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] + always_enabled = False + if len(trap_info) > 2: + always_enabled = True if "sample_packet" not in trap_ids: continue - trap_group = copp_trap[traps] trap_found = False trap_type = traps_to_trap_type["sample_packet"] for key in trap_keys: @@ -363,10 +373,14 @@ def test_policer_set(self, dvs, testlog): trap_keys = self.trap_atbl.getKeys() for traps in copp_trap: - if copp_trap[traps] != copp_group_queue4_group2: + trap_info = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] + always_enabled = False + if len(trap_info) > 2: + always_enabled = True + if trap_group != copp_group_queue4_group2: continue - trap_ids = traps.split(",") - trap_group = copp_trap[traps] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -390,12 +404,19 @@ def test_trap_group_set(self, dvs, testlog): traps = "bgp,bgpv6" fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group1")]) self.trap_ctbl.set("bgp", fvs) - copp_trap[traps] = copp_group_queue1_group1 + + for c_trap in copp_trap: + trap_info = copp_trap[c_trap] + ids = trap_info[0].replace(';', ',') + if traps == ids: + break + + trap_info[1] = copp_group_queue1_group1 time.sleep(2) trap_keys = self.trap_atbl.getKeys() trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -423,8 +444,14 @@ def test_trap_ids_set(self, dvs, testlog): old_traps = "bgp,bgpv6" trap_keys = self.trap_atbl.getKeys() + for c_trap in copp_trap: + trap_info = copp_trap[c_trap] + ids = trap_info[0].replace(';', ',') + if old_traps == ids: + break + trap_ids = old_traps.split(",") - trap_group = copp_trap[old_traps] + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -451,7 +478,7 @@ def test_trap_ids_set(self, dvs, testlog): trap_keys = self.trap_atbl.getKeys() trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -478,10 +505,11 @@ def test_trap_action_set(self, dvs, testlog): trap_keys = self.trap_atbl.getKeys() for traps in copp_trap: - if copp_trap[traps] != copp_group_queue4_group1: + trap_info = copp_trap[traps] + if trap_info[1] != copp_group_queue4_group1: continue - trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -499,18 +527,21 @@ def test_trap_action_set(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + def test_new_trap_add(self, dvs, testlog): self.setup_copp(dvs) global copp_trap traps = "eapol,isis,bfd_micro,bfdv6_micro,ldp" - fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group2"),("trap_ids", traps)]) + fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group2"),("trap_ids", traps),("always_enabled", "true")]) self.trap_ctbl.set(traps, fvs) - copp_trap[traps] = copp_group_queue1_group2 + + + copp_trap["eapol"] = [traps, copp_group_queue1_group2, "always_enabled"] time.sleep(2) trap_keys = self.trap_atbl.getKeys() trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = copp_group_queue1_group2 for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -534,13 +565,19 @@ def test_new_trap_del(self, dvs, testlog): traps = "eapol,isis,bfd_micro,bfdv6_micro,ldp" fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group2"),("trap_ids", traps)]) self.trap_ctbl.set(traps, fvs) - copp_trap[traps] = copp_group_queue1_group2 + for c_trap in copp_trap: + trap_info = copp_trap[c_trap] + ids = trap_info[0].replace(';', ',') + if traps == ids: + break + + trap_info[1] = copp_group_queue1_group2 time.sleep(2) self.trap_ctbl._del(traps) time.sleep(2) trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = trap_info[1] trap_keys = self.trap_atbl.getKeys() for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] @@ -568,14 +605,19 @@ def test_new_trap_group_add(self, dvs, testlog): fvs = swsscommon.FieldValuePairs(list_val) self.trap_group_ctbl.set("queue5_group1", fvs) traps = "igmp_v1_report" - t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report")]) + t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report"),("always_enabled", "true")]) self.trap_ctbl.set(traps, t_fvs) - copp_trap[traps] = copp_group_queue5_group1 + for c_trap in copp_trap: + trap_info = copp_trap[c_trap] + ids = trap_info[0].replace(';', ',') + if traps == ids: + break + trap_info[1] = copp_group_queue5_group1 time.sleep(2) trap_keys = self.trap_atbl.getKeys() trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -602,16 +644,21 @@ def test_new_trap_group_del(self, dvs, testlog): fvs = swsscommon.FieldValuePairs(list_val) self.trap_group_ctbl.set("queue5_group1", fvs) traps = "igmp_v1_report" - t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report")]) + t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report"),("always_enabled", "true")]) self.trap_ctbl.set(traps, t_fvs) - copp_trap[traps] = copp_group_queue5_group1 + for c_trap in copp_trap: + trap_info = copp_trap[c_trap] + ids = trap_info[0].replace(';', ',') + if traps == ids: + break + trap_info[1] = copp_group_queue5_group1 self.trap_group_ctbl._del("queue5_group1") time.sleep(2) trap_keys = self.trap_atbl.getKeys() trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -643,10 +690,11 @@ def test_override_trap_grp_cfg_del (self, dvs, testlog): trap_keys = self.trap_atbl.getKeys() for traps in copp_trap: - if copp_trap[traps] != copp_group_queue1_group1: + trap_info = copp_trap[traps] + if trap_info[1] != copp_group_queue1_group1: continue - trap_ids = traps.split(",") - trap_group = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -675,7 +723,7 @@ def test_override_trap_cfg_del(self, dvs, testlog): self.trap_ctbl._del("ip2me") time.sleep(2) trap_ids = traps.split(",") - trap_group = copp_trap["ip2me"] + trap_group = copp_trap["ip2me"][1] trap_keys = self.trap_atbl.getKeys() for trap_id in trap_ids: trap_type = traps_to_trap_type[trap_id] @@ -705,7 +753,7 @@ def test_empty_trap_cfg(self, dvs, testlog): time.sleep(2) trap_id = "ip2me" - trap_group = copp_trap["ip2me"] + trap_group = copp_trap["ip2me"][1] trap_keys = self.trap_atbl.getKeys() trap_type = traps_to_trap_type[trap_id] trap_found = False @@ -740,3 +788,56 @@ def test_empty_trap_cfg(self, dvs, testlog): self.validate_trap_group(key,trap_group) break assert trap_found == True + + + def test_disabled_feature_always_enabled_trap(self, dvs, testlog): + self.setup_copp(dvs) + fvs = swsscommon.FieldValuePairs([("trap_ids", "lldp"), ("trap_group", "queue4_group3"), ("always_enabled", "true")]) + self.trap_ctbl.set("lldp", fvs) + fvs = swsscommon.FieldValuePairs([("state", "disabled")]) + self.feature_tbl.set("lldp", fvs) + + time.sleep(2) + global copp_trap + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + trap_info = copp_trap[traps] + trap_ids = trap_info[0].split(";") + trap_group = trap_info[1] + + if "lldp" not in trap_ids: + continue + + trap_found = False + trap_type = traps_to_trap_type["lldp"] + for key in trap_keys: + (status, fvs) = self.trap_atbl.get(key) + assert status == True + for fv in fvs: + if fv[0] == "SAI_HOSTIF_TRAP_ATTR_TRAP_TYPE": + if fv[1] == trap_type: + trap_found = True + if trap_found: + self.validate_trap_group(key,trap_group) + break + assert trap_found == True + + # change always_enabled to be false and check the trap is not installed: + fvs = swsscommon.FieldValuePairs([("trap_ids", "lldp"), ("trap_group", "queue4_group3"), ("always_enabled", "false")]) + self.trap_ctbl.set("lldp", fvs) + time.sleep(2) + + table_found = True + for key in trap_keys: + (status, fvs) = self.trap_atbl.get(key) + if status == False: + table_found = False + + # teardown + fvs = swsscommon.FieldValuePairs([("trap_ids", "lldp"), ("trap_group", "queue4_group3")]) + self.trap_ctbl.set("lldp", fvs) + fvs = swsscommon.FieldValuePairs([("state", "enabled")]) + self.feature_tbl.set("lldp", fvs) + + assert table_found == False