diff --git a/orchagent/pfcactionhandler.cpp b/orchagent/pfcactionhandler.cpp index 32f984cea1..7025ef155c 100644 --- a/orchagent/pfcactionhandler.cpp +++ b/orchagent/pfcactionhandler.cpp @@ -324,9 +324,24 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, 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); - createPfcAclRule(newRule, queueId, m_strIngressTable, port); + if (createPfcAclTable(port, m_strIngressTable, true)) + { + shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strIngressTable); + if (!createPfcAclRule(newRule, queueId, m_strIngressTable, port)) + { + SWSS_LOG_ERROR("Failed to create ingress PFCWD drop rule %s, last SAI status %s", + m_strRule.c_str(), sai_serialize_status(newRule->getLastSaiStatus()).c_str()); + m_rolledBack = true; + return; + } + } + else + { + SWSS_LOG_ERROR("Failed to create ingress PFCWD ACL table %s; PFCWD drop action is not installed", + m_strIngressTable.c_str()); + m_rolledBack = true; + return; + } } else { @@ -334,9 +349,15 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, if (rule == nullptr) { shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strIngressTable); - createPfcAclRule(newRule, queueId, m_strIngressTable, port); - } - else + if (!createPfcAclRule(newRule, queueId, m_strIngressTable, port)) + { + SWSS_LOG_ERROR("Failed to create ingress PFCWD drop rule %s, last SAI status %s", + m_strRule.c_str(), sai_serialize_status(newRule->getLastSaiStatus()).c_str()); + m_rolledBack = true; + return; + } + } + else { gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_ADD); } @@ -365,9 +386,28 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, if (found == m_aclTables.end()) { // First time of handling PFC, create ACL table and also ACL rule. - createPfcAclTable(port, m_strEgressTable, false); - shared_ptr newRule = make_shared(gAclOrch, m_strEgressRule, m_strEgressTable); - createPfcAclRule(newRule, queueId, m_strEgressTable, port); + if (createPfcAclTable(port, m_strEgressTable, false)) + { + shared_ptr newRule = make_shared(gAclOrch, m_strEgressRule, m_strEgressTable); + if (!createPfcAclRule(newRule, queueId, m_strEgressTable, port)) + { + SWSS_LOG_ERROR("Failed to create egress PFCWD drop rule %s, last SAI status %s", + m_strEgressRule.c_str(), sai_serialize_status(newRule->getLastSaiStatus()).c_str()); + // Roll back the egress table created just above (no rule could be + // installed in it) along with the ingress drop rule. + gAclOrch->removeAclTable(m_strEgressTable); + m_aclTables.erase(m_strEgressTable); + removeIngressBinding(port); + m_rolledBack = true; + } + } + else + { + SWSS_LOG_ERROR("Failed to create egress PFCWD ACL table %s; rolling back ingress PFCWD drop rule %s", + m_strEgressTable.c_str(), m_strRule.c_str()); + removeIngressBinding(port); + m_rolledBack = true; + } } else { @@ -376,7 +416,14 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, if (rule == nullptr) { shared_ptr newRule = make_shared(gAclOrch, m_strEgressRule, m_strEgressTable); - createPfcAclRule(newRule, queueId, m_strEgressTable, port); + if (!createPfcAclRule(newRule, queueId, m_strEgressTable, port)) + { + // The egress table is shared with other queues/ports - leave it in place. + SWSS_LOG_ERROR("Failed to create egress PFCWD drop rule %s, last SAI status %s", + m_strEgressRule.c_str(), sai_serialize_status(newRule->getLastSaiStatus()).c_str()); + removeIngressBinding(port); + m_rolledBack = true; + } } } } @@ -387,9 +434,28 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, 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); - createPfcAclRule(newRule, queueId, m_strEgressTable, port); + if (createPfcAclTable(port, m_strEgressTable, false)) + { + shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strEgressTable); + if (!createPfcAclRule(newRule, queueId, m_strEgressTable, port)) + { + SWSS_LOG_ERROR("Failed to create egress PFCWD drop rule %s, last SAI status %s", + m_strRule.c_str(), sai_serialize_status(newRule->getLastSaiStatus()).c_str()); + // Roll back the egress table created just above (no rule could be + // installed in it) along with the ingress drop rule. + gAclOrch->removeAclTable(m_strEgressTable); + m_aclTables.erase(m_strEgressTable); + removeIngressBinding(port); + m_rolledBack = true; + } + } + else + { + SWSS_LOG_ERROR("Failed to create egress PFCWD ACL table %s; rolling back ingress PFCWD drop rule %s", + m_strEgressTable.c_str(), m_strRule.c_str()); + removeIngressBinding(port); + m_rolledBack = true; + } } else { @@ -399,41 +465,60 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, } } -PfcWdAclHandler::~PfcWdAclHandler(void) +void PfcWdAclHandler::removeIngressBinding(sai_object_id_t port) { SWSS_LOG_ENTER(); + // The rule may be absent if creation was skipped (egress PMF full) - clean up + // gracefully; this must not throw when called from the destructor. AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule); if (rule == nullptr) { - SWSS_LOG_THROW("ACL Rule does not exist for rule %s", m_strRule.c_str()); + SWSS_LOG_NOTICE("ACL Rule %s does not exist for table %s", m_strRule.c_str(), m_strIngressTable.c_str()); + return; } vector port_set = rule->getInPorts(); - sai_object_id_t port = getPort(); - if ((port_set.size() == 1) && (port_set[0] == port)) { gAclOrch->removeAclRule(m_strIngressTable, m_strRule); } - else + else { gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_DELETE); - } + } +} + +PfcWdAclHandler::~PfcWdAclHandler(void) +{ + SWSS_LOG_ENTER(); + + sai_object_id_t port = getPort(); + + if (!m_rolledBack) + { + removeIngressBinding(port); + } if (shared_egress_acl_table) { - rule = gAclOrch->getAclRule(m_strEgressTable, m_strEgressRule); + AclRule* rule = gAclOrch->getAclRule(m_strEgressTable, m_strEgressRule); if (rule == nullptr) { - SWSS_LOG_THROW("Egress ACL Rule does not exist for rule %s", m_strEgressRule.c_str()); + SWSS_LOG_NOTICE("Egress ACL Rule %s does not exist for table %s", m_strEgressRule.c_str(), m_strEgressTable.c_str()); + } + else + { + gAclOrch->removeAclRule(m_strEgressTable, m_strEgressRule); } - gAclOrch->removeAclRule(m_strEgressTable, m_strEgressRule); } else { auto found = m_aclTables.find(m_strEgressTable); - found->second.unbind(port); + if (found != m_aclTables.end()) + { + found->second.unbind(port); + } } } @@ -448,7 +533,7 @@ void PfcWdAclHandler::clear() } } -void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, bool ingress) +bool PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, bool ingress) { SWSS_LOG_ENTER(); @@ -466,7 +551,7 @@ void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, b // DROP ACL table is already created SWSS_LOG_NOTICE("ACL table %s exists, reuse the same", strTable.c_str()); aclTable = *(gAclOrch->getTableByOid(table_oid)); - return; + return true; } // Link port only for ingress ACL table or unshared egress ACL table. @@ -489,11 +574,20 @@ void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, b aclTable.validateAddType(*pfcwdType); aclTable.stage = ACL_STAGE_EGRESS; } - - gAclOrch->addAclTable(aclTable); + + if (!gAclOrch->addAclTable(aclTable)) + { + // ACL table create failed (e.g. egress PMF full). Drop the half-built + // entry so the caller skips rule creation instead of throwing later. + SWSS_LOG_ERROR("Failed to create PFCWD ACL table %s", strTable.c_str()); + m_aclTables.erase(strTable); + return false; + } + + return true; } -void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, sai_object_id_t portOid) +bool PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, sai_object_id_t portOid) { SWSS_LOG_ENTER(); @@ -522,9 +616,9 @@ void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t q if (!gPortsOrch->getPort(portOid, p)) { SWSS_LOG_ERROR("Failed to get port structure from port oid 0x%" PRIx64, portOid); - return; + return false; } - + attr_value = p.m_alias; rule->validateAddMatch(attr_name, attr_value); } @@ -533,7 +627,7 @@ void PfcWdAclHandler::createPfcAclRule(shared_ptr rule, uint8_t q attr_value = PACKET_ACTION_DROP; rule->validateAddAction(attr_name, attr_value); - gAclOrch->addAclRule(rule, strTable); + return gAclOrch->addAclRule(rule, strTable); } std::map PfcWdAclHandler::m_aclTables; diff --git a/orchagent/pfcactionhandler.h b/orchagent/pfcactionhandler.h index 9fa409af30..98557b542a 100644 --- a/orchagent/pfcactionhandler.h +++ b/orchagent/pfcactionhandler.h @@ -46,6 +46,8 @@ class PfcWdActionHandler return m_queueId; } + virtual bool isValid(void) const { return true; } + static void initWdCounters(shared_ptr countersTable, const string &queueIdStr); void initCounters(void); void commitCounters(bool periodic = false); @@ -103,19 +105,24 @@ class PfcWdAclHandler: public PfcWdLossyHandler // class shared cleanup static void clear(); + + bool isValid(void) const override { return !m_rolledBack; } private: // class shared dict: ACL table name -> ACL table static std::map m_aclTables; bool shared_egress_acl_table = false; + bool m_rolledBack = false; + string m_strIngressTable; string m_strEgressTable; string m_strRule; string m_strEgressRule; - void createPfcAclTable(sai_object_id_t port, string strTable, bool ingress); - void createPfcAclRule(shared_ptr rule, uint8_t queueId, string strTable, sai_object_id_t port); + bool createPfcAclTable(sai_object_id_t port, string strTable, bool ingress); + bool 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); + void removeIngressBinding(sai_object_id_t port); }; class PfcWdDlrHandler: public PfcWdLossyHandler diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index 8aefd244f3..123436dd56 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -18,6 +18,7 @@ #define PFC_STAT_HISTORY "pfc_stat_history" #define BIG_RED_SWITCH_FIELD "BIG_RED_SWITCH" #define PFC_WD_IN_STORM "storm" +#define PFC_WD_SW_STATE_TABLE "PFC_WD_SW_STATE_TABLE" #define PFC_WD_DETECTION_TIME_MAX (5 * 1000) #define PFC_WD_DETECTION_TIME_MIN 100 @@ -538,6 +539,13 @@ void PfcWdSwOrch::enableBigRedSwitchMode() } } +template +void PfcWdSwOrch::setSwWdState(const string& portAlias, uint8_t queueIdx, const char* status) +{ + vector fvs = { { "status", status } }; + m_pfcWdSwStateTable->set(portAlias + ":" + to_string(queueIdx), fvs); +} + template bool PfcWdSwOrch::registerInWdDb(const Port& port, uint32_t detectionTime, uint32_t restorationTime, PfcWdAction action, string pfcStatHistory) @@ -614,6 +622,8 @@ bool PfcWdSwOrch::registerInWdDb(const Port& port, PfcWdActionHandler::initWdCounters( this->getCountersTable(), sai_serialize_object_id(queueId)); + + setSwWdState(port.m_alias, i, "configured"); } // We do NOT need to create ACL table group here. It will be @@ -688,6 +698,8 @@ void PfcWdSwOrch::unregisterFromWdDb(const Port& po + m_applTable->getTableNameSeparator() + port.m_alias; m_applDb->hdel(instormKey, to_string(i)); + + m_pfcWdSwStateTable->del(port.m_alias + ":" + to_string(i)); } } @@ -706,7 +718,9 @@ PfcWdSwOrch::PfcWdSwOrch( c_queueAttrIds(queueAttrIds), m_pollInterval(pollInterval), m_applDb(make_shared("APPL_DB", 0)), - m_applTable(make_shared
(m_applDb.get(), APP_PFC_WD_TABLE_NAME "_INSTORM")) + m_applTable(make_shared
(m_applDb.get(), APP_PFC_WD_TABLE_NAME "_INSTORM")), + m_stateDb(make_shared("STATE_DB", 0)), + m_pfcWdSwStateTable(make_shared
(m_stateDb.get(), PFC_WD_SW_STATE_TABLE)) { SWSS_LOG_ENTER(); @@ -998,6 +1012,10 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string SWSS_LOG_NOTICE("Receive notification, %s", event.c_str()); + // Contain handler-construction failures (e.g. ACL table create on a full + // egress PMF) so no PFC event path aborts orchagent. + try + { if (m_bigRedSwitchFlag) { SWSS_LOG_NOTICE("Big_RED_SWITCH mode is on, ignore syncd pfc watchdog notification"); @@ -1016,9 +1034,20 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string entry->second.index, this->getCountersTable()); entry->second.handler->initCounters(); - // Log storm event to APPL_DB for warm-reboot purpose - string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; - m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + if (entry->second.handler->isValid()) + { + // Log storm event to APPL_DB for warm-reboot purpose + string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; + m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + setSwWdState(entry->second.portAlias, entry->second.index, "configured"); + } + else + { + // No INSTORM: a storm we can't mitigate must not replay on warm reboot. + SWSS_LOG_WARN("PFC storm on port %s queue %d detected but drop action could not be installed (ACL create failed); queue is NOT being mitigated", + entry->second.portAlias.c_str(), entry->second.index); + setSwWdState(entry->second.portAlias, entry->second.index, "failed"); + } } } else if (entry->second.action == PfcWdAction::PFC_WD_ACTION_DROP) @@ -1033,9 +1062,20 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string entry->second.index, this->getCountersTable()); entry->second.handler->initCounters(); - // Log storm event to APPL_DB for warm-reboot purpose - string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; - m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + if (entry->second.handler->isValid()) + { + // Log storm event to APPL_DB for warm-reboot purpose + string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; + m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + setSwWdState(entry->second.portAlias, entry->second.index, "configured"); + } + else + { + // No INSTORM: a storm we can't mitigate must not replay on warm reboot. + SWSS_LOG_WARN("PFC storm on port %s queue %d detected but drop action could not be installed (ACL create failed); queue is NOT being mitigated", + entry->second.portAlias.c_str(), entry->second.index); + setSwWdState(entry->second.portAlias, entry->second.index, "failed"); + } } } else if (entry->second.action == PfcWdAction::PFC_WD_ACTION_FORWARD) @@ -1050,9 +1090,20 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string entry->second.index, this->getCountersTable()); entry->second.handler->initCounters(); - // Log storm event to APPL_DB for warm-reboot purpose - string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; - m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + if (entry->second.handler->isValid()) + { + // Log storm event to APPL_DB for warm-reboot purpose + string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; + m_applDb->hset(key, to_string(entry->second.index), PFC_WD_IN_STORM); + setSwWdState(entry->second.portAlias, entry->second.index, "configured"); + } + else + { + // No INSTORM: a storm we can't mitigate must not replay on warm reboot. + SWSS_LOG_WARN("PFC storm on port %s queue %d detected but drop action could not be installed (ACL create failed); queue is NOT being mitigated", + entry->second.portAlias.c_str(), entry->second.index); + setSwWdState(entry->second.portAlias, entry->second.index, "failed"); + } } } else @@ -1077,6 +1128,7 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string // Remove storm status in APPL_DB for warm-reboot purpose string key = m_applTable->getTableName() + m_applTable->getTableNameSeparator() + entry->second.portAlias; m_applDb->hdel(key, to_string(entry->second.index)); + setSwWdState(entry->second.portAlias, entry->second.index, "configured"); } } else @@ -1084,6 +1136,12 @@ bool PfcWdSwOrch::startWdActionOnQueue(const string SWSS_LOG_ERROR("Received unknown event from plugin, %s", event.c_str()); return false; } + } + catch (const std::exception &e) + { + SWSS_LOG_ERROR("PFC watchdog %s action failed on queue 0x%" PRIx64 ": %s", event.c_str(), queueId, e.what()); + return false; + } return true; } diff --git a/orchagent/pfcwdorch.h b/orchagent/pfcwdorch.h index 69ed9ecfcc..0afa438530 100644 --- a/orchagent/pfcwdorch.h +++ b/orchagent/pfcwdorch.h @@ -123,6 +123,7 @@ class PfcWdSwOrch: public PfcWdOrch bool registerInWdDb(const Port& port, uint32_t detectionTime, uint32_t restorationTime, PfcWdAction action, string pfcStatHistory); void unregisterFromWdDb(const Port& port); + void setSwWdState(const string& portAlias, uint8_t queueIdx, const char* status); void doTask(swss::NotificationConsumer &wdNotification); unordered_set filterPfcCounters(const unordered_set &counters, set& losslessTc); @@ -147,6 +148,8 @@ class PfcWdSwOrch: public PfcWdOrch shared_ptr m_applDb = nullptr; // Track queues in storm shared_ptr
m_applTable = nullptr; + shared_ptr m_stateDb = nullptr; + shared_ptr
m_pfcWdSwStateTable = nullptr; }; #endif