Skip to content
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

[COPP]Fix coppmgr when there are multiple features using same trap #42

Closed
wants to merge 1 commit into from
Closed

[COPP]Fix coppmgr when there are multiple features using same trap #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions cfgmgr/coppmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,13 @@ bool CoppMgr::isTrapIdDisabled(string trap_id)
{
return false;
}
break;
if (isFeatureEnabled(trap_name))
Copy link

Choose a reason for hiding this comment

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

So basically, to install a Trap we are checking if any of the dependent features are enabled. Previously the assumption is a 1<->1 mapping between trap and feature. Correct me if i'm wrong

Copy link
Owner Author

Choose a reason for hiding this comment

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

Correct. Now if any feature containing the trap is enabled the trap should be installed. Recently there is a new feature pac which uses eapol trap same as macsec. But if macsec is not enabled this feature didn't work as trap is getting installed with only macsec. With this change the issue will be resolved.

Copy link

Choose a reason for hiding this comment

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

ok, sounds good

{
return false;
}
}
}

if (isFeatureEnabled(trap_name))
{
return false;
}
return true;
}

Expand Down Expand Up @@ -941,7 +940,9 @@ void CoppMgr::doFeatureTask(Consumer &consumer)
{
if (m_featuresCfgTable.find(key) == m_featuresCfgTable.end())
{
m_featuresCfgTable.emplace(key, kfvFieldsValues(t));
// Init with empty feature state which will be updated in setFeatureTrapIdsStatus
FieldValueTuple fv("state", "");
m_featuresCfgTable[key].push_back(fv);
}
for (auto i : kfvFieldsValues(t))
{
Expand Down
2 changes: 1 addition & 1 deletion orchagent/copporch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer)
if (!trap_id_attribs.empty())
{
vector<sai_hostif_trap_type_t> group_trap_ids;
TrapIdAttribs trap_attr;
TrapIdAttribs trap_attr = m_trap_group_trap_id_attrs[trap_group_name];
getTrapIdsFromTrapGroup(m_trap_group_map[trap_group_name],
group_trap_ids);
for (auto trap_id : group_trap_ids)
Expand Down
35 changes: 35 additions & 0 deletions tests/test_copp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,3 +841,38 @@ def test_disabled_feature_always_enabled_trap(self, dvs, testlog):
self.feature_tbl.set("lldp", fvs)

assert table_found == False

def test_multi_feature_trap_add(self, dvs, testlog):
self.setup_copp(dvs)
global copp_trap
traps = "eapol"
fvs = swsscommon.FieldValuePairs([("state", "disbled")])
self.feature_tbl.set("macsec", fvs)
fvs = swsscommon.FieldValuePairs([("state", "enabled")])
self.feature_tbl.set("pac", fvs)
fvs = swsscommon.FieldValuePairs([("trap_group", "queue4_group1"),("trap_ids", traps)])
self.trap_ctbl.set("pac", fvs)


copp_trap["eapol"] = [traps, copp_group_queue4_group1]
time.sleep(2)

trap_keys = self.trap_atbl.getKeys()
trap_ids = traps.split(",")
trap_group = copp_group_queue4_group1
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 disabled_traps:
assert trap_found == True
Loading