[portsorch]: Bring the physical ports up are only after buffer config…

…uration was applied (#515)

* Don't up ports, until buffer configuration is applied

* Set MTU first, then set port state to UP

* Introduce the test

* Use logical operator && for boolean values

orchagent/bufferorch.cpp

@@ -29,6 +29,7 @@ BufferOrch::BufferOrch(DBConnector *db, vector<string> &tableNames) : Orch(db, t
 {
     SWSS_LOG_ENTER();
     initTableHandlers();
+    initBufferReadyLists(db);
 };
 
 void BufferOrch::initTableHandlers()
@@ -42,6 +43,59 @@ void BufferOrch::initTableHandlers()
     m_bufferHandlerMap.insert(buffer_handler_pair(CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME, &BufferOrch::processEgressBufferProfileList));
 }
 
+void BufferOrch::initBufferReadyLists(DBConnector *db)
+{
+    SWSS_LOG_ENTER();
+
+    Table pg_table(db, CFG_BUFFER_PG_TABLE_NAME);
+    initBufferReadyList(pg_table);
+
+    Table queue_table(db, CFG_BUFFER_QUEUE_TABLE_NAME);
+    initBufferReadyList(queue_table);
+}
+
+void BufferOrch::initBufferReadyList(Table& table)
+{
+    SWSS_LOG_ENTER();
+
+    std::vector<std::string> keys;
+    table.getKeys(keys);
+
+    for (const auto& key: keys)
+    {
+        m_ready_list[key] = false;
+
+        auto tokens = tokenize(key, config_db_key_delimiter);
+        if (tokens.size() != 2)
+        {
+            SWSS_LOG_ERROR("Wrong format of a table '%s' key '%s'. Skip it", table.getTableName().c_str(), key.c_str());
+            continue;
+        }
+
+        auto port_names = tokenize(tokens[0], list_item_delimiter);
+
+        for(const auto& port_name: port_names)
+        {
+            m_port_ready_list_ref[port_name].push_back(key);
+        }
+    }
+}
+
+bool BufferOrch::isPortReady(const std::string& port_name) const
+{
+    SWSS_LOG_ENTER();
+
+    const auto& list_of_keys = m_port_ready_list_ref.at(port_name);
+
+    bool result = true;
+    for (const auto& key: list_of_keys)
+    {
+        result = result && m_ready_list.at(key);
+    }
+
+    return result;
+}
+
 task_process_status BufferOrch::processBufferPool(Consumer &consumer)
 {
     SWSS_LOG_ENTER();
@@ -315,7 +369,7 @@ task_process_status BufferOrch::processQueue(Consumer &consumer)
     auto it = consumer.m_toSync.begin();
     KeyOpFieldsValuesTuple tuple = it->second;
     sai_object_id_t sai_buffer_profile;
-    string key = kfvKey(tuple);
+    const string key = kfvKey(tuple);
     string op = kfvOp(tuple);
     vector<string> tokens;
     sai_uint32_t range_low, range_high;
@@ -374,6 +428,16 @@ task_process_status BufferOrch::processQueue(Consumer &consumer)
             }
         }
     }
+
+    if (m_ready_list.find(key) != m_ready_list.end())
+    {
+        m_ready_list[key] = true;
+    }
+    else
+    {
+        SWSS_LOG_ERROR("Queue profile '%s' was inserted after BufferOrch init", key.c_str());
+    }
+
     return task_process_status::task_success;
 }
 
@@ -386,7 +450,7 @@ task_process_status BufferOrch::processPriorityGroup(Consumer &consumer)
     auto it = consumer.m_toSync.begin();
     KeyOpFieldsValuesTuple tuple = it->second;
     sai_object_id_t sai_buffer_profile;
-    string key = kfvKey(tuple);
+    const string key = kfvKey(tuple);
     string op = kfvOp(tuple);
     vector<string> tokens;
     sai_uint32_t range_low, range_high;
@@ -451,6 +515,16 @@ task_process_status BufferOrch::processPriorityGroup(Consumer &consumer)
             }
         }
     }
+
+    if (m_ready_list.find(key) != m_ready_list.end())
+    {
+        m_ready_list[key] = true;
+    }
+    else
+    {
+        SWSS_LOG_ERROR("PG profile '%s' was inserted after BufferOrch init", key.c_str());
+    }
+
     return task_process_status::task_success;
 }
 

orchagent/bufferorch.h

@@ -1,7 +1,9 @@
 #ifndef SWSS_BUFFORCH_H
 #define SWSS_BUFFORCH_H
 
+#include <string>
 #include <map>
+#include <unordered_map>
 #include "orch.h"
 #include "portsorch.h"
 
@@ -26,6 +28,7 @@ class BufferOrch : public Orch
 {
 public:
     BufferOrch(DBConnector *db, vector<string> &tableNames);
+    bool isPortReady(const std::string& port_name) const;
     static type_map m_buffer_type_maps;
 private:
     typedef task_process_status (BufferOrch::*buffer_table_handler)(Consumer& consumer);
@@ -34,6 +37,8 @@ class BufferOrch : public Orch
 
     virtual void doTask(Consumer& consumer);
     void initTableHandlers();
+    void initBufferReadyLists(DBConnector *db);
+    void initBufferReadyList(Table& table);
     task_process_status processBufferPool(Consumer &consumer);
     task_process_status processBufferProfile(Consumer &consumer);
     task_process_status processQueue(Consumer &consumer);
@@ -42,6 +47,8 @@ class BufferOrch : public Orch
     task_process_status processEgressBufferProfileList(Consumer &consumer);
 
     buffer_table_handler_map m_bufferHandlerMap;
+    std::unordered_map<std::string, bool> m_ready_list;
+    std::unordered_map<std::string, std::vector<std::string>> m_port_ready_list_ref;
 };
 #endif /* SWSS_BUFFORCH_H */
 

