Skip to content

Commit

Permalink
[Buffermgr]Graceful handling of buffer model change (sonic-net#1956)
Browse files Browse the repository at this point in the history
Signed-off-by: Sudharsan Dhamal Gopalarathnam [email protected]

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.
  • Loading branch information
dgsudharsan authored Oct 20, 2021
1 parent b0aa6a0 commit d95823d
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 1 deletion.
51 changes: 51 additions & 0 deletions cfgmgr/buffermgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<FieldValueTuple> 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);
Expand Down
2 changes: 2 additions & 0 deletions cfgmgr/buffermgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -63,6 +64,7 @@ class BufferMgr : public Orch
void transformSeperator(std::string &name);

void doTask(Consumer &consumer);
void doBufferMetaTask(Consumer &consumer);
};

}
Expand Down
3 changes: 2 additions & 1 deletion cfgmgr/buffermgrd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
20 changes: 20 additions & 0 deletions tests/test_buffer_mode.py
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit d95823d

Please sign in to comment.