diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 33ef6c3e5e95..4dd69840992d 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/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()) diff --git a/cfgmgr/buffermgrdyn.h b/cfgmgr/buffermgrdyn.h index 1a6830590992..e8e031315bbb 100644 --- a/cfgmgr/buffermgrdyn.h +++ b/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 { diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 5979445731d8..59fcce590ee2 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -2,6 +2,7 @@ #include "bufferorch.h" #include "logger.h" #include "sai_serialize.h" +#include "warm_restart.h" #include #include @@ -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 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) { diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index dc3752181368..ef4153e6a467 100644 --- a/orchagent/bufferorch.h +++ b/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); diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index 0d831f703548..74575c6ee31b 100644 --- a/tests/test_buffer_dynamic.py +++ b/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 + "]"})