Skip to content

[CoPP] Add redesign change - always_enabled field #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
noaOrMlnx wants to merge 5 commits into from
Closed

[CoPP] Add redesign change - always_enabled field #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f22615d
[CoPP] Add always_enabled field to coppmgr logic
noaOrMlnx Nov 7, 2021
d31852d
[CoPP] Add always_enabled field to coppmgr logic
noaOrMlnx Nov 7, 2021
37df59f
Fix CR comments
noaOrMlnx Nov 11, 2021
1def31f
Fix CR comments
noaOrMlnx Nov 11, 2021
3182ebf
Merge branch 'noaor_copp_redesign' of github.com:noaOrMlnx/sonic-swss…
noaOrMlnx Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 152 additions & 32 deletions cfgmgr/coppmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,17 @@ void CoppMgr::setFeatureTrapIdsStatus(string feature, bool enable)

if (!enable)
{
m_coppDisabledTraps.insert(feature);
if (m_coppTrapConfMap[feature].is_always_enabled != "true")
{
m_coppDisabledTraps.insert(feature);
}
}
else
{
m_coppDisabledTraps.erase(feature);
if (m_coppDisabledTraps.find(feature) != m_coppDisabledTraps.end())
{
m_coppDisabledTraps.erase(feature);
}
}

/* Trap group moved to pending state when feature is disabled. Remove trap group
Expand Down Expand Up @@ -159,6 +165,7 @@ bool CoppMgr::isTrapIdDisabled(string trap_id)
}
return false;
}

void CoppMgr::mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector<std::string> &cfg_keys, Table &cfgTable)
{
/* Read the init configuration first. If the same key is present in
Expand Down Expand Up @@ -254,6 +261,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
{
std::vector<FieldValueTuple> feature_fvs;
m_cfgFeatureTable.get(i, feature_fvs);
m_featuresCfgTable.emplace(i, feature_fvs);

for (auto j: feature_fvs)
{
Expand All @@ -264,12 +272,35 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
}
}

/* If there is a trap that has always_enabled = true, remove it from the disabledTraps list */
for (auto trap: m_coppTrapInitCfg)
{
auto trap_name = trap.first;
auto trap_info = trap.second;
bool always_enabled = false;

if (std::find(trap_info.begin(), trap_info.end(), FieldValueTuple("always_enabled", "true")) != trap_info.end())
{
always_enabled = true;
if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), trap_name) != m_coppDisabledTraps.end())
{
m_coppDisabledTraps.erase(trap_name);
}
}
/* if trap has no feature entry and doesn't have the always_enabled:true field, add to disabledTraps list */
if (!(std::count(feature_keys.begin(), feature_keys.end(), trap_name)) && !always_enabled)
{
m_coppDisabledTraps.insert(trap_name);
}
}

mergeConfig(m_coppTrapInitCfg, trap_cfg, trap_cfg_keys, m_cfgCoppTrapTable);

for (auto i : trap_cfg)
{
string trap_group;
string trap_ids;
string is_always_enabled = "false";
std::vector<FieldValueTuple> trap_fvs = i.second;

for (auto j: trap_fvs)
Expand All @@ -282,13 +313,22 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
{
trap_group = fvValue(j);
}
else if (fvField(j) == COPP_ALWAYS_ENABLED_FIELD)
{
is_always_enabled = fvValue(j);
}
}

if (!trap_group.empty() && !trap_ids.empty())
{
addTrapIdsToTrapGroup(trap_group, trap_ids);
m_coppTrapConfMap[i.first].trap_group = trap_group;
m_coppTrapConfMap[i.first].trap_ids = trap_ids;
setCoppTrapStateOk(i.first);
m_coppTrapConfMap[i.first].is_always_enabled = is_always_enabled;
if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end())
Copy link
Copy Markdown

@dgsudharsan dgsudharsan Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this if check is needed here. It may be needed only for setCoppTrapStateOk. The configs need to be captured in the global variables but should not programmed.

Copy link
Copy Markdown
Owner Author

@noaOrMlnx noaOrMlnx Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
setCoppTrapStateOk(i.first);
}
}
}

Expand Down Expand Up @@ -406,6 +446,36 @@ void CoppMgr::getTrapGroupTrapIds(string trap_group, string &trap_ids)
}
}

void CoppMgr::removeTrap(std::string key)
{
std::string trap_ids;
std::vector<FieldValueTuple> fvs;
removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids);
getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids);
FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids);
fvs.push_back(fv);
if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group))
{
m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs);
setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group);
}
}

