-
Notifications
You must be signed in to change notification settings - Fork 522
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.
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 Manager Changes #1333
Copp Manager Changes #1333
Conversation
Pytest Results: test_copp.py::TestCopp::test_defaults remove extra link dummy ===================================================== 11 passed in 74.12 seconds ====================================================== |
Manual test cases done:
|
cfgmgr/coppmgr.cpp
Outdated
|
||
vector<FieldValueTuple> trap_app_fvs; | ||
|
||
coppGroupGetModifiedFvs (i.first, trap_group_fvs, trap_app_fvs, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also support Trap delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@michaelli10, can you please review? |
@amaneti, Can you help review as well? |
Latest VS test update test_copp.py::TestCopp::test_defaults remove extra link dummy ===================================================== 13 passed in 82.83 seconds ====================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments for coppmgr, copporch review is in progress..
cfgmgr/coppmgr.h
Outdated
|
||
using Orch::doTask; | ||
private: | ||
Table m_cfgCoppTrapTable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can group similar tables to one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.cpp
Outdated
/* Check if the trap group has traps that can be installed only when | ||
* feature is enabled | ||
*/ | ||
bool CoppMgr::checkIfTrapGroupFeaturePending(string trap_group_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest rename to checkTrapGroupPending
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.h
Outdated
typedef std::map<std::string, std::string> CoppTrapIdTrapGroupMap; | ||
|
||
/* Trap Id to Enable/Disabled map */ | ||
typedef std::map<std::string, bool> CoppTrapDisabledMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?bool-True
means "disabled" ? if not, i would suggest to rename to CoppTrapStatusMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to a set. Renamed to m_coppDisabledTrapIds
cfgmgr/coppmgr.cpp
Outdated
if (fvField(i) == "status") | ||
{ | ||
bool status = false; | ||
if (fvValue(i) == "enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified for indentation level. if (it.second != feauture) continue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.cpp
Outdated
* 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<FieldValueTuple> &trap_group_fvs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this with db_migrator changes to remove the CoPP tables from APP_DB during warmboot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is not just used in warm reboot but also in general set. When a set is done in config DB, we do not get just the modified fields but rather all the key value pairs. Thus we would be doing redundant set operations into app db. This will also create issues when the ASIC DB has create only fields and the set to APP_DB triggers set operation. This API would just set the modified key value pairs into app DB and thus preventing such issues
cfgmgr/coppmgr.cpp
Outdated
fvs.push_back(fv); | ||
} | ||
coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false); | ||
if (!modified_fvs.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this. Why do we check for modified_fvs? If a feature is enabled, just add that to the trap_id list right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as explained above. The modified fvs would program either the full table if not already programmed or trap ids if only trap ids change
cfgmgr/coppmgr.cpp
Outdated
/* The genetlink fields are restricted and needs to be installed only on | ||
* feature(sflow) enable | ||
*/ | ||
bool CoppMgr::coppGroupHasRestrictedFields (vector<FieldValueTuple> &fvs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we should have a check for Restricted Fields. Lets discuss offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this logic
cfgmgr/coppmgr.cpp
Outdated
} | ||
} | ||
} | ||
setFeatureTrapIdsStatus(i, status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see this code doing anything valid in this context, since m_coppTrapIdTrapGroupMap
is not initialized yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will remove the trap ID from disabled trap ID list.
cfgmgr/coppmgr.cpp
Outdated
/* 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this to another function for clarity?. The constructor has become pretty big and check if you can reuse the same for m_coppGroupInitCfg as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@praveen-li, please review |
void CoppMgr::parseInitFile(void) | ||
{ | ||
std::ifstream ifs(COPP_INIT_FILE); | ||
if (ifs.fail()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is one-time processing then, should we log an error message here before returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
return; | ||
} | ||
json j = json::parse(ifs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the jinja template for json file is committed via another PR or will be done later?, wanted to check format of this json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okie, got 5053 and 1366.
{ | ||
string table_name = tbl.key(); | ||
json keys = tbl.value(); | ||
for (auto k = keys.begin(); k != keys.end(); k++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for "COPP_TRAP|lldp": {
"trap_ids": "lldp",
"trap_group": "queue4_group2"
},
table_name = COPP_TRAP and keys will have [lldp]... and then rest are in FV....hope my assumption is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Correct
|
||
for (auto it: m_coppTrapIdTrapGroupMap) | ||
{ | ||
if (it.second == trap_group_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_coppTrapIdTrapGroupMap == {COPP_TRAP_TYPE_SAMPLEPACKET, "sflow"} right, if yes then,
Any reason, why sflow is not the key here. .......from further code, I think my assumption is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not trapID to feature map. This is trap ID to trap group map which gets generated from COPP_GROUP and COPP_TRAP tables. Let me know if you need any more clarifications.
cfgmgr/coppmgr.cpp
Outdated
(checkIfTrapGroupFeaturePending(trap_group))) | ||
{ | ||
m_appCoppTable.del(trap_group); | ||
delCoppGroupStateOk(trap_group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes just the state DB right, should we update state DB only after successfully process the COPP for the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The statedb reflects whether a trap has been installed or not. Sometimes the trap might be present but might not have installed due to the feature being disabled. The statedb will therefore will not contain the state for the trap. Same applies to trap group.
cfgmgr/coppmgr.cpp
Outdated
bool status = false; | ||
for (auto j: feature_fvs) | ||
{ | ||
if (fvField(j) == "status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this kind of 2 conditions. ( fvField(j) == "status" && fvValue(j) == "enabled" )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
vector<string> 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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get why we need to traverse trap_id list for disabled traps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_coppDisabledTraps is not trap ids but COPP_TRAP key. This will contain list of trap ids. Thus to find if a trap id is disabled, we need to go through the list of disabled traps and iterate through their trap ids
cfgmgr/coppmgr.cpp
Outdated
} | ||
if (null_cfg) | ||
{ | ||
if (!m_coppTrapConfMap[key].trap_group.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if m_coppTrapConfMap[key] is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If empty there is no trap group associated and no action need to be taken. This is for handling duplicate case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, lets check for key present?
removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, | ||
m_coppTrapConfMap[key].trap_ids); | ||
trap_ids.clear(); | ||
setCoppTrapStateOk(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this remove sequence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Remove sequence of NULL key
orchagent/copporch.cpp
Outdated
/* 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this since we delete the entries from APP_DB during warmboot?
orchagent/copporch.cpp
Outdated
*/ | ||
if (trap_ids.size() == 0) | ||
{ | ||
if (!gIsNatSupported) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the NAT check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added earlier for warmboot compatibility. Since now we erase the appDB table during warmboot we can remove it.
cfgmgr/coppmgr.cpp
Outdated
m_cfgCoppGroupTable.getKeys(group_cfg_keys); | ||
m_cfgCoppTrapTable.getKeys(trap_cfg_keys); | ||
m_cfgFeatureTable.getKeys(feature_keys); | ||
m_coppTable.getKeys(app_keys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this app keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.cpp
Outdated
} | ||
if (null_cfg) | ||
{ | ||
if (!m_coppTrapConfMap[key].trap_group.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, lets check for key present?
cfgmgr/coppmgr.cpp
Outdated
{ | ||
m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs); | ||
} | ||
setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be inside the if case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.cpp
Outdated
it = consumer.m_toSync.erase(it); | ||
continue; | ||
} | ||
removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment for explaining why the trapid is removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cfgmgr/coppmgr.cpp
Outdated
* should also be reprogrammed as some of its associated traps got | ||
* removed | ||
*/ | ||
if ((!m_coppTrapConfMap[key].trap_group.empty()) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check for conf_present
before accessing the key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
orchagent/copporch.h
Outdated
protected: | ||
object_map m_trap_group_map; | ||
bool enable_sflow_trap; | ||
std::unique_ptr<swss::Table> m_coppTable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this if not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
orchagent/copporch.cpp
Outdated
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
orchagent/copporch.cpp
Outdated
|
||
for (auto it : m_syncdTrapIds) | ||
{ | ||
if (it.second.trap_group_obj == m_trap_group_map[trap_group_name]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dgsudharsan , @praveen-li , could you please re-review/sign-off?
Introduce Copp Manager Subscribe to feature table and install trap based on feature status Support delete of Copp traps Co-authored-by: dgsudharsan <sudharsan_gopalarat@dell.com>
What I did
Added changes to Introduce Copp Manager
Why I did it
The Copp infra need to subscribe to features and hence manager logic is introduced
How I verified it
The verification is done using manual testing and pytests.
Details if related
This Pull request contains the changes for