Skip to content

Commit 067928d

Browse files
author
Shuotian Cheng
authored
[aclorch]: Use the Observer class to listen to port changes (sonic-net#689)
The previous implementation was using state database to check LAG creation/removal. However, state database only reflects the changes in the kernel. This pull request changes the logic so that LAG changes will directly notify the AclOrch to update pending ports per table. This simplifies the logic in AclOrch and make it reliable on port change events. Right now, only LAG creation is support, which aligns the legacy codes. An extra pull request will be provided to ensure both LAG creation/removal will be supported. The unit test is provided to cover the two scenarios: 1. creating ACL table before creating LAG 2. creating LAG before creating ACL table Signed-off-by: Shu0T1an ChenG <[email protected]>
1 parent d7d887a commit 067928d

File tree

6 files changed

+257
-148
lines changed

6 files changed

+257
-148
lines changed

‎orchagent/aclorch.cpp

+97-141
Original file line numberDiff line numberDiff line change
@@ -1054,7 +1054,7 @@ bool AclTable::validate()
10541054
// Control plane ACLs are handled by a separate process
10551055
if (type == ACL_TABLE_UNKNOWN || type == ACL_TABLE_CTRLPLANE) return false;
10561056
if (stage == ACL_STAGE_UNKNOWN) return false;
1057-
if (portSet.empty()) return false;
1057+
if (portSet.empty() && pendingPortSet.empty()) return false;
10581058

10591059
return true;
10601060
}
@@ -1177,6 +1177,50 @@ bool AclTable::create()
11771177
return status == SAI_STATUS_SUCCESS;
11781178
}
11791179

1180+
void AclTable::update(SubjectType type, void *cntx)
1181+
{
1182+
SWSS_LOG_ENTER();
1183+
1184+
// Only interested in port change
1185+
if (type != SUBJECT_TYPE_PORT_CHANGE)
1186+
{
1187+
return;
1188+
}
1189+
1190+
PortUpdate *update = static_cast<PortUpdate *>(cntx);
1191+
1192+
Port &port = update->port;
1193+
if (update->add)
1194+
{
1195+
if (pendingPortSet.find(port.m_alias) != pendingPortSet.end())
1196+
{
1197+
sai_object_id_t bind_port_id;
1198+
if (gPortsOrch->getAclBindPortId(port.m_alias, bind_port_id))
1199+
{
1200+
link(bind_port_id);
1201+
bind(bind_port_id);
1202+
1203+
pendingPortSet.erase(port.m_alias);
1204+
portSet.emplace(port.m_alias);
1205+
1206+
SWSS_LOG_NOTICE("Bound port %s to ACL table %s",
1207+
port.m_alias.c_str(), id.c_str());
1208+
}
1209+
else
1210+
{
1211+
SWSS_LOG_ERROR("Failed to get port %s bind port ID",
1212+
port.m_alias.c_str());
1213+
return;
1214+
}
1215+
}
1216+
}
1217+
else
1218+
{
1219+
// TODO: deal with port removal scenario
1220+
}
1221+
}
1222+
1223+
// TODO: make bind/unbind symmetric
11801224
bool AclTable::bind(sai_object_id_t portOid)
11811225
{
11821226
SWSS_LOG_ENTER();
@@ -1737,7 +1781,9 @@ void AclOrch::init(vector<TableConnector>& connectors, PortsOrch *portOrch, Mirr
17371781
throw "AclOrch initialization failure";
17381782
}
17391783

1784+
// Attach observers
17401785
m_mirrorOrch->attach(this);
1786+
gPortsOrch->attach(this);
17411787

17421788
// Should be initialized last to guaranty that object is
17431789
// initialized before thread start.
@@ -1798,18 +1844,30 @@ void AclOrch::update(SubjectType type, void *cntx)
17981844
{
17991845
SWSS_LOG_ENTER();
18001846

1801-
if (type != SUBJECT_TYPE_MIRROR_SESSION_CHANGE && type != SUBJECT_TYPE_INT_SESSION_CHANGE)
1847+
if (type != SUBJECT_TYPE_MIRROR_SESSION_CHANGE &&
1848+
type != SUBJECT_TYPE_INT_SESSION_CHANGE &&
1849+
type != SUBJECT_TYPE_PORT_CHANGE)
18021850
{
1851+
SWSS_LOG_WARN("Received unwanted change update %d", type);
18031852
return;
18041853
}
18051854

18061855
unique_lock<mutex> lock(m_countersMutex);
18071856

1808-
for (const auto& table : m_AclTables)
1857+
// ACL table deals with port change
1858+
// ACL rule deals with mirror session change and int session change
1859+
for (auto& table : m_AclTables)
18091860
{
1810-
for (auto& rule : table.second.rules)
1861+
if (type == SUBJECT_TYPE_PORT_CHANGE)
18111862
{
1812-
rule.second->update(type, cntx);
1863+
table.second.update(type, cntx);
1864+
}
1865+
else
1866+
{
1867+
for (auto& rule : table.second.rules)
1868+
{
1869+
rule.second->update(type, cntx);
1870+
}
18131871
}
18141872
}
18151873
}
@@ -1835,11 +1893,6 @@ void AclOrch::doTask(Consumer &consumer)
18351893
unique_lock<mutex> lock(m_countersMutex);
18361894
doAclRuleTask(consumer);
18371895
}
1838-
else if (table_name == STATE_LAG_TABLE_NAME)
1839-
{
1840-
unique_lock<mutex> lock(m_countersMutex);
1841-
doAclTablePortUpdateTask(consumer);
1842-
}
18431896
else
18441897
{
18451898
SWSS_LOG_ERROR("Invalid table %s", table_name.c_str());
@@ -1857,20 +1910,22 @@ bool AclOrch::addAclTable(AclTable &newTable, string table_id)
18571910
/* If ACL table exists, remove the table first.*/
18581911
if (!removeAclTable(table_id))
18591912
{
1860-
SWSS_LOG_ERROR("Fail to remove the exsiting ACL table %s when try to add the new one.", table_id.c_str());
1913+
SWSS_LOG_ERROR("Failed to remove exsiting ACL table %s before adding the new one",
1914+
table_id.c_str());
18611915
return false;
18621916
}
18631917
}
18641918

