Copp Manager Changes #1333

dgsudharsan · 2020-06-27T02:51:43Z

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

Copp Manager support
Copporch support for certain set attributes which were not present
Reconciliation logic during warm reboot in Copporch and to rename old group name to new group name
Cleaned up copporch code and split processCoppRule API into multiple APIs for better readability of code
Removed existing sflow checks in copporch code
Pytest for coppmanager infra

Copp #2 changes Copp test changes Copp changes cleanup Cosmetic change#1 Cosmetic changes #2 NAT fix Fixing nat case

dgsudharsan · 2020-06-27T02:52:17Z

Pytest Results:
sudo pytest -s -v --dvsname=vs test_copp.py
========================================================= test session starts =========================================================
platform linux2 -- Python 2.7.17, pytest-4.6.9, py-1.8.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/sudharsan/copp
plugins: flaky-3.6.1
collected 11 items

test_copp.py::TestCopp::test_defaults remove extra link dummy
PASSED
test_copp.py::TestCopp::test_restricted_trap_sflow PASSED
test_copp.py::TestCopp::test_policer_set PASSED
test_copp.py::TestCopp::test_trap_group_set PASSED
test_copp.py::TestCopp::test_trap_action_set PASSED
test_copp.py::TestCopp::test_new_trap_add PASSED
test_copp.py::TestCopp::test_new_trap_del PASSED
test_copp.py::TestCopp::test_new_trap_group_add PASSED
test_copp.py::TestCopp::test_new_trap_group_del PASSED
test_copp.py::TestCopp::test_init_group_del PASSED
test_copp.py::TestCopp::test_init_trap_del PASSED

===================================================== 11 passed in 74.12 seconds ======================================================

dgsudharsan · 2020-06-27T03:34:44Z

Manual test cases done:

Upgrade from old image without coppmgr infra to new image with coppmgr and verify COPP_GROUP entries with old name have migrated to new entry names
Perform fresh onie install and verify copp entries are populated through coppmgr
Perform feature enable/disable and verify copp group entries are added and removed for restricted trap group.

prsunny · 2020-06-30T01:19:38Z

cfgmgr/coppmgr.cpp

+
+        vector<FieldValueTuple> trap_app_fvs;
+
+        coppGroupGetModifiedFvs (i.first, trap_group_fvs, trap_app_fvs, true);


We should also support Trap delete.

prsunny · 2020-06-30T01:20:34Z

@michaelli10, can you please review?

michaelli10 · 2020-06-30T04:34:09Z

@amaneti, Can you help review as well?

into copp_changes

…table

dgsudharsan · 2020-07-03T03:48:50Z

Latest VS test update
sudo pytest -s -v --dvsname=vs test_copp.py
========================================================= test session starts =========================================================
platform linux2 -- Python 2.7.17, pytest-4.6.9, py-1.8.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/sudharsan/copp
plugins: flaky-3.6.1
collected 13 items

test_copp.py::TestCopp::test_defaults remove extra link dummy
PASSED
test_copp.py::TestCopp::test_restricted_trap_sflow PASSED
test_copp.py::TestCopp::test_policer_set PASSED
test_copp.py::TestCopp::test_trap_group_set PASSED
test_copp.py::TestCopp::test_trap_ids_set PASSED
test_copp.py::TestCopp::test_trap_action_set PASSED
test_copp.py::TestCopp::test_new_trap_add PASSED
test_copp.py::TestCopp::test_new_trap_del PASSED
test_copp.py::TestCopp::test_new_trap_group_add PASSED
test_copp.py::TestCopp::test_new_trap_group_del PASSED
test_copp.py::TestCopp::test_override_trap_grp_cfg_del PASSED
test_copp.py::TestCopp::test_override_trap_cfg_del PASSED
test_copp.py::TestCopp::test_empty_trap_cfg PASSED

===================================================== 13 passed in 82.83 seconds ======================================================

prsunny

As comments for coppmgr, copporch review is in progress..

prsunny · 2020-09-30T22:43:43Z

cfgmgr/coppmgr.h

+
+    using Orch::doTask;
+private:
+    Table                    m_cfgCoppTrapTable;


you can group similar tables to one line

