[AclOrch] aclOrch enhancement to handle LAG port not configured case (sonic-net#494)

keboliu · qiluo-msft · commit 0f433515d1dc · 2018-05-27T18:46:18.000Z
* enhance acl orch to handle the LAG port not configured case
* rename variable, function; redunce redundant code; fix memory issue
* revise according to comments
* one more blank line
* add new VS test cases for acl on LAG
* update with PR comments fix
* rename getValidPortId and move it to portOrch
* fix indent
* fix test case comments
diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
@@ -936,7 +936,7 @@ bool AclTable::validate()
 {
     // Control plane ACLs are handled by a separate process
     if (type == ACL_TABLE_UNKNOWN || type == ACL_TABLE_CTRLPLANE) return false;
-    if (ports.empty()) return false;
+    if (portSet.empty()) return false;
     return true;
 }
 
@@ -1365,8 +1365,8 @@ bool AclRange::remove()
     return true;
 }
 
-AclOrch::AclOrch(DBConnector *db, vector<string> tableNames, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch) :
-        Orch(db, tableNames),
+AclOrch::AclOrch(vector<TableConnector>& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch) :
+        Orch(connectors),
         m_mirrorOrch(mirrorOrch),
         m_neighOrch(neighOrch),
         m_routeOrch(routeOrch)
@@ -1449,6 +1449,11 @@ void AclOrch::doTask(Consumer &consumer)
         unique_lock<mutex> lock(m_countersMutex);
         doAclRuleTask(consumer);
     }