void CoppMgr::addTrap(std::string trap_ids, std::string trap_group)
{
string trap_group_trap_ids;
std::vector<FieldValueTuple> fvs;
addTrapIdsToTrapGroup(trap_group, trap_ids);
getTrapGroupTrapIds(trap_group, trap_group_trap_ids);
FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids);
fvs.push_back(fv1);
if (!checkTrapGroupPending(trap_group))
{
m_appCoppTable.set(trap_group, fvs);
setCoppGroupStateOk(trap_group);
}
}

void CoppMgr::doCoppTrapTask(Consumer &consumer)
{
auto it = consumer.m_toSync.begin();
Expand All @@ -418,12 +488,21 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
vector<FieldValueTuple> fvs;
string trap_ids = "";
string trap_group = "";
string is_always_enabled = "";
bool conf_present = false;

if (m_coppTrapConfMap.find(key) != m_coppTrapConfMap.end())
{
trap_ids = m_coppTrapConfMap[key].trap_ids;
trap_group = m_coppTrapConfMap[key].trap_group;
if (m_coppTrapConfMap[key].is_always_enabled.empty())
{
is_always_enabled = "false";
}
else
{
is_always_enabled = m_coppTrapConfMap[key].is_always_enabled;
}
conf_present = true;
}

Expand All @@ -441,6 +520,10 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
{
trap_ids = fvValue(i);
}
else if (fvField(i) == COPP_ALWAYS_ENABLED_FIELD)
{
is_always_enabled = fvValue(i);
}
else if (fvField(i) == "NULL")
{
null_cfg = true;
Expand All @@ -450,20 +533,8 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
{
if (conf_present)
{
SWSS_LOG_DEBUG("Deleting trap key %s", key.c_str());
removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group,
m_coppTrapConfMap[key].trap_ids);
trap_ids.clear();
removeTrap(key);
setCoppTrapStateOk(key);
getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_ids);
fvs.clear();
FieldValueTuple fv(COPP_TRAP_ID_LIST_FIELD, trap_ids);
fvs.push_back(fv);
if (!checkTrapGroupPending(m_coppTrapConfMap[key].trap_group))
{
m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs);
setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group);
}
m_coppTrapConfMap.erase(key);
}
it = consumer.m_toSync.erase(it);
Expand All @@ -472,11 +543,13 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
/*Duplicate check*/
if (conf_present &&
(trap_group == m_coppTrapConfMap[key].trap_group) &&
(trap_ids == m_coppTrapConfMap[key].trap_ids))
(trap_ids == m_coppTrapConfMap[key].trap_ids) &&
(is_always_enabled == m_coppTrapConfMap[key].is_always_enabled))
{
it = consumer.m_toSync.erase(it);
continue;
}

/* Incomplete configuration. Do not process until both trap group
* and trap_ids are available
*/
Expand All @@ -485,33 +558,73 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
it = consumer.m_toSync.erase(it);
continue;
}

/* if always_enabled field has been changed */
if (conf_present &&
Copy link
Copy Markdown

@dgsudharsan dgsudharsan Nov 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pkease move the code below the if code
/* 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;
}

Else if there is incomplete configuration there might be a crash

Copy link
Copy Markdown
Owner Author

@noaOrMlnx noaOrMlnx Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

(trap_group == m_coppTrapConfMap[key].trap_group) &&
(trap_ids == m_coppTrapConfMap[key].trap_ids))
{
std::vector<FieldValueTuple> feature_fvs;
if (is_always_enabled == "true")
{
/* if the value was changed from false to true,
if the trap is not installed, install it.
otherwise, do nothing. */

if (m_coppTrapConfMap.find(key) == m_coppTrapConfMap.end())
{
addTrap(trap_ids, trap_group);
}
}
else
{
/* if the value was changed from true to false,
check if there is a feature enabled.
if no, remove the trap. is yes, do nothing. */

if (m_featuresCfgTable.find(key) != m_featuresCfgTable.end())
{
bool isEnabled {false};
feature_fvs = m_featuresCfgTable[key];
for (auto i: feature_fvs)
{
if (fvField(i) == "state" && fvValue(i) == "enabled")
{
isEnabled = true;
it = consumer.m_toSync.erase(it);
continue;
Copy link
Copy Markdown

@dgsudharsan dgsudharsan Nov 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'continue' will be applicable for the immediately preceding for loop. So this logic will remove trap even if enabled. Can you double check?

Copy link
Copy Markdown
Owner Author

@noaOrMlnx noaOrMlnx Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}
if (isEnabled)
{
continue;
}
}

removeTrap(key);
delCoppTrapStateOk(key);
m_coppTrapConfMap.erase(key);
m_coppDisabledTraps.insert(key);
}
}

