From 5557914202705cdc697fccfa54faf805c652d8f4 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 8 Jun 2021 18:23:15 +0000 Subject: [PATCH 1/5] Don't create lossy PG for admin down ports during initialization - Don't create lossy PG for admin down ports - Treat admin down ports as ready during orchagent initialization Signed-off-by: Stephen Sun --- cfgmgr/buffermgrdyn.cpp | 10 +++++++--- cfgmgr/buffermgrdyn.h | 6 +++--- orchagent/portsorch.cpp | 4 +++- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 33ef6c3e5e..602fd8c180 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -1862,9 +1862,13 @@ 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; + } } if (!bufferPg.configured_profile_name.empty()) diff --git a/cfgmgr/buffermgrdyn.h b/cfgmgr/buffermgrdyn.h index 1a68305909..e8e031315b 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/portsorch.cpp b/orchagent/portsorch.cpp index 3238ee795e..828ba026c3 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2732,9 +2732,11 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } - if (!gBufferOrch->isPortReady(alias)) + if (!gBufferOrch->isPortReady(alias) && (admin_status == "up")) { // buffer configuration hasn't been applied yet. save it for future retry + // We don't need to check it if the port isn't admin up because + // the buffer configuration won't be applied until the port is admin up m_pendingPortSet.emplace(alias); it++; continue; From e98fea57c585c903233d8b03340ce9e9a9a577a4 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Thu, 10 Jun 2021 20:38:46 +0800 Subject: [PATCH 2/5] Add checks to admin down test - Check whether the PGs is removed after a port has been shut down - No lossy/lossless PG added to APPL_DB when a port is admin down Signed-off-by: Stephen Sun --- tests/test_buffer_dynamic.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index 0d831f7035..74575c6ee3 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 + "]"}) From ce511f05693246ca724484a068b8b5c3884afe21 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 23 Jun 2021 04:13:24 +0000 Subject: [PATCH 3/5] Fix review comments Explicitly remove PGs from an admin down port when a PG is configured during initialization This is to make sure the items will be added to m_ready_list Signed-off-by: Stephen Sun --- cfgmgr/buffermgrdyn.cpp | 12 ++++++++++++ orchagent/portsorch.cpp | 4 +--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 602fd8c180..4dd6984099 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: @@ -1869,6 +1875,12 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, 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/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 828ba026c3..3238ee795e 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2732,11 +2732,9 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } - if (!gBufferOrch->isPortReady(alias) && (admin_status == "up")) + if (!gBufferOrch->isPortReady(alias)) { // buffer configuration hasn't been applied yet. save it for future retry - // We don't need to check it if the port isn't admin up because - // the buffer configuration won't be applied until the port is admin up m_pendingPortSet.emplace(alias); it++; continue; From ece086974a0409d2f52e178e282078c9eaeedd9e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 27 Jun 2021 01:01:07 +0000 Subject: [PATCH 4/5] Use APPL_DB as the source of initial information of m_port_ready_list_ref This is to make sure admin down ports can be ready in warmreboot in dynamic model Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 55 +++++++++++++++++++++++++++++++--------- orchagent/bufferorch.h | 4 +-- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index f44b90f96c..8c234cefa2 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(confDb, applDb); initFlexCounterGroupTable(); initBufferConstants(); }; @@ -61,31 +62,61 @@ 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(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(db, CFG_BUFFER_QUEUE_TABLE_NAME); - initBufferReadyList(queue_table); + 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 +127,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 cb14fa3f43..c8810d43fc 100644 --- a/orchagent/bufferorch.h +++ b/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); From 7af4d12b0e6f8470386a400ed562e7df41b99397 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 27 Jun 2021 14:21:55 +0000 Subject: [PATCH 5/5] Fix vs test error Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 43 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 8c234cefa2..d91b503a0a 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -46,7 +46,7 @@ BufferOrch::BufferOrch(DBConnector *applDb, DBConnector *confDb, DBConnector *st { SWSS_LOG_ENTER(); initTableHandlers(); - initBufferReadyLists(confDb, applDb); + initBufferReadyLists(applDb, confDb); initFlexCounterGroupTable(); initBufferConstants(); }; @@ -64,26 +64,27 @@ void BufferOrch::initTableHandlers() void BufferOrch::initBufferReadyLists(DBConnector *applDb, DBConnector *confDb) { - // 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 - // + /* + 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(); if (WarmStart::isWarmStart())