From c07fc3fcdfc50cd967d5716cf3983b2070b3b177 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 15 Sep 2021 15:52:34 +0000 Subject: [PATCH 01/10] [FlexCounterManager] Add ACL_COUNTER FC type Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 274 +++++++++++------- orchagent/aclorch.h | 73 ++--- .../flex_counter/flex_counter_manager.cpp | 1 + orchagent/flex_counter/flex_counter_manager.h | 1 + orchagent/flexcounterorch.cpp | 2 + tests/dvslib/dvs_acl.py | 16 +- tests/test_flex_counters.py | 24 +- 7 files changed, 236 insertions(+), 155 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index c7cfe200a1..99ffe6c540 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -15,10 +15,7 @@ using namespace std; using namespace swss; -mutex AclOrch::m_countersMutex; map AclRange::m_ranges; -condition_variable AclOrch::m_sleepGuard; -bool AclOrch::m_bCollectCounters = true; sai_uint32_t AclRule::m_minPriority = 0; sai_uint32_t AclRule::m_maxPriority = 0; @@ -35,6 +32,10 @@ extern CrmOrch *gCrmOrch; #define MIN_VLAN_ID 1 // 0 is a reserved VLAN ID #define MAX_VLAN_ID 4095 // 4096 is a reserved VLAN ID +#define COUNTERS_ACL_COUNTER_RULE_MAP "ACL_COUNTER_RULE_MAP" +#define ACL_COUNTER_DEFAULT_POLLING_INTERVAL_MS 10000 // ms +#define ACL_COUNTER_DEFAULT_ENABLED_STATE false + const int TCP_PROTOCOL_NUM = 6; // TCP protocol number acl_rule_attr_lookup_t aclMatchLookup = @@ -157,6 +158,12 @@ static acl_ip_type_lookup_t aclIpTypeLookup = { IP_TYPE_ARP_REPLY, SAI_ACL_IP_TYPE_ARP_REPLY } }; +static map aclCounterLookup = +{ + {SAI_ACL_COUNTER_ATTR_ENABLE_BYTE_COUNT, SAI_ACL_COUNTER_ATTR_BYTES}, + {SAI_ACL_COUNTER_ATTR_ENABLE_PACKET_COUNT, SAI_ACL_COUNTER_ATTR_PACKETS}, +}; + AclRule::AclRule(AclOrch *pAclOrch, string rule, string table, acl_table_type_t type, bool createCounter) : m_pAclOrch(pAclOrch), m_id(rule), @@ -610,14 +617,19 @@ bool AclRule::isActionSupported(sai_acl_entry_attr_t action) const return m_pAclOrch->isAclActionSupported(pTable->stage, action_type); } -bool AclRule::remove() +bool AclRule::removeRule() { SWSS_LOG_ENTER(); - sai_status_t res; - if (sai_acl_api->remove_acl_entry(m_ruleOid) != SAI_STATUS_SUCCESS) + if (m_ruleOid == SAI_NULL_OBJECT_ID) { - SWSS_LOG_ERROR("Failed to delete ACL rule"); + return true; + } + + auto status = sai_acl_api->remove_acl_entry(m_ruleOid); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to delete ACL rule, status %s", sai_serialize_status(status).c_str()); return false; } @@ -627,7 +639,19 @@ bool AclRule::remove() decreaseNextHopRefCount(); - res = removeRanges(); + return true; +} + +bool AclRule::remove() +{ + SWSS_LOG_ENTER(); + + if (!removeRule()) + { + return false; + } + + auto res = removeRanges(); res &= removeCounter(); return res; @@ -650,28 +674,6 @@ void AclRule::updateInPorts() } } -AclRuleCounters AclRule::getCounters() -{ - SWSS_LOG_ENTER(); - - if (m_counterOid == SAI_NULL_OBJECT_ID) - { - return AclRuleCounters(); - } - - sai_attribute_t counter_attr[2]; - counter_attr[0].id = SAI_ACL_COUNTER_ATTR_PACKETS; - counter_attr[1].id = SAI_ACL_COUNTER_ATTR_BYTES; - - if (sai_acl_api->get_acl_counter_attribute(m_counterOid, 2, counter_attr) != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to get counters for %s rule", m_id.c_str()); - return AclRuleCounters(); - } - - return AclRuleCounters(counter_attr[0].value.u64, counter_attr[1].value.u64); -} - shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple& data) { string action; @@ -851,13 +853,12 @@ bool AclRule::createCounter() attr.value.oid = m_tableOid; counter_attrs.push_back(attr); - attr.id = SAI_ACL_COUNTER_ATTR_ENABLE_BYTE_COUNT; - attr.value.booldata = true; - counter_attrs.push_back(attr); - - attr.id = SAI_ACL_COUNTER_ATTR_ENABLE_PACKET_COUNT; - attr.value.booldata = true; - counter_attrs.push_back(attr); + for (auto counterAttrPair: aclCounterLookup) + { + tie(attr.id, ignore) = counterAttrPair; + attr.value.booldata = true; + counter_attrs.push_back(attr); + } if (sai_acl_api->create_acl_counter(&m_counterOid, gSwitchId, (uint32_t)counter_attrs.size(), counter_attrs.data()) != SAI_STATUS_SUCCESS) { @@ -903,9 +904,6 @@ bool AclRule::removeCounter() gCrmOrch->decCrmAclTableUsedCounter(CrmResourceType::CRM_ACL_COUNTER, m_tableOid); - SWSS_LOG_INFO("Removing record about the counter %" PRIx64 " from the DB", m_counterOid); - AclOrch::getCountersTable().del(getTableId() + ":" + getId()); - m_counterOid = SAI_NULL_OBJECT_ID; SWSS_LOG_INFO("Removed counter for the rule %s in table %s", m_id.c_str(), m_tableId.c_str()); @@ -1290,6 +1288,28 @@ bool AclRuleMirror::create() { SWSS_LOG_ENTER(); + if (!createCounter()) + { + return false; + } + + return activate(); +} + +bool AclRuleMirror::remove() +{ + if (!deactivate()) + { + return false; + } + + return AclRule::remove(); +} + +bool AclRuleMirror::activate() +{ + SWSS_LOG_ENTER(); + sai_object_id_t oid = SAI_NULL_OBJECT_ID; bool state = false; @@ -1334,23 +1354,26 @@ bool AclRuleMirror::create() m_state = true; return true; + } -bool AclRuleMirror::remove() +bool AclRuleMirror::deactivate() { + SWSS_LOG_ENTER(); + if (!m_state) { return true; } - if (!AclRule::remove()) + if (!AclRule::removeRule()) { return false; } if (!m_pMirrorOrch->decreaseRefCount(m_sessionName)) { - throw runtime_error("Failed to decrease mirror session reference count"); + SWSS_LOG_THROW("Failed to decrease mirror session reference count for session %s", m_sessionName.c_str()); } m_state = false; @@ -1375,15 +1398,12 @@ void AclRuleMirror::update(SubjectType type, void *cntx) if (update->active) { SWSS_LOG_INFO("Activating mirroring ACL %s for session %s", m_id.c_str(), m_sessionName.c_str()); - create(); + activate(); } else { - // Store counters before deactivating ACL rule - counters += AclRule::getCounters(); - SWSS_LOG_INFO("Deactivating mirroring ACL %s for session %s", m_id.c_str(), m_sessionName.c_str()); - remove(); + deactivate(); } } @@ -2014,18 +2034,6 @@ bool AclTable::clear() return true; } -AclRuleCounters AclRuleMirror::getCounters() -{ - AclRuleCounters cnt(counters); - - if (m_state) - { - cnt += AclRule::getCounters(); - } - - return cnt; -} - AclRuleDTelFlowWatchListEntry::AclRuleDTelFlowWatchListEntry(AclOrch *aclOrch, DTelOrch *dtel, string rule, string table, acl_table_type_t type) : AclRule(aclOrch, rule, table, type), m_pDTelOrch(dtel) @@ -2137,26 +2145,45 @@ bool AclRuleDTelFlowWatchListEntry::create() { SWSS_LOG_ENTER(); - if (!m_pDTelOrch) + if (!createCounter()) { return false; } - if (INT_enabled && !INT_session_valid) + return activate(); +} + +bool AclRuleDTelFlowWatchListEntry::remove() +{ + if (!deactivate()) { - return true; + return false; } - if (!AclRule::create()) + return AclRule::remove(); +} + +bool AclRuleDTelFlowWatchListEntry::activate() +{ + SWSS_LOG_ENTER(); + + if (!m_pDTelOrch) { return false; } - return true; + if (INT_enabled && !INT_session_valid) + { + return true; + } + + return AclRule::create(); } -bool AclRuleDTelFlowWatchListEntry::remove() +bool AclRuleDTelFlowWatchListEntry::deactivate() { + SWSS_LOG_ENTER(); + if (!m_pDTelOrch) { return false; @@ -2167,7 +2194,7 @@ bool AclRuleDTelFlowWatchListEntry::remove() return true; } - if (!AclRule::remove()) + if (!AclRule::removeRule()) { return false; } @@ -2231,12 +2258,12 @@ void AclRuleDTelFlowWatchListEntry::update(SubjectType type, void *cntx) INT_session_valid = true; - create(); + activate(); } else { SWSS_LOG_INFO("Deactivating INT watchlist %s for session %s", m_id.c_str(), m_intSessionId.c_str()); - remove(); + deactivate(); INT_session_valid = false; } } @@ -2524,14 +2551,6 @@ void AclOrch::init(vector& connectors, PortsOrch *portOrch, Mirr // Attach observers m_mirrorOrch->attach(this); gPortsOrch->attach(this); - - // Should be initialized last to guaranty that object is - // initialized before thread start. - auto interv = timespec { .tv_sec = COUNTERS_READ_INTERVAL, .tv_nsec = 0 }; - auto timer = new SelectableTimer(interv); - auto executor = new ExecutableTimer(timer, this, "ACL_POLL_TIMER"); - Orch::addExecutor(executor); - timer->start(); } void AclOrch::queryAclActionCapability() @@ -2759,7 +2778,14 @@ AclOrch::AclOrch(vector& connectors, SwitchOrch *switchOrch, m_mirrorOrch(mirrorOrch), m_neighOrch(neighOrch), m_routeOrch(routeOrch), - m_dTelOrch(dtelOrch) + m_dTelOrch(dtelOrch), + m_acl_counter_rule_map(&m_db, COUNTERS_ACL_COUNTER_RULE_MAP), + m_flex_counter_manager( + ACL_COUNTER_FLEX_COUNTER_GROUP, + StatsMode::READ, + ACL_COUNTER_DEFAULT_POLLING_INTERVAL_MS, + ACL_COUNTER_DEFAULT_ENABLED_STATE + ) { SWSS_LOG_ENTER(); @@ -2781,9 +2807,6 @@ AclOrch::~AclOrch() m_dTelOrch->detach(this); } - m_bCollectCounters = false; - m_sleepGuard.notify_all(); - deleteDTelWatchListTables(); } @@ -2798,8 +2821,6 @@ void AclOrch::update(SubjectType type, void *cntx) return; } - unique_lock lock(m_countersMutex); - // ACL table deals with port change // ACL rule deals with mirror session change and int session change for (auto& table : m_AclTables) @@ -2831,12 +2852,10 @@ void AclOrch::doTask(Consumer &consumer) if (table_name == CFG_ACL_TABLE_TABLE_NAME || table_name == APP_ACL_TABLE_TABLE_NAME) { - unique_lock lock(m_countersMutex); doAclTableTask(consumer); } else if (table_name == CFG_ACL_RULE_TABLE_NAME || table_name == APP_ACL_RULE_TABLE_NAME) { - unique_lock lock(m_countersMutex); doAclRuleTask(consumer); } else @@ -3165,7 +3184,17 @@ bool AclOrch::addAclRule(shared_ptr newRule, string table_id) return false; } - return m_AclTables[table_oid].add(newRule); + if (!m_AclTables[table_oid].add(newRule)) + { + return false; + } + + if (newRule->hasCounter()) + { + registerFlexCounter(*newRule); + } + + return true; } bool AclOrch::removeAclRule(string table_id, string rule_id) @@ -3177,6 +3206,17 @@ bool AclOrch::removeAclRule(string table_id, string rule_id) return true; } + auto rule = getAclRule(table_id, rule_id); + if (!rule) + { + return false; + } + + if (rule->hasCounter()) + { + deregisterFlexCounter(*rule); + } + return m_AclTables[table_oid].remove(rule_id); } @@ -3308,6 +3348,7 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) return false; } + registerFlexCounter(*rule); return true; } @@ -3321,6 +3362,7 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) return false; } + deregisterFlexCounter(*rule); return true; } @@ -3824,30 +3866,6 @@ sai_status_t AclOrch::deleteUnbindAclTable(sai_object_id_t table_oid) return sai_acl_api->remove_acl_table(table_oid); } -void AclOrch::doTask(SelectableTimer &timer) -{ - SWSS_LOG_ENTER(); - - for (auto& table_it : m_AclTables) - { - vector values; - - for (auto rule_it : table_it.second.rules) - { - AclRuleCounters cnt = rule_it.second->getCounters(); - - swss::FieldValueTuple fvtp("Packets", to_string(cnt.packets)); - values.push_back(fvtp); - swss::FieldValueTuple fvtb("Bytes", to_string(cnt.bytes)); - values.push_back(fvtb); - - AclOrch::getCountersTable().set(rule_it.second->getTableId() + ":" - + rule_it.second->getId(), values, ""); - } - values.clear(); - } -} - sai_status_t AclOrch::bindAclTable(AclTable &aclTable, bool bind) { SWSS_LOG_ENTER(); @@ -4083,6 +4101,42 @@ sai_status_t AclOrch::deleteDTelWatchListTables() return SAI_STATUS_SUCCESS; } +void AclOrch::registerFlexCounter(const AclRule& rule) +{ + SWSS_LOG_ENTER(); + + string ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); + FieldValueTuple ruleNameToCounterOid(ruleIdentifier, sai_serialize_object_id(rule.getCounterOid())); + + unordered_set serializedCounterStatAttrs; + for (auto counterAttrPair: aclCounterLookup) + { + sai_acl_counter_attr_t id {}; + tie(ignore, id) = counterAttrPair; + auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_COUNTER, id); + if (!meta) + { + SWSS_LOG_THROW("SAI Bug: Failed to get metadata of attribute %d for SAI_OBJECT_TYPE_ACL_COUNTER", id); + } + serializedCounterStatAttrs.insert(sai_serialize_attr_id(*meta)); + } + + m_flex_counter_manager.setCounterIdList(rule.getCounterOid(), CounterType::ACL_COUNTER, serializedCounterStatAttrs); + m_acl_counter_rule_map.set("", {ruleNameToCounterOid}); +} + +void AclOrch::deregisterFlexCounter(const AclRule& rule) +{ + string ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); + m_db.hdel(COUNTERS_ACL_COUNTER_RULE_MAP, rule.getId()); + m_flex_counter_manager.clearCounterIdList(rule.getCounterOid()); +} + +string AclOrch::generateAclRuleIdentifierInCountersDb(const AclRule& rule) const +{ + return rule.getTableId() + m_acl_counter_rule_map.getTableNameSeparator() + rule.getId(); +} + bool AclOrch::getAclBindPortId(Port &port, sai_object_id_t &port_id) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index f2a5e0826b..081056603b 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -15,14 +15,10 @@ #include "mirrororch.h" #include "dtelorch.h" #include "observer.h" +#include "flex_counter_manager.h" #include "acltable.h" -// ACL counters update interval in the DB -// Value is in seconds. Should not be less than 5 seconds -// (in worst case update of 1265 counters takes almost 5 sec) -#define COUNTERS_READ_INTERVAL 10 - #define RULE_PRIORITY "PRIORITY" #define MATCH_IN_PORTS "IN_PORTS" #define MATCH_OUT_PORTS "OUT_PORTS" @@ -94,6 +90,8 @@ #define RULE_OPER_ADD 0 #define RULE_OPER_DELETE 1 +#define ACL_COUNTER_FLEX_COUNTER_GROUP "ACL_STAT_COUNTER" + typedef map acl_rule_attr_lookup_t; typedef map acl_ip_type_lookup_t; typedef map acl_dtel_flow_op_type_lookup_t; @@ -126,31 +124,6 @@ class AclRange static map m_ranges; }; -struct AclRuleCounters -{ - uint64_t packets; - uint64_t bytes; - - AclRuleCounters(uint64_t p = 0, uint64_t b = 0) : - packets(p), - bytes(b) - { - } - - AclRuleCounters(const AclRuleCounters& rhs) : - packets(rhs.packets), - bytes(rhs.bytes) - { - } - - AclRuleCounters& operator +=(const AclRuleCounters& rhs) - { - packets += rhs.packets; - bytes += rhs.bytes; - return *this; - } -}; - class AclRule { public: @@ -173,24 +146,33 @@ class AclRule virtual bool enableCounter(); virtual bool disableCounter(); - virtual AclRuleCounters getCounters(); - string getId() + sai_object_id_t getOid() const + { + return m_ruleOid; + } + + string getId() const { return m_id; } - string getTableId() + string getTableId() const { return m_tableId; } - sai_object_id_t getCounterOid() + sai_object_id_t getCounterOid() const { return m_counterOid; } - vector getInPorts() + bool hasCounter() const + { + return getCounterOid() != SAI_NULL_OBJECT_ID; + } + + vector getInPorts() { return m_inPorts; } @@ -202,6 +184,7 @@ class AclRule virtual bool createCounter(); virtual bool removeCounter(); virtual bool removeRanges(); + virtual bool removeRule(); void decreaseNextHopRefCount(); @@ -273,12 +256,13 @@ class AclRuleMirror: public AclRule bool create(); bool remove(); void update(SubjectType, void *); - AclRuleCounters getCounters(); + + bool activate(); + bool deactivate(); protected: bool m_state {false}; string m_sessionName; - AclRuleCounters counters; MirrorOrch *m_pMirrorOrch {nullptr}; }; @@ -292,6 +276,9 @@ class AclRuleDTelFlowWatchListEntry: public AclRule bool remove(); void update(SubjectType, void *); + bool activate(); + bool deactivate(); + protected: DTelOrch *m_pDTelOrch; string m_intSessionId; @@ -428,7 +415,7 @@ class AclOrch : public Orch, public Observer map m_mirrorTableCapabilities; static sai_acl_action_type_t getAclActionFromAclEntry(sai_acl_entry_attr_t attr); - + // Get the OID for the ACL bind point for a given port static bool getAclBindPortId(Port& port, sai_object_id_t& port_id); @@ -443,7 +430,6 @@ class AclOrch : public Orch, public Observer void doTask(Consumer &consumer); void doAclTableTask(Consumer &consumer); void doAclRuleTask(Consumer &consumer); - void doTask(SelectableTimer &timer); void init(vector& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch); void queryMirrorTableCapability(); @@ -476,13 +462,14 @@ class AclOrch : public Orch, public Observer sai_status_t createDTelWatchListTables(); sai_status_t deleteDTelWatchListTables(); + void registerFlexCounter(const AclRule& rule); + void deregisterFlexCounter(const AclRule& rule); + string generateAclRuleIdentifierInCountersDb(const AclRule& rule) const; + map m_AclTables; // TODO: Move all ACL tables into one map: name -> instance map m_ctrlAclTables; - static mutex m_countersMutex; - static condition_variable m_sleepGuard; - static bool m_bCollectCounters; static DBConnector m_db; static Table m_countersTable; @@ -491,6 +478,8 @@ class AclOrch : public Orch, public Observer acl_capabilities_t m_aclCapabilities; acl_action_enum_values_capabilities_t m_aclEnumActionCapabilities; + Table m_acl_counter_rule_map; + FlexCounterManager m_flex_counter_manager; }; #endif /* SWSS_ACLORCH_H */ diff --git a/orchagent/flex_counter/flex_counter_manager.cpp b/orchagent/flex_counter/flex_counter_manager.cpp index 299e238d37..de0f0fab98 100644 --- a/orchagent/flex_counter/flex_counter_manager.cpp +++ b/orchagent/flex_counter/flex_counter_manager.cpp @@ -39,6 +39,7 @@ const unordered_map FlexCounterManager::counter_id_field_lo { CounterType::PORT, PORT_COUNTER_ID_LIST }, { CounterType::QUEUE, QUEUE_COUNTER_ID_LIST }, { CounterType::MACSEC_SA_ATTR, MACSEC_SA_ATTR_ID_LIST }, + { CounterType::ACL_COUNTER, ACL_COUNTER_ATTR_ID_LIST }, }; FlexCounterManager::FlexCounterManager( diff --git a/orchagent/flex_counter/flex_counter_manager.h b/orchagent/flex_counter/flex_counter_manager.h index 4df99c90bd..6a6d03e71c 100644 --- a/orchagent/flex_counter/flex_counter_manager.h +++ b/orchagent/flex_counter/flex_counter_manager.h @@ -24,6 +24,7 @@ enum class CounterType PORT_DEBUG, SWITCH_DEBUG, MACSEC_SA_ATTR, + ACL_COUNTER, }; // FlexCounterManager allows users to manage a group of flex counters. diff --git a/orchagent/flexcounterorch.cpp b/orchagent/flexcounterorch.cpp index 51c9c9325a..d91b36c517 100644 --- a/orchagent/flexcounterorch.cpp +++ b/orchagent/flexcounterorch.cpp @@ -23,6 +23,7 @@ extern BufferOrch *gBufferOrch; #define QUEUE_KEY "QUEUE" #define PG_WATERMARK_KEY "PG_WATERMARK" #define RIF_KEY "RIF" +#define ACL_KEY "ACL" unordered_map flexCounterGroupMap = { @@ -38,6 +39,7 @@ unordered_map flexCounterGroupMap = {"RIF", RIF_STAT_COUNTER_FLEX_COUNTER_GROUP}, {"RIF_RATES", RIF_RATE_COUNTER_FLEX_COUNTER_GROUP}, {"DEBUG_COUNTER", DEBUG_COUNTER_FLEX_COUNTER_GROUP}, + {"ACL", ACL_COUNTER_FLEX_COUNTER_GROUP}, }; diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index 6e35113904..37ae9c712c 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -1,5 +1,5 @@ """Utilities for interacting with ACLs when writing VS tests.""" -from typing import Callable, Dict, List +from typing import Callable, Dict, List, Optional, Tuple class DVSAcl: @@ -427,6 +427,7 @@ def verify_acl_rule( fvs = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) self._check_acl_entry_base(fvs, sai_qualifiers, action, priority) self._check_acl_entry_packet_action(fvs, action) + self._check_acl_entry_counters_map(acl_rule_id) def verify_redirect_acl_rule( self, @@ -451,6 +452,7 @@ def verify_redirect_acl_rule( fvs = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) self._check_acl_entry_base(fvs, sai_qualifiers, "REDIRECT", priority) self._check_acl_entry_redirect_action(fvs, expected_destination) + self._check_acl_entry_counters_map(acl_rule_id) def verify_nat_acl_rule( self, @@ -471,6 +473,7 @@ def verify_nat_acl_rule( fvs = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) self._check_acl_entry_base(fvs, sai_qualifiers, "DO_NOT_NAT", priority) + self._check_acl_entry_counters_map(acl_rule_id) def verify_mirror_acl_rule( self, @@ -496,6 +499,7 @@ def verify_mirror_acl_rule( fvs = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) self._check_acl_entry_base(fvs, sai_qualifiers, "MIRROR", priority) self._check_acl_entry_mirror_action(fvs, session_oid, stage) + self._check_acl_entry_counters_map(acl_rule_id) def verify_acl_rule_set( self, @@ -517,7 +521,6 @@ def verify_acl_rule_set( for entry in acl_entries: rule = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", entry) priority = rule.get("SAI_ACL_ENTRY_ATTR_PRIORITY", None) - assert priority in priorities self.verify_acl_rule(expected[priority], in_actions[priority], priority, entry) # FIXME: This `get_x_comparator` abstraction is a bit clunky, we should try to improve this later. @@ -628,3 +631,12 @@ def _check_acl_entry_redirect_action(self, entry: Dict[str, str], expected_desti def _check_acl_entry_mirror_action(self, entry: Dict[str, str], session_oid: str, stage: str) -> None: assert stage in self.ADB_MIRROR_ACTION_LOOKUP assert entry.get(self.ADB_MIRROR_ACTION_LOOKUP[stage]) == session_oid + + def _check_acl_entry_counters_map(self, acl_entry_oid: str): + entry = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_entry_oid) + counter_oid = entry.get("SAI_ACL_ENTRY_ATTR_ACTION_COUNTER") + if counter_oid is None: + return + rule_to_counter_map = self.counters_db.get_entry("ACL_COUNTER_RULE_MAP", "") + counter_to_rule_map = {v: k for k, v in rule_to_counter_map.items()} + assert counter_oid in counter_to_rule_map diff --git a/tests/test_flex_counters.py b/tests/test_flex_counters.py index ecdc844572..a35487ba6e 100644 --- a/tests/test_flex_counters.py +++ b/tests/test_flex_counters.py @@ -8,6 +8,7 @@ BUFFER_POOL_WATERMARK_KEY = "BUFFER_POOL_WATERMARK" PORT_BUFFER_DROP_KEY = "PORT_BUFFER_DROP" PG_WATERMARK_KEY = "PG_WATERMARK" +ACL_KEY = "ACL" # Counter stats on FlexCountersDB PORT_STAT = "PORT_STAT_COUNTER" @@ -16,6 +17,7 @@ BUFFER_POOL_WATERMARK_STAT = "BUFFER_POOL_WATERMARK_STAT_COUNTER" PORT_BUFFER_DROP_STAT = "PORT_BUFFER_DROP_STAT" PG_WATERMARK_STAT = "PG_WATERMARK_STAT_COUNTER" +ACL_STAT = "ACL_STAT_COUNTER" # Counter maps on CountersDB PORT_MAP = "COUNTERS_PORT_NAME_MAP" @@ -24,6 +26,7 @@ BUFFER_POOL_WATERMARK_MAP = "COUNTERS_BUFFER_POOL_NAME_MAP" PORT_BUFFER_DROP_MAP = "COUNTERS_PORT_NAME_MAP" PG_WATERMARK_MAP = "COUNTERS_PG_NAME_MAP" +ACL_MAP = "ACL_COUNTER_RULE_MAP" NUMBER_OF_RETRIES = 10 CPU_PORT_OID = "0x0" @@ -33,7 +36,8 @@ "rif_counter":[RIF_KEY, RIF_STAT, RIF_MAP], "buffer_pool_watermark_counter":[BUFFER_POOL_WATERMARK_KEY, BUFFER_POOL_WATERMARK_STAT, BUFFER_POOL_WATERMARK_MAP], "port_buffer_drop_counter":[PORT_BUFFER_DROP_KEY, PORT_BUFFER_DROP_STAT, PORT_BUFFER_DROP_MAP], - "pg_watermark_counter":[PG_WATERMARK_KEY, PG_WATERMARK_STAT, PG_WATERMARK_MAP]} + "pg_watermark_counter":[PG_WATERMARK_KEY, PG_WATERMARK_STAT, PG_WATERMARK_MAP], + "acl_counter":[ACL_KEY, ACL_STAT, ACL_MAP]} class TestFlexCounters(object): @@ -101,6 +105,21 @@ def test_flex_counters(self, dvs, counter_type): if counter_type == "rif_counter": self.config_db.db_connection.hset('INTERFACE|Ethernet0', "NULL", "NULL") self.config_db.db_connection.hset('INTERFACE|Ethernet0|192.168.0.1/24', "NULL", "NULL") + elif counter_type == "acl_counter": + self.config_db.create_entry('ACL_TABLE', 'DATAACL', + { + 'STAGE': 'INGRESS', + 'PORTS': 'Ethernet0', + 'TYPE': 'L3' + } + ) + self.config_db.create_entry('ACL_RULE', 'DATAACL|RULE0', + { + 'MATCH_ETHER_TYPE': '2048', + 'PACKET_ACTION': 'FORWARD', + 'PRIORITY': '9999' + } + ) self.enable_flex_counter_group(counter_key, counter_map) self.verify_flex_counters_populated(counter_map, counter_stat) @@ -110,3 +129,6 @@ def test_flex_counters(self, dvs, counter_type): if counter_type == "rif_counter": self.config_db.db_connection.hdel('INTERFACE|Ethernet0|192.168.0.1/24', "NULL") + elif counter_type == "acl_counter": + self.config_db.delete_entry('ACL_RULE', 'DATAACL|RULE0') + self.config_db.delete_entry('ACL_TABLE', 'DATAACL') From c07009e96d98db721f2b8bef38937347b9ef89f4 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Tue, 28 Sep 2021 17:30:14 +0300 Subject: [PATCH 02/10] internal review comments handling Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 24 ++++++++++++------------ orchagent/aclorch.h | 3 +-- tests/dvslib/dvs_acl.py | 1 + 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 99ffe6c540..dd26aa08d2 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -19,8 +19,8 @@ map AclRange::m_ranges; sai_uint32_t AclRule::m_minPriority = 0; sai_uint32_t AclRule::m_maxPriority = 0; -swss::DBConnector AclOrch::m_db("COUNTERS_DB", 0); -swss::Table AclOrch::m_countersTable(&m_db, "COUNTERS"); +swss::DBConnector AclOrch::m_countersDb("COUNTERS_DB", 0); +swss::Table AclOrch::m_countersTable(&m_countersDb, "COUNTERS"); extern sai_acl_api_t* sai_acl_api; extern sai_port_api_t* sai_port_api; @@ -855,7 +855,7 @@ bool AclRule::createCounter() for (auto counterAttrPair: aclCounterLookup) { - tie(attr.id, ignore) = counterAttrPair; + tie(attr.id, std::ignore) = counterAttrPair; attr.value.booldata = true; counter_attrs.push_back(attr); } @@ -2779,7 +2779,6 @@ AclOrch::AclOrch(vector& connectors, SwitchOrch *switchOrch, m_neighOrch(neighOrch), m_routeOrch(routeOrch), m_dTelOrch(dtelOrch), - m_acl_counter_rule_map(&m_db, COUNTERS_ACL_COUNTER_RULE_MAP), m_flex_counter_manager( ACL_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, @@ -3352,6 +3351,7 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) return true; } + deregisterFlexCounter(*rule); if (!rule->disableCounter()) { SWSS_LOG_ERROR( @@ -3359,10 +3359,10 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) rule_id.c_str(), table_id.c_str() ); + registerFlexCounter(*rule); return false; } - deregisterFlexCounter(*rule); return true; } @@ -4105,14 +4105,14 @@ void AclOrch::registerFlexCounter(const AclRule& rule) { SWSS_LOG_ENTER(); - string ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); - FieldValueTuple ruleNameToCounterOid(ruleIdentifier, sai_serialize_object_id(rule.getCounterOid())); + auto ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); + auto counterOidStr = sai_serialize_object_id(rule.getCounterOid()); unordered_set serializedCounterStatAttrs; for (auto counterAttrPair: aclCounterLookup) { sai_acl_counter_attr_t id {}; - tie(ignore, id) = counterAttrPair; + tie(std::ignore, id) = counterAttrPair; auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_COUNTER, id); if (!meta) { @@ -4122,19 +4122,19 @@ void AclOrch::registerFlexCounter(const AclRule& rule) } m_flex_counter_manager.setCounterIdList(rule.getCounterOid(), CounterType::ACL_COUNTER, serializedCounterStatAttrs); - m_acl_counter_rule_map.set("", {ruleNameToCounterOid}); + m_countersDb.hset(COUNTERS_ACL_COUNTER_RULE_MAP, ruleIdentifier, counterOidStr); } void AclOrch::deregisterFlexCounter(const AclRule& rule) { - string ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); - m_db.hdel(COUNTERS_ACL_COUNTER_RULE_MAP, rule.getId()); + auto ruleIdentifier = generateAclRuleIdentifierInCountersDb(rule); + m_countersDb.hdel(COUNTERS_ACL_COUNTER_RULE_MAP, rule.getId()); m_flex_counter_manager.clearCounterIdList(rule.getCounterOid()); } string AclOrch::generateAclRuleIdentifierInCountersDb(const AclRule& rule) const { - return rule.getTableId() + m_acl_counter_rule_map.getTableNameSeparator() + rule.getId(); + return rule.getTableId() + m_countersTable.getTableNameSeparator() + rule.getId(); } bool AclOrch::getAclBindPortId(Port &port, sai_object_id_t &port_id) diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 081056603b..a06a912cdd 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -470,7 +470,7 @@ class AclOrch : public Orch, public Observer // TODO: Move all ACL tables into one map: name -> instance map m_ctrlAclTables; - static DBConnector m_db; + static DBConnector m_countersDb; static Table m_countersTable; map m_mirrorTableId; @@ -478,7 +478,6 @@ class AclOrch : public Orch, public Observer acl_capabilities_t m_aclCapabilities; acl_action_enum_values_capabilities_t m_aclEnumActionCapabilities; - Table m_acl_counter_rule_map; FlexCounterManager m_flex_counter_manager; }; diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index 37ae9c712c..eadf84176e 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -521,6 +521,7 @@ def verify_acl_rule_set( for entry in acl_entries: rule = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", entry) priority = rule.get("SAI_ACL_ENTRY_ATTR_PRIORITY", None) + assert priority in priorities self.verify_acl_rule(expected[priority], in_actions[priority], priority, entry) # FIXME: This `get_x_comparator` abstraction is a bit clunky, we should try to improve this later. From 1a08896033f42f176e1151e67dbe875ae90c0da2 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Sun, 3 Oct 2021 18:27:16 +0000 Subject: [PATCH 03/10] use const auto& in for loops Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index dd26aa08d2..9b825b9350 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -853,7 +853,7 @@ bool AclRule::createCounter() attr.value.oid = m_tableOid; counter_attrs.push_back(attr); - for (auto counterAttrPair: aclCounterLookup) + for (const auto& counterAttrPair: aclCounterLookup) { tie(attr.id, std::ignore) = counterAttrPair; attr.value.booldata = true; @@ -2783,7 +2783,7 @@ AclOrch::AclOrch(vector& connectors, SwitchOrch *switchOrch, ACL_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, ACL_COUNTER_DEFAULT_POLLING_INTERVAL_MS, - ACL_COUNTER_DEFAULT_ENABLED_STATE + ACL_COUNTER_DEFAULT_ENABLED_STATE ) { SWSS_LOG_ENTER(); @@ -4109,7 +4109,7 @@ void AclOrch::registerFlexCounter(const AclRule& rule) auto counterOidStr = sai_serialize_object_id(rule.getCounterOid()); unordered_set serializedCounterStatAttrs; - for (auto counterAttrPair: aclCounterLookup) + for (const auto& counterAttrPair: aclCounterLookup) { sai_acl_counter_attr_t id {}; tie(std::ignore, id) = counterAttrPair; From 2463f2851af42bfa7d23b95bbea4f70f79808156 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Wed, 6 Oct 2021 17:32:48 +0300 Subject: [PATCH 04/10] remove unneeded import Signed-off-by: Stepan Blyshchak --- tests/dvslib/dvs_acl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index eadf84176e..dbf9791b53 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -1,5 +1,5 @@ """Utilities for interacting with ACLs when writing VS tests.""" -from typing import Callable, Dict, List, Optional, Tuple +from typing import Callable, Dict, List class DVSAcl: From 31aff03f09570035742e74a156d4ee010c3f2426 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Mon, 18 Oct 2021 15:12:58 +0300 Subject: [PATCH 05/10] avoid double resource create/remove calls Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 51 +++++++++++++++---------------------------- orchagent/aclorch.h | 9 ++++---- 2 files changed, 23 insertions(+), 37 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 9b825b9350..1250bd5e60 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -477,6 +477,16 @@ bool AclRule::processIpType(string type, sai_uint32_t &ip_type) } bool AclRule::create() +{ + if (m_createCounter && !createCounter()) + { + return false; + } + + return createRule(); +} + +bool AclRule::createRule() { SWSS_LOG_ENTER(); @@ -488,11 +498,6 @@ bool AclRule::create() sai_attribute_t attr; sai_status_t status; - if (m_createCounter && !createCounter()) - { - return false; - } - // store table oid this rule belongs to attr.id = SAI_ACL_ENTRY_ATTR_TABLE_ID; attr.value.oid = table_oid; @@ -1284,26 +1289,16 @@ bool AclRuleMirror::validate() return true; } -bool AclRuleMirror::create() +bool AclRuleMirror::createRule() { SWSS_LOG_ENTER(); - if (!createCounter()) - { - return false; - } - return activate(); } -bool AclRuleMirror::remove() +bool AclRuleMirror::removeRule() { - if (!deactivate()) - { - return false; - } - - return AclRule::remove(); + return deactivate(); } bool AclRuleMirror::activate() @@ -1341,7 +1336,7 @@ bool AclRuleMirror::activate() it.second.aclaction.parameter.objlist.count = 1; } - if (!AclRule::create()) + if (!AclRule::createRule()) { return false; } @@ -2141,26 +2136,16 @@ bool AclRuleDTelFlowWatchListEntry::validate() return true; } -bool AclRuleDTelFlowWatchListEntry::create() +bool AclRuleDTelFlowWatchListEntry::createRule() { SWSS_LOG_ENTER(); - if (!createCounter()) - { - return false; - } - return activate(); } -bool AclRuleDTelFlowWatchListEntry::remove() +bool AclRuleDTelFlowWatchListEntry::removeRule() { - if (!deactivate()) - { - return false; - } - - return AclRule::remove(); + return deactivate(); } bool AclRuleDTelFlowWatchListEntry::activate() @@ -2177,7 +2162,7 @@ bool AclRuleDTelFlowWatchListEntry::activate() return true; } - return AclRule::create(); + return AclRule::createRule(); } bool AclRuleDTelFlowWatchListEntry::deactivate() diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index a06a912cdd..a34d06846e 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -182,6 +182,7 @@ class AclRule protected: virtual bool createCounter(); + virtual bool createRule(); virtual bool removeCounter(); virtual bool removeRanges(); virtual bool removeRule(); @@ -253,8 +254,8 @@ class AclRuleMirror: public AclRule bool validateAddAction(string attr_name, string attr_value); bool validateAddMatch(string attr_name, string attr_value); bool validate(); - bool create(); - bool remove(); + bool createRule(); + bool removeRule(); void update(SubjectType, void *); bool activate(); @@ -272,8 +273,8 @@ class AclRuleDTelFlowWatchListEntry: public AclRule AclRuleDTelFlowWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table, acl_table_type_t type); bool validateAddAction(string attr_name, string attr_value); bool validate(); - bool create(); - bool remove(); + bool createRule(); + bool removeRule(); void update(SubjectType, void *); bool activate(); From 94f1ee7c1c78f4e9af57f3c80760f53b024b9d0d Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Mon, 18 Oct 2021 15:13:59 +0300 Subject: [PATCH 06/10] dont register FC counter on counter disablement failure Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 1250bd5e60..5dead2fc75 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -3344,7 +3344,6 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) rule_id.c_str(), table_id.c_str() ); - registerFlexCounter(*rule); return false; } From f6d788185bcb6be32b46c390dc3a4fde34575592 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Mon, 8 Nov 2021 18:22:52 +0200 Subject: [PATCH 07/10] fix merge issue Signed-off-by: Stepan Blyschak --- tests/test_flex_counters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_flex_counters.py b/tests/test_flex_counters.py index 3bb62c5b45..d8b8670050 100644 --- a/tests/test_flex_counters.py +++ b/tests/test_flex_counters.py @@ -41,7 +41,7 @@ "buffer_pool_watermark_counter":[BUFFER_POOL_WATERMARK_KEY, BUFFER_POOL_WATERMARK_STAT, BUFFER_POOL_WATERMARK_MAP], "port_buffer_drop_counter":[PORT_BUFFER_DROP_KEY, PORT_BUFFER_DROP_STAT, PORT_BUFFER_DROP_MAP], "pg_watermark_counter":[PG_WATERMARK_KEY, PG_WATERMARK_STAT, PG_WATERMARK_MAP], - "acl_counter":[ACL_KEY, ACL_STAT, ACL_MAP]} + "acl_counter":[ACL_KEY, ACL_STAT, ACL_MAP], "vxlan_tunnel_counter":[TUNNEL_KEY, TUNNEL_STAT, TUNNEL_MAP]} From 5d670fa1abcb3f859818c9845799bcbbd0057f75 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Tue, 9 Nov 2021 01:00:49 +0200 Subject: [PATCH 08/10] fix more merge issues Signed-off-by: Stepan Blyshchak --- tests/test_flex_counters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_flex_counters.py b/tests/test_flex_counters.py index d8b8670050..7ea5da0ef1 100644 --- a/tests/test_flex_counters.py +++ b/tests/test_flex_counters.py @@ -155,12 +155,12 @@ def test_flex_counters(self, dvs, counter_type): if counter_type == "port_counter": self.verify_only_phy_ports_created() - else if counter_type == "rif_counter": + elif counter_type == "rif_counter": self.config_db.db_connection.hdel('INTERFACE|Ethernet0|192.168.0.1/24', "NULL") elif counter_type == "acl_counter": self.config_db.delete_entry('ACL_RULE', 'DATAACL|RULE0') self.config_db.delete_entry('ACL_TABLE', 'DATAACL') - else if counter_type == "vxlan_tunnel_counter": + elif counter_type == "vxlan_tunnel_counter": self.verify_tunnel_type_vxlan(counter_map, TUNNEL_TYPE_MAP) self.config_db.delete_entry("VLAN","Vlan10") self.config_db.delete_entry("VLAN_TUNNEL","vtep1") From 25d3b84be75ede4d4b68b8fd51d0a438e33b4c5f Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Tue, 9 Nov 2021 15:51:06 +0200 Subject: [PATCH 09/10] [test_flex_counters] fix the name of the match Signed-off-by: Stepan Blyschak --- tests/test_flex_counters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_flex_counters.py b/tests/test_flex_counters.py index 7ea5da0ef1..60a3a129d1 100644 --- a/tests/test_flex_counters.py +++ b/tests/test_flex_counters.py @@ -138,7 +138,7 @@ def test_flex_counters(self, dvs, counter_type): ) self.config_db.create_entry('ACL_RULE', 'DATAACL|RULE0', { - 'MATCH_ETHER_TYPE': '2048', + 'ETHER_TYPE': '2048', 'PACKET_ACTION': 'FORWARD', 'PRIORITY': '9999' } From 8365820b87ded4b0c4efbcd4ab2e3e52c726dff7 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Tue, 9 Nov 2021 15:52:35 +0200 Subject: [PATCH 10/10] fix counter leak when createRule fails Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 4733cfab0b..d9dc26e99e 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -483,7 +483,13 @@ bool AclRule::create() return false; } - return createRule(); + if (!createRule()) + { + removeCounter(); + return false; + } + + return true; } bool AclRule::createRule()