Modify coppmgr mergeConfig to support preserving copp tables through reboot. (#2548)

arfeigin · web-flow · commit bdedf694f10b · 2023-01-01T17:10:35.000+02:00
This PR should be merged together with sonic-net/sonic-utilities#2524 and is required to 202205 and 202211. This PR implements [fastboot] Preserve CoPP table HLD to improve fastboot flow (sonic-net/SONiC#1107). - What I did Modified coppmgr mergeConfig logic to support preserving copp tables contents through reboot. Handle duplicates, overwrites, unsupported keys preserved and new keys. According to sonic-net/SONiC#1107 - Why I did it To shorten dataplane downtime on fast-reboot. - How I verified it Manual tests (community fast-reboot test)
diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp
@@ -9,6 +9,8 @@
 #include "shellcmd.h"
 #include "warm_restart.h"
 #include "json.hpp"
+#include <unordered_map>
+#include <unordered_set>
 
 using json = nlohmann::json;
 
@@ -255,6 +257,42 @@ void CoppMgr::mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector<std::st
     }
 }
 
+bool CoppMgr::isDupEntry(const std::string &key, std::vector<FieldValueTuple> &fvs)
+{
+    /* Compare with the existing contents of copp tables, in case for a key K preserved fvs are the same
+     * as the fvs in trap_group_fvs it will be ignored as a duplicate continue to next key.
+     * In case one of the fvs differs the preserved entry will be deleted and new entry will be set instead.
+     */
+    std::vector<FieldValueTuple> preserved_fvs;
+    bool key_found = m_coppTable.get(key, preserved_fvs);
+    if (!key_found)
+    {
+        return false;
+    }
+    else
+    {
+        unordered_map<string, string> preserved_copp_entry;
+        for (auto prev_fv : preserved_fvs)
+        {
+            preserved_copp_entry[fvField(prev_fv)] = fvValue(prev_fv);
+        }
+        for (auto fv: fvs)
+        {
+            string field = fvField(fv);
+            string value = fvValue(fv);
+            auto preserved_copp_it = preserved_copp_entry.find(field);
+            bool field_found = (preserved_copp_it != preserved_copp_entry.end());
+            if ((!field_found) || (field_found && preserved_copp_it->second.compare(value)))
+            {
+                // overwrite -> delete preserved entry from copp table and set a new entry instead
+                m_coppTable.del(key);
+                return false;
+            }
+        }
+    }
+    return true;
+}
+
 CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) :
         Orch(cfgDb, tableNames),
         m_cfgCoppTrapTable(cfgDb, CFG_COPP_TRAP_TABLE_NAME),
@@ -270,16 +308,19 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
     std::vector<string> group_keys;
     std::vector<string> trap_keys;
     std::vector<string> feature_keys;
+    std::vector<string> preserved_copp_keys;
 
     std::vector<string> group_cfg_keys;
     std::vector<string> trap_cfg_keys;
+    unordered_set<string> supported_copp_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(preserved_copp_keys);
 
 
     for (auto i: feature_keys)
@@ -352,15 +393,31 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
 
         if (!trap_group_fvs.empty())
         {
+            supported_copp_keys.emplace(i.first);
+            if (isDupEntry(i.first, trap_group_fvs))
+            {
+                continue;
+            }
             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);
         if (g_cfg != group_cfg_keys.end())
         {
             g_copp_init_set.insert(i.first);
         }
     }
+
+    // Delete unsupported keys from preserved copp tables
+    for (auto it : preserved_copp_keys)
+    {
+        auto copp_it = supported_copp_keys.find(it);
+        if (copp_it == supported_copp_keys.end())
+        {
+            m_coppTable.del(it);
+        }
+    }
 }
 
 void CoppMgr::setCoppGroupStateOk(string alias)
diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h
@@ -100,6 +100,7 @@ class CoppMgr : public Orch
     bool isTrapGroupInstalled(std::string key);
     bool isFeatureEnabled(std::string feature);
     void mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector<std::string> &cfg_keys, Table &cfgTable);
+    bool isDupEntry(const std::string &key, std::vector<FieldValueTuple> &fvs);
 
     void removeTrap(std::string key);
     void addTrap(std::string trap_ids, std::string trap_group);
diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am
@@ -30,6 +30,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 bufferorch_ut.cpp \
                 buffermgrdyn_ut.cpp \
                 fdborch/flush_syncd_notif_ut.cpp \
+                copp_ut.cpp \
                 copporch_ut.cpp \
                 saispy_ut.cpp \
                 consumer_ut.cpp \
