From bdedf694f10b6f8b5ea71fb30eef5d0c3b354409 Mon Sep 17 00:00:00 2001 From: Aryeh Feigin <101218333+arfeigin@users.noreply.github.com> Date: Sun, 1 Jan 2023 17:10:35 +0200 Subject: [PATCH] Modify coppmgr mergeConfig to support preserving copp tables through reboot. (#2548) 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) --- cfgmgr/coppmgr.cpp | 57 +++++++++++++++++ cfgmgr/coppmgr.h | 1 + tests/mock_tests/Makefile.am | 1 + tests/mock_tests/copp_cfg.json | 111 +++++++++++++++++++++++++++++++++ tests/mock_tests/copp_ut.cpp | 76 ++++++++++++++++++++++ 5 files changed, 246 insertions(+) create mode 100644 tests/mock_tests/copp_cfg.json create mode 100644 tests/mock_tests/copp_ut.cpp diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index 1721cc8593f8..5595096a278c 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -9,6 +9,8 @@ #include "shellcmd.h" #include "warm_restart.h" #include "json.hpp" +#include +#include using json = nlohmann::json; @@ -255,6 +257,42 @@ void CoppMgr::mergeConfig(CoppCfg &init_cfg, CoppCfg &m_cfg, std::vector &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 preserved_fvs; + bool key_found = m_coppTable.get(key, preserved_fvs); + if (!key_found) + { + return false; + } + else + { + unordered_map 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 &tableNames) : Orch(cfgDb, tableNames), m_cfgCoppTrapTable(cfgDb, CFG_COPP_TRAP_TABLE_NAME), @@ -270,9 +308,11 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c std::vector group_keys; std::vector trap_keys; std::vector feature_keys; + std::vector preserved_copp_keys; std::vector group_cfg_keys; std::vector trap_cfg_keys; + unordered_set supported_copp_keys; CoppCfg group_cfg; CoppCfg trap_cfg; @@ -280,6 +320,7 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c 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,8 +393,14 @@ 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()) @@ -361,6 +408,16 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c 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 index 1d53756fceba..44549d3bec7f 100644 --- 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 &cfg_keys, Table &cfgTable); + bool isDupEntry(const std::string &key, std::vector &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 index 16f5e429ff67..8faab837c6ed 100644 --- 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 new file mode 100644 index 000000000000..46d921b82765 --- /dev/null +++ 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 new file mode 100644 index 000000000000..1c3b766e1cac --- /dev/null +++ b/tests/mock_tests/copp_ut.cpp @@ -0,0 +1,76 @@ +#include "gtest/gtest.h" +#include +#include "schema.h" +#include "warm_restart.h" +#include "ut_helper.h" +#include "coppmgr.h" +#include "coppmgr.cpp" +#include +#include +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 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(); +} +