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.

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

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

Merged
merged 11 commits into from
Jan 1, 2023
57 changes: 57 additions & 0 deletions cfgmgr/coppmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "shellcmd.h"
#include "warm_restart.h"
#include "json.hpp"
#include <unordered_map>
#include <unordered_set>

using json = nlohmann::json;

Expand Down Expand Up @@ -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;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems this code covers case where new copp entry has a field that preserved does not. isn't it missing a case where preserved copp entry had a field that new copp entry does not carry anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I missed it.
I guess this can be done easily by comparing the sizes of fvs and preserved_fvs. I will test and raise a new PR with this fix.

Comparing sizes will be sufficient since if sizes differ it says that there is at least one difference so it is not a duplicate and the preserved entry should be deleted and new entry set.
In case the size of both is equal but some of the fields differs it will be caught then iterating over fvs.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, size check should work.

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),
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/coppmgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions tests/mock_tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
111 changes: 111 additions & 0 deletions tests/mock_tests/copp_cfg.json
Original file line number Diff line number Diff line change
@@ -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"
}
}
}
76 changes: 76 additions & 0 deletions tests/mock_tests/copp_ut.cpp
Original file line number Diff line number Diff line change
@@ -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();
}