prsunny · 2020-09-30T22:48:55Z

cfgmgr/coppmgr.cpp

+/* Check if the trap group has traps that can be installed only when
+ * feature is enabled
+ */
+bool CoppMgr::checkIfTrapGroupFeaturePending(string trap_group_name)


Suggest rename to checkTrapGroupPending.

prsunny · 2020-09-30T22:50:54Z

cfgmgr/coppmgr.h

+typedef std::map<std::string, std::string> CoppTrapIdTrapGroupMap;
+
+/* Trap Id to Enable/Disabled map */
+typedef std::map<std::string, bool> CoppTrapDisabledMap;


What does this mean?bool-Truemeans "disabled" ? if not, i would suggest to rename to CoppTrapStatusMap

Changed this to a set. Renamed to m_coppDisabledTrapIds

prsunny · 2020-09-30T22:56:26Z

cfgmgr/coppmgr.cpp

+                if (fvField(i) == "status")
+                {
+                    bool status = false;
+					if (fvValue(i) == "enabled")


Please align here

prsunny · 2020-09-30T23:49:49Z

cfgmgr/coppmgr.cpp

+                    }
+                }
+            }
+        }


I think this can be simplified for indentation level. if (it.second != feauture) continue

prsunny · 2020-10-01T01:27:19Z

cfgmgr/coppmgr.cpp

+ * 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, 


Why do we need this with db_migrator changes to remove the CoPP tables from APP_DB during warmboot?

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

prsunny · 2020-10-01T01:31:09Z

cfgmgr/coppmgr.cpp

+                                fvs.push_back(fv);
+                            }
+                            coppGroupGetModifiedFvs (trap_group, fvs, modified_fvs, false);
+                            if (!modified_fvs.empty())


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?

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

prsunny · 2020-10-01T01:34:27Z

cfgmgr/coppmgr.cpp

+/* The genetlink  fields are restricted and needs to be installed only on
+ * feature(sflow) enable
+ */
+bool CoppMgr::coppGroupHasRestrictedFields (vector<FieldValueTuple> &fvs)


I dont think we should have a check for Restricted Fields. Lets discuss offline

Removed this logic

prsunny · 2020-10-01T01:42:17Z

cfgmgr/coppmgr.cpp

+                }
+            }
+        }
+        setFeatureTrapIdsStatus(i, status);


I dont see this code doing anything valid in this context, since m_coppTrapIdTrapGroupMap is not initialized yet

This will remove the trap ID from disabled trap ID list.

prsunny · 2020-10-01T01:44:56Z

cfgmgr/coppmgr.cpp