18651919
if (createBindAclTable(newTable, table_oid))
18661920
{
18671921
m_AclTables[table_oid] = newTable;
1868-
SWSS_LOG_NOTICE("Successfully created ACL table %s, oid: %lX", newTable.description.c_str(), table_oid);
1922+
SWSS_LOG_NOTICE("Created ACL table %s oid:%lx",
1923+
newTable.id.c_str(), table_oid);
18691924
return true;
18701925
}
18711926
else
18721927
{
1873-
SWSS_LOG_ERROR("Failed to create table %s", table_id.c_str());
1928+
SWSS_LOG_ERROR("Failed to create ACL table %s", table_id.c_str());
18741929
return false;
18751930
}
18761931
}
@@ -1968,20 +2023,18 @@ void AclOrch::doAclTableTask(Consumer &consumer)
19682023
{
19692024
if (!processAclTableType(attr_value, newTable.type))
19702025
{
1971-
SWSS_LOG_ERROR("Failed to process table type for table %s", table_id.c_str());
2026+
SWSS_LOG_ERROR("Failed to process ACL table %s type",
2027+
table_id.c_str());
19722028
bAllAttributesOk = false;
19732029
break;
19742030
}
19752031
}
19762032
else if (attr_name == TABLE_PORTS)
19772033
{
1978-
bool suc = processPorts(newTable, attr_value, [&](sai_object_id_t portOid) {
1979-
newTable.link(portOid);
1980-
});
1981-
1982-
if (!suc)
2034+
if (!processAclTablePorts(attr_value, newTable))
19832035
{
1984-
SWSS_LOG_ERROR("Failed to process table ports for table %s", table_id.c_str());
2036+
SWSS_LOG_ERROR("Failed to process ACL table %s ports",
2037+
table_id.c_str());
19852038
bAllAttributesOk = false;
19862039
break;
19872040
}
@@ -1990,7 +2043,8 @@ void AclOrch::doAclTableTask(Consumer &consumer)
19902043
{
19912044
if (!processAclTableStage(attr_value, newTable.stage))
19922045
{
1993-
SWSS_LOG_ERROR("Failed to process table stage for table %s", table_id.c_str());
2046+
SWSS_LOG_ERROR("Failed to process ACL table %s stage",
2047+
table_id.c_str());
19942048
bAllAttributesOk = false;
19952049
break;
19962050
}
@@ -2013,7 +2067,8 @@ void AclOrch::doAclTableTask(Consumer &consumer)
20132067
else
20142068
{
20152069
it = consumer.m_toSync.erase(it);
2016-
SWSS_LOG_ERROR("Failed to create ACL table. Table configuration is invalid");
2070+
SWSS_LOG_ERROR("Failed to create ACL table %s, invalid configuration",
2071+
table_id.c_str());
20172072
}
20182073
}
20192074
else if (op == DEL_COMMAND)
@@ -2120,140 +2175,41 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
21202175
}
21212176
}
21222177

