[Dynamic Buffer Calc] Avoid creating lossy PG for admin down ports during initialization

cfgmgr/buffermgrdyn.cpp

@@ -1052,6 +1052,12 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const
 
     case PORT_ADMIN_DOWN:
         SWSS_LOG_NOTICE("Skip setting BUFFER_PG for %s because the port is administratively down", port.c_str());
+        if (!m_portInitDone)
+        {
+            // In case the port is admin down during initialization, the PG will be removed from the port,
+            // which effectively notifies bufferOrch to add the item to the m_ready_list
+            m_applBufferPgTable.del(pg_key);
+        }
         break;
 
     default:
@@ -1862,9 +1868,19 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
         }
         else
         {
-            SWSS_LOG_NOTICE("Inserting BUFFER_PG table entry %s into APPL_DB directly", key.c_str());
-            m_applBufferPgTable.set(key, fvVector);
-            bufferPg.running_profile_name = bufferPg.configured_profile_name;
+            port_info_t &portInfo = m_portInfoLookup[port];
+            if (PORT_ADMIN_DOWN != portInfo.state)
+            {
+                SWSS_LOG_NOTICE("Inserting BUFFER_PG table entry %s into APPL_DB directly", key.c_str());
+                m_applBufferPgTable.set(key, fvVector);
+                bufferPg.running_profile_name = bufferPg.configured_profile_name;
+            }
+            else if (!m_portInitDone)
+            {
+                // In case the port is admin down during initialization, the PG will be removed from the port,
+                // which effectively notifies bufferOrch to add the item to the m_ready_list
+                m_applBufferPgTable.del(key);
+            }
         }
 
         if (!bufferPg.configured_profile_name.empty())

cfgmgr/buffermgrdyn.h