/* Remove the current trap IDs and add the new trap IDS to recompute the
* trap IDs for the trap group
* trap IDs for the trap group
*/
if (conf_present)
{
removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group,
m_coppTrapConfMap[key].trap_ids);
}
fvs.clear();
string trap_group_trap_ids;
addTrapIdsToTrapGroup(trap_group, trap_ids);
getTrapGroupTrapIds(trap_group, trap_group_trap_ids);
FieldValueTuple fv1(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids);
fvs.push_back(fv1);
if (!checkTrapGroupPending(trap_group))
{
m_appCoppTable.set(trap_group, fvs);
setCoppGroupStateOk(trap_group);
}
addTrap(trap_ids, trap_group);

/* When the trap table's trap group is changed, the old trap group
* should also be reprogrammed as some of its associated traps got
* removed
*/
if (conf_present && (trap_group != m_coppTrapConfMap[key].trap_group))
{
trap_group_trap_ids.clear();
string trap_group_trap_ids;
fvs.clear();
getTrapGroupTrapIds(m_coppTrapConfMap[key].trap_group, trap_group_trap_ids);
FieldValueTuple fv2(COPP_TRAP_ID_LIST_FIELD, trap_group_trap_ids);
Expand All @@ -524,6 +637,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
}
m_coppTrapConfMap[key].trap_group = trap_group;
m_coppTrapConfMap[key].trap_ids = trap_ids;
m_coppTrapConfMap[key].is_always_enabled = is_always_enabled;
setCoppTrapStateOk(key);
}
else if (op == DEL_COMMAND)
Expand All @@ -546,7 +660,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group);
}
}
if (conf_present)
if (conf_present)
{
m_coppTrapConfMap.erase(key);
}
Expand All @@ -569,6 +683,10 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
{
trap_ids = fvValue(i);
}
else if (fvField(i) == COPP_ALWAYS_ENABLED_FIELD)
{
is_always_enabled = fvValue(i);
}
}
vector<FieldValueTuple> g_fvs;
string trap_group_trap_ids;
Expand All @@ -583,6 +701,7 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
}
m_coppTrapConfMap[key].trap_group = trap_group;
m_coppTrapConfMap[key].trap_ids = trap_ids;
m_coppTrapConfMap[key].is_always_enabled = is_always_enabled;
setCoppTrapStateOk(key);
}
}
Expand Down Expand Up @@ -706,6 +825,7 @@ void CoppMgr::doCoppGroupTask(Consumer &consumer)
}
}


void CoppMgr::doFeatureTask(Consumer &consumer)
{
auto it = consumer.m_toSync.begin();
Expand All @@ -715,11 +835,11 @@ void CoppMgr::doFeatureTask(Consumer &consumer)

string key = kfvKey(t);
string op = kfvOp(t);
vector<FieldValueTuple> fvs;
string trap_ids;

if (op == SET_COMMAND)
{
m_featuresCfgTable.emplace(key, kfvFieldsValues(t));
for (auto i : kfvFieldsValues(t))
{
if (fvField(i) == "state")
Expand Down
8 changes: 7 additions & 1 deletion cfgmgr/coppmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace swss {
/* COPP Trap Table Fields */
#define COPP_TRAP_ID_LIST_FIELD "trap_ids"
#define COPP_TRAP_GROUP_FIELD "trap_group"
#define COPP_ALWAYS_ENABLED_FIELD "always_enabled"

/* COPP Group Table Fields */
#define COPP_GROUP_QUEUE_FIELD "queue"
Expand Down Expand Up @@ -42,6 +43,7 @@ struct CoppTrapConf
{
std::string trap_ids;
std::string trap_group;
std::string is_always_enabled;
};

/* TrapName to TrapConf map */
Expand Down Expand Up @@ -73,7 +75,8 @@ class CoppMgr : public Orch
std::set<std::string> m_coppDisabledTraps;
CoppCfg m_coppGroupInitCfg;
CoppCfg m_coppTrapInitCfg;

CoppCfg m_featuresCfgTable;


void doTask(Consumer &consumer);
void doCoppGroupTask(Consumer &consumer);
Expand All @@ -98,6 +101,9 @@ class CoppMgr : public Orch
bool isTrapGroupInstalled(std::string key);
void mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector<std::string> &cfg_keys, Table &cfgTable);

void removeTrap(std::string key);
void addTrap(std::string trap_ids, std::string trap_group);

};

}
Loading