2123-
void AclOrch::doAclTablePortUpdateTask(Consumer &consumer)
2178+
bool AclOrch::processAclTablePorts(string portList, AclTable &aclTable)
21242179
{
21252180
SWSS_LOG_ENTER();
21262181

2127-
auto it = consumer.m_toSync.begin();
2128-
while (it != consumer.m_toSync.end())
2129-
{
2130-
KeyOpFieldsValuesTuple t = it->second;
2131-
string key = kfvKey(t);
2132-
size_t found = key.find(consumer.getConsumerTable()->getTableNameSeparator().c_str());
2133-
string port_alias = key.substr(0, found);
2134-
string op = kfvOp(t);
2135-
2136-
SWSS_LOG_INFO("doAclTablePortUpdateTask: OP: %s, port_alias: %s", op.c_str(), port_alias.c_str());
2137-
2138-
if (op == SET_COMMAND)
2139-
{
2140-
for (auto itmap : m_AclTables)
2141-
{
2142-
auto table = itmap.second;
2143-
if (table.pendingPortSet.find(port_alias) != table.pendingPortSet.end())
2144-
{
2145-
SWSS_LOG_INFO("found the port: %s in ACL table: %s pending port list, bind it to ACL table.", port_alias.c_str(), table.description.c_str());
2146-
2147-
bool suc = processPendingPort(table, port_alias, [&](sai_object_id_t portOid) {
2148-
table.link(portOid);
2149-
});
2182+
auto port_list = tokenize(portList, ',');
2183+
set<string> ports(port_list.begin(), port_list.end());
21502184

2151-
if (!suc)
2152-
{
2153-
SWSS_LOG_ERROR("Failed to bind the ACL table: %s to port: %s", table.description.c_str(), port_alias.c_str());
2154-
}
2155-
else
2156-
{
2157-
table.pendingPortSet.erase(port_alias);
2158-
SWSS_LOG_DEBUG("port: %s bound to ACL table table: %s, remove it from pending list", port_alias.c_str(), table.description.c_str());
2159-
}
2160-
}
2161-
}
2162-
}
2163-
else if (op == DEL_COMMAND)
2164-
{
2165-
for (auto itmap : m_AclTables)
2166-
{
2167-
auto table = itmap.second;
2168-
if (table.portSet.find(port_alias) != table.portSet.end())
2169-
{
2170-
/*TODO: update the ACL table after port/lag deleted*/
2171-
table.pendingPortSet.emplace(port_alias);
2172-
SWSS_LOG_INFO("Add deleted port: %s to the pending list of ACL table: %s", port_alias.c_str(), table.description.c_str());
2173-
}
2174-
}
2175-
}
2176-
else
2177-
{
2178-
SWSS_LOG_ERROR("Unknown operation type %s", op.c_str());
2179-
}
2180-
it = consumer.m_toSync.erase(it);
2181-
}
2182-
}
2183-
2184-
bool AclOrch::processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter)
2185-
{
2186-
SWSS_LOG_ENTER();
2187-
2188-
vector<string> strList;
2189-
2190-
SWSS_LOG_DEBUG("Processing ACL table port list %s", portsList.c_str());
2191-
2192-
split(portsList, strList, ',');
2193-
2194-
set<string> strSet(strList.begin(), strList.end());
2195-
aclTable.portSet = strSet;
2196-
2197-
if (strList.size() != strSet.size())
2198-
{
2199-
SWSS_LOG_ERROR("Failed to process port list. Duplicate port entry");
2200-
return false;
2201-
}
2202-
2203-
if (strList.empty())
2185+
// TODO: Support adding ports afterwards
2186+
if (ports.empty())
22042187
{
2205-
SWSS_LOG_ERROR("Failed to process port list. List is empty");
2188+
SWSS_LOG_ERROR("Failed to process empty port list");
22062189
return false;
22072190
}
22082191

2209-
for (const auto& alias : strList)
2192+
for (auto alias : ports)
22102193
{
2211-
sai_object_id_t port_id;
22122194
Port port;
22132195
if (!gPortsOrch->getPort(alias, port))
22142196
{
2215-
SWSS_LOG_INFO("Port %s not configured yet, add it to ACL table %s pending list", alias.c_str(), aclTable.description.c_str());
2197+
SWSS_LOG_INFO("Add unready port %s to pending list for ACL table %s",
2198+
alias.c_str(), aclTable.id.c_str());
22162199
aclTable.pendingPortSet.emplace(alias);
22172200
continue;
22182201
}
22192202

2220-
if (gPortsOrch->getAclBindPortId(alias, port_id))
2203+
sai_object_id_t bind_port_id;
2204+
if (!gPortsOrch->getAclBindPortId(alias, bind_port_id))
22212205
{
2222-
inserter(port_id);
2223-
}
2224-
else
2225-
{
2226-
return false;
2206+
SWSS_LOG_ERROR("Failed to get port %s bind port ID for ACL table %s",
2207+
alias.c_str(), aclTable.id.c_str());
2208+
continue;
22272209
}
2228-
}
2229-
2230-
return true;
2231-
}
2232-
2233-
bool AclOrch::processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter)
2234-
{
2235-
SWSS_LOG_ENTER();
2236-
2237-
SWSS_LOG_DEBUG("Processing ACL table port %s", portAlias.c_str());
22382210

2239-
sai_object_id_t port_id;
2240-
2241-
Port port;
2242-
if (!gPortsOrch->getPort(portAlias, port))
2243-
{
2244-
SWSS_LOG_INFO("Port %s not configured yet, add it to ACL table %s pending list", portAlias.c_str(), aclTable.description.c_str());
2245-
aclTable.pendingPortSet.insert(portAlias);
2246-
return true;
2247-
}
2248-
2249-
if (gPortsOrch->getAclBindPortId(portAlias, port_id))
2250-
{
2251-
inserter(port_id);
2252-
aclTable.bind(port_id);
2253-
}
2254-
else
2255-
{
2256-
return false;
2211+
aclTable.link(bind_port_id);
2212+
aclTable.portSet.emplace(alias);
22572213
}
22582214

22592215
return true;
@@ -2292,8 +2248,6 @@ bool AclOrch::processAclTableStage(string stage, acl_stage_type_t &acl_stage)
22922248
return true;
22932249
}
22942250