+    else if (table_name == STATE_LAG_TABLE_NAME)
+    {
+        unique_lock<mutex> lock(m_countersMutex);
+        doAclTablePortUpdateTask(consumer);
+    }
     else
     {
         SWSS_LOG_ERROR("Invalid table %s", table_name.c_str());
@@ -1549,7 +1554,7 @@ void AclOrch::doAclTableTask(Consumer &consumer)
     {
         KeyOpFieldsValuesTuple t = it->second;
         string key = kfvKey(t);
-        size_t found = key.find('|');
+        size_t found = key.find(consumer.getConsumerTable()->getTableNameSeparator().c_str());
         string table_id = key.substr(0, found);
         string op = kfvOp(t);
 
@@ -1584,7 +1589,7 @@ void AclOrch::doAclTableTask(Consumer &consumer)
                 }
                 else if (attr_name == TABLE_PORTS)
                 {
-                    bool suc = processPorts(attr_value, [&](sai_object_id_t portOid) {
+                    bool suc = processPorts(newTable, attr_value, [&](sai_object_id_t portOid) {
                         newTable.link(portOid);
                     });
 
@@ -1649,7 +1654,7 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
     {
         KeyOpFieldsValuesTuple t = it->second;
         string key = kfvKey(t);
-        size_t found = key.find('|');
+        size_t found = key.find(consumer.getConsumerTable()->getTableNameSeparator().c_str());
         string table_id = key.substr(0, found);
         string rule_id = key.substr(found + 1);
         string op = kfvOp(t);
@@ -1729,17 +1734,79 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
     }
 }
 
-bool AclOrch::processPorts(string portsList, std::function<void (sai_object_id_t)> inserter)
+void AclOrch::doAclTablePortUpdateTask(Consumer &consumer)
+{
+    SWSS_LOG_ENTER();
+
+    auto it = consumer.m_toSync.begin();
+    while (it != consumer.m_toSync.end())
+    {
+        KeyOpFieldsValuesTuple t = it->second;
+        string key = kfvKey(t);
+        size_t found = key.find(consumer.getConsumerTable()->getTableNameSeparator().c_str());
+        string port_alias = key.substr(0, found);
+        string op = kfvOp(t);
+
+        SWSS_LOG_INFO("doAclTablePortUpdateTask: OP: %s, port_alias: %s", op.c_str(), port_alias.c_str());
+
+        if (op == SET_COMMAND)
+        {
+            for (auto itmap : m_AclTables)
+            {
+                auto table = itmap.second;
+                if (table.pendingPortSet.find(port_alias) != table.pendingPortSet.end())
+                {
+                    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());
+
+                    bool suc = processPendingPort(table, port_alias, [&](sai_object_id_t portOid) {
+                        table.link(portOid);
+                    });
+
+                    if (!suc)
+                    {
+                        SWSS_LOG_ERROR("Failed to bind the ACL table: %s to port: %s", table.description.c_str(), port_alias.c_str());
+                    }
+                    else
+                    {
+                        table.pendingPortSet.erase(port_alias);
+                        SWSS_LOG_DEBUG("port: %s bound to ACL table table: %s, remove it from pending list", port_alias.c_str(), table.description.c_str());
+                    }
+                }
+            }
+        }
+        else if (op == DEL_COMMAND)
+        {
+            for (auto itmap : m_AclTables)
+            {
+                auto table = itmap.second;
+                if (table.portSet.find(port_alias) != table.portSet.end())
+                {
+                    /*TODO: update the ACL table after port/lag deleted*/
+                    table.pendingPortSet.emplace(port_alias);
+                    SWSS_LOG_INFO("Add deleted port: %s to the pending list of ACL table: %s", port_alias.c_str(), table.description.c_str());
+                }
+            }
+        }
+        else
+        {
+            SWSS_LOG_ERROR("Unknown operation type %s", op.c_str());
+        }
+        it = consumer.m_toSync.erase(it);
+    }
+}
+
+bool AclOrch::processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter)
 {
     SWSS_LOG_ENTER();
 
     vector<string> strList;
 
-    SWSS_LOG_INFO("Processing ACL table port list %s", portsList.c_str());
+    SWSS_LOG_DEBUG("Processing ACL table port list %s", portsList.c_str());
 
     split(portsList, strList, ',');
 
     set<string> strSet(strList.begin(), strList.end());
+    aclTable.portSet = strSet;
 
     if (strList.size() != strSet.size())
     {
@@ -1755,33 +1822,52 @@ bool AclOrch::processPorts(string portsList, std::function<void (sai_object_id_t
 
     for (const auto& alias : strList)
     {
+        sai_object_id_t port_id;
         Port port;
         if (!gPortsOrch->getPort(alias, port))
         {
-            SWSS_LOG_ERROR("Failed to process port. Port %s doesn't exist", alias.c_str());
-            return false;
+            SWSS_LOG_INFO("Port %s not configured yet, add it to ACL table %s pending list", alias.c_str(), aclTable.description.c_str());
+            aclTable.pendingPortSet.emplace(alias);
+            continue;
         }
 
-        switch (port.m_type)
+        if (gPortsOrch->getAclBindPortId(alias, port_id))
         {
-        case Port::PHY:
-            if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
-            {
-                SWSS_LOG_ERROR("Failed to process port. Bind table to LAG member %s is not allowed", alias.c_str());
-                return false;
-            }
-            inserter(port.m_port_id);
-            break;
-        case Port::LAG:
-            inserter(port.m_lag_id);
-            break;
-        case Port::VLAN:
-            inserter(port.m_vlan_info.vlan_oid);
-            break;
-        default:
-          SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type);
-          return false;
-      }
+            inserter(port_id);
+        }
+        else
+        {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+bool AclOrch::processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter)
+{
+    SWSS_LOG_ENTER();
+
+    SWSS_LOG_DEBUG("Processing ACL table port %s", portAlias.c_str());
+
+    sai_object_id_t port_id;
+
+    Port port;
+    if (!gPortsOrch->getPort(portAlias, port))
+    {
+        SWSS_LOG_INFO("Port %s not configured yet, add it to ACL table %s pending list", portAlias.c_str(), aclTable.description.c_str());
+        aclTable.pendingPortSet.insert(portAlias);
+        return true;
+    }
+
+    if (gPortsOrch->getAclBindPortId(portAlias, port_id))
+    {
+        inserter(port_id);
+        aclTable.bind(port_id);
+    }
+    else
+    {
+        return false;
     }
 
     return true;
@@ -1898,18 +1984,14 @@ sai_status_t AclOrch::bindAclTable(sai_object_id_t table_oid, AclTable &aclTable
     sai_status_t status = SAI_STATUS_SUCCESS;
 
     SWSS_LOG_INFO("%s table %s to ports", bind ? "Bind" : "Unbind", aclTable.id.c_str());
-
+    
     if (aclTable.ports.empty())
     {
         if (bind)
         {
-            SWSS_LOG_ERROR("Port list is not configured for %s table", aclTable.id.c_str());
-            return SAI_STATUS_FAILURE;
-        }
-        else
-        {
-            return SAI_STATUS_SUCCESS;
+            SWSS_LOG_WARN("Binding port list is empty for %s table", aclTable.id.c_str());
         }
+        return SAI_STATUS_SUCCESS;
     }
 
     if (bind)
diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h
@@ -248,6 +248,10 @@ class AclTable {
     std::map<sai_object_id_t, sai_object_id_t> ports;
     // Map rule name to rule data
     map<string, shared_ptr<AclRule>> rules;
+    // Set to store the ACL table port alias
+    set<string> portSet;
+    // Set to store the not cofigured ACL table port alias
+    set<string> pendingPortSet;
 
     AclTable()
         : type(ACL_TABLE_UNKNOWN)
@@ -294,7 +298,7 @@ inline void split(string str, Iterable& out, char delim = ' ')
 class AclOrch : public Orch, public Observer
 {
 public:
-    AclOrch(DBConnector *db, vector<string> tableNames, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch);
+    AclOrch(vector<TableConnector>& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch);
     ~AclOrch();
     void update(SubjectType, void *);
 
@@ -319,6 +323,7 @@ class AclOrch : public Orch, public Observer
     void doTask(Consumer &consumer);
     void doAclTableTask(Consumer &consumer);
     void doAclRuleTask(Consumer &consumer);
+    void doAclTablePortUpdateTask(Consumer &consumer);
     void doTask(SelectableTimer &timer);
 
     static void collectCountersThread(AclOrch *pAclOrch);
@@ -329,7 +334,8 @@ class AclOrch : public Orch, public Observer
 
     bool processAclTableType(string type, acl_table_type_t &table_type);
     bool processAclTableStage(string stage, acl_stage_type_t &acl_stage);
-    bool processPorts(string portsList, std::function<void (sai_object_id_t)> inserter);
+    bool processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter);
+    bool processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter);
     bool validateAclTable(AclTable &aclTable);
 
     //vector <AclTable> m_AclTables;
diff --git a/orchagent/main.cpp b/orchagent/main.cpp
@@ -251,13 +251,15 @@ int main(int argc, char **argv)
     SWSS_LOG_NOTICE("Created underlay router interface ID %lx", gUnderlayIfId);
 
     /* Initialize orchestration components */
-    DBConnector *appl_db = new DBConnector(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
-    DBConnector *config_db = new DBConnector(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
+    DBConnector appl_db(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
+    DBConnector config_db(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
+    DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
 
-    OrchDaemon *orchDaemon = new OrchDaemon(appl_db, config_db);
+    OrchDaemon *orchDaemon = new OrchDaemon(&appl_db, &config_db, &state_db);
     if (!orchDaemon->init())
     {
         SWSS_LOG_ERROR("Failed to initialize orchstration daemon");
+        delete orchDaemon;
         exit(EXIT_FAILURE);
     }
 
@@ -272,6 +274,7 @@ int main(int argc, char **argv)
         if (status != SAI_STATUS_SUCCESS)
         {
             SWSS_LOG_ERROR("Failed to notify syncd APPLY_VIEW %d", status);
+            delete orchDaemon;
             exit(EXIT_FAILURE);
         }
 
@@ -286,5 +289,6 @@ int main(int argc, char **argv)
         SWSS_LOG_ERROR("Failed due to exception: %s", e.what());
     }
 
+    delete orchDaemon;
     return 0;
 }
diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp
@@ -358,7 +358,7 @@ bool Orch::parseIndexRange(const string &input, sai_uint32_t &range_low, sai_uin
 
 void Orch::addConsumer(DBConnector *db, string tableName)
 {
-    if (db->getDB() == CONFIG_DB)
+    if (db->getDB() == CONFIG_DB || db->getDB() == STATE_DB)
     {
         addExecutor(tableName, new Consumer(new SubscriberStateTable(db, tableName), this));
     }
diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp
@@ -27,9 +27,10 @@ RouteOrch *gRouteOrch;
 AclOrch *gAclOrch;
 CrmOrch *gCrmOrch;
 
-OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb) :
+OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb, DBConnector *stateDb) :
         m_applDb(applDb),
-        m_configDb(configDb)
+        m_configDb(configDb),
+        m_stateDb(stateDb)
 {
     SWSS_LOG_ENTER();
 }
@@ -39,9 +40,6 @@ OrchDaemon::~OrchDaemon()
     SWSS_LOG_ENTER();
     for (Orch *o : m_orchList)
         delete(o);
-
-    delete(m_configDb);
-    delete(m_applDb);
 }
 
 bool OrchDaemon::init()
@@ -97,11 +95,17 @@ bool OrchDaemon::init()
     MirrorOrch *mirror_orch = new MirrorOrch(appDbMirrorSession, confDbMirrorSession, gPortsOrch, gRouteOrch, gNeighOrch, gFdbOrch);
     VRFOrch *vrf_orch = new VRFOrch(m_configDb, CFG_VRF_TABLE_NAME);
 
-    vector<string> acl_tables = {
-        CFG_ACL_TABLE_NAME,
-        CFG_ACL_RULE_TABLE_NAME
+    TableConnector confDbAclTable(m_configDb, CFG_ACL_TABLE_NAME);
+    TableConnector confDbAclRuleTable(m_configDb, CFG_ACL_RULE_TABLE_NAME);
+    TableConnector stateDbLagTable(m_stateDb, STATE_LAG_TABLE_NAME);
+
+    vector<TableConnector> acl_table_connectors = {
+        confDbAclTable,
+        confDbAclRuleTable,
+        stateDbLagTable
     };
-    gAclOrch = new AclOrch(m_configDb, acl_tables, gPortsOrch, mirror_orch, gNeighOrch, gRouteOrch);
+
+    gAclOrch = new AclOrch(acl_table_connectors, gPortsOrch, mirror_orch, gNeighOrch, gRouteOrch);
 
     m_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, buffer_orch, mirror_orch, gAclOrch, gFdbOrch, vrf_orch };
     m_select = new Select();
diff --git a/orchagent/orchdaemon.h b/orchagent/orchdaemon.h
@@ -28,14 +28,15 @@ using namespace swss;
 class OrchDaemon
 {
 public:
-    OrchDaemon(DBConnector *, DBConnector *);
+    OrchDaemon(DBConnector *, DBConnector *, DBConnector *);
     ~OrchDaemon();
 
     bool init();
     void start();
 private:
     DBConnector *m_applDb;
     DBConnector *m_configDb;
+    DBConnector *m_stateDb;
 
     std::vector<Orch *> m_orchList;
     Select *m_select;
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h
diff --git a/tests/test_acl.py b/tests/test_acl.py

Original file line number	Diff line number	Diff line change
`@@ -251,13 +251,15 @@ int main(int argc, char **argv)`
`251`	`251`	`SWSS_LOG_NOTICE("Created underlay router interface ID %lx", gUnderlayIfId);`
`252`	`252`
`253`	`253`	`/* Initialize orchestration components */`
`254`		`- DBConnector *appl_db = new DBConnector(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);`
`255`		`- DBConnector *config_db = new DBConnector(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);`
	`254`	`+ DBConnector appl_db(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);`
	`255`	`+ DBConnector config_db(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);`
	`256`	`+ DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);`
`256`	`257`
`257`		`- OrchDaemon *orchDaemon = new OrchDaemon(appl_db, config_db);`
	`258`	`+ OrchDaemon *orchDaemon = new OrchDaemon(&appl_db, &config_db, &state_db);`
`258`	`259`	`if (!orchDaemon->init())`
`259`	`260`	`{`
`260`	`261`	`SWSS_LOG_ERROR("Failed to initialize orchstration daemon");`
	`262`	`+ delete orchDaemon;`
`261`	`263`	`exit(EXIT_FAILURE);`
`262`	`264`	`}`
`263`	`265`
`@@ -272,6 +274,7 @@ int main(int argc, char **argv)`
`272`	`274`	`if (status != SAI_STATUS_SUCCESS)`
`273`	`275`	`{`
`274`	`276`	`SWSS_LOG_ERROR("Failed to notify syncd APPLY_VIEW %d", status);`
	`277`	`+ delete orchDaemon;`
`275`	`278`	`exit(EXIT_FAILURE);`
`276`	`279`	`}`
`277`	`280`
`@@ -286,5 +289,6 @@ int main(int argc, char **argv)`
`286`	`289`	`SWSS_LOG_ERROR("Failed due to exception: %s", e.what());`
`287`	`290`	`}`
`288`	`291`
	`292`	`+ delete orchDaemon;`
`289`	`293`	`return 0;`
`290`	`294`	`}`
Original file line number	Diff line number	Diff line change
`@@ -358,7 +358,7 @@ bool Orch::parseIndexRange(const string &input, sai_uint32_t &range_low, sai_uin`
`358`	`358`
`359`	`359`	`void Orch::addConsumer(DBConnector *db, string tableName)`
`360`	`360`	`{`
`361`		`- if (db->getDB() == CONFIG_DB)`
	`361`	`+ if (db->getDB() == CONFIG_DB \|\| db->getDB() == STATE_DB)`
`362`	`362`	`{`
`363`	`363`	`addExecutor(tableName, new Consumer(new SubscriberStateTable(db, tableName), this));`
`364`	`364`	`}`