From d95823d057035e518c38cdea40e3a330e4bb9255 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Wed, 20 Oct 2021 15:57:32 -0700 Subject: [PATCH] [Buffermgr]Graceful handling of buffer model change (#1956) Signed-off-by: Sudharsan Dhamal Gopalarathnam sudharsand@nvidia.com What I did Handled buffer model changes gracefully. The use case is when config_db.json is loaded initially with no buffer configurations and when user executes 'config qos reload', it would result in buffer model changing from traditional to dynamic. This transition requires swss restart which will be shown as message to user. Why I did it When config qos reload is given in platforms with dynamic buffer model, swss restart is required. However, if swss is not restarted the buffermgrd will stay in static model and will program the orchagent with 'size' field not set in buffer pool due to dynamic mode checks in jinja2 template. This will result in orchagent calling SAI without SAI_BUFFER_POOL_ATTR_SIZE which is mandatory attribute. Since this results in a SAI create API failure, it will result in orchagent crash. So when the mode is changed to dynamic, buffermgrd will not process any configurations when running in static mode. How I verified it Added UT. Manually verified that config qos reload doesn't crash. --- cfgmgr/buffermgr.cpp | 51 +++++++++++++++++++++++++++++++++++++++ cfgmgr/buffermgr.h | 2 ++ cfgmgr/buffermgrd.cpp | 3 ++- tests/test_buffer_mode.py | 20 +++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/cfgmgr/buffermgr.cpp b/cfgmgr/buffermgr.cpp index 14d4caa3a82e..c051d681b397 100644 --- a/cfgmgr/buffermgr.cpp +++ b/cfgmgr/buffermgr.cpp @@ -30,6 +30,7 @@ BufferMgr::BufferMgr(DBConnector *cfgDb, DBConnector *applDb, string pg_lookup_f m_applBufferEgressProfileListTable(applDb, APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME) { readPgProfileLookupFile(pg_lookup_file); + dynamic_buffer_model = false; } //# speed, cable, size, xon, xoff, threshold, xon_offset @@ -273,12 +274,62 @@ void BufferMgr::doBufferTableTask(Consumer &consumer, ProducerStateTable &applTa } } +void BufferMgr::doBufferMetaTask(Consumer &consumer) +{ + SWSS_LOG_ENTER(); + + auto it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + KeyOpFieldsValuesTuple t = it->second; + string key = kfvKey(t); + + string op = kfvOp(t); + if (op == SET_COMMAND) + { + vector fvVector; + + for (auto i : kfvFieldsValues(t)) + { + if (fvField(i) == "buffer_model") + { + if (fvValue(i) == "dynamic") + { + dynamic_buffer_model = true; + } + else + { + dynamic_buffer_model = false; + } + break; + } + } + } + else if (op == DEL_COMMAND) + { + dynamic_buffer_model = false; + } + it = consumer.m_toSync.erase(it); + } +} + void BufferMgr::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); string table_name = consumer.getTableName(); + if (table_name == CFG_DEVICE_METADATA_TABLE_NAME) + { + doBufferMetaTask(consumer); + return; + } + + if (dynamic_buffer_model) + { + SWSS_LOG_DEBUG("Dynamic buffer model enabled. Skipping further processing"); + return; + } if (table_name == CFG_BUFFER_POOL_TABLE_NAME) { doBufferTableTask(consumer, m_applBufferPoolTable); diff --git a/cfgmgr/buffermgr.h b/cfgmgr/buffermgr.h index 652e84dafb45..0c71f1b8c0a6 100644 --- a/cfgmgr/buffermgr.h +++ b/cfgmgr/buffermgr.h @@ -50,6 +50,7 @@ class BufferMgr : public Orch ProducerStateTable m_applBufferEgressProfileListTable; bool m_pgfile_processed; + bool dynamic_buffer_model; pg_profile_lookup_t m_pgProfileLookup; port_cable_length_t m_cableLenLookup; @@ -63,6 +64,7 @@ class BufferMgr : public Orch void transformSeperator(std::string &name); void doTask(Consumer &consumer); + void doBufferMetaTask(Consumer &consumer); }; } diff --git a/cfgmgr/buffermgrd.cpp b/cfgmgr/buffermgrd.cpp index 7824de8df15b..9f4d97923752 100644 --- a/cfgmgr/buffermgrd.cpp +++ b/cfgmgr/buffermgrd.cpp @@ -195,7 +195,8 @@ int main(int argc, char **argv) CFG_BUFFER_PG_TABLE_NAME, CFG_BUFFER_QUEUE_TABLE_NAME, CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, - CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME + CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME, + CFG_DEVICE_METADATA_TABLE_NAME }; cfgOrchList.emplace_back(new BufferMgr(&cfgDb, &applDb, pg_lookup_file, cfg_buffer_tables)); } diff --git a/tests/test_buffer_mode.py b/tests/test_buffer_mode.py index db39bea66a34..5d5eaac8b140 100644 --- a/tests/test_buffer_mode.py +++ b/tests/test_buffer_mode.py @@ -1,7 +1,27 @@ import pytest +import time class TestBufferModel(object): def test_bufferModel(self, dvs, testlog): config_db = dvs.get_config_db() metadata = config_db.get_entry("DEVICE_METADATA", "localhost") assert metadata["buffer_model"] == "traditional" + + def test_update_bufferModel(self, dvs, testlog): + config_db = dvs.get_config_db() + app_db = dvs.get_app_db() + keys = app_db.get_keys("BUFFER_POOL_TABLE") + num_keys = len(keys) + + try: + fvs = {'buffer_model' : 'dynamic'} + config_db.update_entry("DEVICE_METADATA", "localhost", fvs) + fvs = {'mode':'dynamic', 'type':'egress'} + config_db.update_entry("BUFFER_POOL", "temp_pool", fvs) + time.sleep(2) + app_db.wait_for_n_keys("BUFFER_POOL_TABLE", num_keys) + + finally: + config_db.delete_entry("BUFFER_POOL", "temp_pool") + fvs = {'buffer_model' : 'traditional'} + config_db.update_entry("DEVICE_METADATA", "localhost", fvs)