+    /* 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)


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?

prsunny · 2020-10-01T01:50:11Z

@praveen-li, please review

praveen-li · 2020-10-01T05:20:33Z

Sure, I will spend 2nd half tomorrow on this. Thx. Thanks, Regards Praveen

…

________________________________ From: Prince Sunny <notifications@github.com> Sent: Wednesday, September 30, 2020 6:50 PM To: Azure/sonic-swss <sonic-swss@noreply.github.com> Cc: Praveen Chaudhary <pchaudhary@linkedin.com>; Mention <mention@noreply.github.com> Subject: Re: [Azure/sonic-swss] Copp Manager Changes (#1333) @praveen-li<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpraveen-li&data=02%7C01%7Cpchaudhary%40linkedin.com%7C325c529822c247642f9608d865ac5f50%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371138318257809&sdata=%2FsD92P%2FfGxGhiCse1OJu8mcZfg8fZL2sqRy2g%2BC7nVw%3D&reserved=0>, please review — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fsonic-swss%2Fpull%2F1333%23issuecomment-701740384&data=02%7C01%7Cpchaudhary%40linkedin.com%7C325c529822c247642f9608d865ac5f50%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371138318257809&sdata=ghAfg%2FGV35WIgSVhdjfQOV8me%2B0R5T%2BcKH4G299LMbw%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJHZHPNMP5M6I3I7UDFOI4TSIPN57ANCNFSM4OJ2FBWQ&data=02%7C01%7Cpchaudhary%40linkedin.com%7C325c529822c247642f9608d865ac5f50%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371138318267801&sdata=2H5RMkLxPLSFwBt1Er9zF3m61OSw%2BriXA8rngp2zTqE%3D&reserved=0>.

praveen-li · 2020-10-05T23:36:02Z

cfgmgr/coppmgr.cpp

+void CoppMgr::parseInitFile(void)
+{
+    std::ifstream ifs(COPP_INIT_FILE);
+    if (ifs.fail())


this is one-time processing then, should we log an error message here before returning?

praveen-li · 2020-10-05T23:44:54Z

cfgmgr/coppmgr.cpp

+    {
+        return; 
+    }
+    json j = json::parse(ifs);


the jinja template for json file is committed via another PR or will be done later?, wanted to check format of this json

Okie, got 5053 and 1366.

praveen-li · 2020-10-06T00:33:03Z

cfgmgr/coppmgr.cpp

+    {
+        string table_name = tbl.key();
+        json keys = tbl.value();
+        for (auto k = keys.begin(); k != keys.end(); k++)


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?

Yes. Correct

praveen-li · 2020-10-06T00:39:37Z

cfgmgr/coppmgr.cpp

+
+    for (auto it: m_coppTrapIdTrapGroupMap)
+    {
+        if (it.second == trap_group_name)


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.

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.

praveen-li · 2020-10-06T01:34:10Z

cfgmgr/coppmgr.cpp

+                             (checkIfTrapGroupFeaturePending(trap_group)))
+                        {
+                            m_appCoppTable.del(trap_group);
+                            delCoppGroupStateOk(trap_group);


This changes just the state DB right, should we update state DB only after successfully process the COPP for the feature.

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.

praveen-li · 2020-10-06T01:44:39Z

cfgmgr/coppmgr.cpp

+        bool status = false;
+        for (auto j: feature_fvs)
+        {
+            if (fvField(j) == "status")


We can merge this kind of 2 conditions. ( fvField(j) == "status" && fvValue(j) == "enabled" )

…p_changes

… than num ports

prsunny · 2020-11-04T06:39:10Z

cfgmgr/coppmgr.cpp

+        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())


Didn't get why we need to traverse trap_id list for disabled traps?

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

prsunny · 2020-11-04T06:53:45Z

cfgmgr/coppmgr.cpp

+            }
+            if (null_cfg)
+            {
+                if (!m_coppTrapConfMap[key].trap_group.empty())


what if m_coppTrapConfMap[key] is empty?

If empty there is no trap group associated and no action need to be taken. This is for handling duplicate case.

As discussed, lets check for key present?

prsunny · 2020-11-04T06:54:16Z

cfgmgr/coppmgr.cpp

+                    removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group,
+                            m_coppTrapConfMap[key].trap_ids);
+                    trap_ids.clear();
+                    setCoppTrapStateOk(key);


is this remove sequence?

Yes. Remove sequence of NULL key

prsunny · 2020-11-04T07:03:10Z

orchagent/copporch.cpp

+/* 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,


Do we still need this since we delete the entries from APP_DB during warmboot?

prsunny · 2020-11-04T07:03:37Z

orchagent/copporch.cpp

+     */
+    if (trap_ids.size() == 0)
+    {
+        if (!gIsNatSupported)


Why the NAT check?

This was added earlier for warmboot compatibility. Since now we erase the appDB table during warmboot we can remove it.

prsunny · 2020-11-04T21:15:34Z

cfgmgr/coppmgr.cpp

+    m_cfgCoppGroupTable.getKeys(group_cfg_keys);
+    m_cfgCoppTrapTable.getKeys(trap_cfg_keys);
+    m_cfgFeatureTable.getKeys(feature_keys);
+    m_coppTable.getKeys(app_keys);


We can remove this app keys

prsunny · 2020-11-04T21:53:25Z

cfgmgr/coppmgr.cpp

+            }
+            if (null_cfg)
+            {
+                if (!m_coppTrapConfMap[key].trap_group.empty())


As discussed, lets check for key present?

prsunny · 2020-11-04T22:02:17Z

cfgmgr/coppmgr.cpp

+                    {
+                        m_appCoppTable.set(m_coppTrapConfMap[key].trap_group, fvs);
+                    }
+                    setCoppGroupStateOk(m_coppTrapConfMap[key].trap_group);


This must be inside the if case

prsunny · 2020-11-04T22:04:24Z

cfgmgr/coppmgr.cpp

+                it = consumer.m_toSync.erase(it);
+                continue;
+            }
+            removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group,