@@ -79,12 +79,12 @@ typedef struct {
 } buffer_pg_t;
 
 typedef enum {
+    // Port is admin down. All PGs programmed to APPL_DB should be removed from the port
+    PORT_ADMIN_DOWN,
     // Port is under initializing, which means its info hasn't been comprehensive for calculating headroom
     PORT_INITIALIZING,
     // All necessary information for calculating headroom is ready
-    PORT_READY,
-    // Port is admin down. All PGs programmed to APPL_DB should be removed from the port
-    PORT_ADMIN_DOWN
+    PORT_READY
 } port_state_t;
 
 typedef struct {

orchagent/bufferorch.cpp

@@ -2,6 +2,7 @@
 #include "bufferorch.h"
 #include "logger.h"
 #include "sai_serialize.h"
+#include "warm_restart.h"
 
 #include <inttypes.h>
 #include <sstream>
@@ -45,7 +46,7 @@ BufferOrch::BufferOrch(DBConnector *applDb, DBConnector *confDb, DBConnector *st
 {
     SWSS_LOG_ENTER();
     initTableHandlers();
-    initBufferReadyLists(confDb);
+    initBufferReadyLists(applDb, confDb);
     initFlexCounterGroupTable();
     initBufferConstants();
 };
@@ -61,31 +62,62 @@ void BufferOrch::initTableHandlers()
     m_bufferHandlerMap.insert(buffer_handler_pair(APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME, &BufferOrch::processEgressBufferProfileList));
 }
 
-void BufferOrch::initBufferReadyLists(DBConnector *db)
+void BufferOrch::initBufferReadyLists(DBConnector *applDb, DBConnector *confDb)
 {
-    // The motivation of m_ready_list is to get the preconfigured buffer pg and buffer queue items
-    // from the database when system starts.
-    // When a buffer pg or queue item is updated, if the item isn't in the m_ready_list
+    /*
+       Map m_ready_list and m_port_ready_list_ref are designed to track whether the ports are "ready" from buffer's POV
+       by testing whether all configured buffer PGs and queues have been applied to SAI. The idea is:
+       - bufferorch read initial configuration and put them into m_port_ready_list_ref.
+       - A buffer pg or queue item will be put into m_ready_list once it is applied to SAI.
+       The rest of port initialization won't be started before the port being ready.
+
+       However, the items won't be applied to admin down ports in dynamic buffer model, which means the admin down ports won't be "ready"
+       The solution is:
+       - buffermgr to notify bufferorch explicitly to remove the PG and queue items configured on admin down ports
+       - bufferorch to add the items to m_ready_list on receiving notifications, which is an existing logic
+
+       Theoretically, the initial configuration should come from CONFIG_DB but APPL_DB is used for warm reboot, because:
+       - For cold/fast start, buffermgr is responsible for injecting items to APPL_DB
+         There is no guarantee that items in APPL_DB are ready when orchagent starts
+       - For warm reboot, APPL_DB is restored from the previous boot, which means they are ready when orchagent starts
+         In addition, bufferorch won't be notified removal of items on admin down ports during warm reboot
+         because buffermgrd hasn't been started yet.
+         Using APPL_DB means items of admin down ports won't be inserted into m_port_ready_list_ref
+         and guarantees the admin down ports always be ready in dynamic buffer model
+    */
     SWSS_LOG_ENTER();
 
-    Table pg_table(db, CFG_BUFFER_PG_TABLE_NAME);
-    initBufferReadyList(pg_table);
+    if (WarmStart::isWarmStart())
+    {
+        Table pg_table(applDb, APP_BUFFER_PG_TABLE_NAME);
+        initBufferReadyList(pg_table, false);
 
-    Table queue_table(db, CFG_BUFFER_QUEUE_TABLE_NAME);
-    initBufferReadyList(queue_table);
+        Table queue_table(applDb, APP_BUFFER_QUEUE_TABLE_NAME);
+        initBufferReadyList(queue_table, false);
+    }
+    else
+    {
+        Table pg_table(confDb, CFG_BUFFER_PG_TABLE_NAME);
+        initBufferReadyList(pg_table, true);
+
+        Table queue_table(confDb, CFG_BUFFER_QUEUE_TABLE_NAME);
+        initBufferReadyList(queue_table, true);
+    }
 }
 
-void BufferOrch::initBufferReadyList(Table& table)
+void BufferOrch::initBufferReadyList(Table& table, bool isConfigDb)
 {
     SWSS_LOG_ENTER();
 
     std::vector<std::string> keys;
     table.getKeys(keys);
 
+    const char dbKeyDelimiter = (isConfigDb ? config_db_key_delimiter : delimiter);
+
     // populate the lists with buffer configuration information
     for (const auto& key: keys)
     {
-        auto tokens = tokenize(key, config_db_key_delimiter);
+        auto &&tokens = tokenize(key, dbKeyDelimiter);
         if (tokens.size() != 2)
         {
             SWSS_LOG_ERROR("Wrong format of a table '%s' key '%s'. Skip it", table.getTableName().c_str(), key.c_str());
@@ -96,7 +128,7 @@ void BufferOrch::initBufferReadyList(Table& table)
         auto appldb_key = tokens[0] + delimiter + tokens[1];
         m_ready_list[appldb_key] = false;
 
-        auto port_names = tokenize(tokens[0], list_item_delimiter);
+        auto &&port_names = tokenize(tokens[0], list_item_delimiter);
 
         for(const auto& port_name: port_names)
         {

orchagent/bufferorch.h

@@ -45,8 +45,8 @@ class BufferOrch : public Orch
     void doTask() override;
     virtual void doTask(Consumer& consumer);
     void initTableHandlers();
-    void initBufferReadyLists(DBConnector *db);
-    void initBufferReadyList(Table& table);
+    void initBufferReadyLists(DBConnector *confDb, DBConnector *applDb);
+    void initBufferReadyList(Table& table, bool isConfigDb);
     void initFlexCounterGroupTable(void);
     void initBufferConstants();
     task_process_status processBufferPool(KeyOpFieldsValuesTuple &tuple);

tests/test_buffer_dynamic.py

@@ -548,6 +548,9 @@ def test_sharedHeadroomPool(self, dvs, testlog):
     def test_shutdownPort(self, dvs, testlog):
         self.setup_db(dvs)
 
+        lossy_pg_reference_config_db = '[BUFFER_PROFILE|ingress_lossy_profile]'
+        lossy_pg_reference_appl_db = '[BUFFER_PROFILE_TABLE:ingress_lossy_profile]'
+
         # Startup interface
         dvs.runcmd('config interface startup Ethernet0')
 
@@ -558,14 +561,23 @@ def test_shutdownPort(self, dvs, testlog):
 
         # Shutdown port and check whether all the PGs have been removed
         dvs.runcmd("config interface shutdown Ethernet0")
+        self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:0")
         self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:3-4")
         self.app_db.wait_for_deleted_entry("BUFFER_PROFILE", expectedProfile)
 
-        # Add another PG when port is administratively down
+        # Add extra lossy and lossless PGs when a port is administratively down
+        self.config_db.update_entry('BUFFER_PG', 'Ethernet0|1', {'profile': lossy_pg_reference_config_db})
         self.config_db.update_entry('BUFFER_PG', 'Ethernet0|6', {'profile': 'NULL'})
 
+        # Make sure they have not been added to APPL_DB
+        time.sleep(30)
+        self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:1")
+        self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:6")
+
         # Startup port and check whether all the PGs haved been added
         dvs.runcmd("config interface startup Ethernet0")
+        self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:0", {"profile": lossy_pg_reference_appl_db})
+        self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:1", {"profile": lossy_pg_reference_appl_db})
         self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"})
         self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"})