diff --git a/tests/mock_tests/copp_cfg.json b/tests/mock_tests/copp_cfg.json
@@ -0,0 +1,111 @@
+{
+    "COPP_GROUP": {
+	    "default": {
+		    "queue": "0",
+		    "meter_type":"packets",
+		    "mode":"sr_tcm",
+		    "cir":"600",
+		    "cbs":"600",
+		    "red_action":"drop"
+	    },
+	    "queue4_group1": {
+		    "trap_action":"trap",
+		    "trap_priority":"4",
+		    "queue": "4"
+	    },
+	    "queue4_group2": {
+		    "trap_action":"copy",
+		    "trap_priority":"4",
+		    "queue": "4",
+		    "meter_type":"packets",
+		    "mode":"sr_tcm",
+		    "cir":"600",
+		    "cbs":"600",
+		    "red_action":"drop"
+	    },
+	    "queue4_group3": {
+		    "trap_action":"trap",
+		    "trap_priority":"4",
+		    "queue": "4"
+	    },
+	    "queue1_group1": {
+		    "trap_action":"trap",
+		    "trap_priority":"1",
+		    "queue": "1",
+		    "meter_type":"packets",
+		    "mode":"sr_tcm",
+		    "cir":"6000",
+		    "cbs":"6000",
+		    "red_action":"drop"
+	    },
+	    "queue1_group2": {
+		    "trap_action":"trap",
+		    "trap_priority":"1",
+		    "queue": "1",
+		    "meter_type":"packets",
+		    "mode":"sr_tcm",
+		    "cir":"600",
+		    "cbs":"600",
+		    "red_action":"drop"
+	    },
+	    "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_TRAP": {
+	    "bgp": {
+		    "trap_ids": "bgp,bgpv6",
+		    "trap_group": "queue4_group1"
+	    },
+	    "lacp": {
+		    "trap_ids": "lacp",
+		    "trap_group": "queue4_group1",
+		    "always_enabled": "true"
+	    },
+	    "arp": {
+		    "trap_ids": "arp_req,arp_resp,neigh_discovery",
+		    "trap_group": "queue4_group2",
+		    "always_enabled": "true"
+	    },
+	    "lldp": {
+		    "trap_ids": "lldp",
+		    "trap_group": "queue4_group3"
+	    },
+	    "dhcp_relay": {
+		    "trap_ids": "dhcp,dhcpv6",
+		    "trap_group": "queue4_group3"
+	    },
+	    "udld": {
+		    "trap_ids": "udld",
+		    "trap_group": "queue4_group3",
+		    "always_enabled": "true"
+	    },
+	    "ip2me": {
+		    "trap_ids": "ip2me",
+		    "trap_group": "queue1_group1",
+		    "always_enabled": "true"
+	    },
+	    "macsec": {
+		    "trap_ids": "eapol",
+		    "trap_group": "queue4_group3"
+	    },
+	    "nat": {
+		    "trap_ids": "src_nat_miss,dest_nat_miss",
+		    "trap_group": "queue1_group2"
+	    },
+	    "sflow": {
+		    "trap_group": "queue2_group1",
+		    "trap_ids": "sample_packet"
+	    }
+    }
+}
diff --git a/tests/mock_tests/copp_ut.cpp b/tests/mock_tests/copp_ut.cpp
@@ -0,0 +1,76 @@
+#include "gtest/gtest.h"
+#include <string>
+#include "schema.h"
+#include "warm_restart.h"
+#include "ut_helper.h"
+#include "coppmgr.h"
+#include "coppmgr.cpp"
+#include <fstream>
+#include <streambuf>
+using namespace std;
+using namespace swss;
+
+void create_init_file()
+{
+    int status = system("sudo mkdir /etc/sonic/");
+    ASSERT_EQ(status, 0);
+
+    status = system("sudo chmod 777 /etc/sonic/");
+    ASSERT_EQ(status, 0);
+    
+    status = system("sudo cp copp_cfg.json /etc/sonic/");
+    ASSERT_EQ(status, 0);
+}
+
+void cleanup()
+{
+    int status = system("sudo rm -rf /etc/sonic/");
+    ASSERT_EQ(status, 0);
+}
+
+TEST(CoppMgrTest, CoppTest)
+{
+    create_init_file();
+
+    const vector<string> 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);
+    
+    /* The test will set an entry with queue1_group1|cbs value which differs from the init value
+     * found in the copp_cfg.json file. Then coppmgr constructor will be called and it will detect
+     * that there is already an entry for queue1_group1|cbs with different value and it should be
+     * overwritten with the init value.
+     * hget will verify that this indeed happened.
+     */
+    Table coppTable = Table(&appDb, APP_COPP_TABLE_NAME);
+    coppTable.set("queue1_group1",
+                {
+                    {"cbs", "6100"},
+                    {"cir", "6000"},
+                    {"meter_type", "packets"},
+                    {"mode", "sr_tcm"},
+                    {"queue", "1"},
+                    {"red_action", "drop"},
+                    {"trap_action", "trap"},
+                    {"trap_priority", "1"},
+                    {"trap_ids", "ip2me"}
+                });
+
+    CoppMgr coppmgr(&cfgDb, &appDb, &stateDb, cfg_copp_tables);
+
+    string overide_val;
+    coppTable.hget("queue1_group1", "cbs",overide_val);
+    EXPECT_EQ( overide_val, "6000");
+
+    cleanup();
+}
+
