From 10804d7ff7bb5eed05abd952f1dab8901bca4bc5 Mon Sep 17 00:00:00 2001 From: Noa Or Date: Thu, 18 Nov 2021 09:01:23 +0000 Subject: [PATCH 1/4] [CoPP] Add always_enabled field to coppmgr logic --- cfgmgr/coppmgr.cpp | 207 ++++++++++++++++++++++++++++++++++++--------- cfgmgr/coppmgr.h | 9 +- tests/test_copp.py | 181 ++++++++++++++++++++++++++++++--------- 3 files changed, 317 insertions(+), 80 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 834b2c5ff0..97957e4952 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -98,11 +98,17 @@ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) if (!enable) { - m_coppDisabledTraps.insert(feature); + if (m_coppAlwaysEnabledTraps.find(feature) == m_coppAlwaysEnabledTraps.end()) + { + m_coppDisabledTraps.insert(feature); + } } else { - m_coppDisabledTraps.erase(feature); + if (m_coppDisabledTraps.find(feature) != m_coppDisabledTraps.end()) + { + m_coppDisabledTraps.erase(feature); + } } /* Trap group moved to pending state when feature is disabled. Remove trap group @@ -159,6 +165,7 @@ bool CoppMgr::isTrapIdDisabled(string trap_id) } return false; } + 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,6 +261,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { std::vector feature_fvs; m_cfgFeatureTable.get(i, feature_fvs); + m_featuresCfgTable.emplace(i, feature_fvs); for (auto j: feature_fvs) { @@ -264,12 +272,37 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c } } + /* If there is a trap that has always_enabled = true, remove it from the disabledTraps list */ + for (auto trap: m_coppTrapInitCfg) + { + auto trap_name = trap.first; + auto trap_info = trap.second; + bool always_enabled = false; + + if (std::find(trap_info.begin(), trap_info.end(), FieldValueTuple("always_enabled", "true")) != trap_info.end()) + { + always_enabled = true; + m_coppAlwaysEnabledTraps.insert(trap_name); + + if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), trap_name) != m_coppDisabledTraps.end()) + { + m_coppDisabledTraps.erase(trap_name); + } + } + /* if trap has no feature entry and doesn't have the always_enabled:true field, add to disabledTraps list */ + if (!(std::count(feature_keys.begin(), feature_keys.end(), trap_name)) && !always_enabled) + { + m_coppDisabledTraps.insert(trap_name); + } + } + mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable); for (auto i : trap_cfg) { string trap_group; string trap_ids; + string is_always_enabled = "false"; std::vector trap_fvs = i.second; for (auto j: trap_fvs) @@ -282,13 +315,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 (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end()) + { + setCoppTrapStateOk(i.first); + } } } @@ -315,15 +357,18 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c trap_group_fvs.push_back(fv); } - if (!trap_group_fvs.empty()) + if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end()) { - m_appCoppTable.set(i.first, trap_group_fvs); - } - setCoppGroupStateOk(i.first); - auto g_cfg = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); - if (g_cfg != group_cfg_keys.end()) - { - g_copp_init_set.insert(i.first); + if (!trap_group_fvs.empty()) + { + m_appCoppTable.set(i.first, trap_group_fvs); + } + setCoppGroupStateOk(i.first); + auto g_cfg = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); + if (g_cfg != group_cfg_keys.end()) + { + g_copp_init_set.insert(i.first); + } } } } @@ -406,6 +451,36 @@ void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) } } +void CoppMgr::removeTrap(std::string key) +{ + std::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(std::string trap_ids, std::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 +493,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 +525,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 +538,8 @@ 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,11 +548,13 @@ 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 */ @@ -485,25 +563,67 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) 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)) + { + std::vector feature_fvs; + 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. */ + + m_coppAlwaysEnabledTraps.insert(key); + if (m_coppTrapConfMap.find(key) == m_coppTrapConfMap.end()) + { + addTrap(trap_ids, 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_coppAlwaysEnabledTraps.erase(key); + + if (m_featuresCfgTable.find(key) != m_featuresCfgTable.end()) + { + bool isEnabled {false}; + feature_fvs = m_featuresCfgTable[key]; + for (auto i: feature_fvs) + { + if (fvField(i) == "state" && fvValue(i) == "enabled") + { + isEnabled = true; + it = consumer.m_toSync.erase(it); + continue; + } + } + if (isEnabled) + { + continue; + } + } + + removeTrap(key); + delCoppTrapStateOk(key); + m_coppTrapConfMap.erase(key); + m_coppDisabledTraps.insert(key); + } + } + /* 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); - } + 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 +631,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 +644,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,7 +667,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } } - if (conf_present) + if (conf_present) { m_coppTrapConfMap.erase(key); } @@ -569,6 +690,10 @@ 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; @@ -583,6 +708,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); } } @@ -706,6 +832,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) } } + void CoppMgr::doFeatureTask(Consumer &consumer) { auto it = consumer.m_toSync.begin(); @@ -715,11 +842,11 @@ void CoppMgr::doFeatureTask(Consumer &consumer) string key = kfvKey(t); string op = kfvOp(t); - vector fvs; string trap_ids; if (op == SET_COMMAND) { + m_featuresCfgTable.emplace(key, kfvFieldsValues(t)); for (auto i : kfvFieldsValues(t)) { if (fvField(i) == "state") diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index b010489f2e..ff8cd29286 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 */ @@ -71,9 +73,11 @@ class CoppMgr : public Orch CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; CoppGroupFvs m_coppGroupFvs; std::set m_coppDisabledTraps; + std::set m_coppAlwaysEnabledTraps; CoppCfg m_coppGroupInitCfg; CoppCfg m_coppTrapInitCfg; - + CoppCfg m_featuresCfgTable; + void doTask(Consumer &consumer); void doCoppGroupTask(Consumer &consumer); @@ -98,6 +102,9 @@ class CoppMgr : public Orch bool isTrapGroupInstalled(std::string key); 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 19faac954f..a570fbfb29 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] @@ -570,12 +607,17 @@ def test_new_trap_group_add(self, dvs, testlog): traps = "igmp_v1_report" t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report")]) 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 @@ -604,14 +646,19 @@ def test_new_trap_group_del(self, dvs, testlog): traps = "igmp_v1_report" t_fvs = swsscommon.FieldValuePairs([("trap_group", "queue5_group1"),("trap_ids", "igmp_v1_report")]) 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,58 @@ 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", "bgp,bgpv6"), ("trap_group", "queue4_group1"), ("always_enabled", "true")]) + self.trap_ctbl.set("bgp", fvs) + fvs = swsscommon.FieldValuePairs([("state", "disabled")]) + self.feature_tbl.set("bgp", 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 "bgp" not in trap_ids: + continue + + trap_found = False + trap_type = traps_to_trap_type["bgp"] + 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", "bgp,bgpv6"), ("trap_group", "queue4_group1"), ("always_enabled", "false")]) + self.trap_ctbl.set("bgp", 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", "bgp,bgpv6"), ("trap_group", "queue4_group1")]) + self.trap_ctbl.set("bgp", fvs) + fvs = swsscommon.FieldValuePairs([("state", "enabled")]) + self.feature_tbl.set("bgp", fvs) + + assert table_found == False + + From e32c891e42a948f9fffeb418689828d3988422ee Mon Sep 17 00:00:00 2001 From: noaOrMlnx <58519608+noaOrMlnx@users.noreply.github.com> Date: Thu, 2 Dec 2021 14:20:17 +0200 Subject: [PATCH 2/4] Update test_disabled_feature_always_enabled_trap Change the assertion logic to be correct --- tests/test_copp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_copp.py b/tests/test_copp.py index a570fbfb29..10719b5d4c 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -840,6 +840,6 @@ def test_disabled_feature_always_enabled_trap(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("state", "enabled")]) self.feature_tbl.set("bgp", fvs) - assert table_found == False + assert table_found == True From b1c4939503604101e4b29931d47c84d04e06d26c Mon Sep 17 00:00:00 2001 From: Noa Or Date: Wed, 22 Dec 2021 08:09:44 +0000 Subject: [PATCH 3/4] Fix review & integration tests --- cfgmgr/coppmgr.cpp | 264 +++++++++++++++++++++++++-------------------- cfgmgr/coppmgr.h | 4 +- tests/test_copp.py | 28 +++-- 3 files changed, 165 insertions(+), 131 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 97957e4952..c1c31bd3c8 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -78,37 +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) { - if (m_coppAlwaysEnabledTraps.find(feature) == m_coppAlwaysEnabledTraps.end()) - { - m_coppDisabledTraps.insert(feature); - } + state = "enabled"; } - else + if (m_featuresCfgTable.find(feature) != m_featuresCfgTable.end()) { - if (m_coppDisabledTraps.find(feature) != m_coppDisabledTraps.end()) + auto vect = m_featuresCfgTable[feature]; + for (long unsigned int i=0; i < vect.size(); i++) { - m_coppDisabledTraps.erase(feature); + 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 @@ -146,24 +151,44 @@ 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) @@ -262,38 +287,6 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c std::vector feature_fvs; m_cfgFeatureTable.get(i, feature_fvs); m_featuresCfgTable.emplace(i, feature_fvs); - - for (auto j: feature_fvs) - { - if (fvField(j) == "state" && fvValue(j) == "disabled") - { - m_coppDisabledTraps.insert(i); - } - } - } - - /* If there is a trap that has always_enabled = true, remove it from the disabledTraps list */ - for (auto trap: m_coppTrapInitCfg) - { - auto trap_name = trap.first; - auto trap_info = trap.second; - bool always_enabled = false; - - if (std::find(trap_info.begin(), trap_info.end(), FieldValueTuple("always_enabled", "true")) != trap_info.end()) - { - always_enabled = true; - m_coppAlwaysEnabledTraps.insert(trap_name); - - if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), trap_name) != m_coppDisabledTraps.end()) - { - m_coppDisabledTraps.erase(trap_name); - } - } - /* if trap has no feature entry and doesn't have the always_enabled:true field, add to disabledTraps list */ - if (!(std::count(feature_keys.begin(), feature_keys.end(), trap_name)) && !always_enabled) - { - m_coppDisabledTraps.insert(trap_name); - } } mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable); @@ -327,7 +320,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_coppTrapConfMap[i.first].trap_group = trap_group; m_coppTrapConfMap[i.first].trap_ids = trap_ids; m_coppTrapConfMap[i.first].is_always_enabled = is_always_enabled; - if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end()) + if (is_always_enabled == "true" || isFeatureEnabled(i.first)) { setCoppTrapStateOk(i.first); } @@ -357,18 +350,15 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c trap_group_fvs.push_back(fv); } - if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end()) + if (!trap_group_fvs.empty()) { - if (!trap_group_fvs.empty()) - { - m_appCoppTable.set(i.first, trap_group_fvs); - } - setCoppGroupStateOk(i.first); - auto g_cfg = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); - if (g_cfg != group_cfg_keys.end()) - { - g_copp_init_set.insert(i.first); - } + m_appCoppTable.set(i.first, trap_group_fvs); + } + setCoppGroupStateOk(i.first); + auto g_cfg = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); + if (g_cfg != group_cfg_keys.end()) + { + g_copp_init_set.insert(i.first); } } } @@ -429,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) { @@ -451,9 +440,9 @@ void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) } } -void CoppMgr::removeTrap(std::string key) +void CoppMgr::removeTrap(string key) { - std::string trap_ids; + 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); @@ -466,7 +455,7 @@ void CoppMgr::removeTrap(std::string key) } } -void CoppMgr::addTrap(std::string trap_ids, std::string trap_group) +void CoppMgr::addTrap(string trap_ids, string trap_group) { string trap_group_trap_ids; std::vector fvs; @@ -540,6 +529,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { removeTrap(key); setCoppTrapStateOk(key); + m_coppTrapConfMap.erase(key); } it = consumer.m_toSync.erase(it); @@ -560,10 +550,61 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) */ if (trap_group.empty() || trap_ids.empty()) { - it = consumer.m_toSync.erase(it); - continue; - } + 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) && @@ -576,43 +617,36 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if the trap is not installed, install it. otherwise, do nothing. */ - m_coppAlwaysEnabledTraps.insert(key); if (m_coppTrapConfMap.find(key) == m_coppTrapConfMap.end()) { addTrap(trap_ids, trap_group); } + else + { + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + } + + 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. */ - m_coppAlwaysEnabledTraps.erase(key); - if (m_featuresCfgTable.find(key) != m_featuresCfgTable.end()) + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; + if (isFeatureEnabled(key)) { - bool isEnabled {false}; - feature_fvs = m_featuresCfgTable[key]; - for (auto i: feature_fvs) - { - if (fvField(i) == "state" && fvValue(i) == "enabled") - { - isEnabled = true; - it = consumer.m_toSync.erase(it); - continue; - } - } - if (isEnabled) - { - continue; - } + it = consumer.m_toSync.erase(it); + continue; } removeTrap(key); delCoppTrapStateOk(key); - m_coppTrapConfMap.erase(key); - m_coppDisabledTraps.insert(key); } + it = consumer.m_toSync.erase(it); + continue; } /* Remove the current trap IDs and add the new trap IDS to recompute the @@ -623,6 +657,10 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); } + + 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 @@ -649,7 +687,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } else if (op == DEL_COMMAND) { - if (conf_present) + if (conf_present && !m_coppTrapConfMap[key].trap_group.empty() && !m_coppTrapConfMap[key].trap_ids.empty()) { removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); @@ -667,8 +705,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); @@ -695,21 +734,15 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) 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 == "true" || isFeatureEnabled(key)) { - m_appCoppTable.set(trap_group, g_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); + setCoppTrapStateOk(key); } - 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); } } it = consumer.m_toSync.erase(it); @@ -846,13 +879,16 @@ void CoppMgr::doFeatureTask(Consumer &consumer) if (op == SET_COMMAND) { - m_featuresCfgTable.emplace(key, kfvFieldsValues(t)); + 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 ff8cd29286..1d53756fce 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -72,8 +72,6 @@ class CoppMgr : public Orch CoppTrapConfMap m_coppTrapConfMap; CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; CoppGroupFvs m_coppGroupFvs; - std::set m_coppDisabledTraps; - std::set m_coppAlwaysEnabledTraps; CoppCfg m_coppGroupInitCfg; CoppCfg m_coppTrapInitCfg; CoppCfg m_featuresCfgTable; @@ -100,11 +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 10719b5d4c..5885a489b5 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -605,7 +605,7 @@ 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) for c_trap in copp_trap: trap_info = copp_trap[c_trap] @@ -644,7 +644,7 @@ 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) for c_trap in copp_trap: trap_info = copp_trap[c_trap] @@ -792,10 +792,10 @@ def test_empty_trap_cfg(self, dvs, testlog): def test_disabled_feature_always_enabled_trap(self, dvs, testlog): self.setup_copp(dvs) - fvs = swsscommon.FieldValuePairs([("trap_ids", "bgp,bgpv6"), ("trap_group", "queue4_group1"), ("always_enabled", "true")]) - self.trap_ctbl.set("bgp", fvs) + 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("bgp", fvs) + self.feature_tbl.set("lldp", fvs) time.sleep(2) global copp_trap @@ -806,11 +806,11 @@ def test_disabled_feature_always_enabled_trap(self, dvs, testlog): trap_ids = trap_info[0].split(";") trap_group = trap_info[1] - if "bgp" not in trap_ids: + if "lldp" not in trap_ids: continue trap_found = False - trap_type = traps_to_trap_type["bgp"] + trap_type = traps_to_trap_type["lldp"] for key in trap_keys: (status, fvs) = self.trap_atbl.get(key) assert status == True @@ -824,8 +824,8 @@ def test_disabled_feature_always_enabled_trap(self, dvs, testlog): assert trap_found == True # change always_enabled to be false and check the trap is not installed: - fvs = swsscommon.FieldValuePairs([("trap_ids", "bgp,bgpv6"), ("trap_group", "queue4_group1"), ("always_enabled", "false")]) - self.trap_ctbl.set("bgp", fvs) + 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 @@ -835,11 +835,9 @@ def test_disabled_feature_always_enabled_trap(self, dvs, testlog): table_found = False # teardown - fvs = swsscommon.FieldValuePairs([("trap_ids", "bgp,bgpv6"), ("trap_group", "queue4_group1")]) - self.trap_ctbl.set("bgp", fvs) + 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("bgp", fvs) - - assert table_found == True - + self.feature_tbl.set("lldp", fvs) + assert table_found == False From 95e7efe1c37b6637626fb81a34fec42b8c53f9d7 Mon Sep 17 00:00:00 2001 From: Noa Or Date: Thu, 23 Dec 2021 14:15:24 +0000 Subject: [PATCH 4/4] Update TrapConfMap in right place --- cfgmgr/coppmgr.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index c1c31bd3c8..1721cc8593 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -610,21 +610,18 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) (trap_group == m_coppTrapConfMap[key].trap_group) && (trap_ids == m_coppTrapConfMap[key].trap_ids)) { - std::vector feature_fvs; + 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 (m_coppTrapConfMap.find(key) == m_coppTrapConfMap.end()) + // if the feature was not enabled, install the trap + if (!isFeatureEnabled(key)) { addTrap(trap_ids, trap_group); } - else - { - m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; - } it = consumer.m_toSync.erase(it); continue; @@ -635,7 +632,6 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) 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); @@ -687,7 +683,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } else if (op == DEL_COMMAND) { - if (conf_present && !m_coppTrapConfMap[key].trap_group.empty() && !m_coppTrapConfMap[key].trap_ids.empty()) + if (conf_present) { removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); @@ -719,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) @@ -734,12 +731,16 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) is_always_enabled = fvValue(i); } } + if (is_always_enabled.empty()) + { + is_always_enabled = "false"; + } + m_coppTrapConfMap[key].trap_group = trap_group; + m_coppTrapConfMap[key].trap_ids = trap_ids; + m_coppTrapConfMap[key].is_always_enabled = is_always_enabled; if (is_always_enabled == "true" || isFeatureEnabled(key)) { - 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); setCoppTrapStateOk(key); }