Skip to content

[pfcwd] Don't abort orchagent when the egress ACL table create fails #4743

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.

Sign up for GitHub

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

Open
rajath-nexthop wants to merge 2 commits into
base: master
Choose a base branch
from
+205 −43
Open

[pfcwd] Don't abort orchagent when the egress ACL table create fails #4743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44e6af9
[pfcwd] Don't abort orchagent when the egress ACL table create fails
rajath-nexthop Jul 10, 2026
d72be06
[pfcwd] Address review: failure logs with SAI status, egress table ro…
rajath-nexthop Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 125 additions & 31 deletions orchagent/pfcactionhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,19 +324,40 @@ 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<AclRulePacket> newRule = make_shared<AclRulePacket>(gAclOrch, m_strRule, m_strIngressTable);
createPfcAclRule(newRule, queueId, m_strIngressTable, port);
if (createPfcAclTable(port, m_strIngressTable, true))
{
shared_ptr<AclRulePacket> newRule = make_shared<AclRulePacket>(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
{

@kperumalbfn kperumalbfn Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add error logs for Create ACL table failures with the error code. Will be good to get the resource exhaustion and other failures in the log

@rajath-nexthop rajath-nexthop Jul 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added relevant info (the rule itself and sai status for the rule)

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
{
AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule);
if (rule == nullptr)
{
shared_ptr<AclRulePacket> newRule = make_shared<AclRulePacket>(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);
}
Expand Down Expand Up @@ -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<AclRulePacket> newRule = make_shared<AclRulePacket>(gAclOrch, m_strEgressRule, m_strEgressTable);
createPfcAclRule(newRule, queueId, m_strEgressTable, port);
if (createPfcAclTable(port, m_strEgressTable, false))
{
shared_ptr<AclRulePacket> newRule = make_shared<AclRulePacket>(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
{
Expand All @@ -376,7 +416,14 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue,
if (rule == nullptr)
{
shared_ptr<AclRulePacket> newRule = make_shared<AclRulePacket>(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;
}
}
}
}
Expand All @@ -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<AclRulePacket> newRule = make_shared<AclRulePacket>(gAclOrch, m_strRule, m_strEgressTable);
createPfcAclRule(newRule, queueId, m_strEgressTable, port);
if (createPfcAclTable(port, m_strEgressTable, false))
{
shared_ptr<AclRulePacket> newRule = make_shared<AclRulePacket>(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);

@kperumalbfn kperumalbfn Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove egress binding when egress ACL rule create fails?

@kperumalbfn kperumalbfn Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you also check for any ACL CRMs

@rajath-nexthop rajath-nexthop Jul 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added code to remove the egress binding. ACL CRMs look okay - CRM_ACL_TABLE is incremented only after create() succeeds and is decremented when removeAclTable is invoked - so there should be no inconsistencies from there.

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
{
Expand All @@ -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<sai_object_id_t> 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);
}
}
}

Expand All @@ -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();

Expand All @@ -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.
Expand All @@ -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<AclRulePacket> rule, uint8_t queueId, string strTable, sai_object_id_t portOid)
bool PfcWdAclHandler::createPfcAclRule(shared_ptr<AclRulePacket> rule, uint8_t queueId, string strTable, sai_object_id_t portOid)
{
SWSS_LOG_ENTER();

Expand Down Expand Up @@ -522,9 +616,9 @@ void PfcWdAclHandler::createPfcAclRule(shared_ptr<AclRulePacket> 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);
}
Expand All @@ -533,7 +627,7 @@ void PfcWdAclHandler::createPfcAclRule(shared_ptr<AclRulePacket> 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<std::string, AclTable> PfcWdAclHandler::m_aclTables;
Expand Down
11 changes: 9 additions & 2 deletions orchagent/pfcactionhandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ class PfcWdActionHandler
return m_queueId;
}

virtual bool isValid(void) const { return true; }

static void initWdCounters(shared_ptr<Table> countersTable, const string &queueIdStr);
void initCounters(void);
void commitCounters(bool periodic = false);
Expand Down Expand Up @@ -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<std::string, AclTable> 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<AclRulePacket> 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<AclRulePacket> rule, uint8_t queueId, string strTable, sai_object_id_t port);
void updatePfcAclRule(shared_ptr<AclRule> rule, uint8_t queueId, string strTable, vector<sai_object_id_t> port);
void removeIngressBinding(sai_object_id_t port);
};

class PfcWdDlrHandler: public PfcWdLossyHandler
Expand Down
Loading
Loading