From 80e0b57d5a85252d4099e9ea1303358e3ad8ac0a Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Wed, 27 Mar 2024 16:28:03 -0700 Subject: [PATCH 1/2] [Copp]Refactor coppmgr tests (#3093) What I did Refactoring coppmgr mock tests Why I did it After migration to bookworm, coppmgr tests started failing due to the use of sudo commands. --- cfgmgr/coppmgr.cpp | 10 ++++++---- cfgmgr/coppmgr.h | 3 ++- tests/mock_tests/Makefile.am | 3 ++- tests/mock_tests/copp_ut.cpp | 26 ++------------------------ 4 files changed, 12 insertions(+), 30 deletions(-) diff --git a/cfgmgr/coppmgr.cpp b/cfgmgr/coppmgr.cpp index cfa94988d9..9b2c3ee4d7 100644 --- a/cfgmgr/coppmgr.cpp +++ b/cfgmgr/coppmgr.cpp @@ -21,10 +21,11 @@ static set g_copp_init_set; void CoppMgr::parseInitFile(void) { - std::ifstream ifs(COPP_INIT_FILE); + std::ifstream ifs(m_coppCfgfile); + if (ifs.fail()) { - SWSS_LOG_ERROR("COPP init file %s not found", COPP_INIT_FILE); + SWSS_LOG_ERROR("COPP init file %s not found", m_coppCfgfile.c_str()); return; } json j = json::parse(ifs); @@ -293,7 +294,7 @@ bool CoppMgr::isDupEntry(const std::string &key, std::vector &f return true; } -CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames) : +CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames, const string copp_init_file) : Orch(cfgDb, tableNames), m_cfgCoppTrapTable(cfgDb, CFG_COPP_TRAP_TABLE_NAME), m_cfgCoppGroupTable(cfgDb, CFG_COPP_GROUP_TABLE_NAME), @@ -301,7 +302,8 @@ CoppMgr::CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_appCoppTable(appDb, APP_COPP_TABLE_NAME), m_stateCoppTrapTable(stateDb, STATE_COPP_TRAP_TABLE_NAME), m_stateCoppGroupTable(stateDb, STATE_COPP_GROUP_TABLE_NAME), - m_coppTable(appDb, APP_COPP_TABLE_NAME) + m_coppTable(appDb, APP_COPP_TABLE_NAME), + m_coppCfgfile(copp_init_file) { SWSS_LOG_ENTER(); parseInitFile(); diff --git a/cfgmgr/coppmgr.h b/cfgmgr/coppmgr.h index 44549d3bec..86f1b0e4e2 100644 --- a/cfgmgr/coppmgr.h +++ b/cfgmgr/coppmgr.h @@ -62,7 +62,7 @@ class CoppMgr : public Orch { public: CoppMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, - const std::vector &tableNames); + const std::vector &tableNames, const std::string copp_init_file = COPP_INIT_FILE); using Orch::doTask; private: @@ -75,6 +75,7 @@ class CoppMgr : public Orch CoppCfg m_coppGroupInitCfg; CoppCfg m_coppTrapInitCfg; CoppCfg m_featuresCfgTable; + std::string m_coppCfgfile; void doTask(Consumer &consumer); diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 651fbcd7b0..b00d4af1f4 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -130,7 +130,8 @@ tests_SOURCES = aclorch_ut.cpp \ $(top_srcdir)/orchagent/dash/dashvnetorch.cpp \ $(top_srcdir)/cfgmgr/buffermgrdyn.cpp \ $(top_srcdir)/warmrestart/warmRestartAssist.cpp \ - $(top_srcdir)/orchagent/dash/pbutils.cpp + $(top_srcdir)/orchagent/dash/pbutils.cpp \ + $(top_srcdir)/cfgmgr/coppmgr.cpp tests_SOURCES += $(FLEX_CTR_DIR)/flex_counter_manager.cpp $(FLEX_CTR_DIR)/flex_counter_stat_manager.cpp $(FLEX_CTR_DIR)/flow_counter_handler.cpp $(FLEX_CTR_DIR)/flowcounterrouteorch.cpp tests_SOURCES += $(DEBUG_CTR_DIR)/debug_counter.cpp $(DEBUG_CTR_DIR)/drop_counter.cpp diff --git a/tests/mock_tests/copp_ut.cpp b/tests/mock_tests/copp_ut.cpp index 1c3b766e1c..f5d0b85cf5 100644 --- a/tests/mock_tests/copp_ut.cpp +++ b/tests/mock_tests/copp_ut.cpp @@ -4,34 +4,14 @@ #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, @@ -65,12 +45,10 @@ TEST(CoppMgrTest, CoppTest) {"trap_ids", "ip2me"} }); - CoppMgr coppmgr(&cfgDb, &appDb, &stateDb, cfg_copp_tables); + CoppMgr coppmgr(&cfgDb, &appDb, &stateDb, cfg_copp_tables, "./copp_cfg.json"); string overide_val; coppTable.hget("queue1_group1", "cbs",overide_val); EXPECT_EQ( overide_val, "6000"); - - cleanup(); } From c96a2f84b506665dca18765932cc4c0de9d0e507 Mon Sep 17 00:00:00 2001 From: Neetha John Date: Fri, 29 Mar 2024 09:11:42 -0700 Subject: [PATCH 2/2] Revert "[acl] Add IN_PORTS qualifier for L3 table (#3078)" (#3092) This reverts commit 9d4a3addacba4289b42ce82626b3bc54a6e8b0bd. *Revert "[acl] Add IN_PORTS qualifier for L3 table" --- orchagent/aclorch.cpp | 2 -- tests/test_acl.py | 42 ------------------------------------------ 2 files changed, 44 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 5ca4d2a44c..6906744cc2 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -3223,7 +3223,6 @@ void AclOrch::initDefaultTableTypes() .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_SRC_PORT)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS)) .build() ); @@ -3241,7 +3240,6 @@ void AclOrch::initDefaultTableTypes() .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_SRC_PORT)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT)) .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS)) - .withMatch(make_shared(SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS)) .build() ); diff --git a/tests/test_acl.py b/tests/test_acl.py index 1dbaa30590..cf68d1516e 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -243,29 +243,6 @@ def test_AclRuleInPorts(self, dvs_acl, mirror_acl_table): dvs_acl.verify_acl_rule_status(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, None) dvs_acl.verify_no_acl_rules() - def test_AclRuleInPortsL3(self, dvs_acl, l3_acl_table): - """ - Verify IN_PORTS matches on ACL rule. - Using L3 table type for IN_PORTS matches. - """ - config_qualifiers = { - "IN_PORTS": "Ethernet8,Ethernet12", - } - - expected_sai_qualifiers = { - "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet8", "Ethernet12"]) - } - - dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) - # Verify status is written into STATE_DB - dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") - dvs_acl.verify_acl_rule(expected_sai_qualifiers) - - dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) - # Verify the STATE_DB entry is removed - dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) - dvs_acl.verify_no_acl_rules() - def test_AclRuleOutPorts(self, dvs_acl, mclag_acl_table): """ Verify OUT_PORTS matches on ACL rule. @@ -569,25 +546,6 @@ def test_V6AclRuleVlanId(self, dvs_acl, l3v6_acl_table): dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() - def test_v6AclRuleInPorts(self, dvs_acl, l3v6_acl_table): - config_qualifiers = { - "IN_PORTS": "Ethernet8,Ethernet12", - } - - expected_sai_qualifiers = { - "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet8", "Ethernet12"]) - } - - dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) - dvs_acl.verify_acl_rule(expected_sai_qualifiers) - # Verify status is written into STATE_DB - dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") - - dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) - # Verify the STATE_DB entry is removed - dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) - dvs_acl.verify_no_acl_rules() - def test_InsertAclRuleBetweenPriorities(self, dvs_acl, l3_acl_table): rule_priorities = ["10", "20", "30", "40"]