orchagent/orchdaemon.cpp

@@ -26,6 +26,7 @@ NeighOrch *gNeighOrch;
 RouteOrch *gRouteOrch;
 AclOrch *gAclOrch;
 CrmOrch *gCrmOrch;
+BufferOrch *gBufferOrch;
 
 OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb, DBConnector *stateDb) :
         m_applDb(applDb),
@@ -90,7 +91,7 @@ bool OrchDaemon::init()
         CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
         CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME
     };
-    BufferOrch *buffer_orch = new BufferOrch(m_configDb, buffer_tables);
+    gBufferOrch = new BufferOrch(m_configDb, buffer_tables);
 
     TableConnector appDbMirrorSession(m_applDb, APP_MIRROR_SESSION_TABLE_NAME);
     TableConnector confDbMirrorSession(m_configDb, CFG_MIRROR_SESSION_TABLE_NAME);
@@ -109,7 +110,7 @@ bool OrchDaemon::init()
 
     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_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, gBufferOrch, mirror_orch, gAclOrch, gFdbOrch, vrf_orch };
     m_select = new Select();
 
 

orchagent/portsorch.cpp

@@ -18,6 +18,7 @@
 #include "sai_serialize.h"
 #include "crmorch.h"
 #include "countercheckorch.h"
+#include "bufferorch.h"
 #include "notifier.h"
 
 extern sai_switch_api_t *sai_switch_api;
@@ -31,6 +32,7 @@ extern sai_queue_api_t *sai_queue_api;
 extern sai_object_id_t gSwitchId;
 extern NeighOrch *gNeighOrch;
 extern CrmOrch *gCrmOrch;
+extern BufferOrch *gBufferOrch;
 
 #define VLAN_PREFIX         "Vlan"
 #define DEFAULT_VLAN_ID     1
@@ -1280,6 +1282,13 @@ void PortsOrch::doPortTask(Consumer &consumer)
                 continue;
             }
 
+            if (alias != "PortConfigDone" && !gBufferOrch->isPortReady(alias))
+            {
+                // buffer configuration hasn't been applied yet. save it for future retry
+                it++;
+                continue;
+            }
+
             Port p;
             if (!getPort(alias, p) && alias != "PortConfigDone")
             {
@@ -1332,31 +1341,31 @@ void PortsOrch::doPortTask(Consumer &consumer)
                     }
                 }
 
-                if (admin_status != "")
+                if (mtu != 0)
                 {
-                    if (setPortAdminStatus(p.m_port_id, admin_status == "up"))
+                    if (setPortMtu(p.m_port_id, mtu))
                     {
-                        SWSS_LOG_NOTICE("Set port %s admin status to %s", alias.c_str(), admin_status.c_str());
+                        p.m_mtu = mtu;
+                        m_portList[alias] = p;
+                        SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
                     }
                     else
                     {
-                        SWSS_LOG_ERROR("Failed to set port %s admin status to %s", alias.c_str(), admin_status.c_str());
+                        SWSS_LOG_ERROR("Failed to set port %s MTU to %u", alias.c_str(), mtu);
                         it++;
                         continue;
                     }
                 }
 
-                if (mtu != 0)
+                if (admin_status != "")
                 {
-                    if (setPortMtu(p.m_port_id, mtu))
+                    if (setPortAdminStatus(p.m_port_id, admin_status == "up"))
                     {
-                        p.m_mtu = mtu;
-                        m_portList[alias] = p;
-                        SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
+                        SWSS_LOG_NOTICE("Set port %s admin status to %s", alias.c_str(), admin_status.c_str());
                     }
                     else
                     {
-                        SWSS_LOG_ERROR("Failed to set port %s MTU to %u", alias.c_str(), mtu);
+                        SWSS_LOG_ERROR("Failed to set port %s admin status to %s", alias.c_str(), admin_status.c_str());
                         it++;
                         continue;
                     }

tests/test_port_buffer_rel.py

@@ -0,0 +1,33 @@
+from swsscommon import swsscommon
+import time
+
+# The test check that the ports will be up, when the admin state is UP by conf db.
+
+def test_PortsAreUpAfterBuffers(dvs):    
+    num_ports = 32
+    asic_db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
+    conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)
+
+    conf_port_table = swsscommon.Table(conf_db, "PORT")
+    asic_port_table = swsscommon.Table(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_PORT")
+
+    # enable all ports
+    fvs = swsscommon.FieldValuePairs([("admin_status", "up")])
+    for i in range(0, num_ports):
+        conf_port_table.set("Ethernet%d" % (i*4), fvs)
+
+    time.sleep(5)
+
+    # check that the ports are enabled in ASIC
+    asic_port_records = asic_port_table.getKeys()
+    assert len(asic_port_records) == (num_ports + 1), "Number of port records doesn't match number of ports" # +CPU port
+    num_set = 0
+    for k in asic_port_records:
+        status, fvs = asic_port_table.get(k)
+        assert status, "Got an error when get a key"
+        for fv in fvs:
+            if fv[0] == "SAI_PORT_ATTR_ADMIN_STATE":
+                assert fv[1] == "true", "The port isn't UP as expected"
+                num_set += 1
+    # make sure that state is set for all "num_ports" ports
+    assert num_set == num_ports, "Not all ports are up"