[Dynamic Buffer Calc] Avoid creating lossy PG for admin down ports during initialization (sonic-net#1776)

- What I did
Avoid creating lossy PGs for admin down ports during system initialization
Make ADMIN_DOWN the default state of a port in dynamic buffer manager.
Explicitly remove configured PGs from an admin down port during initialization
This is to make sure the items will be added to m_ready_list.
Use APPL_DB as the source of initial PGs and queues for port ready deducting for warm reboot
Signed-off-by: Stephen Sun [email protected]

- Why I did it
Lossy PGs for admin down ports should not be applied to APPL_DB.
Originally, we had the logic to remove lossy/lossless PGs when shutting a port. The same logic should be applied when the system is starting. Allocating lossy PG for an admin down port doesn't affect the system negatively except for wasting buffer. So we would like to resolve it.

- How I verified it
Manually test

Author: stephenxs

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

@@ -46,8 +46,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 + "]"})