From 353becb6ed6d04ee6a290c4ac95fdd3a3a940fb4 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Mon, 22 Jun 2020 07:45:38 -0700 Subject: [PATCH 01/14] Copp Changes Copp #2 changes Copp test changes Copp changes cleanup Cosmetic change#1 Cosmetic changes #2 NAT fix Fixing nat case --- cfgmgr/Makefile.am | 8 +- cfgmgr/coppmgr.cpp | 789 +++++++++++++++++++++++++++++++++ cfgmgr/coppmgr.h | 109 +++++ cfgmgr/coppmgrd.cpp | 93 ++++ orchagent/copporch.cpp | 922 ++++++++++++++++++++++++++------------- orchagent/copporch.h | 34 +- orchagent/orchdaemon.cpp | 10 +- tests/test_copp.py | 674 ++++++++++++++++++++++++++++ 8 files changed, 2313 insertions(+), 326 deletions(-) create mode 100644 cfgmgr/coppmgr.cpp create mode 100644 cfgmgr/coppmgr.h create mode 100644 cfgmgr/coppmgrd.cpp create mode 100644 tests/test_copp.py diff --git a/cfgmgr/Makefile.am b/cfgmgr/Makefile.am index 7178cad1c6..25030112f7 100644 --- a/cfgmgr/Makefile.am +++ b/cfgmgr/Makefile.am @@ -3,7 +3,7 @@ CFLAGS_SAI = -I /usr/include/sai LIBNL_CFLAGS = -I/usr/include/libnl3 LIBNL_LIBS = -lnl-genl-3 -lnl-route-3 -lnl-3 -bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd sflowmgrd natmgrd +bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd sflowmgrd natmgrd coppmgrd if DEBUG DBGFLAGS = -ggdb -DDEBUG @@ -60,3 +60,9 @@ natmgrd_SOURCES = natmgrd.cpp natmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_ natmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) natmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) natmgrd_LDADD = -lswsscommon + +coppmgrd_SOURCES = coppmgrd.cpp coppmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +coppmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) +coppmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) +coppmgrd_LDADD = -lswsscommon + diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp new file mode 100644 index 0000000000..61b6c3fef6 --- /dev/null +++ b/cfgmgr/coppmgr.cpp @@ -0,0 +1,789 @@ +#include +#include "logger.h" +#include "dbconnector.h" +#include "producerstatetable.h" +#include "tokenize.h" +#include "ipprefix.h" +#include "coppmgr.h" +#include "exec.h" +#include "shellcmd.h" +#include "warm_restart.h" +#include "json.hpp" + +using json = nlohmann::json; + +using namespace std; +using namespace swss; + +static set g_copp_init_set; +static map trap_feature_map = { + {COPP_TRAP_TYPE_SAMPLEPACKET, "sflow"} +}; + +void CoppMgr::parseInitFile(void) +{ + std::ifstream ifs(COPP_INIT_FILE); + if (ifs.fail()) + { + return; + } + json j = json::parse(ifs); + for(auto tbl = j.begin(); tbl != j.end(); tbl++) + { + string table_name = tbl.key(); + json keys = tbl.value(); + for (auto k = keys.begin(); k != keys.end(); k++) + { + string table_key = k.key(); + json fvp = k.value(); + vector fvs; + + for (auto f = fvp.begin(); f != fvp.end(); f++) + { + FieldValueTuple fv(f.key(), f.value().get()); + fvs.push_back(fv); + } + if (table_name == CFG_COPP_TRAP_TABLE_NAME) + { + m_coppTrapInitCfg[table_key] = fvs; + } + else if (table_name == CFG_COPP_GROUP_TABLE_NAME) + { + m_coppGroupInitCfg[table_key] = fvs; + } + } + } +} + +/* Check if the trap group has traps that can be installed only when + * feature is enabled + */ +bool CoppMgr::checkIfTrapGroupFeaturePending(string trap_group_name) +{ + std::vector fvs; + + if (m_coppGroupRestrictedMap.find(trap_group_name) == m_coppGroupRestrictedMap.end()) + { + return false; + } + + for (auto it: m_coppTrapIdTrapGroupMap) + { + if (it.second == trap_group_name) + { + /* At least one trap should be enabled to install the + * trap group with restricted attributes + */ + if (!isTrapDisabled(it.first)) + { + return false; + } + } + } + return true; +} + +void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) +{ + for (auto it: trap_feature_map) + { + if (it.second == feature) + { + if (enable) + { + if (m_coppTrapDisabledMap.find(it.first) != m_coppTrapDisabledMap.end()) + { + if (m_coppTrapIdTrapGroupMap.find(it.first) != m_coppTrapIdTrapGroupMap.end()) + { + string trap_group = m_coppTrapIdTrapGroupMap[it.first]; + + /* Enable only when the restricted group is already pending */ + if ((m_coppGroupRestrictedMap.find(trap_group) + != m_coppGroupRestrictedMap.end()) && + (checkIfTrapGroupFeaturePending(trap_group))) + { + vector fvs; + vector modified_fvs; + string trap_ids; + + for (auto i : m_coppGroupRestrictedMap[trap_group]) + { + FieldValueTuple fv(i.first, i.second); + fvs.push_back(fv); + } + getTrapGroupTrapIds(trap_group, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false); + if (!modified_fvs.empty()) + { + m_appCoppTable.set(trap_group, modified_fvs); + } + + } + } + m_coppTrapDisabledMap.erase(it.first); + } + } + else + { + if (m_coppTrapDisabledMap.find(it.first) == m_coppTrapDisabledMap.end()) + { + m_coppTrapDisabledMap[it.first] = true; + if (m_coppTrapIdTrapGroupMap.find(it.first) != m_coppTrapIdTrapGroupMap.end()) + { + string trap_group = m_coppTrapIdTrapGroupMap[it.first]; + + /* Delete the restricted copp group when it goes to pending state + * on feature disable + */ + if ((m_coppGroupRestrictedMap.find(trap_group) + != m_coppGroupRestrictedMap.end()) && + (checkIfTrapGroupFeaturePending(trap_group))) + { + m_appCoppTable.del(trap_group); + } + } + } + } + } + } +} + +bool CoppMgr::isTrapDisabled(string trap_id) +{ + auto trap_idx = m_coppTrapDisabledMap.find(trap_id); + if (trap_idx != m_coppTrapDisabledMap.end()) + { + return m_coppTrapDisabledMap[trap_id]; + } + return false; +} + +/* The genetlink fields are restricted and needs to be installed only on + * feature(sflow) enable + */ +bool CoppMgr::coppGroupHasRestrictedFields (vector &fvs) +{ + for (auto i: fvs) + { + if ((fvField(i) == COPP_GROUP_GENETLINK_NAME_FIELD) || + (fvField(i) == COPP_GROUP_GENETLINK_MCGRP_NAME_FIELD)) + { + return true; + } + } + return false; +} + +CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames) : + Orch(cfgDb, tableNames), + m_cfgCoppTrapTable(cfgDb, CFG_COPP_TRAP_TABLE_NAME), + m_cfgCoppGroupTable(cfgDb, CFG_COPP_GROUP_TABLE_NAME), + m_cfgFeatureTable(cfgDb, CFG_FEATURE_TABLE_NAME), + m_appCoppTable(appDb, APP_COPP_TABLE_NAME), + m_stateCoppTrapTable(stateDb, STATE_COPP_TRAP_TABLE_NAME), + m_stateCoppGroupTable(stateDb, STATE_COPP_GROUP_TABLE_NAME), + m_coppTable(appDb, APP_COPP_TABLE_NAME) +{ + SWSS_LOG_ENTER(); + parseInitFile(); + std::vector group_keys; + std::vector trap_keys; + std::vector feature_keys; + std::vector app_keys; + + std::vector group_cfg_keys; + std::vector trap_cfg_keys; + + CoppCfg group_cfg; + CoppCfg trap_cfg; + + m_cfgCoppGroupTable.getKeys(group_cfg_keys); + m_cfgCoppTrapTable.getKeys(trap_cfg_keys); + m_cfgFeatureTable.getKeys(feature_keys); + m_coppTable.getKeys(app_keys); + + + for (auto it: trap_feature_map) + { + m_coppTrapDisabledMap[it.first] = true; + } + for (auto i: feature_keys) + { + std::vector feature_fvs; + m_cfgFeatureTable.get(i, feature_fvs); + + bool status = false; + for (auto j: feature_fvs) + { + if (fvField(j) == "status") + { + if (fvValue(j) == "enabled") + { + status = true; + } + } + } + setFeatureTrapIdsStatus(i, status); + } + + /* Read the init configuration first. If the same key is present in + * user configuration, override the init fields with user fields + */ + for (auto i : m_coppTrapInitCfg) + { + std::vector trap_init_fvs = i.second; + std::vector trap_fvs; + auto key = std::find(trap_cfg_keys.begin(), trap_cfg_keys.end(), i.first); + if (key != trap_cfg_keys.end()) + { + std::vector trap_cfg_fvs; + m_cfgCoppTrapTable.get(i.first, trap_cfg_fvs); + + trap_fvs = trap_cfg_fvs; + for (auto it1: trap_init_fvs) + { + bool field_found = false; + for (auto it2: trap_cfg_fvs) + { + if(fvField(it1) == fvField(it2)) + { + field_found = true; + break; + } + } + if (!field_found) + { + trap_fvs.push_back(it1); + } + } + trap_cfg[i.first] = trap_fvs; + } + else + { + trap_cfg[i.first] = m_coppTrapInitCfg[i.first]; + } + } + + /* Read the user configuration keys that were not present in + * init configuration. + */ + for (auto i : trap_cfg_keys) + { + if(m_coppTrapInitCfg.find(i) == m_coppTrapInitCfg.end()) + { + std::vector trap_cfg_fvs; + m_cfgCoppTrapTable.get(i, trap_cfg_fvs); + trap_cfg[i] = trap_cfg_fvs; + } + } + + for (auto i : trap_cfg) + { + string trap_group; + string trap_ids; + std::vector trap_fvs = i.second; + + for (auto j: trap_fvs) + { + if (fvField(j) == COPP_TRAP_ID_LIST_FIELD) + { + trap_ids = fvValue(j); + } + else if (fvField(j) == COPP_TRAP_GROUP_FIELD) + { + trap_group = 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; + } + } + + /* Read the init configuration first. If the same key is present in + * user configuration, override the init fields with user fields + */ + for (auto i : m_coppGroupInitCfg) + { + std::vector group_init_fvs = i.second; + std::vector group_fvs; + auto key = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); + if (key != group_cfg_keys.end()) + { + std::vector group_cfg_fvs; + m_cfgCoppGroupTable.get(i.first, group_cfg_fvs); + + group_fvs = group_cfg_fvs; + for (auto it1: group_init_fvs) + { + bool field_found = false; + for (auto it2: group_cfg_fvs) + { + if(fvField(it1) == fvField(it2)) + { + field_found = true; + break; + } + } + if (!field_found) + { + group_fvs.push_back(it1); + } + } + group_cfg[i.first] = group_fvs; + } + else + { + group_cfg[i.first] = m_coppGroupInitCfg[i.first]; + } + } + + /* Read the user configuration keys that were not present in + * init configuration. + */ + for (auto i : group_cfg_keys) + { + if(m_coppGroupInitCfg.find(i) == m_coppGroupInitCfg.end()) + { + std::vector group_cfg_fvs; + m_cfgCoppGroupTable.get(i, group_cfg_fvs); + group_cfg[i] = group_cfg_fvs; + } + } + for (auto i: group_cfg) + { + string trap_ids; + vector trap_group_fvs = i.second; + + if (coppGroupHasRestrictedFields (trap_group_fvs)) + { + for (auto fv: trap_group_fvs) + { + m_coppGroupRestrictedMap[i.first][fvField(fv)]= fvValue(fv); + } + } + if (checkIfTrapGroupFeaturePending(i.first)) + { + continue; + } + + getTrapGroupTrapIds(i.first, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + trap_group_fvs.push_back(fv); + } + + vector trap_app_fvs; + + coppGroupGetModifiedFvs (i.first, trap_group_fvs, trap_app_fvs, true); + if (!trap_app_fvs.empty()) + { + m_appCoppTable.set(i.first, trap_app_fvs); + } + 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); + } + } +} + +void CoppMgr::setCoppGroupStateOk(string alias) +{ + FieldValueTuple tuple("state", "ok"); + vector fvs; + fvs.push_back(tuple); + m_stateCoppGroupTable.set(alias, fvs); + SWSS_LOG_NOTICE("Publish %s(ok) to state db", alias.c_str()); +} + +void CoppMgr::delCoppGroupStateOk(string alias) +{ + m_stateCoppGroupTable.del(alias); + SWSS_LOG_NOTICE("Delete %s(ok) from state db", alias.c_str()); +} + +void CoppMgr::setCoppTrapStateOk(string alias) +{ + FieldValueTuple tuple("state", "ok"); + vector fvs; + fvs.push_back(tuple); + m_stateCoppTrapTable.set(alias, fvs); + SWSS_LOG_NOTICE("Publish %s(ok) to state db", alias.c_str()); +} + +void CoppMgr::delCoppTrapStateOk(string alias) +{ + m_stateCoppTrapTable.del(alias); + SWSS_LOG_NOTICE("Delete %s(ok) from state db", alias.c_str()); +} + +void CoppMgr::addTrapIdsToTrapGroup(string trap_group, string trap_ids) +{ + vector trap_id_list; + + trap_id_list = tokenize(trap_ids, list_item_delimiter); + + for (auto i: trap_id_list) + { + m_coppTrapIdTrapGroupMap[i] = trap_group; + } +} + +void CoppMgr::removeTrapIdsFromTrapGroup(string trap_group, string trap_ids) +{ + vector trap_id_list; + + trap_id_list = tokenize(trap_ids, list_item_delimiter); + + for (auto i: trap_id_list) + { + m_coppTrapIdTrapGroupMap.erase(i); + } +} + +void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) +{ + + trap_ids.clear(); + for (auto it: m_coppTrapIdTrapGroupMap) + { + if (it.second == trap_group) + { + if (trap_ids.empty()) + { + trap_ids = it.first; + } + else + { + trap_ids += list_item_delimiter + it.first; + } + } + } +} + +void CoppMgr::doCoppTrapTask(Consumer &consumer) +{ + auto it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + vector fvs; + string trap_ids; + string trap_group; + bool conf_present = false; + + if (op == SET_COMMAND) + { + /*Create case*/ + if (m_coppTrapConfMap.find(key) != m_coppTrapConfMap.end()) + { + trap_ids = m_coppTrapConfMap[key].trap_ids; + trap_group = m_coppTrapConfMap[key].trap_group; + conf_present = true; + } + for (auto i: kfvFieldsValues(t)) + { + if (fvField(i) == COPP_TRAP_GROUP_FIELD) + { + trap_group = fvValue(i); + } + else if (fvField(i) == COPP_TRAP_ID_LIST_FIELD) + { + trap_ids = fvValue(i); + } + } + /*Duplicate check*/ + if (conf_present && + (trap_group == m_coppTrapConfMap[key].trap_group) && + (trap_ids == m_coppTrapConfMap[key].trap_ids)) + { + it = consumer.m_toSync.erase(it); + continue; + } + 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 (!checkIfTrapGroupFeaturePending(trap_group)) + { + m_appCoppTable.set(trap_group, fvs); + } + + /* When the trap table's trap group is changed, the old trap group + * should also be reprogrammed as some of its associated traps got + * removed + */ + if ((!m_coppTrapConfMap[key].trap_group.empty()) && + (trap_group != m_coppTrapConfMap[key].trap_group)) + { + trap_group_trap_ids.clear(); + fvs.clear(); + getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_group_trap_ids); + FieldValueTuple fv2(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); + fvs.push_back(fv2); + if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + { + m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + } + } + m_coppTrapConfMap[key].trap_group = trap_group; + m_coppTrapConfMap[key].trap_ids = trap_ids; + setCoppTrapStateOk(key); + } + else if (op == DEL_COMMAND) + { + removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, + m_coppTrapConfMap[key].trap_ids); + fvs.clear(); + trap_ids.clear(); + getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + { + m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + } + m_coppTrapConfMap.erase(key); + delCoppTrapStateOk(key); + + /* If the COPP trap was part of init config, it needs to be recreated + * with field values from init. The configuration delete should just clear + * the externally applied user configuration + */ + if (m_coppTrapInitCfg.find(key) != m_coppTrapInitCfg.end()) + { + auto fvs = m_coppTrapInitCfg[key]; + for (auto i: fvs) + { + if (fvField(i) == COPP_TRAP_GROUP_FIELD) + { + trap_group = fvValue(i); + } + else if (fvField(i) == COPP_TRAP_ID_LIST_FIELD) + { + trap_ids = 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 (!checkIfTrapGroupFeaturePending(trap_group)) + { + m_appCoppTable.set(trap_group, g_fvs); + } + } + } + it = consumer.m_toSync.erase(it); + } +} + +/* This API is used to fetch only the modified configurations. During warmboot + * when certain fields in APP table are not present in new init config file + * the APP table needs to be removed and recreated + */ +void CoppMgr::coppGroupGetModifiedFvs(string key, vector &trap_group_fvs, + vector &modified_fvs, bool del_on_field_remove) +{ + vector app_fvs; + std::vector app_keys; + + m_coppTable.get(key,app_fvs); + modified_fvs = trap_group_fvs; + + m_coppTable.getKeys(app_keys); + auto app_key = std::find(app_keys.begin(), app_keys.end(),key); + if (app_key != app_keys.end()) + { + vector app_fvs; + m_coppTable.get(key, app_fvs); + for (auto app_idx: app_fvs) + { + bool field_removed = true; + for (auto cfg_idx: trap_group_fvs) + { + if (fvField(cfg_idx) == fvField(app_idx)) + { + field_removed = false; + if (fvValue(cfg_idx) == fvValue(app_idx)) + { + auto vec_idx = std::find(modified_fvs.begin(), modified_fvs.end(), cfg_idx); + modified_fvs.erase(vec_idx); + } + } + } + if (field_removed && del_on_field_remove) + { + m_appCoppTable.del(key); + bool key_present = true; + while (key_present) + { + vector app_keys; + m_coppTable.getKeys(app_keys); + /* Loop until key is removed from app table */ + auto del_key = std::find(app_keys.begin(), app_keys.end(), key); + if (del_key == app_keys.end()) + { + key_present = false; + } + else + { + usleep(100); + } + } + modified_fvs = trap_group_fvs; + break; + } + } + } +} + +void CoppMgr::doCoppGroupTask(Consumer &consumer) +{ + auto it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + auto fvs = kfvFieldsValues(t); + string trap_ids; + + if (op == SET_COMMAND) + { + if (g_copp_init_set.find(key) != g_copp_init_set.end()) + { + g_copp_init_set.erase(key); + it = consumer.m_toSync.erase(it); + continue; + } + if (coppGroupHasRestrictedFields (fvs)) + { + for (auto fv: fvs) + { + m_coppGroupRestrictedMap[key][fvField(fv)] = fvValue(fv); + } + } + if (checkIfTrapGroupFeaturePending(key)) + { + it = consumer.m_toSync.erase(it); + continue; + } + getTrapGroupTrapIds(key, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + + vector modified_fvs; + coppGroupGetModifiedFvs(key, fvs, modified_fvs, false); + if (!modified_fvs.empty()) + { + m_appCoppTable.set(key, modified_fvs); + } + setCoppGroupStateOk(key); + } + else if (op == DEL_COMMAND) + { + SWSS_LOG_NOTICE("%s: DEL",key.c_str()); + m_appCoppTable.del(key); + + /* If the COPP group was part of init config, it needs to be recreated + * with field values from init. The configuration delete should just clear + * the externally applied user configuration + */ + if (m_coppGroupInitCfg.find(key) != m_coppGroupInitCfg.end()) + { + std::vector fvs = m_coppGroupInitCfg[key]; + getTrapGroupTrapIds(key, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + m_appCoppTable.set(key, fvs); + } + else + { + if (m_coppGroupRestrictedMap.find(key) != m_coppGroupRestrictedMap.end()) + { + m_coppGroupRestrictedMap.erase(key); + } + } + delCoppGroupStateOk(key); + } + it = consumer.m_toSync.erase(it); + } +} + +void CoppMgr::doFeatureTask(Consumer &consumer) +{ + auto it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + vector fvs; + string trap_ids; + + if (op == SET_COMMAND) + { + for (auto i : kfvFieldsValues(t)) + { + if (fvField(i) == "status") + { + bool status = false; + if (fvValue(i) == "enabled") + { + status = true; + } + setFeatureTrapIdsStatus(key, status); + } + } + } + else if (op == DEL_COMMAND) + { + setFeatureTrapIdsStatus(key, false); + } + it = consumer.m_toSync.erase(it); + } +} + +void CoppMgr::doTask(Consumer &consumer) +{ + SWSS_LOG_ENTER(); + auto table = consumer.getTableName(); + + if (table == CFG_COPP_TRAP_TABLE_NAME) + { + doCoppTrapTask(consumer); + } + else if (table == CFG_COPP_GROUP_TABLE_NAME) + { + doCoppGroupTask(consumer); + } + else if (table == CFG_FEATURE_TABLE_NAME) + { + doFeatureTask(consumer); + } +} diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h new file mode 100644 index 0000000000..281886d065 --- /dev/null +++ b/cfgmgr/coppmgr.h @@ -0,0 +1,109 @@ +#pragma once + +#include "dbconnector.h" +#include "orch.h" +#include "producerstatetable.h" +#include + +#include +#include +#include + +namespace swss { + +/* COPP Trap Table Fields */ +#define COPP_TRAP_ID_LIST_FIELD "trap_ids" +#define COPP_TRAP_GROUP_FIELD "trap_group" + +/* COPP Group Table Fields */ +#define COPP_GROUP_QUEUE_FIELD "queue" +#define COPP_GROUP_TRAP_ACTION_FIELD "trap_action" +#define COPP_GROUP_TRAP_PRIORITY_FIELD "trap_priority" +#define COPP_GROUP_POLICER_METER_TYPE_FIELD "meter_type" +#define COPP_GROUP_POLICER_MODE_FIELD "mode" +#define COPP_GROUP_POLICER_COLOR_FIELD "color" +#define COPP_GROUP_POLICER_CBS_FIELD "cbs" +#define COPP_GROUP_POLICER_CIR_FIELD "cir" +#define COPP_GROUP_POLICER_PBS_FIELD "pbs" +#define COPP_GROUP_POLICER_PIR_FIELD "pir" +#define COPP_GROUP_POLICER_ACTION_GREEN_FIELD "green_action" +#define COPP_GROUP_POLICER_ACTION_RED_FIELD "red_action" +#define COPP_GROUP_POLICER_ACTION_YELLOW_FIELD "yellow_action" + +/* sflow genetlink fields */ +#define COPP_GROUP_GENETLINK_NAME_FIELD "genetlink_name" +#define COPP_GROUP_GENETLINK_MCGRP_NAME_FIELD "genetlink_mcgrp_name" + +#define COPP_TRAP_TYPE_SAMPLEPACKET "sample_packet" + +#define COPP_INIT_FILE "/etc/sonic/copp_cfg.json" + +struct CoppTrapConf +{ + std::string trap_ids; + std::string trap_group; +}; + +/* TrapName to TrapConf map */ +typedef std::map CoppTrapConfMap; + +/* TrapGroupName to GroupConf map */ +typedef std::map CoppTrapIdTrapGroupMap; + +/* Trap Id to Enable/Disabled map */ +typedef std::map CoppTrapDisabledMap; + +/* Key to Field value Tuple map */ +typedef std::map> CoppCfg; + +/* Restricted Copp group key to Field value map's map */ +typedef std::map> CoppGroupRestrictedConf; + +class CoppMgr : public Orch +{ +public: + CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, + const std::vector &tableNames); + + using Orch::doTask; +private: + Table m_cfgCoppTrapTable; + Table m_cfgCoppGroupTable; + ProducerStateTable m_appCoppTable; + Table m_stateCoppTrapTable; + Table m_stateCoppGroupTable; + Table m_cfgFeatureTable; + Table m_coppTable; + CoppTrapConfMap m_coppTrapConfMap; + CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; + CoppGroupRestrictedConf m_coppGroupRestrictedMap; + CoppTrapDisabledMap m_coppTrapDisabledMap; + CoppCfg m_coppGroupInitCfg; + CoppCfg m_coppTrapInitCfg; + + + void doTask(Consumer &consumer); + void doCoppGroupTask(Consumer &consumer); + void doCoppTrapTask(Consumer &consumer); + void doFeatureTask(Consumer &consumer); + + void getTrapGroupTrapIds(std::string trap_group, std::string &trap_ids); + void removeTrapIdsFromTrapGroup(std::string trap_group, std::string trap_ids); + void addTrapIdsToTrapGroup(std::string trap_group, std::string trap_ids); + bool coppGroupHasRestrictedFields (std::vector &fvs); + bool isTrapDisabled(std::string trap_id); + void setFeatureTrapIdsStatus(std::string feature, bool enable); + bool checkIfTrapGroupFeaturePending(std::string trap_group_name); + + void setCoppGroupStateOk(std::string alias); + void delCoppGroupStateOk(std::string alias); + + void setCoppTrapStateOk(std::string alias); + void delCoppTrapStateOk(std::string alias); + void coppGroupGetModifiedFvs(std::string key, std::vector &trap_group_fvs, + std::vector &modified_fvs, bool del_on_field_remove); + void parseInitFile(void); + +}; + +} diff --git a/cfgmgr/coppmgrd.cpp b/cfgmgr/coppmgrd.cpp new file mode 100644 index 0000000000..1995405cd6 --- /dev/null +++ b/cfgmgr/coppmgrd.cpp @@ -0,0 +1,93 @@ +#include +#include +#include +#include +#include + +#include "exec.h" +#include "coppmgr.h" +#include "schema.h" +#include "select.h" +#include "warm_restart.h" + +using namespace std; +using namespace swss; + +/* select() function timeout retry time, in millisecond */ +#define SELECT_TIMEOUT 1000 + +/* + * Following global variables are defined here for the purpose of + * using existing Orch class which is to be refactored soon to + * eliminate the direct exposure of the global variables. + * + * Once Orch class refactoring is done, these global variables + * should be removed from here. + */ +int gBatchSize = 0; +bool gSwssRecord = false; +bool gLogRotate = false; +ofstream gRecordOfs; +string gRecordFile; +/* Global database mutex */ +mutex gDbMutex; + +int main(int argc, char **argv) +{ + Logger::linkToDbNative("coppmgrd"); + SWSS_LOG_ENTER(); + + SWSS_LOG_NOTICE("--- Starting coppmgrd ---"); + + try + { + vector cfg_copp_tables = { + CFG_COPP_TRAP_TABLE_NAME, + CFG_COPP_GROUP_TABLE_NAME, + CFG_FEATURE_TABLE_NAME, + }; + + WarmStart::initialize("coppmgrd", "swss"); + WarmStart::checkWarmStart("coppmgrd", "swss"); + + DBConnector cfgDb("CONFIG_DB", 0); + DBConnector appDb("APPL_DB", 0); + DBConnector stateDb("STATE_DB", 0); + + CoppMgr coppmgr(&cfgDb, &appDb, &stateDb, cfg_copp_tables); + + vector cfgOrchList = {&coppmgr}; + + swss::Select s; + for (Orch *o : cfgOrchList) + { + s.addSelectables(o->getSelectables()); + } + + while (true) + { + Selectable *sel; + int ret; + + ret = s.select(&sel, SELECT_TIMEOUT); + if (ret == Select::ERROR) + { + SWSS_LOG_NOTICE("Error: %s!", strerror(errno)); + continue; + } + if (ret == Select::TIMEOUT) + { + coppmgr.doTask(); + continue; + } + + auto *c = (Executor *)sel; + c->execute(); + } + } + catch (const exception &e) + { + SWSS_LOG_ERROR("Runtime error: %s", e.what()); + } + return -1; +} diff --git a/orchagent/copporch.cpp b/orchagent/copporch.cpp index a3e1cb4a78..5b99839dd1 100644 --- a/orchagent/copporch.cpp +++ b/orchagent/copporch.cpp @@ -20,6 +20,8 @@ extern sai_object_id_t gSwitchId; extern PortsOrch* gPortsOrch; extern bool gIsNatSupported; +static string old_nat_group_name = "trap.group.nat"; + static map policer_meter_map = { {"packets", SAI_METER_TYPE_PACKETS}, {"bytes", SAI_METER_TYPE_BYTES} @@ -94,11 +96,12 @@ const vector default_trap_ids = { SAI_HOSTIF_TRAP_TYPE_TTL_ERROR }; -CoppOrch::CoppOrch(vector &tableConnectors) : - Orch( tableConnectors) +CoppOrch::CoppOrch(DBConnector* db, string tableName) : + Orch(db, tableName) { SWSS_LOG_ENTER(); + m_coppTable = unique_ptr(new Table(db, APP_COPP_TABLE_NAME)); initDefaultHostIntfTable(); initDefaultTrapGroup(); initDefaultTrapIds(); @@ -203,14 +206,10 @@ void CoppOrch::getTrapIdList(vector &trap_id_name_list, vector &trap_id_name_list) +bool CoppOrch::createGenetlinkHostIfTable(vector &trap_id_list) { SWSS_LOG_ENTER(); - vector trap_id_list; - - getTrapIdList(trap_id_name_list, trap_id_list); - for (auto trap_id : trap_id_list) { auto host_tbl_entry = m_trapid_hostif_table_map.find(trap_id); @@ -257,6 +256,28 @@ bool CoppOrch::createGenetlinkHostIfTable(vector &trap_id_name_list) return true; } +bool CoppOrch::removeGenetlinkHostIfTable(vector &trap_id_list) +{ + sai_status_t sai_status; + + for (auto trap_id : trap_id_list) + { + if ( m_trapid_hostif_table_map.find(trap_id) != m_trapid_hostif_table_map.end()) + { + sai_status = sai_hostif_api->remove_hostif_table_entry( + m_trapid_hostif_table_map[trap_id]); + if(sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to delete hostif table entry %" PRId64 " \ + rc=%d", m_trapid_hostif_table_map[trap_id], sai_status); + return false; + } + m_trapid_hostif_table_map.erase(trap_id); + } + } + return true; + +} bool CoppOrch::applyAttributesToTrapIds(sai_object_id_t trap_group_id, const vector &trap_id_list, vector &trap_id_attribs) @@ -285,23 +306,6 @@ bool CoppOrch::applyAttributesToTrapIds(sai_object_id_t trap_group_id, return true; } -bool CoppOrch::applyTrapIds(sai_object_id_t trap_group, vector &trap_id_name_list, vector &trap_id_attribs) -{ - SWSS_LOG_ENTER(); - - vector trap_id_list; - - getTrapIdList(trap_id_name_list, trap_id_list); - - sai_attribute_t attr; - attr.id = SAI_HOSTIF_TRAP_ATTR_TRAP_GROUP; - attr.value.oid = trap_group; - trap_id_attribs.push_back(attr); - - return applyAttributesToTrapIds(trap_group, trap_id_list, trap_id_attribs); -} - - bool CoppOrch::removePolicer(string trap_group_name) { SWSS_LOG_ENTER(); @@ -414,25 +418,12 @@ bool CoppOrch::removeGenetlinkHostIf(string trap_group_name) SWSS_LOG_ENTER(); sai_status_t sai_status; + vector group_trap_ids; - for (auto it : m_syncdTrapIds) + getTrapIdsFromTrapGroup (m_trap_group_map[trap_group_name], group_trap_ids); + if (!removeGenetlinkHostIfTable(group_trap_ids)) { - if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) - { - auto hostTableEntry = m_trapid_hostif_table_map.find(it.first); - if (hostTableEntry != m_trapid_hostif_table_map.end()) - { - sai_status = sai_hostif_api->remove_hostif_table_entry(hostTableEntry->second); - if(sai_status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to delete hostif table entry %" PRId64 " \ - on trap group %s. rc=%d", hostTableEntry->second, - trap_group_name.c_str(), sai_status); - return false; - } - m_trapid_hostif_table_map.erase(it.first); - } - } + return false; } auto hostInfo = m_trap_group_hostif_map.find(m_trap_group_map[trap_group_name]); @@ -468,180 +459,36 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer) vector policer_attribs; vector genetlink_attribs; + vector trap_ids; + vector add_trap_ids; + vector rem_trap_ids; + std::vector fv_tuple = kfvFieldsValues(tuple); + if (op == SET_COMMAND) { - for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++) + if(checkDupCoppGrpNameAndUpdate(trap_group_name, fv_tuple)) + { + return task_process_status::task_success; + } + for (auto i = fv_tuple.begin(); i != fv_tuple.end(); i++) { - sai_attribute_t attr; - if (fvField(*i) == copp_trap_id_list) { trap_id_list = tokenize(fvValue(*i), list_item_delimiter); - auto it = std::find(trap_id_list.begin(), trap_id_list.end(), "sample_packet"); - if (it != trap_id_list.end()) - { - if (!enable_sflow_trap) - { - return task_process_status::task_need_retry; - } - } - } - else if (fvField(*i) == copp_queue_field) - { - attr.id = SAI_HOSTIF_TRAP_GROUP_ATTR_QUEUE; - attr.value.u32 = (uint32_t)stoul(fvValue(*i)); - trap_gr_attribs.push_back(attr); - } - // - // Trap related attributes - // - else if (fvField(*i) == copp_trap_action_field) - { - sai_packet_action_t trap_action = packet_action_map.at(fvValue(*i)); - attr.id = SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION; - attr.value.s32 = trap_action; - trap_id_attribs.push_back(attr); - } - else if (fvField(*i) == copp_trap_priority_field) - { - /* Mellanox platform doesn't support trap priority setting */ - /* Marvell platform doesn't support trap priority. */ - char *platform = getenv("platform"); - if (!platform || (!strstr(platform, MLNX_PLATFORM_SUBSTRING) && (!strstr(platform, MRVL_PLATFORM_SUBSTRING)))) - { - attr.id = SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY, - attr.value.u32 = (uint32_t)stoul(fvValue(*i)); - trap_id_attribs.push_back(attr); - } - } - // - // process policer attributes - // - else if (fvField(*i) == copp_policer_meter_type_field) - { - sai_meter_type_t meter_value = policer_meter_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_METER_TYPE; - attr.value.s32 = meter_value; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_mode_field) - { - sai_policer_mode_t mode = policer_mode_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_MODE; - attr.value.s32 = mode; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_color_field) - { - sai_policer_color_source_t color = policer_color_aware_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_COLOR_SOURCE; - attr.value.s32 = color; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_cbs_field) - { - attr.id = SAI_POLICER_ATTR_CBS; - attr.value.u64 = stoul(fvValue(*i)); - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_cir_field) - { - attr.id = SAI_POLICER_ATTR_CIR; - attr.value.u64 = stoul(fvValue(*i)); - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_pbs_field) - { - attr.id = SAI_POLICER_ATTR_PBS; - attr.value.u64 = stoul(fvValue(*i)); - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_pir_field) - { - attr.id = SAI_POLICER_ATTR_PIR; - attr.value.u64 = stoul(fvValue(*i)); - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_action_green_field) - { - sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_GREEN_PACKET_ACTION; - attr.value.s32 = policer_action; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_action_red_field) - { - sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_RED_PACKET_ACTION; - attr.value.s32 = policer_action; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_policer_action_yellow_field) - { - sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); - attr.id = SAI_POLICER_ATTR_YELLOW_PACKET_ACTION; - attr.value.s32 = policer_action; - policer_attribs.push_back(attr); - } - else if (fvField(*i) == copp_genetlink_name) - { - attr.id = SAI_HOSTIF_ATTR_TYPE; - attr.value.s32 = SAI_HOSTIF_TYPE_GENETLINK; - genetlink_attribs.push_back(attr); - - attr.id = SAI_HOSTIF_ATTR_NAME; - strncpy(attr.value.chardata, fvValue(*i).c_str(), - sizeof(attr.value.chardata)); - genetlink_attribs.push_back(attr); - - } - else if (fvField(*i) == copp_genetlink_mcgrp_name) - { - attr.id = SAI_HOSTIF_ATTR_GENETLINK_MCGRP_NAME; - strncpy(attr.value.chardata, fvValue(*i).c_str(), - sizeof(attr.value.chardata)); - genetlink_attribs.push_back(attr); - } - else - { - SWSS_LOG_ERROR("Unknown copp field specified:%s\n", fvField(*i).c_str()); - return task_process_status::task_invalid_entry; + getTrapIdList(trap_id_list, trap_ids); + getTrapAddandRemoveList(trap_group_name, trap_ids, add_trap_ids, rem_trap_ids); } } + if (!getAttribsFromTrapGroup(fv_tuple, trap_gr_attribs, trap_id_attribs, + policer_attribs, genetlink_attribs)) + { + return task_process_status::task_invalid_entry; + } + /* Set host interface trap group */ if (m_trap_group_map.find(trap_group_name) != m_trap_group_map.end()) { - /* Create or set policer */ - if (!policer_attribs.empty()) - { - sai_object_id_t policer_id = getPolicer(trap_group_name); - if (SAI_NULL_OBJECT_ID == policer_id) - { - SWSS_LOG_WARN("Creating policer for existing Trap group:%" PRIx64 " (name:%s).", m_trap_group_map[trap_group_name], trap_group_name.c_str()); - if (!createPolicer(trap_group_name, policer_attribs)) - { - return task_process_status::task_failed; - } - SWSS_LOG_DEBUG("Created policer:%" PRIx64 " for existing trap group", policer_id); - } - else - { - /* TODO: We should really only set changed attributes. - The changes need to detected either by orch agent submodule or by the orch agent framework. */ - for (sai_uint32_t ind = 0; ind < policer_attribs.size(); ind++) - { - auto policer_attr = policer_attribs[ind]; - sai_status = sai_policer_api->set_policer_attribute(policer_id, &policer_attr); - if (sai_status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to apply attribute[%d].id=%d to policer for trap group:%s, error:%d\n", ind, policer_attr.id, trap_group_name.c_str(), sai_status); - return task_process_status::task_failed; - } - } - } - } - for (sai_uint32_t ind = 0; ind < trap_gr_attribs.size(); ind++) { auto trap_gr_attr = trap_gr_attribs[ind]; @@ -669,54 +516,69 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer) SWSS_LOG_NOTICE("Create host interface trap group %s", trap_group_name.c_str()); m_trap_group_map[trap_group_name] = new_trap; + } - /* Create policer */ - if (!policer_attribs.empty()) + if (!policer_attribs.empty()) + { + if (!trapGroupUpdatePolicer(trap_group_name, policer_attribs)) { - if (!createPolicer(trap_group_name, policer_attribs)) - { - return task_process_status::task_failed; - } + return task_process_status::task_failed; } - - if (!genetlink_attribs.empty()) + } + if (!trap_id_attribs.empty()) + { + vector group_trap_ids; + TrapIdAttribs trap_attr; + getTrapIdsFromTrapGroup(m_trap_group_map[trap_group_name], + group_trap_ids); + for (auto trap_id : group_trap_ids) { - if (!createGenetlinkHostIf(trap_group_name, genetlink_attribs)) + for (auto i: trap_id_attribs) { - return task_process_status::task_failed; + sai_status = sai_hostif_api->set_hostif_trap_attribute( + m_syncdTrapIds[trap_id].trap_obj, &i); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set attribute %d on trap %" PRIx64 "" + " on group %s", i.id, m_syncdTrapIds[trap_id].trap_obj, + trap_group_name.c_str()); + return task_process_status::task_failed; + } } } + for (auto i: trap_id_attribs) + { + trap_attr[i.id] = i.value; + } + m_trap_group_trap_id_attrs[trap_group_name] = trap_attr; } - - /* Apply traps to trap group */ - if (!applyTrapIds(m_trap_group_map[trap_group_name], trap_id_list, trap_id_attribs)) - { - return task_process_status::task_failed; - } - if (!genetlink_attribs.empty()) { - if (!createGenetlinkHostIfTable(trap_id_list)) + if (m_trap_group_hostif_map.find(m_trap_group_map[trap_group_name]) != + m_trap_group_hostif_map.end()) + { + SWSS_LOG_ERROR("Genetlink hostif exists for the trap group %s", + trap_group_name.c_str()); + return task_process_status::task_failed; + } + vector genetlink_trap_ids; + getTrapIdsFromTrapGroup(m_trap_group_map[trap_group_name], genetlink_trap_ids); + if (!createGenetlinkHostIf(trap_group_name, genetlink_attribs)) + { + return task_process_status::task_failed; + } + if (!createGenetlinkHostIfTable(genetlink_trap_ids)) { return task_process_status::task_failed; } } - } - else if (op == DEL_COMMAND) - { - /* Remove policer if any */ - if (!removePolicer(trap_group_name)) - { - SWSS_LOG_ERROR("Failed to remove policer from trap group %s", trap_group_name.c_str()); - return task_process_status::task_failed; - } - - if (!removeGenetlinkHostIf(trap_group_name)) + if (!trapGroupProcessTrapIdChange(trap_group_name, add_trap_ids, rem_trap_ids)) { - SWSS_LOG_ERROR("Failed to remove hostif from trap group %s", trap_group_name.c_str()); return task_process_status::task_failed; } - + } + else if (op == DEL_COMMAND) + { /* Do not remove default trap group */ if (trap_group_name == default_trap_group) { @@ -724,48 +586,10 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer) return task_process_status::task_ignore; } - /* Reset the trap IDs to default trap group with default attributes */ - vector trap_ids_to_reset; - for (auto it : m_syncdTrapIds) - { - if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) - { - trap_ids_to_reset.push_back(it.first); - } - sai_status = sai_hostif_api->remove_hostif_trap(it.second.trap_obj); - if (sai_status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to remove trap object %" PRId64 "", it.second.trap_obj); - return task_process_status::task_failed; - } - } - - sai_attribute_t attr; - vector default_trap_attrs; - - attr.id = SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION; - attr.value.s32 = SAI_PACKET_ACTION_FORWARD; - default_trap_attrs.push_back(attr); - - attr.id = SAI_HOSTIF_TRAP_ATTR_TRAP_GROUP; - attr.value.oid = m_trap_group_map[default_trap_group]; - default_trap_attrs.push_back(attr); - - if (!applyAttributesToTrapIds(m_trap_group_map[default_trap_group], trap_ids_to_reset, default_trap_attrs)) + if (!processTrapGroupDel(trap_group_name)) { - SWSS_LOG_ERROR("Failed to reset traps to default trap group with default attributes"); return task_process_status::task_failed; } - - sai_status = sai_hostif_api->remove_hostif_trap_group(m_trap_group_map[trap_group_name]); - if (sai_status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to remove trap group %s", trap_group_name.c_str()); - return task_process_status::task_failed; - } - - auto it_del = m_trap_group_map.find(trap_group_name); - m_trap_group_map.erase(it_del); } else { @@ -775,50 +599,11 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer) return task_process_status::task_success; } -/* Program Sflow trap once we get sflow enable command */ -void CoppOrch::coppProcessSflow(Consumer &consumer) -{ - auto it = consumer.m_toSync.begin(); - - while (it != consumer.m_toSync.end()) - { - auto tuple = it->second; - string op = kfvOp(tuple); - - /* - * Need to handled just 'config sflow enable' command to install the sflow trap group - * for the first time to ensure support of genetlink attributes. Rest of the fields or - * disable value or DEL command are not required to be handled - * - */ - if (op == SET_COMMAND) - { - for (auto i : kfvFieldsValues(tuple)) - { - if (fvField(i) == "admin_state") - { - if (fvValue(i) == "up") - { - enable_sflow_trap = true; - } - } - } - } - it = consumer.m_toSync.erase(it); - } -} - void CoppOrch::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); string table_name = consumer.getTableName(); - if (table_name == CFG_SFLOW_TABLE_NAME) - { - coppProcessSflow(consumer); - return; - } - if (!gPortsOrch->allPortsReady()) { return; @@ -869,3 +654,516 @@ void CoppOrch::doTask(Consumer &consumer) } } } + +/* Check if the Copp Group name is different while all trap ids remain the same + * This is going to be warm boot scenario from an old image which has different naming scheme + */ +bool CoppOrch::checkDupCoppGrpNameAndUpdate(string new_copp_grp_name, + vector &fv_tuple) +{ + vector trap_id_list; + sai_object_id_t trap_group_obj = SAI_NULL_OBJECT_ID; + vector mod_fv_tuple; + vector trap_ids; + bool ret = false; + + if (m_trap_group_map.find(new_copp_grp_name) != m_trap_group_map.end()) + { + return false; + } + + for (auto i = fv_tuple.begin(); i != fv_tuple.end(); i++) + { + if (fvField(*i) == copp_trap_id_list) + { + trap_id_list = tokenize(fvValue(*i), list_item_delimiter); + getTrapIdList(trap_id_list, trap_ids); + } + } + + /* In the case where NAT is not supported, there will be no trap IDs associated with + * the nat trap group and hence group ID cannot be figured out by iterating through traps. + * Using the hardcoded name for this specific case + */ + if (trap_ids.size() == 0) + { + if (!gIsNatSupported) + { + auto src_nat = std::find(trap_id_list.begin(), trap_id_list.end(), "src_nat_miss"); + auto dst_nat = std::find(trap_id_list.begin(), trap_id_list.end(), "dest_nat_miss"); + if ((src_nat != trap_id_list.end()) && (dst_nat != trap_id_list.end())) + { + if(m_trap_group_map.find(old_nat_group_name) != m_trap_group_map.end()) + { + trap_group_obj = m_trap_group_map[old_nat_group_name]; + } + else + { + return false; + } + } + else + { + return false; + } + } + else + { + return false; + } + } + else + { + for(auto it = trap_ids.begin(); it != trap_ids.end(); it++) + { + if (m_syncdTrapIds.find(*it) == m_syncdTrapIds.end()) + { + return false; + } + sai_object_id_t trap_id_group = m_syncdTrapIds[*it].trap_group_obj; + if (trap_id_group == SAI_NULL_OBJECT_ID) + { + return false; + } + if (trap_group_obj == SAI_NULL_OBJECT_ID) + { + trap_group_obj = trap_id_group; + } + else if (trap_group_obj != trap_id_group) + { + return false; + } + } + } + + for (auto it : m_trap_group_map) + { + if (it.second == trap_group_obj) + { + vector old_tuples; + vector new_tuples; + m_coppTable->get(it.first, old_tuples); + m_coppTable->get(new_copp_grp_name, new_tuples); + vector tmp_tuples = new_tuples; + for (auto o: old_tuples) + { + if (fvField(o) == copp_trap_id_list) + { + continue; + } + bool not_found = true; + for (auto n : new_tuples) + { + if (fvField(o) == fvField(n)) + { + if (fvValue(o) != fvValue(n)) + { + mod_fv_tuple.push_back(n); + } + auto vec_idx = std::find(tmp_tuples.begin(), tmp_tuples.end(), n); + tmp_tuples.erase(vec_idx); + not_found = false; + } + } + /* Old table has a field which is not present in new table + * Delete the entry and return false so it is recreated + */ + if (not_found) + { + processTrapGroupDel(it.first); + m_coppTable->del(it.first); + return false; + } + } + if (!tmp_tuples.empty()) + { + mod_fv_tuple.insert(mod_fv_tuple.end(), tmp_tuples.begin(), tmp_tuples.end()); + } + if (!mod_fv_tuple.empty()) + { + fv_tuple = mod_fv_tuple; + } + else + { + ret = true; + } + m_trap_group_map[new_copp_grp_name] = it.second; + m_trap_group_map.erase(m_trap_group_map.find(it.first)); + m_coppTable->del(it.first); + break; + } + } + return ret; +} + +void CoppOrch::getTrapAddandRemoveList(string trap_group_name, + vector &trap_ids, + vector &add_trap_ids, + vector &rem_trap_ids) +{ + + vector tmp_trap_ids = trap_ids; + if(m_trap_group_map.find(trap_group_name) == m_trap_group_map.end()) + { + add_trap_ids = trap_ids; + rem_trap_ids.clear(); + return; + } + + + for (auto it : m_syncdTrapIds) + { + if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) + { + /* If new trap list contains already mapped ID remove it */ + auto i = std::find(std::begin(tmp_trap_ids), std::end(tmp_trap_ids), it.first); + + if (i != std::end(tmp_trap_ids)) + { + tmp_trap_ids.erase(i); + } + /* The mapped Trap ID is not found on newly set list and to be removed*/ + else + { + if ((trap_group_name != default_trap_group) || + ((trap_group_name == default_trap_group) && + (it.first != SAI_HOSTIF_TRAP_TYPE_TTL_ERROR))) + { + rem_trap_ids.push_back(it.first); + } + } + } + } + + add_trap_ids = tmp_trap_ids; +} + +void CoppOrch::getTrapIdsFromTrapGroup (sai_object_id_t trap_group_obj, + vector &trap_ids) +{ + for(auto it: m_syncdTrapIds) + { + if (it.second.trap_group_obj == trap_group_obj) + { + trap_ids.push_back(it.first); + } + } +} + +bool CoppOrch::trapGroupProcessTrapIdChange (string trap_group_name, + vector &add_trap_ids, + vector &rem_trap_ids) +{ + if (!add_trap_ids.empty()) + { + sai_attribute_t attr; + vector add_trap_attr; + attr.id = SAI_HOSTIF_TRAP_ATTR_TRAP_GROUP; + attr.value.oid = m_trap_group_map[trap_group_name]; + + add_trap_attr.push_back(attr); + + for(auto i: add_trap_ids) + { + if (m_syncdTrapIds.find(i)!= m_syncdTrapIds.end()) + { + sai_status_t sai_status = sai_hostif_api->remove_hostif_trap( + m_syncdTrapIds[i].trap_obj); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove trap object %" PRId64 "", + m_syncdTrapIds[i].trap_obj); + return false; + } + } + } + + for (auto it: m_trap_group_trap_id_attrs[trap_group_name]) + { + attr.id = it.first; + attr.value = it.second; + add_trap_attr.push_back(attr); + } + if (!applyAttributesToTrapIds(m_trap_group_map[trap_group_name], add_trap_ids, + add_trap_attr)) + { + SWSS_LOG_ERROR("Failed to set traps to trap group %s", trap_group_name.c_str()); + return false; + } + if (m_trap_group_hostif_map.find(m_trap_group_map[trap_group_name]) != + m_trap_group_hostif_map.end()) + { + if (!createGenetlinkHostIfTable(add_trap_ids)) + { + return task_process_status::task_failed; + } + } + } + if (!rem_trap_ids.empty()) + { + for (auto i: rem_trap_ids) + { + if (m_syncdTrapIds.find(i)!= m_syncdTrapIds.end()) + { + /* + * A trap ID will be present in rem_trap_id in two scenarios + * 1) When trap group for a trap ID is changed + * 2) When trap ID is completely removed + * In case 1 the first call would be to add the trap ids to a different + * group. This would result in changing the mapping of trap id to trap group + * In case 2 the mapping will remain the same. In this case the trap + * object needs to be deleted + */ + if (m_syncdTrapIds[i].trap_group_obj == m_trap_group_map[trap_group_name]) + { + sai_status_t sai_status = sai_hostif_api->remove_hostif_trap( + m_syncdTrapIds[i].trap_obj); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove trap object %" PRId64 "", + m_syncdTrapIds[i].trap_obj); + return false; + } + m_syncdTrapIds.erase(i); + } + } + } + if (!removeGenetlinkHostIfTable(rem_trap_ids)) + { + return false; + } + } + return true; +} + +bool CoppOrch::processTrapGroupDel (string trap_group_name) +{ + auto it_del = m_trap_group_map.find(trap_group_name); + + if (it_del == m_trap_group_map.end()) + { + return true; + } + /* Remove policer if any */ + if (!removePolicer(trap_group_name)) + { + SWSS_LOG_ERROR("Failed to remove policer from trap group %s", trap_group_name.c_str()); + return false; + } + + if (!removeGenetlinkHostIf(trap_group_name)) + { + SWSS_LOG_ERROR("Failed to remove hostif from trap group %s", trap_group_name.c_str()); + return false; + } + + /* Reset the trap IDs to default trap group with default attributes */ + vector trap_ids_to_reset; + for (auto it : m_syncdTrapIds) + { + if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) + { + trap_ids_to_reset.push_back(it.first); + sai_status_t sai_status = sai_hostif_api->remove_hostif_trap(it.second.trap_obj); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove trap object %" PRId64 "", it.second.trap_obj); + return false; + } + } + } + + for (auto it: trap_ids_to_reset) + { + m_syncdTrapIds.erase(it); + } + + sai_status_t sai_status = sai_hostif_api->remove_hostif_trap_group( + m_trap_group_map[trap_group_name]); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove trap group %s", trap_group_name.c_str()); + return false; + } + + m_trap_group_map.erase(it_del); + return true; +} + +bool CoppOrch::getAttribsFromTrapGroup (vector &fv_tuple, + vector &trap_gr_attribs, + vector &trap_id_attribs, + vector &policer_attribs, + vector &genetlink_attribs) +{ + sai_attribute_t attr; + + for (auto i = fv_tuple.begin(); i != fv_tuple.end(); i++) + { + if (fvField(*i) == copp_trap_id_list) + { + continue; + } + else if (fvField(*i) == copp_queue_field) + { + attr.id = SAI_HOSTIF_TRAP_GROUP_ATTR_QUEUE; + attr.value.u32 = (uint32_t)stoul(fvValue(*i)); + trap_gr_attribs.push_back(attr); + } + // + // Trap related attributes + // + else if (fvField(*i) == copp_trap_action_field) + { + sai_packet_action_t trap_action = packet_action_map.at(fvValue(*i)); + attr.id = SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION; + attr.value.s32 = trap_action; + trap_id_attribs.push_back(attr); + } + else if (fvField(*i) == copp_trap_priority_field) + { + /* Mellanox platform doesn't support trap priority setting */ + /* Marvell platform doesn't support trap priority. */ + char *platform = getenv("platform"); + if (!platform || (!strstr(platform, MLNX_PLATFORM_SUBSTRING) && (!strstr(platform, MRVL_PLATFORM_SUBSTRING)))) + { + attr.id = SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY, + attr.value.u32 = (uint32_t)stoul(fvValue(*i)); + trap_id_attribs.push_back(attr); + } + } + // + // process policer attributes + // + else if (fvField(*i) == copp_policer_meter_type_field) + { + sai_meter_type_t meter_value = policer_meter_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_METER_TYPE; + attr.value.s32 = meter_value; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_mode_field) + { + sai_policer_mode_t mode = policer_mode_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_MODE; + attr.value.s32 = mode; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_color_field) + { + sai_policer_color_source_t color = policer_color_aware_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_COLOR_SOURCE; + attr.value.s32 = color; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_cbs_field) + { + attr.id = SAI_POLICER_ATTR_CBS; + attr.value.u64 = stoul(fvValue(*i)); + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_cir_field) + { + attr.id = SAI_POLICER_ATTR_CIR; + attr.value.u64 = stoul(fvValue(*i)); + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_pbs_field) + { + attr.id = SAI_POLICER_ATTR_PBS; + attr.value.u64 = stoul(fvValue(*i)); + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_pir_field) + { + attr.id = SAI_POLICER_ATTR_PIR; + attr.value.u64 = stoul(fvValue(*i)); + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_action_green_field) + { + sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_GREEN_PACKET_ACTION; + attr.value.s32 = policer_action; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_action_red_field) + { + sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_RED_PACKET_ACTION; + attr.value.s32 = policer_action; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_policer_action_yellow_field) + { + sai_packet_action_t policer_action = packet_action_map.at(fvValue(*i)); + attr.id = SAI_POLICER_ATTR_YELLOW_PACKET_ACTION; + attr.value.s32 = policer_action; + policer_attribs.push_back(attr); + } + else if (fvField(*i) == copp_genetlink_name) + { + attr.id = SAI_HOSTIF_ATTR_TYPE; + attr.value.s32 = SAI_HOSTIF_TYPE_GENETLINK; + genetlink_attribs.push_back(attr); + + attr.id = SAI_HOSTIF_ATTR_NAME; + strncpy(attr.value.chardata, fvValue(*i).c_str(), + sizeof(attr.value.chardata)); + genetlink_attribs.push_back(attr); + + } + else if (fvField(*i) == copp_genetlink_mcgrp_name) + { + attr.id = SAI_HOSTIF_ATTR_GENETLINK_MCGRP_NAME; + strncpy(attr.value.chardata, fvValue(*i).c_str(), + sizeof(attr.value.chardata)); + genetlink_attribs.push_back(attr); + } + else + { + SWSS_LOG_ERROR("Unknown copp field specified:%s\n", fvField(*i).c_str()); + return false; + } + } + return true; +} + +bool CoppOrch::trapGroupUpdatePolicer (string trap_group_name, + vector &policer_attribs) +{ + sai_object_id_t policer_id = getPolicer(trap_group_name); + + if (m_trap_group_map.find(trap_group_name) == m_trap_group_map.end()) + { + return false; + } + if (SAI_NULL_OBJECT_ID == policer_id) + { + SWSS_LOG_WARN("Creating policer for existing Trap group: %" PRIx64 " (name:%s).", + m_trap_group_map[trap_group_name], trap_group_name.c_str()); + if (!createPolicer(trap_group_name, policer_attribs)) + { + return false; + } + SWSS_LOG_DEBUG("Created policer:%" PRIx64 " for existing trap group", policer_id); + } + else + { + for (sai_uint32_t ind = 0; ind < policer_attribs.size(); ind++) + { + auto policer_attr = policer_attribs[ind]; + sai_status_t sai_status = sai_policer_api->set_policer_attribute(policer_id, + &policer_attr); + if (sai_status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to apply attribute[%d].id=%d to policer for trap group:" + "%s, error:%d\n",ind, policer_attr.id, trap_group_name.c_str(), + sai_status); + return false; + } + } + } + return true; +} + diff --git a/orchagent/copporch.h b/orchagent/copporch.h index c46ac6e62e..d75571b327 100644 --- a/orchagent/copporch.h +++ b/orchagent/copporch.h @@ -28,6 +28,7 @@ const std::string copp_policer_action_yellow_field = "yellow_action"; const std::string copp_genetlink_name = "genetlink_name"; const std::string copp_genetlink_mcgrp_name = "genetlink_mcgrp_name"; +typedef std::map TrapIdAttribs; struct copp_trap_objects { sai_object_id_t trap_obj; @@ -42,20 +43,24 @@ typedef std::map TrapIdTrapObjectsTab typedef std::map TrapGroupHostIfMap; /* TrapIdHostIfTableMap: trap type, host table entry ID*/ typedef std::map TrapIdHostIfTableMap; +/* Trap group to trap ID attributes */ +typedef std::map TrapGroupTrapIdAttribs; class CoppOrch : public Orch { public: - CoppOrch(std::vector &tableConnectors); + CoppOrch(swss::DBConnector* db, std::string tableName); protected: object_map m_trap_group_map; bool enable_sflow_trap; + std::unique_ptr m_coppTable; TrapGroupPolicerTable m_trap_group_policer_map; TrapIdTrapObjectsTable m_syncdTrapIds; TrapGroupHostIfMap m_trap_group_hostif_map; TrapIdHostIfTableMap m_trapid_hostif_table_map; + TrapGroupTrapIdAttribs m_trap_group_trap_id_attrs; void initDefaultHostIntfTable(); void initDefaultTrapGroup(); @@ -64,7 +69,6 @@ class CoppOrch : public Orch task_process_status processCoppRule(Consumer& consumer); bool isValidList(std::vector &trap_id_list, std::vector &all_items) const; void getTrapIdList(std::vector &trap_id_name_list, std::vector &trap_id_list) const; - bool applyTrapIds(sai_object_id_t trap_group, std::vector &trap_id_name_list, std::vector &trap_id_attribs); bool applyAttributesToTrapIds(sai_object_id_t trap_group_id, const std::vector &trap_id_list, std::vector &trap_id_attribs); bool createPolicer(std::string trap_group, std::vector &policer_attribs); @@ -74,8 +78,30 @@ class CoppOrch : public Orch bool createGenetlinkHostIf(std::string trap_group_name, std::vector &hostif_attribs); bool removeGenetlinkHostIf(std::string trap_group_name); - bool createGenetlinkHostIfTable(std::vector &trap_id_name_list); - void coppProcessSflow(Consumer& consumer); + bool createGenetlinkHostIfTable(std::vector &trap_id_list); + bool removeGenetlinkHostIfTable(std::vector &trap_id_list); + bool checkDupCoppGrpNameAndUpdate(std::string new_copp_grp_name, + std::vector &fv_tuple); + void getTrapAddandRemoveList(std::string trap_group_name, std::vector &trap_ids, + std::vector &add_trap_ids, + std::vector &rem_trap_ids); + + void getTrapIdsFromTrapGroup (sai_object_id_t trap_group_obj, + std::vector &trap_ids); + + bool trapGroupProcessTrapIdChange (std::string trap_group_name, + std::vector &add_trap_ids, + std::vector &rem_trap_ids); + + bool processTrapGroupDel (std::string trap_group_name); + + bool getAttribsFromTrapGroup (std::vector &fv_tuple, + std::vector &trap_gr_attribs, + std::vector &trap_id_attribs, + std::vector &policer_attribs, + std::vector &genetlink_attribs); + + bool trapGroupUpdatePolicer (std::string trap_group_name, std::vector &policer_attribs); virtual void doTask(Consumer& consumer); }; diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index 26ff1f0301..6aa61ee607 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -126,15 +126,7 @@ bool OrchDaemon::init() gIntfsOrch = new IntfsOrch(m_applDb, APP_INTF_TABLE_NAME, vrf_orch); gNeighOrch = new NeighOrch(m_applDb, APP_NEIGH_TABLE_NAME, gIntfsOrch); gRouteOrch = new RouteOrch(m_applDb, APP_ROUTE_TABLE_NAME, gNeighOrch, gIntfsOrch, vrf_orch); - - TableConnector confDbSflowTable(m_configDb, CFG_SFLOW_TABLE_NAME); - TableConnector appCoppTable(m_applDb, APP_COPP_TABLE_NAME); - - vector copp_table_connectors = { - confDbSflowTable, - appCoppTable - }; - CoppOrch *copp_orch = new CoppOrch(copp_table_connectors); + CoppOrch *copp_orch = new CoppOrch(m_applDb, APP_COPP_TABLE_NAME); TunnelDecapOrch *tunnel_decap_orch = new TunnelDecapOrch(m_applDb, APP_TUNNEL_DECAP_TABLE_NAME); VxlanTunnelOrch *vxlan_tunnel_orch = new VxlanTunnelOrch(m_applDb, APP_VXLAN_TUNNEL_TABLE_NAME); diff --git a/tests/test_copp.py b/tests/test_copp.py new file mode 100644 index 0000000000..ed009728d2 --- /dev/null +++ b/tests/test_copp.py @@ -0,0 +1,674 @@ +import time +import os +import pytest + +from swsscommon import swsscommon + + +field_to_sai_attr = { + "queue": "SAI_HOSTIF_TRAP_GROUP_ATTR_QUEUE", + "meter_type": "SAI_POLICER_ATTR_METER_TYPE", + "mode": "SAI_POLICER_ATTR_MODE", + "trap_action": "SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION", + "trap_priority": "SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY", + "cbs": "SAI_POLICER_ATTR_CBS", + "cir": "SAI_POLICER_ATTR_CIR", + "red_action": "SAI_POLICER_ATTR_RED_PACKET_ACTION" +} + +field_to_sai_obj_type = { + "queue": "SAI_OBJECT_TYPE_HOSTIF_TRAP_GROUP", + "meter_type": "SAI_OBJECT_TYPE_POLICER", + "mode": "SAI_OBJECT_TYPE_POLICER", + "trap_action": "SAI_OBJECT_TYPE_HOSTIF_TRAP", + "trap_priority": "SAI_OBJECT_TYPE_HOSTIF_TRAP", + "cbs": "SAI_OBJECT_TYPE_POLICER", + "cir": "SAI_OBJECT_TYPE_POLICER", + "red_action": "SAI_OBJECT_TYPE_POLICER", + "genetlink_mcgrp_name": "SAI_OBJECT_TYPE_HOSTIF", + "genetlink_name": "SAI_OBJECT_TYPE_HOSTIF" +} + +traps_to_trap_type = { + "stp": "SAI_HOSTIF_TRAP_TYPE_STP", + "lacp": "SAI_HOSTIF_TRAP_TYPE_LACP", + "eapol": "SAI_HOSTIF_TRAP_TYPE_EAPOL", + "lldp": "SAI_HOSTIF_TRAP_TYPE_LLDP", + "pvrst": "SAI_HOSTIF_TRAP_TYPE_PVRST", + "igmp_query": "SAI_HOSTIF_TRAP_TYPE_IGMP_TYPE_QUERY", + "igmp_leave": "SAI_HOSTIF_TRAP_TYPE_IGMP_TYPE_LEAVE", + "igmp_v1_report": "SAI_HOSTIF_TRAP_TYPE_IGMP_TYPE_V1_REPORT", + "igmp_v2_report": "SAI_HOSTIF_TRAP_TYPE_IGMP_TYPE_V2_REPORT", + "igmp_v3_report": "SAI_HOSTIF_TRAP_TYPE_IGMP_TYPE_V3_REPORT", + "sample_packet": "SAI_HOSTIF_TRAP_TYPE_SAMPLEPACKET", + "switch_cust_range": "SAI_HOSTIF_TRAP_TYPE_SWITCH_CUSTOM_RANGE_BASE", + "arp_req": "SAI_HOSTIF_TRAP_TYPE_ARP_REQUEST", + "arp_resp": "SAI_HOSTIF_TRAP_TYPE_ARP_RESPONSE", + "dhcp": "SAI_HOSTIF_TRAP_TYPE_DHCP", + "ospf": "SAI_HOSTIF_TRAP_TYPE_OSPF", + "pim": "SAI_HOSTIF_TRAP_TYPE_PIM", + "vrrp": "SAI_HOSTIF_TRAP_TYPE_VRRP", + "bgp": "SAI_HOSTIF_TRAP_TYPE_BGP", + "dhcpv6": "SAI_HOSTIF_TRAP_TYPE_DHCPV6", + "ospfv6": "SAI_HOSTIF_TRAP_TYPE_OSPFV6", + "vrrpv6": "SAI_HOSTIF_TRAP_TYPE_VRRPV6", + "bgpv6": "SAI_HOSTIF_TRAP_TYPE_BGPV6", + "neigh_discovery": "SAI_HOSTIF_TRAP_TYPE_IPV6_NEIGHBOR_DISCOVERY", + "mld_v1_v2": "SAI_HOSTIF_TRAP_TYPE_IPV6_MLD_V1_V2", + "mld_v1_report": "SAI_HOSTIF_TRAP_TYPE_IPV6_MLD_V1_REPORT", + "mld_v1_done": "SAI_HOSTIF_TRAP_TYPE_IPV6_MLD_V1_DONE", + "mld_v2_report": "SAI_HOSTIF_TRAP_TYPE_MLD_V2_REPORT", + "ip2me": "SAI_HOSTIF_TRAP_TYPE_IP2ME", + "ssh": "SAI_HOSTIF_TRAP_TYPE_SSH", + "snmp": "SAI_HOSTIF_TRAP_TYPE_SNMP", + "router_custom_range": "SAI_HOSTIF_TRAP_TYPE_ROUTER_CUSTOM_RANGE_BASE", + "l3_mtu_error": "SAI_HOSTIF_TRAP_TYPE_L3_MTU_ERROR", + "ttl_error": "SAI_HOSTIF_TRAP_TYPE_TTL_ERROR", + "udld": "SAI_HOSTIF_TRAP_TYPE_UDLD", + "bfd": "SAI_HOSTIF_TRAP_TYPE_BFD", + "bfdv6": "SAI_HOSTIF_TRAP_TYPE_BFDV6", + "src_nat_miss": "SAI_HOSTIF_TRAP_TYPE_SNAT_MISS", + "dest_nat_miss": "SAI_HOSTIF_TRAP_TYPE_DNAT_MISS" + } + +copp_group_default = { + "queue": "0", + "meter_type":"packets", + "mode":"sr_tcm", + "cir":"600", + "cbs":"600", + "red_action":"drop" +} + +copp_group_queue4_group1 = { + "trap_action":"trap", + "trap_priority":"4", + "queue": "4" +} + +copp_group_queue4_group2 = { + "trap_action":"copy", + "trap_priority":"4", + "queue": "4", + "meter_type":"packets", + "mode":"sr_tcm", + "cir":"600", + "cbs":"600", + "red_action":"drop" +} + +copp_group_queue4_group3 = { + "trap_action":"trap", + "trap_priority":"4", + "queue": "4" +} + +copp_group_queue1_group1 = { + "trap_action":"trap", + "trap_priority":"1", + "queue": "1", + "meter_type":"packets", + "mode":"sr_tcm", + "cir":"6000", + "cbs":"6000", + "red_action":"drop" +} + +copp_group_queue1_group2 = { + "trap_action":"trap", + "trap_priority":"1", + "queue": "1", + "meter_type":"packets", + "mode":"sr_tcm", + "cir":"600", + "cbs":"600", + "red_action":"drop" +} + +copp_group_queue2_group1 = { + "cbs": "1000", + "cir": "1000", + "genetlink_mcgrp_name": "packets", + "genetlink_name": "psample", + "meter_type": "packets", + "mode": "sr_tcm", + "queue": "2", + "red_action": "drop", + "trap_action": "trap", + "trap_priority": "1" +} + +copp_group_queue5_group1 = { + "cbs": "2000", + "cir": "2000", + "meter_type": "packets", + "mode": "sr_tcm", + "queue": "5", + "red_action": "drop", + "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 +} + +restricted_traps = ["sample_packet"] + +policer_meter_map = { + "packets": "SAI_METER_TYPE_PACKETS", + "bytes": "SAI_METER_TYPE_BYTES" +} + +policer_mode_map = { + "sr_tcm": "SAI_POLICER_MODE_SR_TCM", + "tr_tcm": "SAI_POLICER_MODE_TR_TCM", + "storm": "SAI_POLICER_MODE_STORM_CONTROL" +} + + +packet_action_map = { + "drop": "SAI_PACKET_ACTION_DROP", + "forward": "SAI_PACKET_ACTION_FORWARD", + "copy": "SAI_PACKET_ACTION_COPY", + "copy_cancel": "SAI_PACKET_ACTION_COPY_CANCEL", + "trap": "SAI_PACKET_ACTION_TRAP", + "log": "SAI_PACKET_ACTION_LOG", + "deny": "SAI_PACKET_ACTION_DENY", + "transit": "SAI_PACKET_ACTION_TRANSIT" +} + +class TestCopp(object): + def setup_copp(self, dvs): + self.adb = swsscommon.DBConnector(1, dvs.redis_sock, 0) + self.cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0) + self.trap_atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF_TRAP") + self.trap_group_atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF_TRAP_GROUP") + self.policer_atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_POLICER") + self.hostiftbl_atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF_TABLE_ENTRY") + self.hostif_atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF") + 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") + + def validate_policer(self, policer_oid, field, value): + (status, fvs) = self.policer_atbl.get(policer_oid) + assert status == True + attr = field_to_sai_attr[field] + + attr_value = value + if field == "mode": + attr_value = policer_mode_map[value] + elif field == "meter_type": + attr_value = policer_meter_map[value] + elif field == "red_action": + attr_value = packet_action_map[value] + + for fv in fvs: + if (fv[0] == attr): + assert attr_value == fv[1] + + def validate_trap_group(self, trap_oid, trap_group): + (status, trap_fvs) = self.trap_atbl.get(trap_oid) + assert status == True + trap_group_oid = "" + policer_oid= "" + queue = "" + trap_action = "" + trap_priority = "" + + for fv in trap_fvs: + if fv[0] == "SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION": + trap_action = fv[1] + elif fv[0] == "SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY": + trap_priority = fv[1] + elif fv[0] == "SAI_HOSTIF_TRAP_ATTR_TRAP_GROUP": + trap_group_oid = fv[1] + + if trap_group_oid != "" and trap_group_oid != "oid:0x0": + (status, fvs) = self.trap_group_atbl.get(trap_group_oid) + assert status == True + for fv in fvs: + if fv[0] == "SAI_HOSTIF_TRAP_GROUP_ATTR_POLICER": + policer_oid = fv[1] + elif fv[0] == "SAI_HOSTIF_TRAP_GROUP_ATTR_QUEUE": + queue = fv[1] + + for keys in trap_group: + obj_type = field_to_sai_obj_type[keys] + if obj_type == "SAI_OBJECT_TYPE_POLICER": + assert policer_oid != "" + assert policer_oid != "oid:0x0" + self.validate_policer(policer_oid, keys, trap_group[keys]) + + elif obj_type == "SAI_OBJECT_TYPE_HOSTIF_TRAP_GROUP": + assert trap_group_oid != "" + assert trap_group_oid != "oid:0x0" + if keys == "queue": + assert queue == trap_group[keys] + else: + assert 0 + + elif obj_type == "SAI_OBJECT_TYPE_HOSTIF_TRAP": + if keys == "trap_action": + assert trap_action == packet_action_map[trap_group[keys]] + elif keys == "trap_priority": + assert trap_priority == trap_group[keys] + + elif obj_type == "SAI_OBJECT_TYPE_HOSTIF": + host_tbl_keys = self.hostiftbl_atbl.getKeys(); + host_tbl_key = None + for host_tbl_entry in host_tbl_keys: + (status, fvs) = self.hostiftbl_atbl.get(host_tbl_entry) + assert status == True + for fv in fvs: + if fv[0] == "SAI_HOSTIF_TABLE_ENTRY_ATTR_TRAP_ID": + if fv[1] == trap_oid: + host_tbl_key = host_tbl_entry + break + if host_tbl_key != None: + break + assert host_tbl_key != None + (status, fvs) = self.hostiftbl_atbl.get(host_tbl_key) + hostif = None + for fv in fvs: + if fv[0] == "SAI_HOSTIF_TABLE_ENTRY_ATTR_HOST_IF": + hostif = fv[1] + elif fv[0] == "SAI_HOSTIF_TABLE_ENTRY_ATTR_CHANNEL_TYPE": + assert fv[1] == "SAI_HOSTIF_TABLE_ENTRY_CHANNEL_TYPE_GENETLINK" + assert hostif != None + (status, fvs) = self.hostif_atbl.get(hostif) + assert status == True + for fv in fvs: + if keys == "genetlink_mcgrp_name": + if fv[0] == "SAI_HOSTIF_ATTR_GENETLINK_MCGRP_NAME": + assert fv[1] == trap_group[keys] + if keys == "genetlink_name": + if fv[0] == "SAI_HOSTIF_ATTR_NAME": + assert fv[1] == trap_group[keys] + + + + 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] + for trap_id in trap_ids: + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_restricted_trap_sflow(self, dvs, testlog): + self.setup_copp(dvs) + fvs = swsscommon.FieldValuePairs([("status", "enabled")]) + self.feature_tbl.set("sflow", fvs) + time.sleep(2) + global copp_trap + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + trap_ids = traps.split(",") + 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: + (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 + + + def test_policer_set(self, dvs, testlog): + self.setup_copp(dvs) + fvs = swsscommon.FieldValuePairs([("cbs", "900")]) + self.trap_group_ctbl.set("queue4_group2", fvs) + copp_group_queue4_group2["cbs"] = "900" + time.sleep(2) + global copp_trap + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + if copp_trap[traps] != 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_trap_group_set(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "bgp,bgpv6" + fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group1")]) + self.trap_ctbl.set("bgp", fvs) + copp_trap[traps] = copp_group_queue1_group1 + time.sleep(2) + + trap_keys = self.trap_atbl.getKeys() + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_trap_action_set(self, dvs, testlog): + self.setup_copp(dvs) + fvs = swsscommon.FieldValuePairs([("trap_action", "copy")]) + self.trap_group_ctbl.set("queue4_group1", fvs) + copp_group_queue4_group1["trap_action"] = "copy" + time.sleep(2) + global copp_trap + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + if copp_trap[traps] != copp_group_queue4_group1: + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_new_trap_add(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "eapol" + fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group2"),("trap_ids", "eapol")]) + self.trap_ctbl.set(traps, fvs) + copp_trap[traps] = copp_group_queue1_group2 + time.sleep(2) + + trap_keys = self.trap_atbl.getKeys() + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_new_trap_del(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "eapol" + fvs = swsscommon.FieldValuePairs([("trap_group", "queue1_group2"),("trap_ids", "eapol")]) + self.trap_ctbl.set(traps, fvs) + copp_trap[traps] = 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_keys = self.trap_atbl.getKeys() + for trap_id in trap_ids: + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == False + + def test_new_trap_group_add(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "igmp_v1_report" + list_val = list(copp_group_queue5_group1.items()) + + 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")]) + self.trap_ctbl.set(traps, t_fvs) + copp_trap[traps] = copp_group_queue5_group1 + time.sleep(2) + + trap_keys = self.trap_atbl.getKeys() + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_new_trap_group_del(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "igmp_v1_report" + list_val = list(copp_group_queue5_group1.items()) + + 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")]) + self.trap_ctbl.set(traps, t_fvs) + copp_trap[traps] = 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] + for trap_id in trap_ids: + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found != True + + def test_init_group_del (self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + fvs = swsscommon.FieldValuePairs([("cbs", "500"),("cir","500")]) + self.trap_group_ctbl.set("queue1_group1", fvs) + time.sleep(2) + + + self.trap_group_ctbl._del("queue1_group1") + time.sleep(2) + + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + if copp_trap[traps] != copp_group_queue1_group1: + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + def test_init_group_del (self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + fvs = swsscommon.FieldValuePairs([("cbs", "500"),("cir","500")]) + self.trap_group_ctbl.set("queue1_group1", fvs) + time.sleep(2) + + + self.trap_group_ctbl._del("queue1_group1") + time.sleep(2) + + + trap_keys = self.trap_atbl.getKeys() + for traps in copp_trap: + if copp_trap[traps] != copp_group_queue1_group1: + 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 + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + assert trap_found == True + + + def test_init_trap_del(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "ip2me,ssh" + fvs = swsscommon.FieldValuePairs([("trap_ids", "ip2me,ssh")]) + self.trap_ctbl.set("ip2me", fvs) + time.sleep(2) + + self.trap_ctbl._del("ip2me") + time.sleep(2) + trap_ids = traps.split(",") + trap_group = copp_trap["ip2me"] + trap_keys = self.trap_atbl.getKeys() + for trap_id in trap_ids: + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 + if trap_id not in restricted_traps: + if trap_id == "ip2me": + assert trap_found == True + elif trap_id == "ssh": + assert trap_found == False From f3d61938aac656af79e6217a61c11975d1c96271 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Wed, 1 Jul 2020 19:16:25 -0700 Subject: [PATCH 02/14] Handling empty trap set for trap delete --- cfgmgr/coppmgr.cpp | 83 +++++++++++++++++++++++++++++++++------- orchagent/copporch.cpp | 1 - orchagent/copporch.h | 1 - tests/test_copp.py | 86 ++++++++++++++++++++++-------------------- 4 files changed, 116 insertions(+), 55 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 61b6c3fef6..7e8cf5a0ea 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -239,6 +239,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c std::vector trap_init_fvs = i.second; std::vector trap_fvs; auto key = std::find(trap_cfg_keys.begin(), trap_cfg_keys.end(), i.first); + bool null_cfg = false; if (key != trap_cfg_keys.end()) { std::vector trap_cfg_fvs; @@ -250,6 +251,12 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c bool field_found = false; for (auto it2: trap_cfg_fvs) { + if(fvField(it2) == "NULL") + { + SWSS_LOG_DEBUG("Ignoring trap create for key %s",i.first.c_str()); + null_cfg = true; + break; + } if(fvField(it1) == fvField(it2)) { field_found = true; @@ -261,7 +268,10 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c trap_fvs.push_back(it1); } } - trap_cfg[i.first] = trap_fvs; + if (!null_cfg) + { + trap_cfg[i.first] = trap_fvs; + } } else { @@ -493,6 +503,8 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) trap_group = m_coppTrapConfMap[key].trap_group; conf_present = true; } + + bool null_cfg = false; for (auto i: kfvFieldsValues(t)) { if (fvField(i) == COPP_TRAP_GROUP_FIELD) @@ -503,6 +515,32 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { trap_ids = fvValue(i); } + else if (fvField(i) == "NULL") + { + null_cfg = true; + } + } + if (null_cfg) + { + if (!m_coppTrapConfMap[key].trap_group.empty()) + { + SWSS_LOG_DEBUG("Deleting trap key %s", key.c_str()); + removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, + m_coppTrapConfMap[key].trap_ids); + trap_ids.clear(); + getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); + fvs.clear(); + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + { + m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + } + m_coppTrapConfMap[key].trap_group = ""; + m_coppTrapConfMap[key].trap_ids = ""; + } + it = consumer.m_toSync.erase(it); + continue; } /*Duplicate check*/ if (conf_present && @@ -550,12 +588,15 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) m_coppTrapConfMap[key].trap_ids); fvs.clear(); trap_ids.clear(); - getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); - fvs.push_back(fv); - if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + if (!m_coppTrapConfMap[key].trap_group.empty()) { - m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + { + m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + } } m_coppTrapConfMap.erase(key); delCoppTrapStateOk(key); @@ -588,6 +629,8 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { m_appCoppTable.set(trap_group, g_fvs); } + m_coppTrapConfMap[key].trap_group = trap_group; + m_coppTrapConfMap[key].trap_ids = trap_ids; } } it = consumer.m_toSync.erase(it); @@ -665,6 +708,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) string op = kfvOp(t); auto fvs = kfvFieldsValues(t); string trap_ids; + vector modified_fvs; if (op == SET_COMMAND) { @@ -689,11 +733,10 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) getTrapGroupTrapIds(key, trap_ids); if (!trap_ids.empty()) { - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); fvs.push_back(fv); } - vector modified_fvs; coppGroupGetModifiedFvs(key, fvs, modified_fvs, false); if (!modified_fvs.empty()) { @@ -713,13 +756,27 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) if (m_coppGroupInitCfg.find(key) != m_coppGroupInitCfg.end()) { std::vector fvs = m_coppGroupInitCfg[key]; - getTrapGroupTrapIds(key, trap_ids); - if (!trap_ids.empty()) + if (m_coppGroupRestrictedMap.find(key) != m_coppGroupRestrictedMap.end()) { - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); - fvs.push_back(fv); + for (auto fv: fvs) + { + m_coppGroupRestrictedMap[key][fvField(fv)] = fvValue(fv); + } + } + if (!checkIfTrapGroupFeaturePending(key)) + { + getTrapGroupTrapIds(key, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + coppGroupGetModifiedFvs(key, fvs, modified_fvs, false); + if (!modified_fvs.empty()) + { + m_appCoppTable.set(key, modified_fvs); + } } - m_appCoppTable.set(key, fvs); } else { diff --git a/orchagent/copporch.cpp b/orchagent/copporch.cpp index 5b99839dd1..9fdc31558a 100644 --- a/orchagent/copporch.cpp +++ b/orchagent/copporch.cpp @@ -105,7 +105,6 @@ CoppOrch::CoppOrch(DBConnector* db, string tableName) : initDefaultHostIntfTable(); initDefaultTrapGroup(); initDefaultTrapIds(); - enable_sflow_trap = false; }; void CoppOrch::initDefaultHostIntfTable() diff --git a/orchagent/copporch.h b/orchagent/copporch.h index d75571b327..a732c7668e 100644 --- a/orchagent/copporch.h +++ b/orchagent/copporch.h @@ -52,7 +52,6 @@ class CoppOrch : public Orch CoppOrch(swss::DBConnector* db, std::string tableName); protected: object_map m_trap_group_map; - bool enable_sflow_trap; std::unique_ptr m_coppTable; TrapGroupPolicerTable m_trap_group_policer_map; diff --git a/tests/test_copp.py b/tests/test_copp.py index ed009728d2..e73891d96a 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -295,8 +295,6 @@ def validate_trap_group(self, trap_oid, trap_group): if fv[0] == "SAI_HOSTIF_ATTR_NAME": assert fv[1] == trap_group[keys] - - def test_defaults(self, dvs, testlog): self.setup_copp(dvs) trap_keys = self.trap_atbl.getKeys() @@ -569,42 +567,7 @@ def test_new_trap_group_del(self, dvs, testlog): if trap_id not in restricted_traps: assert trap_found != True - def test_init_group_del (self, dvs, testlog): - self.setup_copp(dvs) - global copp_trap - fvs = swsscommon.FieldValuePairs([("cbs", "500"),("cir","500")]) - self.trap_group_ctbl.set("queue1_group1", fvs) - time.sleep(2) - - - self.trap_group_ctbl._del("queue1_group1") - time.sleep(2) - - - trap_keys = self.trap_atbl.getKeys() - for traps in copp_trap: - if copp_trap[traps] != copp_group_queue1_group1: - 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 - trap_group_oid = "" - 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 - if trap_id not in restricted_traps: - assert trap_found == True - - def test_init_group_del (self, dvs, testlog): + def test_override_trap_grp_cfg_del (self, dvs, testlog): self.setup_copp(dvs) global copp_trap fvs = swsscommon.FieldValuePairs([("cbs", "500"),("cir","500")]) @@ -639,8 +602,7 @@ def test_init_group_del (self, dvs, testlog): if trap_id not in restricted_traps: assert trap_found == True - - def test_init_trap_del(self, dvs, testlog): + def test_override_trap_cfg_del(self, dvs, testlog): self.setup_copp(dvs) global copp_trap traps = "ip2me,ssh" @@ -672,3 +634,47 @@ def test_init_trap_del(self, dvs, testlog): assert trap_found == True elif trap_id == "ssh": assert trap_found == False + + def test_empty_trap_cfg(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) + self.trap_ctbl.set("ip2me", fvs) + time.sleep(2) + + trap_id = "ip2me" + trap_group = copp_trap["ip2me"] + trap_keys = self.trap_atbl.getKeys() + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 == False + + self.trap_ctbl._del("ip2me") + time.sleep(2) + + trap_keys = self.trap_atbl.getKeys() + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 From 585ca808c50d60db845ba922be13fa935121950e Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Thu, 2 Jul 2020 20:22:08 -0700 Subject: [PATCH 03/14] Fixing the case where trapid has been removed from trap list in trap table --- cfgmgr/coppmgr.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 7e8cf5a0ea..0ff3664a10 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -550,6 +550,8 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) it = consumer.m_toSync.erase(it); continue; } + removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, + m_coppTrapConfMap[key].trap_ids); fvs.clear(); string trap_group_trap_ids; addTrapIdsToTrapGroup(trap_group, trap_ids); From 5a01337a2ec6fbe127f057735ea68a6af934db48 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Thu, 2 Jul 2020 20:47:52 -0700 Subject: [PATCH 04/14] Adding test case for trap table trap ids set --- tests/test_copp.py | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/test_copp.py b/tests/test_copp.py index e73891d96a..aa9507ee44 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -406,6 +406,61 @@ def test_trap_group_set(self, dvs, testlog): if trap_id not in restricted_traps: assert trap_found == True + def test_trap_ids_set(self, dvs, testlog): + self.setup_copp(dvs) + global copp_trap + traps = "bgp" + fvs = swsscommon.FieldValuePairs([("trap_ids", traps)]) + self.trap_ctbl.set("bgp", fvs) + time.sleep(2) + + old_traps = "bgp,bgpv6" + trap_keys = self.trap_atbl.getKeys() + trap_ids = old_traps.split(",") + trap_group = copp_trap[old_traps] + for trap_id in trap_ids: + trap_type = traps_to_trap_type[trap_id] + trap_found = False + trap_group_oid = "" + 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 + if trap_id == "bgp": + assert trap_found == True + elif trap_id == "bgpv6": + assert trap_found == False + + traps = "bgp,bgpv6" + fvs = swsscommon.FieldValuePairs([("trap_ids", traps)]) + self.trap_ctbl.set("bgp", fvs) + time.sleep(2) + + trap_keys = self.trap_atbl.getKeys() + 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 + trap_group_oid = "" + 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 + def test_trap_action_set(self, dvs, testlog): self.setup_copp(dvs) fvs = swsscommon.FieldValuePairs([("trap_action", "copy")]) From 610d197b3bd41d9f6423e008105a6c159a4e7253 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Fri, 24 Jul 2020 09:58:36 -0700 Subject: [PATCH 05/14] Removing 00-copp.config.json --- swssconfig/sample/00-copp.config.json | 73 --------------------------- 1 file changed, 73 deletions(-) delete mode 100644 swssconfig/sample/00-copp.config.json diff --git a/swssconfig/sample/00-copp.config.json b/swssconfig/sample/00-copp.config.json deleted file mode 100644 index fadb6294c6..0000000000 --- a/swssconfig/sample/00-copp.config.json +++ /dev/null @@ -1,73 +0,0 @@ -[ - { - "COPP_TABLE:default": { - "queue": "0", - "meter_type":"packets", - "mode":"sr_tcm", - "cir":"600", - "cbs":"600", - "red_action":"drop" - }, - "OP": "SET" - }, - { - "COPP_TABLE:trap.group.bgp.lacp": { - "trap_ids": "bgp,bgpv6,lacp", - "trap_action":"trap", - "trap_priority":"4", - "queue": "4" - }, - "OP": "SET" - }, - { - "COPP_TABLE:trap.group.arp": { - "trap_ids": "arp_req,arp_resp,neigh_discovery", - "trap_action":"copy", - "trap_priority":"4", - "queue": "4", - "meter_type":"packets", - "mode":"sr_tcm", - "cir":"600", - "cbs":"600", - "red_action":"drop" - }, - "OP": "SET" - }, - { - "COPP_TABLE:trap.group.lldp.dhcp.dhcpv6.udld": { - "trap_ids": "lldp,dhcp,dhcpv6,udld", - "trap_action":"trap", - "trap_priority":"4", - "queue": "4" - }, - "OP": "SET" - }, - { - "COPP_TABLE:trap.group.ip2me": { - "trap_ids": "ip2me", - "trap_action":"trap", - "trap_priority":"1", - "queue": "1", - "meter_type":"packets", - "mode":"sr_tcm", - "cir":"6000", - "cbs":"6000", - "red_action":"drop" - }, - "OP": "SET" - }, - { - "COPP_TABLE:trap.group.nat": { - "trap_ids": "src_nat_miss,dest_nat_miss", - "trap_action":"trap", - "trap_priority":"1", - "queue": "1", - "meter_type":"packets", - "mode":"sr_tcm", - "cir":"600", - "cbs":"600", - "red_action":"drop" - }, - "OP": "SET" - } -] From 0634de2b1a57f7e3c2226fc8c639e43e13d7dc28 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Fri, 24 Jul 2020 10:35:00 -0700 Subject: [PATCH 06/14] Removing copp json file from swss.install --- debian/swss.install | 1 - 1 file changed, 1 deletion(-) diff --git a/debian/swss.install b/debian/swss.install index 7dd28196ac..dc5ff8ea90 100644 --- a/debian/swss.install +++ b/debian/swss.install @@ -1,4 +1,3 @@ swssconfig/sample/netbouncer.json etc/swss/config.d -swssconfig/sample/00-copp.config.json etc/swss/config.d neighsyncd/restore_neighbors.py usr/bin fpmsyncd/bgp_eoiu_marker.py usr/bin From 972d0045f38f64b0887d401b7a47afd4a1a13158 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Fri, 24 Jul 2020 15:41:35 -0700 Subject: [PATCH 07/14] Fix for populating state db for default entries --- cfgmgr/coppmgr.cpp | 15 +++++++++++++-- orchagent/copporch.cpp | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 0ff3664a10..6a364a1ea5 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -122,7 +122,7 @@ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) { m_appCoppTable.set(trap_group, modified_fvs); } - + setCoppGroupStateOk(trap_group); } } m_coppTrapDisabledMap.erase(it.first); @@ -145,6 +145,7 @@ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) (checkIfTrapGroupFeaturePending(trap_group))) { m_appCoppTable.del(trap_group); + delCoppGroupStateOk(trap_group); } } } @@ -314,6 +315,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c addTrapIdsToTrapGroup(trap_group, trap_ids); m_coppTrapConfMap[i.first].trap_group = trap_group; m_coppTrapConfMap[i.first].trap_ids = trap_ids; + setCoppTrapStateOk(i.first); } } @@ -398,6 +400,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { m_appCoppTable.set(i.first, trap_app_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()) { @@ -528,6 +531,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids); trap_ids.clear(); + setCoppTrapStateOk(key); getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); fvs.clear(); FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); @@ -536,6 +540,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); } + setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); m_coppTrapConfMap[key].trap_group = ""; m_coppTrapConfMap[key].trap_ids = ""; } @@ -561,6 +566,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (!checkIfTrapGroupFeaturePending(trap_group)) { m_appCoppTable.set(trap_group, fvs); + setCoppGroupStateOk(trap_group); } /* When the trap table's trap group is changed, the old trap group @@ -578,6 +584,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } } m_coppTrapConfMap[key].trap_group = trap_group; @@ -598,6 +605,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } } m_coppTrapConfMap.erase(key); @@ -630,9 +638,11 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (!checkIfTrapGroupFeaturePending(trap_group)) { m_appCoppTable.set(trap_group, g_fvs); + setCoppGroupStateOk(trap_group); } m_coppTrapConfMap[key].trap_group = trap_group; m_coppTrapConfMap[key].trap_ids = trap_ids; + setCoppTrapStateOk(key); } } it = consumer.m_toSync.erase(it); @@ -750,6 +760,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) { SWSS_LOG_NOTICE("%s: DEL",key.c_str()); m_appCoppTable.del(key); + delCoppGroupStateOk(key); /* If the COPP group was part of init config, it needs to be recreated * with field values from init. The configuration delete should just clear @@ -778,6 +789,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) { m_appCoppTable.set(key, modified_fvs); } + setCoppGroupStateOk(key); } } else @@ -787,7 +799,6 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) m_coppGroupRestrictedMap.erase(key); } } - delCoppGroupStateOk(key); } it = consumer.m_toSync.erase(it); } diff --git a/orchagent/copporch.cpp b/orchagent/copporch.cpp index 9fdc31558a..321f6daf20 100644 --- a/orchagent/copporch.cpp +++ b/orchagent/copporch.cpp @@ -894,7 +894,7 @@ bool CoppOrch::trapGroupProcessTrapIdChange (string trap_group_name, { if (!createGenetlinkHostIfTable(add_trap_ids)) { - return task_process_status::task_failed; + return false; } } } From 0ae6d984a47dae4b418542d888e108a24b2ca373 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Fri, 9 Oct 2020 16:53:15 -0700 Subject: [PATCH 08/14] Addressing code review comments --- cfgmgr/coppmgr.cpp | 463 ++++++++++++++++++--------------------------- cfgmgr/coppmgr.h | 32 ++-- 2 files changed, 201 insertions(+), 294 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 6a364a1ea5..41a067ab85 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -25,7 +25,8 @@ void CoppMgr::parseInitFile(void) std::ifstream ifs(COPP_INIT_FILE); if (ifs.fail()) { - return; + SWSS_LOG_ERROR("COPP init file %s not found", COPP_INIT_FILE); + return; } json j = json::parse(ifs); for(auto tbl = j.begin(); tbl != j.end(); tbl++) @@ -58,126 +59,150 @@ void CoppMgr::parseInitFile(void) /* Check if the trap group has traps that can be installed only when * feature is enabled */ -bool CoppMgr::checkIfTrapGroupFeaturePending(string trap_group_name) +bool CoppMgr::checkTrapGroupPending(string trap_group_name) { - std::vector fvs; - - if (m_coppGroupRestrictedMap.find(trap_group_name) == m_coppGroupRestrictedMap.end()) - { - return false; - } - + bool traps_present = false; for (auto it: m_coppTrapIdTrapGroupMap) { if (it.second == trap_group_name) { - /* At least one trap should be enabled to install the - * trap group with restricted attributes - */ + traps_present = true; + /* At least one trap should be enabled to install the trap group + */ if (!isTrapDisabled(it.first)) { return false; } } } - return true; + return traps_present; } void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) { for (auto it: trap_feature_map) { - if (it.second == feature) + if (it.second != feature) + { + continue; + } + bool disabled_trap = isTrapDisabled(it.first); + if ((enable && !disabled_trap) || (!enable && disabled_trap)) + { + continue; + } + if (enable) + { + m_coppDisabledTrapIds.erase(it.first); + } + else { - if (enable) + m_coppDisabledTrapIds.insert(it.first); + } + if (m_coppTrapIdTrapGroupMap.find(it.first) == m_coppTrapIdTrapGroupMap.end()) + { + continue; + } + string trap_group = m_coppTrapIdTrapGroupMap[it.first]; + + /* Trap group moved to pending state when feature is disabled. Remove trap group + */ + if (checkTrapGroupPending(trap_group) && isTrapGroupInstalled(trap_group)) + { + m_appCoppTable.del(trap_group); + delCoppGroupStateOk(trap_group); + continue; + } + vector fvs; + vector modified_fvs; + string trap_ids; + + for (auto i : m_coppGroupFvs[trap_group]) + { + FieldValueTuple fv(i.first, i.second); + fvs.push_back(fv); + } + getTrapGroupTrapIds(trap_group, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false); + if (!modified_fvs.empty()) + { + m_appCoppTable.set(trap_group, modified_fvs); + } + setCoppGroupStateOk(trap_group); + } +} + +bool CoppMgr::isTrapDisabled(string trap_id) +{ + return (m_coppDisabledTrapIds.find(trap_id) != m_coppDisabledTrapIds.end()); +} +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 + * user configuration, override the init fields with user fields + */ + for (auto i : init_cfg) + { + std::vector init_fvs = i.second; + std::vector merged_fvs; + auto key = std::find(cfg_keys.begin(), cfg_keys.end(), i.first); + bool null_cfg = false; + if (key != cfg_keys.end()) + { + std::vector cfg_fvs; + cfgTable.get(i.first, cfg_fvs); + + merged_fvs = cfg_fvs; + for (auto it1: init_fvs) { - if (m_coppTrapDisabledMap.find(it.first) != m_coppTrapDisabledMap.end()) + bool field_found = false; + for (auto it2: cfg_fvs) { - if (m_coppTrapIdTrapGroupMap.find(it.first) != m_coppTrapIdTrapGroupMap.end()) + if(fvField(it2) == "NULL") { - string trap_group = m_coppTrapIdTrapGroupMap[it.first]; - - /* Enable only when the restricted group is already pending */ - if ((m_coppGroupRestrictedMap.find(trap_group) - != m_coppGroupRestrictedMap.end()) && - (checkIfTrapGroupFeaturePending(trap_group))) - { - vector fvs; - vector modified_fvs; - string trap_ids; - - for (auto i : m_coppGroupRestrictedMap[trap_group]) - { - FieldValueTuple fv(i.first, i.second); - fvs.push_back(fv); - } - getTrapGroupTrapIds(trap_group, trap_ids); - if (!trap_ids.empty()) - { - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); - fvs.push_back(fv); - } - coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false); - if (!modified_fvs.empty()) - { - m_appCoppTable.set(trap_group, modified_fvs); - } - setCoppGroupStateOk(trap_group); - } + SWSS_LOG_DEBUG("Ignoring create for key %s",i.first.c_str()); + null_cfg = true; + break; } - m_coppTrapDisabledMap.erase(it.first); - } - } - else - { - if (m_coppTrapDisabledMap.find(it.first) == m_coppTrapDisabledMap.end()) - { - m_coppTrapDisabledMap[it.first] = true; - if (m_coppTrapIdTrapGroupMap.find(it.first) != m_coppTrapIdTrapGroupMap.end()) + if(fvField(it1) == fvField(it2)) { - string trap_group = m_coppTrapIdTrapGroupMap[it.first]; - - /* Delete the restricted copp group when it goes to pending state - * on feature disable - */ - if ((m_coppGroupRestrictedMap.find(trap_group) - != m_coppGroupRestrictedMap.end()) && - (checkIfTrapGroupFeaturePending(trap_group))) - { - m_appCoppTable.del(trap_group); - delCoppGroupStateOk(trap_group); - } + field_found = true; + break; } } + if (!field_found) + { + merged_fvs.push_back(it1); + } } + if (!null_cfg) + { + m_cfg[i.first] = merged_fvs; + } + } + else + { + m_cfg[i.first] = init_cfg[i.first]; } } -} -bool CoppMgr::isTrapDisabled(string trap_id) -{ - auto trap_idx = m_coppTrapDisabledMap.find(trap_id); - if (trap_idx != m_coppTrapDisabledMap.end()) - { - return m_coppTrapDisabledMap[trap_id]; - } - return false; -} - -/* The genetlink fields are restricted and needs to be installed only on - * feature(sflow) enable - */ -bool CoppMgr::coppGroupHasRestrictedFields (vector &fvs) -{ - for (auto i: fvs) + /* Read the user configuration keys that were not present in + * init configuration. + */ + for (auto i : cfg_keys) { - if ((fvField(i) == COPP_GROUP_GENETLINK_NAME_FIELD) || - (fvField(i) == COPP_GROUP_GENETLINK_MCGRP_NAME_FIELD)) + if(init_cfg.find(i) == init_cfg.end()) { - return true; + std::vector cfg_fvs; + cfgTable.get(i, cfg_fvs); + m_cfg[i] = cfg_fvs; } } - return false; } CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames) : @@ -211,7 +236,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c for (auto it: trap_feature_map) { - m_coppTrapDisabledMap[it.first] = true; + m_coppDisabledTrapIds.insert(it.first); } for (auto i: feature_keys) { @@ -221,77 +246,15 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c bool status = false; for (auto j: feature_fvs) { - if (fvField(j) == "status") + if (fvField(j) == "status" && fvValue(j) == "enabled") { - if (fvValue(j) == "enabled") - { - status = true; - } + status = true; } } setFeatureTrapIdsStatus(i, status); } - /* Read the init configuration first. If the same key is present in - * user configuration, override the init fields with user fields - */ - for (auto i : m_coppTrapInitCfg) - { - std::vector trap_init_fvs = i.second; - std::vector trap_fvs; - auto key = std::find(trap_cfg_keys.begin(), trap_cfg_keys.end(), i.first); - bool null_cfg = false; - if (key != trap_cfg_keys.end()) - { - std::vector trap_cfg_fvs; - m_cfgCoppTrapTable.get(i.first, trap_cfg_fvs); - - trap_fvs = trap_cfg_fvs; - for (auto it1: trap_init_fvs) - { - bool field_found = false; - for (auto it2: trap_cfg_fvs) - { - if(fvField(it2) == "NULL") - { - SWSS_LOG_DEBUG("Ignoring trap create for key %s",i.first.c_str()); - null_cfg = true; - break; - } - if(fvField(it1) == fvField(it2)) - { - field_found = true; - break; - } - } - if (!field_found) - { - trap_fvs.push_back(it1); - } - } - if (!null_cfg) - { - trap_cfg[i.first] = trap_fvs; - } - } - else - { - trap_cfg[i.first] = m_coppTrapInitCfg[i.first]; - } - } - - /* Read the user configuration keys that were not present in - * init configuration. - */ - for (auto i : trap_cfg_keys) - { - if(m_coppTrapInitCfg.find(i) == m_coppTrapInitCfg.end()) - { - std::vector trap_cfg_fvs; - m_cfgCoppTrapTable.get(i, trap_cfg_fvs); - trap_cfg[i] = trap_cfg_fvs; - } - } + mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable); for (auto i : trap_cfg) { @@ -319,69 +282,18 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c } } - /* Read the init configuration first. If the same key is present in - * user configuration, override the init fields with user fields - */ - for (auto i : m_coppGroupInitCfg) - { - std::vector group_init_fvs = i.second; - std::vector group_fvs; - auto key = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first); - if (key != group_cfg_keys.end()) - { - std::vector group_cfg_fvs; - m_cfgCoppGroupTable.get(i.first, group_cfg_fvs); - - group_fvs = group_cfg_fvs; - for (auto it1: group_init_fvs) - { - bool field_found = false; - for (auto it2: group_cfg_fvs) - { - if(fvField(it1) == fvField(it2)) - { - field_found = true; - break; - } - } - if (!field_found) - { - group_fvs.push_back(it1); - } - } - group_cfg[i.first] = group_fvs; - } - else - { - group_cfg[i.first] = m_coppGroupInitCfg[i.first]; - } - } + mergeConfig(m_coppGroupInitCfg, group_cfg, group_cfg_keys, m_cfgCoppGroupTable); - /* Read the user configuration keys that were not present in - * init configuration. - */ - for (auto i : group_cfg_keys) - { - if(m_coppGroupInitCfg.find(i) == m_coppGroupInitCfg.end()) - { - std::vector group_cfg_fvs; - m_cfgCoppGroupTable.get(i, group_cfg_fvs); - group_cfg[i] = group_cfg_fvs; - } - } for (auto i: group_cfg) { string trap_ids; vector trap_group_fvs = i.second; - if (coppGroupHasRestrictedFields (trap_group_fvs)) + for (auto fv: trap_group_fvs) { - for (auto fv: trap_group_fvs) - { - m_coppGroupRestrictedMap[i.first][fvField(fv)]= fvValue(fv); - } + m_coppGroupFvs[i.first][fvField(fv)]= fvValue(fv); } - if (checkIfTrapGroupFeaturePending(i.first)) + if (checkTrapGroupPending(i.first)) { continue; } @@ -465,12 +377,16 @@ void CoppMgr::removeTrapIdsFromTrapGroup(string trap_group, string trap_ids) void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) { - - trap_ids.clear(); + + trap_ids.clear(); for (auto it: m_coppTrapIdTrapGroupMap) { if (it.second == trap_group) { + if (isTrapDisabled(it.first)) + { + continue; + } if (trap_ids.empty()) { trap_ids = it.first; @@ -536,7 +452,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) fvs.clear(); FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); fvs.push_back(fv); - if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); } @@ -563,7 +479,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) getTrapGroupTrapIds(trap_group, trap_group_trap_ids); FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); fvs.push_back(fv1); - if (!checkIfTrapGroupFeaturePending(trap_group)) + if (!checkTrapGroupPending(trap_group)) { m_appCoppTable.set(trap_group, fvs); setCoppGroupStateOk(trap_group); @@ -573,7 +489,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) * should also be reprogrammed as some of its associated traps got * removed */ - if ((!m_coppTrapConfMap[key].trap_group.empty()) && + if ((!m_coppTrapConfMap[key].trap_group.empty()) && (trap_group != m_coppTrapConfMap[key].trap_group)) { trap_group_trap_ids.clear(); @@ -581,7 +497,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_group_trap_ids); FieldValueTuple fv2(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); fvs.push_back(fv2); - if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); @@ -602,7 +518,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); fvs.push_back(fv); - if (!checkIfTrapGroupFeaturePending(m_coppTrapConfMap[key].trap_group)) + if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); @@ -635,7 +551,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) getTrapGroupTrapIds(trap_group, trap_group_trap_ids); FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids); g_fvs.push_back(fv1); - if (!checkIfTrapGroupFeaturePending(trap_group)) + if (!checkTrapGroupPending(trap_group)) { m_appCoppTable.set(trap_group, g_fvs); setCoppGroupStateOk(trap_group); @@ -649,62 +565,66 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } } +bool CoppMgr::isTrapGroupInstalled(string key) +{ + std::vector app_keys; + + m_coppTable.getKeys(app_keys); + + return (std::find(app_keys.begin(), app_keys.end(),key) != app_keys.end()); +} /* This API is used to fetch only the modified configurations. During warmboot * when certain fields in APP table are not present in new init config file * the APP table needs to be removed and recreated */ -void CoppMgr::coppGroupGetModifiedFvs(string key, vector &trap_group_fvs, +void CoppMgr::coppGroupGetModifiedFvs(string key, vector &trap_group_fvs, vector &modified_fvs, bool del_on_field_remove) { - vector app_fvs; - std::vector app_keys; - - m_coppTable.get(key,app_fvs); modified_fvs = trap_group_fvs; - m_coppTable.getKeys(app_keys); - auto app_key = std::find(app_keys.begin(), app_keys.end(),key); - if (app_key != app_keys.end()) + if (!isTrapGroupInstalled(key)) + { + return; + } + + vector app_fvs; + m_coppTable.get(key, app_fvs); + for (auto app_idx: app_fvs) { - vector app_fvs; - m_coppTable.get(key, app_fvs); - for (auto app_idx: app_fvs) + bool field_removed = true; + for (auto cfg_idx: trap_group_fvs) { - bool field_removed = true; - for (auto cfg_idx: trap_group_fvs) + if (fvField(cfg_idx) == fvField(app_idx)) { - if (fvField(cfg_idx) == fvField(app_idx)) + field_removed = false; + if (fvValue(cfg_idx) == fvValue(app_idx)) { - field_removed = false; - if (fvValue(cfg_idx) == fvValue(app_idx)) - { - auto vec_idx = std::find(modified_fvs.begin(), modified_fvs.end(), cfg_idx); - modified_fvs.erase(vec_idx); - } + auto vec_idx = std::find(modified_fvs.begin(), modified_fvs.end(), cfg_idx); + modified_fvs.erase(vec_idx); } } - if (field_removed && del_on_field_remove) - { - m_appCoppTable.del(key); - bool key_present = true; - while (key_present) + } + if (field_removed && del_on_field_remove) + { + m_appCoppTable.del(key); + bool key_present = true; + while (key_present) + { + vector app_keys; + m_coppTable.getKeys(app_keys); + /* Loop until key is removed from app table */ + auto del_key = std::find(app_keys.begin(), app_keys.end(), key); + if (del_key == app_keys.end()) { - vector app_keys; - m_coppTable.getKeys(app_keys); - /* Loop until key is removed from app table */ - auto del_key = std::find(app_keys.begin(), app_keys.end(), key); - if (del_key == app_keys.end()) - { - key_present = false; - } - else - { - usleep(100); - } + key_present = false; + } + else + { + usleep(100); } - modified_fvs = trap_group_fvs; - break; } + modified_fvs = trap_group_fvs; + break; } } } @@ -730,14 +650,11 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) it = consumer.m_toSync.erase(it); continue; } - if (coppGroupHasRestrictedFields (fvs)) + for (auto fv: fvs) { - for (auto fv: fvs) - { - m_coppGroupRestrictedMap[key][fvField(fv)] = fvValue(fv); - } + m_coppGroupFvs[key][fvField(fv)] = fvValue(fv); } - if (checkIfTrapGroupFeaturePending(key)) + if (checkTrapGroupPending(key)) { it = consumer.m_toSync.erase(it); continue; @@ -759,8 +676,10 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) else if (op == DEL_COMMAND) { SWSS_LOG_NOTICE("%s: DEL",key.c_str()); - m_appCoppTable.del(key); - delCoppGroupStateOk(key); + if (!checkTrapGroupPending(key)) { + m_appCoppTable.del(key); + delCoppGroupStateOk(key); + } /* If the COPP group was part of init config, it needs to be recreated * with field values from init. The configuration delete should just clear @@ -769,14 +688,11 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) if (m_coppGroupInitCfg.find(key) != m_coppGroupInitCfg.end()) { std::vector fvs = m_coppGroupInitCfg[key]; - if (m_coppGroupRestrictedMap.find(key) != m_coppGroupRestrictedMap.end()) + for (auto fv: fvs) { - for (auto fv: fvs) - { - m_coppGroupRestrictedMap[key][fvField(fv)] = fvValue(fv); - } + m_coppGroupFvs[key][fvField(fv)] = fvValue(fv); } - if (!checkIfTrapGroupFeaturePending(key)) + if (!checkTrapGroupPending(key)) { getTrapGroupTrapIds(key, trap_ids); if (!trap_ids.empty()) @@ -794,10 +710,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) } else { - if (m_coppGroupRestrictedMap.find(key) != m_coppGroupRestrictedMap.end()) - { - m_coppGroupRestrictedMap.erase(key); - } + m_coppGroupFvs.erase(key); } } it = consumer.m_toSync.erase(it); @@ -823,7 +736,7 @@ void CoppMgr::doFeatureTask(Consumer &consumer) if (fvField(i) == "status") { bool status = false; - if (fvValue(i) == "enabled") + if (fvValue(i) == "enabled") { status = true; } diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index 281886d065..8e9cc90e65 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -50,14 +50,11 @@ typedef std::map CoppTrapConfMap; /* TrapGroupName to GroupConf map */ typedef std::map CoppTrapIdTrapGroupMap; -/* Trap Id to Enable/Disabled map */ -typedef std::map CoppTrapDisabledMap; - /* Key to Field value Tuple map */ typedef std::map> CoppCfg; /* Restricted Copp group key to Field value map's map */ -typedef std::map> CoppGroupRestrictedConf; +typedef std::map> CoppGroupFvs; class CoppMgr : public Orch { @@ -67,19 +64,15 @@ class CoppMgr : public Orch using Orch::doTask; private: - Table m_cfgCoppTrapTable; - Table m_cfgCoppGroupTable; - ProducerStateTable m_appCoppTable; - Table m_stateCoppTrapTable; - Table m_stateCoppGroupTable; - Table m_cfgFeatureTable; - Table m_coppTable; - CoppTrapConfMap m_coppTrapConfMap; - CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; - CoppGroupRestrictedConf m_coppGroupRestrictedMap; - CoppTrapDisabledMap m_coppTrapDisabledMap; - CoppCfg m_coppGroupInitCfg; - CoppCfg m_coppTrapInitCfg; + Table m_cfgCoppTrapTable, m_cfgCoppGroupTable, m_cfgFeatureTable, m_coppTable; + Table m_stateCoppTrapTable, m_stateCoppGroupTable; + ProducerStateTable m_appCoppTable; + CoppTrapConfMap m_coppTrapConfMap; + CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; + CoppGroupFvs m_coppGroupFvs; + std::set m_coppDisabledTrapIds; + CoppCfg m_coppGroupInitCfg; + CoppCfg m_coppTrapInitCfg; void doTask(Consumer &consumer); @@ -90,10 +83,9 @@ class CoppMgr : public Orch void getTrapGroupTrapIds(std::string trap_group, std::string &trap_ids); void removeTrapIdsFromTrapGroup(std::string trap_group, std::string trap_ids); void addTrapIdsToTrapGroup(std::string trap_group, std::string trap_ids); - bool coppGroupHasRestrictedFields (std::vector &fvs); bool isTrapDisabled(std::string trap_id); void setFeatureTrapIdsStatus(std::string feature, bool enable); - bool checkIfTrapGroupFeaturePending(std::string trap_group_name); + bool checkTrapGroupPending(std::string trap_group_name); void setCoppGroupStateOk(std::string alias); void delCoppGroupStateOk(std::string alias); @@ -103,6 +95,8 @@ class CoppMgr : public Orch void coppGroupGetModifiedFvs(std::string key, std::vector &trap_group_fvs, std::vector &modified_fvs, bool del_on_field_remove); void parseInitFile(void); + bool isTrapGroupInstalled(std::string key); + void mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &cfg_keys, Table &cfgTable); }; From 5c6a49ab22a9880012d8491a509a99e7a6ebeec4 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Mon, 12 Oct 2020 17:56:28 -0700 Subject: [PATCH 09/14] Addressing code review comments --- cfgmgr/coppmgr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index 8e9cc90e65..03ba7da171 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -47,7 +47,7 @@ struct CoppTrapConf /* TrapName to TrapConf map */ typedef std::map CoppTrapConfMap; -/* TrapGroupName to GroupConf map */ +/* TrapID to Trap group name map */ typedef std::map CoppTrapIdTrapGroupMap; /* Key to Field value Tuple map */ From 33197d246e92d1f4ee0912552ed91ea0ec8b7bdc Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Mon, 19 Oct 2020 19:51:39 -0700 Subject: [PATCH 10/14] Addressing code review comments --- cfgmgr/coppmgr.cpp | 231 ++++++++++++++++++++------------------------- cfgmgr/coppmgr.h | 6 +- tests/test_copp.py | 26 ++--- 3 files changed, 120 insertions(+), 143 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 41a067ab85..beca6db918 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -16,9 +16,6 @@ using namespace std; using namespace swss; static set g_copp_init_set; -static map trap_feature_map = { - {COPP_TRAP_TYPE_SAMPLEPACKET, "sflow"} -}; void CoppMgr::parseInitFile(void) { @@ -69,7 +66,7 @@ bool CoppMgr::checkTrapGroupPending(string trap_group_name) traps_present = true; /* At least one trap should be enabled to install the trap group */ - if (!isTrapDisabled(it.first)) + if (!isTrapIdDisabled(it.first)) { return false; } @@ -78,68 +75,89 @@ bool CoppMgr::checkTrapGroupPending(string trap_group_name) return traps_present; } +/* Feature name and CoPP Trap table name must match */ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable) { - for (auto it: trap_feature_map) + bool disabled_trap = (m_coppDisabledTraps.find(feature) != m_coppDisabledTraps.end()); + + if ((enable && !disabled_trap) || (!enable && disabled_trap)) { - if (it.second != feature) - { - continue; - } - bool disabled_trap = isTrapDisabled(it.first); - if ((enable && !disabled_trap) || (!enable && disabled_trap)) - { - continue; - } - if (enable) - { - m_coppDisabledTrapIds.erase(it.first); - } - else - { - m_coppDisabledTrapIds.insert(it.first); - } - if (m_coppTrapIdTrapGroupMap.find(it.first) == m_coppTrapIdTrapGroupMap.end()) - { - continue; - } - string trap_group = m_coppTrapIdTrapGroupMap[it.first]; + return; + } - /* Trap group moved to pending state when feature is disabled. Remove trap group - */ - if (checkTrapGroupPending(trap_group) && isTrapGroupInstalled(trap_group)) + if (m_coppTrapConfMap.find(feature) == m_coppTrapConfMap.end()) + { + if (!enable) { - m_appCoppTable.del(trap_group); - delCoppGroupStateOk(trap_group); - continue; + m_coppDisabledTraps.insert(feature); } - vector fvs; - vector modified_fvs; - string trap_ids; + return; + } + string trap_group = m_coppTrapConfMap[feature].trap_group; + bool prev_group_state = checkTrapGroupPending(trap_group); + if (!enable) + { + m_coppDisabledTraps.insert(feature); + } + else + { + m_coppDisabledTraps.erase(feature); + } + + /* Trap group moved to pending state when feature is disabled. Remove trap group + */ + if (checkTrapGroupPending(trap_group) && !prev_group_state) + { + m_appCoppTable.del(trap_group); + delCoppGroupStateOk(trap_group); + return; + } + vector fvs; + string trap_ids; + + /* Trap group moved from pending state to enabled state + * Hence install include all fields to create the group + */ + if (prev_group_state && !checkTrapGroupPending(trap_group)) + { for (auto i : m_coppGroupFvs[trap_group]) { FieldValueTuple fv(i.first, i.second); fvs.push_back(fv); } - getTrapGroupTrapIds(trap_group, trap_ids); - if (!trap_ids.empty()) - { - FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); - fvs.push_back(fv); - } - coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false); - if (!modified_fvs.empty()) - { - m_appCoppTable.set(trap_group, modified_fvs); - } + } + getTrapGroupTrapIds(trap_group, trap_ids); + if (!trap_ids.empty()) + { + FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); + fvs.push_back(fv); + } + if (!fvs.empty()) + { + m_appCoppTable.set(trap_group, fvs); setCoppGroupStateOk(trap_group); } } -bool CoppMgr::isTrapDisabled(string trap_id) +bool CoppMgr::isTrapIdDisabled(string trap_id) { - return (m_coppDisabledTrapIds.find(trap_id) != m_coppDisabledTrapIds.end()); + for (auto &m: m_coppDisabledTraps) + { + if (m_coppTrapConfMap.find(m) == m_coppTrapConfMap.end()) + { + continue; + } + vector trap_id_list; + + 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()) + { + return true; + } + + } + return false; } void CoppMgr::mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &cfg_keys, Table &cfgTable) { @@ -234,24 +252,18 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_coppTable.getKeys(app_keys); - for (auto it: trap_feature_map) - { - m_coppDisabledTrapIds.insert(it.first); - } for (auto i: feature_keys) { std::vector feature_fvs; m_cfgFeatureTable.get(i, feature_fvs); - bool status = false; for (auto j: feature_fvs) { - if (fvField(j) == "status" && fvValue(j) == "enabled") + if (fvField(j) == "status" && fvValue(j) == "disabled") { - status = true; + m_coppDisabledTraps.insert(i); } } - setFeatureTrapIdsStatus(i, status); } mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable); @@ -305,12 +317,9 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c trap_group_fvs.push_back(fv); } - vector trap_app_fvs; - - coppGroupGetModifiedFvs (i.first, trap_group_fvs, trap_app_fvs, true); - if (!trap_app_fvs.empty()) + if (!trap_group_fvs.empty()) { - m_appCoppTable.set(i.first, trap_app_fvs); + 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); @@ -383,7 +392,7 @@ void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids) { if (it.second == trap_group) { - if (isTrapDisabled(it.first)) + if (isTrapIdDisabled(it.first)) { continue; } @@ -565,70 +574,35 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } } -bool CoppMgr::isTrapGroupInstalled(string key) -{ - std::vector app_keys; - - m_coppTable.getKeys(app_keys); - - return (std::find(app_keys.begin(), app_keys.end(),key) != app_keys.end()); -} -/* This API is used to fetch only the modified configurations. During warmboot - * when certain fields in APP table are not present in new init config file - * the APP table needs to be removed and recreated +/* This API is used to fetch only the modified configurations */ void CoppMgr::coppGroupGetModifiedFvs(string key, vector &trap_group_fvs, - vector &modified_fvs, bool del_on_field_remove) + vector &modified_fvs) { modified_fvs = trap_group_fvs; - if (!isTrapGroupInstalled(key)) + if (m_coppGroupFvs.find(key) == m_coppGroupFvs.end()) { return; } - vector app_fvs; - m_coppTable.get(key, app_fvs); - for (auto app_idx: app_fvs) + for (auto fv: trap_group_fvs) { - bool field_removed = true; - for (auto cfg_idx: trap_group_fvs) + if(fvField(fv) == COPP_TRAP_ID_LIST_FIELD || + m_coppGroupFvs[key].find(fvField(fv)) == m_coppGroupFvs[key].end()) { - if (fvField(cfg_idx) == fvField(app_idx)) - { - field_removed = false; - if (fvValue(cfg_idx) == fvValue(app_idx)) - { - auto vec_idx = std::find(modified_fvs.begin(), modified_fvs.end(), cfg_idx); - modified_fvs.erase(vec_idx); - } - } + continue; } - if (field_removed && del_on_field_remove) + + if (m_coppGroupFvs[key][fvField(fv)] == fvValue(fv)) { - m_appCoppTable.del(key); - bool key_present = true; - while (key_present) - { - vector app_keys; - m_coppTable.getKeys(app_keys); - /* Loop until key is removed from app table */ - auto del_key = std::find(app_keys.begin(), app_keys.end(), key); - if (del_key == app_keys.end()) - { - key_present = false; - } - else - { - usleep(100); - } - } - modified_fvs = trap_group_fvs; - break; + auto vec_idx = std::find(modified_fvs.begin(), modified_fvs.end(), fv); + modified_fvs.erase(vec_idx); } } } + void CoppMgr::doCoppGroupTask(Consumer &consumer) { auto it = consumer.m_toSync.begin(); @@ -650,15 +624,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) it = consumer.m_toSync.erase(it); continue; } - for (auto fv: fvs) - { - m_coppGroupFvs[key][fvField(fv)] = fvValue(fv); - } - if (checkTrapGroupPending(key)) - { - it = consumer.m_toSync.erase(it); - continue; - } + getTrapGroupTrapIds(key, trap_ids); if (!trap_ids.empty()) { @@ -666,17 +632,28 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) fvs.push_back(fv); } - coppGroupGetModifiedFvs(key, fvs, modified_fvs, false); - if (!modified_fvs.empty()) + coppGroupGetModifiedFvs(key, fvs, modified_fvs); + if (!checkTrapGroupPending(key)) { - m_appCoppTable.set(key, modified_fvs); + if (!modified_fvs.empty()) + { + m_appCoppTable.set(key, modified_fvs); + setCoppGroupStateOk(key); + } + } + for (auto fv: fvs) + { + if(fvField(fv) != COPP_TRAP_ID_LIST_FIELD) + { + m_coppGroupFvs[key][fvField(fv)] = fvValue(fv); + } } - setCoppGroupStateOk(key); } else if (op == DEL_COMMAND) { SWSS_LOG_NOTICE("%s: DEL",key.c_str()); - if (!checkTrapGroupPending(key)) { + if (!checkTrapGroupPending(key)) + { m_appCoppTable.del(key); delCoppGroupStateOk(key); } @@ -700,11 +677,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer) FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); fvs.push_back(fv); } - coppGroupGetModifiedFvs(key, fvs, modified_fvs, false); - if (!modified_fvs.empty()) - { - m_appCoppTable.set(key, modified_fvs); - } + m_appCoppTable.set(key, fvs); setCoppGroupStateOk(key); } } diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index 03ba7da171..b010489f2e 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -70,7 +70,7 @@ class CoppMgr : public Orch CoppTrapConfMap m_coppTrapConfMap; CoppTrapIdTrapGroupMap m_coppTrapIdTrapGroupMap; CoppGroupFvs m_coppGroupFvs; - std::set m_coppDisabledTrapIds; + std::set m_coppDisabledTraps; CoppCfg m_coppGroupInitCfg; CoppCfg m_coppTrapInitCfg; @@ -83,7 +83,7 @@ class CoppMgr : public Orch void getTrapGroupTrapIds(std::string trap_group, std::string &trap_ids); void removeTrapIdsFromTrapGroup(std::string trap_group, std::string trap_ids); void addTrapIdsToTrapGroup(std::string trap_group, std::string trap_ids); - bool isTrapDisabled(std::string trap_id); + bool isTrapIdDisabled(std::string trap_id); void setFeatureTrapIdsStatus(std::string feature, bool enable); bool checkTrapGroupPending(std::string trap_group_name); @@ -93,7 +93,7 @@ class CoppMgr : public Orch void setCoppTrapStateOk(std::string alias); void delCoppTrapStateOk(std::string alias); void coppGroupGetModifiedFvs(std::string key, std::vector &trap_group_fvs, - std::vector &modified_fvs, bool del_on_field_remove); + std::vector &modified_fvs); void parseInitFile(void); bool isTrapGroupInstalled(std::string key); void mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &cfg_keys, Table &cfgTable); diff --git a/tests/test_copp.py b/tests/test_copp.py index aa9507ee44..c08b14baa6 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -161,7 +161,7 @@ "ttl_error": copp_group_default } -restricted_traps = ["sample_packet"] +disabled_traps = ["sample_packet"] policer_meter_map = { "packets": "SAI_METER_TYPE_PACKETS", @@ -198,6 +198,10 @@ 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([("status", "disbled")]) + self.feature_tbl.set("sflow", fvs) + time.sleep(2) + def validate_policer(self, policer_oid, field, value): (status, fvs) = self.policer_atbl.get(policer_oid) @@ -315,7 +319,7 @@ def test_defaults(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_restricted_trap_sflow(self, dvs, testlog): @@ -374,7 +378,7 @@ def test_policer_set(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_trap_group_set(self, dvs, testlog): @@ -403,7 +407,7 @@ def test_trap_group_set(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_trap_ids_set(self, dvs, testlog): @@ -489,7 +493,7 @@ def test_trap_action_set(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_new_trap_add(self, dvs, testlog): @@ -518,7 +522,7 @@ def test_new_trap_add(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_new_trap_del(self, dvs, testlog): @@ -549,7 +553,7 @@ def test_new_trap_del(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == False def test_new_trap_group_add(self, dvs, testlog): @@ -583,7 +587,7 @@ def test_new_trap_group_add(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_new_trap_group_del(self, dvs, testlog): @@ -619,7 +623,7 @@ def test_new_trap_group_del(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found != True def test_override_trap_grp_cfg_del (self, dvs, testlog): @@ -654,7 +658,7 @@ def test_override_trap_grp_cfg_del (self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: assert trap_found == True def test_override_trap_cfg_del(self, dvs, testlog): @@ -684,7 +688,7 @@ def test_override_trap_cfg_del(self, dvs, testlog): if trap_found: self.validate_trap_group(key,trap_group) break - if trap_id not in restricted_traps: + if trap_id not in disabled_traps: if trap_id == "ip2me": assert trap_found == True elif trap_id == "ssh": From e3a6c7a8d56d0896147b39ef46d1cd3221d11165 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Tue, 20 Oct 2020 16:46:17 -0700 Subject: [PATCH 11/14] Addressing field changes in feature table --- cfgmgr/coppmgr.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index beca6db918..b5db937dad 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -259,7 +259,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c for (auto j: feature_fvs) { - if (fvField(j) == "status" && fvValue(j) == "disabled") + if (fvField(j) == "state" && fvValue(j) == "disabled") { m_coppDisabledTraps.insert(i); } @@ -706,7 +706,7 @@ void CoppMgr::doFeatureTask(Consumer &consumer) { for (auto i : kfvFieldsValues(t)) { - if (fvField(i) == "status") + if (fvField(i) == "state") { bool status = false; if (fvValue(i) == "enabled") From 1fcfd2df927e49d8c0c1d942dc6a982cb03ffb07 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Tue, 20 Oct 2020 18:55:51 -0700 Subject: [PATCH 12/14] Updating UT with feature table modifications --- tests/test_copp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_copp.py b/tests/test_copp.py index c08b14baa6..82ab7fd78b 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -198,7 +198,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([("status", "disbled")]) + fvs = swsscommon.FieldValuePairs([("state", "disbled")]) self.feature_tbl.set("sflow", fvs) time.sleep(2) @@ -324,7 +324,7 @@ def test_defaults(self, dvs, testlog): def test_restricted_trap_sflow(self, dvs, testlog): self.setup_copp(dvs) - fvs = swsscommon.FieldValuePairs([("status", "enabled")]) + fvs = swsscommon.FieldValuePairs([("state", "enabled")]) self.feature_tbl.set("sflow", fvs) time.sleep(2) global copp_trap From 414375a873a6ad99debc00242c6399f07e82426c Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Wed, 21 Oct 2020 17:34:24 -0700 Subject: [PATCH 13/14] Since copp creates sflow as hostif the num host interfaces is greater than num ports --- tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 8793c086e3..fe924638cc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -76,7 +76,7 @@ def _verify_db_contents(): if len(self.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_VLAN")) != 1: return (False, None) - if len(self.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF")) != NUM_PORTS: + if len(self.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF")) < NUM_PORTS: return (False, None) if len(self.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")) != 0: From 67edb8b9fc5e9a8fb4579d655b4129da82525c1e Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Thu, 5 Nov 2020 14:27:59 -0800 Subject: [PATCH 14/14] Addressing code review comments --- cfgmgr/coppmgr.cpp | 62 +++++++++------ orchagent/copporch.cpp | 166 +++-------------------------------------- orchagent/copporch.h | 3 - tests/test_copp.py | 13 ++++ 4 files changed, 61 insertions(+), 183 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index b5db937dad..834b2c5ff0 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -238,7 +238,6 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c std::vector group_keys; std::vector trap_keys; std::vector feature_keys; - std::vector app_keys; std::vector group_cfg_keys; std::vector trap_cfg_keys; @@ -249,7 +248,6 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_cfgCoppGroupTable.getKeys(group_cfg_keys); m_cfgCoppTrapTable.getKeys(trap_cfg_keys); m_cfgFeatureTable.getKeys(feature_keys); - m_coppTable.getKeys(app_keys); for (auto i: feature_keys) @@ -418,20 +416,20 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) string key = kfvKey(t); string op = kfvOp(t); vector fvs; - string trap_ids; - string trap_group; + string trap_ids = ""; + string trap_group = ""; 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; + conf_present = true; + } + if (op == SET_COMMAND) { /*Create case*/ - if (m_coppTrapConfMap.find(key) != m_coppTrapConfMap.end()) - { - trap_ids = m_coppTrapConfMap[key].trap_ids; - trap_group = m_coppTrapConfMap[key].trap_group; - conf_present = true; - } - bool null_cfg = false; for (auto i: kfvFieldsValues(t)) { @@ -450,7 +448,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } if (null_cfg) { - if (!m_coppTrapConfMap[key].trap_group.empty()) + if (conf_present) { SWSS_LOG_DEBUG("Deleting trap key %s", key.c_str()); removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, @@ -464,10 +462,9 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group)) { m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); + setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } - setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); - m_coppTrapConfMap[key].trap_group = ""; - m_coppTrapConfMap[key].trap_ids = ""; + m_coppTrapConfMap.erase(key); } it = consumer.m_toSync.erase(it); continue; @@ -480,8 +477,22 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) it = consumer.m_toSync.erase(it); continue; } - removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, - m_coppTrapConfMap[key].trap_ids); + /* Incomplete configuration. Do not process until both trap group + * and trap_ids are available + */ + if (trap_group.empty() || trap_ids.empty()) + { + 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 + */ + 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); @@ -498,8 +509,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) * should also be reprogrammed as some of its associated traps got * removed */ - if ((!m_coppTrapConfMap[key].trap_group.empty()) && - (trap_group != m_coppTrapConfMap[key].trap_group)) + if (conf_present && (trap_group != m_coppTrapConfMap[key].trap_group)) { trap_group_trap_ids.clear(); fvs.clear(); @@ -518,11 +528,14 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) } else if (op == DEL_COMMAND) { - removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, - m_coppTrapConfMap[key].trap_ids); + if (conf_present) + { + removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, + m_coppTrapConfMap[key].trap_ids); + } fvs.clear(); trap_ids.clear(); - if (!m_coppTrapConfMap[key].trap_group.empty()) + if (conf_present && !m_coppTrapConfMap[key].trap_group.empty()) { getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids); FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids); @@ -533,7 +546,10 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer) setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); } } - m_coppTrapConfMap.erase(key); + if (conf_present) + { + m_coppTrapConfMap.erase(key); + } delCoppTrapStateOk(key); /* If the COPP trap was part of init config, it needs to be recreated diff --git a/orchagent/copporch.cpp b/orchagent/copporch.cpp index 321f6daf20..fa3f82d746 100644 --- a/orchagent/copporch.cpp +++ b/orchagent/copporch.cpp @@ -20,8 +20,6 @@ extern sai_object_id_t gSwitchId; extern PortsOrch* gPortsOrch; extern bool gIsNatSupported; -static string old_nat_group_name = "trap.group.nat"; - static map policer_meter_map = { {"packets", SAI_METER_TYPE_PACKETS}, {"bytes", SAI_METER_TYPE_BYTES} @@ -101,7 +99,6 @@ CoppOrch::CoppOrch(DBConnector* db, string tableName) : { SWSS_LOG_ENTER(); - m_coppTable = unique_ptr
(new Table(db, APP_COPP_TABLE_NAME)); initDefaultHostIntfTable(); initDefaultTrapGroup(); initDefaultTrapIds(); @@ -465,10 +462,6 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer) if (op == SET_COMMAND) { - if(checkDupCoppGrpNameAndUpdate(trap_group_name, fv_tuple)) - { - return task_process_status::task_success; - } for (auto i = fv_tuple.begin(); i != fv_tuple.end(); i++) { if (fvField(*i) == copp_trap_id_list) @@ -654,153 +647,12 @@ void CoppOrch::doTask(Consumer &consumer) } } -/* Check if the Copp Group name is different while all trap ids remain the same - * This is going to be warm boot scenario from an old image which has different naming scheme - */ -bool CoppOrch::checkDupCoppGrpNameAndUpdate(string new_copp_grp_name, - vector &fv_tuple) -{ - vector trap_id_list; - sai_object_id_t trap_group_obj = SAI_NULL_OBJECT_ID; - vector mod_fv_tuple; - vector trap_ids; - bool ret = false; - - if (m_trap_group_map.find(new_copp_grp_name) != m_trap_group_map.end()) - { - return false; - } - - for (auto i = fv_tuple.begin(); i != fv_tuple.end(); i++) - { - if (fvField(*i) == copp_trap_id_list) - { - trap_id_list = tokenize(fvValue(*i), list_item_delimiter); - getTrapIdList(trap_id_list, trap_ids); - } - } - - /* In the case where NAT is not supported, there will be no trap IDs associated with - * the nat trap group and hence group ID cannot be figured out by iterating through traps. - * Using the hardcoded name for this specific case - */ - if (trap_ids.size() == 0) - { - if (!gIsNatSupported) - { - auto src_nat = std::find(trap_id_list.begin(), trap_id_list.end(), "src_nat_miss"); - auto dst_nat = std::find(trap_id_list.begin(), trap_id_list.end(), "dest_nat_miss"); - if ((src_nat != trap_id_list.end()) && (dst_nat != trap_id_list.end())) - { - if(m_trap_group_map.find(old_nat_group_name) != m_trap_group_map.end()) - { - trap_group_obj = m_trap_group_map[old_nat_group_name]; - } - else - { - return false; - } - } - else - { - return false; - } - } - else - { - return false; - } - } - else - { - for(auto it = trap_ids.begin(); it != trap_ids.end(); it++) - { - if (m_syncdTrapIds.find(*it) == m_syncdTrapIds.end()) - { - return false; - } - sai_object_id_t trap_id_group = m_syncdTrapIds[*it].trap_group_obj; - if (trap_id_group == SAI_NULL_OBJECT_ID) - { - return false; - } - if (trap_group_obj == SAI_NULL_OBJECT_ID) - { - trap_group_obj = trap_id_group; - } - else if (trap_group_obj != trap_id_group) - { - return false; - } - } - } - - for (auto it : m_trap_group_map) - { - if (it.second == trap_group_obj) - { - vector old_tuples; - vector new_tuples; - m_coppTable->get(it.first, old_tuples); - m_coppTable->get(new_copp_grp_name, new_tuples); - vector tmp_tuples = new_tuples; - for (auto o: old_tuples) - { - if (fvField(o) == copp_trap_id_list) - { - continue; - } - bool not_found = true; - for (auto n : new_tuples) - { - if (fvField(o) == fvField(n)) - { - if (fvValue(o) != fvValue(n)) - { - mod_fv_tuple.push_back(n); - } - auto vec_idx = std::find(tmp_tuples.begin(), tmp_tuples.end(), n); - tmp_tuples.erase(vec_idx); - not_found = false; - } - } - /* Old table has a field which is not present in new table - * Delete the entry and return false so it is recreated - */ - if (not_found) - { - processTrapGroupDel(it.first); - m_coppTable->del(it.first); - return false; - } - } - if (!tmp_tuples.empty()) - { - mod_fv_tuple.insert(mod_fv_tuple.end(), tmp_tuples.begin(), tmp_tuples.end()); - } - if (!mod_fv_tuple.empty()) - { - fv_tuple = mod_fv_tuple; - } - else - { - ret = true; - } - m_trap_group_map[new_copp_grp_name] = it.second; - m_trap_group_map.erase(m_trap_group_map.find(it.first)); - m_coppTable->del(it.first); - break; - } - } - return ret; -} - void CoppOrch::getTrapAddandRemoveList(string trap_group_name, vector &trap_ids, vector &add_trap_ids, vector &rem_trap_ids) { - + vector tmp_trap_ids = trap_ids; if(m_trap_group_map.find(trap_group_name) == m_trap_group_map.end()) { @@ -810,28 +662,28 @@ void CoppOrch::getTrapAddandRemoveList(string trap_group_name, } - for (auto it : m_syncdTrapIds) - { - if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) - { + for (auto it : m_syncdTrapIds) + { + if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) + { /* If new trap list contains already mapped ID remove it */ auto i = std::find(std::begin(tmp_trap_ids), std::end(tmp_trap_ids), it.first); if (i != std::end(tmp_trap_ids)) { - tmp_trap_ids.erase(i); + tmp_trap_ids.erase(i); } /* The mapped Trap ID is not found on newly set list and to be removed*/ else { if ((trap_group_name != default_trap_group) || - ((trap_group_name == default_trap_group) && - (it.first != SAI_HOSTIF_TRAP_TYPE_TTL_ERROR))) + ((trap_group_name == default_trap_group) && + (it.first != SAI_HOSTIF_TRAP_TYPE_TTL_ERROR))) { rem_trap_ids.push_back(it.first); } } - } + } } add_trap_ids = tmp_trap_ids; diff --git a/orchagent/copporch.h b/orchagent/copporch.h index a732c7668e..4794cfd2a6 100644 --- a/orchagent/copporch.h +++ b/orchagent/copporch.h @@ -52,7 +52,6 @@ class CoppOrch : public Orch CoppOrch(swss::DBConnector* db, std::string tableName); protected: object_map m_trap_group_map; - std::unique_ptr m_coppTable; TrapGroupPolicerTable m_trap_group_policer_map; TrapIdTrapObjectsTable m_syncdTrapIds; @@ -79,8 +78,6 @@ class CoppOrch : public Orch bool removeGenetlinkHostIf(std::string trap_group_name); bool createGenetlinkHostIfTable(std::vector &trap_id_list); bool removeGenetlinkHostIfTable(std::vector &trap_id_list); - bool checkDupCoppGrpNameAndUpdate(std::string new_copp_grp_name, - std::vector &fv_tuple); void getTrapAddandRemoveList(std::string trap_group_name, std::vector &trap_ids, std::vector &add_trap_ids, std::vector &rem_trap_ids); diff --git a/tests/test_copp.py b/tests/test_copp.py index 82ab7fd78b..59cf8b4c3f 100644 --- a/tests/test_copp.py +++ b/tests/test_copp.py @@ -299,6 +299,7 @@ def validate_trap_group(self, trap_oid, trap_group): if fv[0] == "SAI_HOSTIF_ATTR_NAME": assert fv[1] == trap_group[keys] + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_defaults(self, dvs, testlog): self.setup_copp(dvs) trap_keys = self.trap_atbl.getKeys() @@ -322,6 +323,7 @@ def test_defaults(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_restricted_trap_sflow(self, dvs, testlog): self.setup_copp(dvs) fvs = swsscommon.FieldValuePairs([("state", "enabled")]) @@ -350,6 +352,7 @@ def test_restricted_trap_sflow(self, dvs, testlog): assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_policer_set(self, dvs, testlog): self.setup_copp(dvs) fvs = swsscommon.FieldValuePairs([("cbs", "900")]) @@ -381,6 +384,7 @@ def test_policer_set(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_trap_group_set(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -410,6 +414,7 @@ def test_trap_group_set(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_trap_ids_set(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -465,6 +470,7 @@ def test_trap_ids_set(self, dvs, testlog): break assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_trap_action_set(self, dvs, testlog): self.setup_copp(dvs) fvs = swsscommon.FieldValuePairs([("trap_action", "copy")]) @@ -496,6 +502,7 @@ def test_trap_action_set(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_new_trap_add(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -525,6 +532,7 @@ def test_new_trap_add(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_new_trap_del(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -556,6 +564,7 @@ def test_new_trap_del(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == False + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_new_trap_group_add(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -590,6 +599,7 @@ def test_new_trap_group_add(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_new_trap_group_del(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -626,6 +636,7 @@ def test_new_trap_group_del(self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found != True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_override_trap_grp_cfg_del (self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -661,6 +672,7 @@ def test_override_trap_grp_cfg_del (self, dvs, testlog): if trap_id not in disabled_traps: assert trap_found == True + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_override_trap_cfg_del(self, dvs, testlog): self.setup_copp(dvs) global copp_trap @@ -694,6 +706,7 @@ def test_override_trap_cfg_del(self, dvs, testlog): elif trap_id == "ssh": assert trap_found == False + @pytest.mark.skip("Skip to be removed after sonic-buildimage changes get merged") def test_empty_trap_cfg(self, dvs, testlog): self.setup_copp(dvs) global copp_trap