Add a comment for explaining why the trapid is removed

prsunny · 2020-11-04T22:12:57Z

cfgmgr/coppmgr.cpp

+             * should also be reprogrammed as some of its associated traps got
+             * removed
+             */
+            if ((!m_coppTrapConfMap[key].trap_group.empty()) &&


Also check for conf_present before accessing the key.

prsunny · 2020-11-04T22:19:28Z

orchagent/copporch.h

 protected:
    object_map m_trap_group_map;
-    bool       enable_sflow_trap;
+    std::unique_ptr<swss::Table> m_coppTable;


Please remove this if not used

prsunny · 2020-11-04T22:25:00Z

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";


Please remove

prsunny · 2020-11-04T22:28:21Z

orchagent/copporch.cpp

+
+	for (auto it : m_syncdTrapIds)
+	{
+		if (it.second.trap_group_obj == m_trap_group_map[trap_group_name])


Please align code

into copp_changes

prsunny

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>

Copp Changes

353becb

Copp #2 changes Copp test changes Copp changes cleanup Cosmetic change#1 Cosmetic changes #2 NAT fix Fixing nat case

dgsudharsan changed the title ~~Copp Changes~~ Copp Manager Changes Jun 27, 2020

prsunny reviewed Jun 30, 2020

View reviewed changes

Sudharsan D.G and others added 5 commits July 1, 2020 10:13

Merge branch 'master' into copp_changes

7a2abc9

Handling empty trap set for trap delete

f3d6193

Merge branch 'copp_changes' of https://github.com/dgsudharsan/sonic-swss

90ad1ca

into copp_changes

Fixing the case where trapid has been removed from trap list in trap …

585ca80

…table

Adding test case for trap table trap ids set

5a01337

dgsudharsan added 4 commits July 24, 2020 09:58

Removing 00-copp.config.json

610d197

Removing copp json file from swss.install

0634de2

Fix for populating state db for default entries

972d004

Merge remote-tracking branch 'upstream/master' into copp_changes

41ab9ff

prsunny reviewed Oct 1, 2020

View reviewed changes

praveen-li reviewed Oct 6, 2020

View reviewed changes

dgsudharsan added 7 commits October 9, 2020 16:53

Addressing code review comments

0ae6d98

Addressing code review comments

5c6a49a

Addressing code review comments

33197d2

Merge branch 'master' of https://github.com/Azure/sonic-swss into cop…

b85196d

…p_changes

Addressing field changes in feature table

e3a6c7a

Updating UT with feature table modifications

1fcfd2d

Since copp creates sflow as hostif the num host interfaces is greater…

414375a

… than num ports

prsunny reviewed Nov 4, 2020

View reviewed changes

lguohan added the copp label Nov 4, 2020

prsunny reviewed Nov 4, 2020

View reviewed changes

dgsudharsan added 2 commits November 5, 2020 14:27

Addressing code review comments

67edb8b

Merge branch 'copp_changes' of https://github.com/dgsudharsan/sonic-swss

fbb582e

into copp_changes

prsunny approved these changes Nov 6, 2020

View reviewed changes

prsunny merged commit 7b76d2e into sonic-net:master Nov 11, 2020

dgsudharsan deleted the copp_changes branch March 9, 2023 02:00


		vector<FieldValueTuple> trap_app_fvs;

		coppGroupGetModifiedFvs (i.first, trap_group_fvs, trap_app_fvs, true);

Copp Manager Changes #1333

Copp Manager Changes #1333

Conversation

dgsudharsan commented Jun 27, 2020

dgsudharsan commented Jun 27, 2020

dgsudharsan commented Jun 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Jun 30, 2020

michaelli10 commented Jun 30, 2020

dgsudharsan commented Jul 3, 2020

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Oct 1, 2020

praveen-li commented Oct 1, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny left a comment

Choose a reason for hiding this comment