2295-
2296-
22972251
sai_object_id_t AclOrch::getTableById(string table_id)
22982252
{
22992253
SWSS_LOG_ENTER();
@@ -2320,7 +2274,8 @@ bool AclOrch::createBindAclTable(AclTable &aclTable, sai_object_id_t &table_oid)
23202274
sai_status_t status = bindAclTable(table_oid, aclTable);
23212275
if (status != SAI_STATUS_SUCCESS)
23222276
{
2323-
SWSS_LOG_ERROR("Failed to bind table %s to ports", aclTable.description.c_str());
2277+
SWSS_LOG_ERROR("Failed to bind table %s to ports",
2278+
aclTable.id.c_str());
23242279
return false;
23252280
}
23262281
return true;
@@ -2333,7 +2288,8 @@ sai_status_t AclOrch::deleteUnbindAclTable(sai_object_id_t table_oid)
23332288

23342289
if ((status = bindAclTable(table_oid, m_AclTables[table_oid], false)) != SAI_STATUS_SUCCESS)
23352290
{
2336-
SWSS_LOG_ERROR("Failed to unbind table %s", m_AclTables[table_oid].description.c_str());
2291+
SWSS_LOG_ERROR("Failed to unbind table %s",
2292+
m_AclTables[table_oid].id.c_str());
23372293
return status;
23382294
}
23392295

0 commit comments

Comments
 (0)