diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index da361a4ea30..2f14c61349f 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -109,7 +109,8 @@ static acl_table_type_lookup_t aclTableTypeLookUp = { TABLE_TYPE_CTRLPLANE, ACL_TABLE_CTRLPLANE }, { TABLE_TYPE_DTEL_FLOW_WATCHLIST, ACL_TABLE_DTEL_FLOW_WATCHLIST }, { TABLE_TYPE_DTEL_DROP_WATCHLIST, ACL_TABLE_DTEL_DROP_WATCHLIST }, - { TABLE_TYPE_MCLAG, ACL_TABLE_MCLAG } + { TABLE_TYPE_MCLAG, ACL_TABLE_MCLAG }, + { TABLE_TYPE_DROP, ACL_TABLE_DROP } }; static acl_stage_type_lookup_t aclStageLookUp = @@ -620,6 +621,23 @@ bool AclRule::remove() return res; } +void AclRule::updateInPorts() +{ + SWSS_LOG_ENTER(); + sai_attribute_t attr; + sai_status_t status; + + attr.id = SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS; + attr.value = m_matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS]; + attr.value.aclfield.enable = true; + + status = sai_acl_api->set_acl_entry_attribute(m_ruleOid, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to update ACL rule %s, rv:%d", m_id.c_str(), status); + } +} + AclRuleCounters AclRule::getCounters() { SWSS_LOG_ENTER(); @@ -675,7 +693,8 @@ shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, Mir type != ACL_TABLE_MIRROR_DSCP && type != ACL_TABLE_DTEL_FLOW_WATCHLIST && type != ACL_TABLE_DTEL_DROP_WATCHLIST && - type != ACL_TABLE_MCLAG) + type != ACL_TABLE_MCLAG && + type != ACL_TABLE_DROP) { throw runtime_error("Unknown table type"); } @@ -723,6 +742,10 @@ shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, Mir { return make_shared(acl, rule, table, type); } + else if (type == ACL_TABLE_DROP) + { + return make_shared(acl, rule, table, type); + } throw runtime_error("Wrong combination of table type and action in rule " + rule); } @@ -998,7 +1021,6 @@ void AclRuleL3::update(SubjectType, void *) // Do nothing } - AclRulePfcwd::AclRulePfcwd(AclOrch *aclOrch, string rule, string table, acl_table_type_t type, bool createCounter) : AclRuleL3(aclOrch, rule, table, type, createCounter) { @@ -1006,12 +1028,6 @@ AclRulePfcwd::AclRulePfcwd(AclOrch *aclOrch, string rule, string table, acl_tabl bool AclRulePfcwd::validateAddMatch(string attr_name, string attr_value) { - if (attr_name != MATCH_TC) - { - SWSS_LOG_ERROR("%s is not supported for the tables of type Pfcwd", attr_name.c_str()); - return false; - } - return AclRule::validateAddMatch(attr_name, attr_value); } @@ -1326,7 +1342,7 @@ bool AclTable::create() vector bpoint_list; // PFC watch dog ACLs are only applied to port - if (type == ACL_TABLE_PFCWD) + if ((type == ACL_TABLE_PFCWD) || (type == ACL_TABLE_DROP)) { bpoint_list = { SAI_ACL_BIND_POINT_TYPE_PORT }; } @@ -1340,7 +1356,7 @@ bool AclTable::create() attr.value.s32list.list = bpoint_list.data(); table_attrs.push_back(attr); - if (type == ACL_TABLE_PFCWD) + if ((type == ACL_TABLE_PFCWD) || (type == ACL_TABLE_DROP)) { attr.id = SAI_ACL_TABLE_ATTR_FIELD_TC; attr.value.booldata = true; @@ -1349,7 +1365,14 @@ bool AclTable::create() attr.id = SAI_ACL_TABLE_ATTR_ACL_STAGE; attr.value.s32 = (stage == ACL_STAGE_INGRESS) ? SAI_ACL_STAGE_INGRESS : SAI_ACL_STAGE_EGRESS; table_attrs.push_back(attr); - + + if (stage == ACL_STAGE_INGRESS) + { + attr.id = SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS; + attr.value.booldata = true; + table_attrs.push_back(attr); + } + sai_status_t status = sai_acl_api->create_acl_table(&m_oid, gSwitchId, (uint32_t)table_attrs.size(), table_attrs.data()); if (status == SAI_STATUS_SUCCESS) @@ -2919,6 +2942,64 @@ bool AclOrch::removeAclRule(string table_id, string rule_id) return m_AclTables[table_oid].remove(rule_id); } +bool AclOrch::updateAclRule(shared_ptr rule, string table_id, string attr_name, void *data, bool oper) +{ + SWSS_LOG_ENTER(); + + sai_object_id_t table_oid = getTableById(table_id); + string attr_value; + + if (table_oid == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Failed to update ACL rule in ACL table %s. Table doesn't exist", table_id.c_str()); + return false; + } + + switch (aclMatchLookup[attr_name]) + { + case SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS: + { + sai_object_id_t port_oid = *(sai_object_id_t *)data; + vector in_ports = rule->getInPorts(); + + if (oper == RULE_OPER_ADD) + { + in_ports.push_back(port_oid); + } + else + { + for (auto port_iter = in_ports.begin(); port_iter != in_ports.end(); port_iter++) + { + if (*port_iter == port_oid) + { + in_ports.erase(port_iter); + break; + } + } + } + + for (const auto& port_iter: in_ports) + { + Port p; + gPortsOrch->getPort(port_iter, p); + attr_value += p.m_alias; + attr_value += ','; + } + attr_value.pop_back(); + + rule->validateAddMatch(MATCH_IN_PORTS, attr_value); + m_AclTables[table_oid].rules[rule->getId()]->updateInPorts(); + } + break; + + default: + SWSS_LOG_ERROR("Acl rule update not supported for attr name %s", attr_name.c_str()); + break; + } + + return true; +} + bool AclOrch::isCombinedMirrorV6Table() { return m_isCombinedMirrorV6Table; diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 159fef8778a..267fb6eeb97 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -36,6 +36,7 @@ #define TABLE_TYPE_DTEL_DROP_WATCHLIST "DTEL_DROP_WATCHLIST" #define TABLE_TYPE_MCLAG "MCLAG" #define TABLE_TYPE_MUX "MUX" +#define TABLE_TYPE_DROP "DROP" #define RULE_PRIORITY "PRIORITY" #define MATCH_IN_PORTS "IN_PORTS" @@ -103,6 +104,9 @@ #define IP_TYPE_ARP_REPLY "ARP_REPLY" #define MLNX_MAX_RANGES_COUNT 16 +#define INGRESS_TABLE_DROP "IngressTableDrop" +#define RULE_OPER_ADD 0 +#define RULE_OPER_DELETE 1 typedef enum { @@ -117,7 +121,8 @@ typedef enum ACL_TABLE_DTEL_FLOW_WATCHLIST, ACL_TABLE_DTEL_DROP_WATCHLIST, ACL_TABLE_MCLAG, - ACL_TABLE_MUX + ACL_TABLE_MUX, + ACL_TABLE_DROP } acl_table_type_t; typedef map acl_table_type_lookup_t; @@ -196,6 +201,7 @@ class AclRule virtual bool create(); virtual bool remove(); virtual void update(SubjectType, void *) = 0; + virtual void updateInPorts(); virtual AclRuleCounters getCounters(); string getId() @@ -213,6 +219,11 @@ class AclRule return m_counterOid; } + vector getInPorts() + { + return m_inPorts; + } + static shared_ptr makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple&); virtual ~AclRule() {} @@ -417,7 +428,6 @@ class AclOrch : public Orch, public Observer return m_countersTable; } - // FIXME: Add getters for them? I'd better to add a common directory of orch objects and use it everywhere MirrorOrch *m_mirrorOrch; NeighOrch *m_neighOrch; @@ -429,6 +439,7 @@ class AclOrch : public Orch, public Observer bool updateAclTable(AclTable ¤tTable, AclTable &newTable); bool addAclRule(shared_ptr aclRule, string table_id); bool removeAclRule(string table_id, string rule_id); + bool updateAclRule(shared_ptr aclRule, string table_id, string attr_name, void *data, bool oper); bool isCombinedMirrorV6Table(); bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const; @@ -443,6 +454,10 @@ class AclOrch : public Orch, public Observer static bool getAclBindPortId(Port& port, sai_object_id_t& port_id); using Orch::doTask; // Allow access to the basic doTask + map getAclTables() + { + return m_AclTables; + } private: SwitchOrch *m_switchOrch; diff --git a/orchagent/pfcactionhandler.cpp b/orchagent/pfcactionhandler.cpp index 6a0664d0f67..6bc65a61145 100644 --- a/orchagent/pfcactionhandler.cpp +++ b/orchagent/pfcactionhandler.cpp @@ -225,35 +225,50 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, { SWSS_LOG_ENTER(); - acl_table_type_t table_type = ACL_TABLE_PFCWD; + acl_table_type_t table_type; + sai_object_id_t table_oid; - // There is one handler instance per queue ID string queuestr = to_string(queueId); - m_strIngressTable = "IngressTable_PfcWdAclHandler_" + queuestr; - m_strEgressTable = "EgressTable_PfcWdAclHandler_" + queuestr; m_strRule = "Rule_PfcWdAclHandler_" + queuestr; + // Ingress table/rule creation + table_type = ACL_TABLE_DROP; + m_strIngressTable = INGRESS_TABLE_DROP; auto found = m_aclTables.find(m_strIngressTable); if (found == m_aclTables.end()) { // First time of handling PFC for this queue, create ACL table, and bind createPfcAclTable(port, m_strIngressTable, true); shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strIngressTable, table_type); - createPfcAclRule(newRule, queueId, m_strIngressTable); + createPfcAclRule(newRule, queueId, m_strIngressTable, port); } else { - // Otherwise just bind ACL table with the port - found->second.bind(port); + table_oid = gAclOrch->getTableById(m_strIngressTable); + map table_map = gAclOrch->getAclTables(); + auto rule_iter = table_map[table_oid].rules.find(m_strRule); + + if (rule_iter == table_map[table_oid].rules.end()) + { + shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strIngressTable, table_type); + createPfcAclRule(newRule, queueId, m_strIngressTable, port); + } + else + { + gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_ADD); + } } + // Egress table/rule creation + table_type = ACL_TABLE_PFCWD; + m_strEgressTable = "EgressTable_PfcWdAclHandler_" + queuestr; found = m_aclTables.find(m_strEgressTable); if (found == m_aclTables.end()) { // First time of handling PFC for this queue, create ACL table, and bind createPfcAclTable(port, m_strEgressTable, false); shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strEgressTable, table_type); - createPfcAclRule(newRule, queueId, m_strEgressTable); + createPfcAclRule(newRule, queueId, m_strEgressTable, port); } else { @@ -266,11 +281,24 @@ PfcWdAclHandler::~PfcWdAclHandler(void) { SWSS_LOG_ENTER(); - auto found = m_aclTables.find(m_strIngressTable); - found->second.unbind(getPort()); + sai_object_id_t table_oid = gAclOrch->getTableById(m_strIngressTable); + map table_map = gAclOrch->getAclTables(); + auto rule_iter = table_map[table_oid].rules.find(m_strRule); - found = m_aclTables.find(m_strEgressTable); - found->second.unbind(getPort()); + vector port_set = rule_iter->second->getInPorts(); + sai_object_id_t port = getPort(); + + if ((port_set.size() == 1) && (port_set[0] == port)) + { + gAclOrch->removeAclRule(m_strIngressTable, m_strRule); + } + else + { + gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_DELETE); + } + + auto found = m_aclTables.find(m_strEgressTable); + found->second.unbind(port); } void PfcWdAclHandler::clear() @@ -295,14 +323,24 @@ void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, b assert(inserted.second); AclTable& aclTable = inserted.first->second; - aclTable.type = ACL_TABLE_PFCWD; aclTable.link(port); aclTable.id = strTable; - aclTable.stage = ingress ? ACL_STAGE_INGRESS : ACL_STAGE_EGRESS; + + if (ingress) + { + aclTable.type = ACL_TABLE_DROP; + aclTable.stage = ACL_STAGE_INGRESS; + } + else + { + aclTable.type = ACL_TABLE_PFCWD; + aclTable.stage = ACL_STAGE_EGRESS; + } + gAclOrch->addAclTable(aclTable); } -void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable) +void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, sai_object_id_t portOid) { SWSS_LOG_ENTER(); @@ -316,6 +354,22 @@ void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t qu attr_value = to_string(queueId); rule->validateAddMatch(attr_name, attr_value); + // Add MATCH_IN_PORTS as match criteria for ingress table + if (strTable == INGRESS_TABLE_DROP) + { + Port p; + attr_name = MATCH_IN_PORTS; + + if (!gPortsOrch->getPort(portOid, p)) + { + SWSS_LOG_ERROR("Failed to get port structure from port oid 0x%" PRIx64, portOid); + return; + } + + attr_value = p.m_alias; + rule->validateAddMatch(attr_name, attr_value); + } + attr_name = ACTION_PACKET_ACTION; attr_value = PACKET_ACTION_DROP; rule->validateAddAction(attr_name, attr_value); diff --git a/orchagent/pfcactionhandler.h b/orchagent/pfcactionhandler.h index 7859709a1e7..e381a798c67 100644 --- a/orchagent/pfcactionhandler.h +++ b/orchagent/pfcactionhandler.h @@ -111,7 +111,8 @@ class PfcWdAclHandler: public PfcWdLossyHandler string m_strEgressTable; string m_strRule; void createPfcAclTable(sai_object_id_t port, string strTable, bool ingress); - void createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable); + void createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, sai_object_id_t port); + void updatePfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, vector port); }; // PFC queue that implements drop action by draining queue with buffer of zero size diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index d920c218222..ca37a85be2d 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -177,8 +177,7 @@ task_process_status PfcWdOrch::createEntry(const st uint32_t detectionTime = 0; uint32_t restorationTime = 0; // According to requirements, drop action is default - PfcWdAction action = PfcWdAction::PFC_WD_ACTION_DROP; - + PfcWdAction action = PfcWdAction::PFC_WD_ACTION_DROP; Port port; if (!gPortsOrch->getPort(key, port)) { diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index 2b48c3b76c6..3a68142b46f 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -254,6 +254,33 @@ def create_acl_rule( self.config_db.create_entry("ACL_RULE", "{}|{}".format(table_name, rule_name), fvs) + def update_acl_rule( + self, + table_name: str, + rule_name: str, + qualifiers: Dict[str, str], + action: str = "FORWARD", + priority: str = "2020" + ) -> None: + """Create a new ACL rule in the given table. + + Args: + table_name: The name of the ACL table to add the rule to. + rule_name: The name of the ACL rule. + qualifiers: The list of qualifiers to add to the rule. + action: The packet action of the rule. + priority: The priority of the rule. + """ + fvs = { + "priority": priority, + "PACKET_ACTION": action + } + + for k, v in qualifiers.items(): + fvs[k] = v + + self.config_db.update_entry("ACL_RULE", "{}|{}".format(table_name, rule_name), fvs) + def create_redirect_acl_rule( self, table_name: str, diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 3921dfe73ee..bed031396ef 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -116,7 +116,7 @@ namespace aclorch_test { "SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT", "true" }, { "SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS", "true" }, { "SAI_ACL_TABLE_ATTR_FIELD_ACL_RANGE_TYPE", "2:SAI_ACL_RANGE_TYPE_L4_DST_PORT_RANGE,SAI_ACL_RANGE_TYPE_L4_SRC_PORT_RANGE" }, - { "SAI_ACL_TABLE_ATTR_ACL_STAGE", "SAI_ACL_STAGE_INGRESS" } }); + { "SAI_ACL_TABLE_ATTR_ACL_STAGE", "SAI_ACL_STAGE_INGRESS" }}); SaiAttributeList attr_list(SAI_OBJECT_TYPE_ACL_TABLE, v, false); ASSERT_TRUE(Check::AttrListEq(SAI_OBJECT_TYPE_ACL_TABLE, res->attr_list, attr_list)); diff --git a/tests/test_pfcwd.py b/tests/test_pfcwd.py new file mode 100644 index 00000000000..c569bc8a43b --- /dev/null +++ b/tests/test_pfcwd.py @@ -0,0 +1,84 @@ +import redis +import time +import os +import pytest +import re +import json +from swsscommon import swsscommon + +PFCWD_TABLE_NAME = "DROP_TEST_TABLE" +PFCWD_TABLE_TYPE = "DROP" +PFCWD_TC = ["3", "4"] +PFCWD_RULE_NAME_1 = "DROP_TEST_RULE_1" +PFCWD_RULE_NAME_2 = "DROP_TEST_RULE_2" + +class TestPfcWd: + def test_PfcWdAclCreationDeletion(self, dvs, dvs_acl, testlog): + try: + dvs_acl.create_acl_table(PFCWD_TABLE_NAME, PFCWD_TABLE_TYPE, ["Ethernet0","Ethernet8", "Ethernet16", "Ethernet24"], stage="ingress") + + config_qualifiers = { + "TC" : PFCWD_TC[0], + "IN_PORTS": "Ethernet0" + } + + expected_sai_qualifiers = { + "SAI_ACL_ENTRY_ATTR_FIELD_TC" : dvs_acl.get_simple_qualifier_comparator("3&mask:0xff"), + "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet0"]) + } + + dvs_acl.create_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_1, config_qualifiers, action="DROP") + time.sleep(5) + dvs_acl.verify_acl_rule(expected_sai_qualifiers, action="DROP") + + config_qualifiers = { + "TC" : PFCWD_TC[0], + "IN_PORTS": "Ethernet0,Ethernet16" + } + + expected_sai_qualifiers = { + "SAI_ACL_ENTRY_ATTR_FIELD_TC" : dvs_acl.get_simple_qualifier_comparator("3&mask:0xff"), + "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet0","Ethernet16"]) + } + + dvs_acl.update_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_1, config_qualifiers, action="DROP") + time.sleep(5) + dvs_acl.verify_acl_rule(expected_sai_qualifiers, action="DROP") + dvs_acl.remove_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_1) + + config_qualifiers = { + "TC" : PFCWD_TC[1], + "IN_PORTS": "Ethernet8" + } + + expected_sai_qualifiers = { + "SAI_ACL_ENTRY_ATTR_FIELD_TC" : dvs_acl.get_simple_qualifier_comparator("4&mask:0xff"), + "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet8"]), + } + + dvs_acl.create_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_2, config_qualifiers, action="DROP") + time.sleep(5) + dvs_acl.verify_acl_rule(expected_sai_qualifiers, action="DROP") + + config_qualifiers = { + "TC" : PFCWD_TC[1], + "IN_PORTS": "Ethernet8,Ethernet24" + } + + expected_sai_qualifiers = { + "SAI_ACL_ENTRY_ATTR_FIELD_TC" : dvs_acl.get_simple_qualifier_comparator("4&mask:0xff"), + "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(["Ethernet8","Ethernet24"]), + } + + dvs_acl.update_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_2, config_qualifiers, action="DROP") + time.sleep(5) + dvs_acl.verify_acl_rule(expected_sai_qualifiers, action="DROP") + dvs_acl.remove_acl_rule(PFCWD_TABLE_NAME, PFCWD_RULE_NAME_2) + + finally: + dvs_acl.remove_acl_table(PFCWD_TABLE_NAME) +# +# Add Dummy always-pass test at end as workaroud +# for issue when Flaky fail on final test it invokes module tear-down before retrying +def test_nonflaky_dummy(): + pass