Skip to content

Commit

Permalink
Fix minor comments
Browse files Browse the repository at this point in the history 
- Remove buffer objects when creating failed only if it didn't exist

Signed-off-by: Stephen Sun <stephens@nvidia.com>
  • Loading branch information
@stephenxs
stephenxs committed May 13, 2022
1 parent 10ad9c0 commit 5013ed5
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 34 deletions.
40 changes: 25 additions & 15 deletions cfgmgr/buffermgrdyn.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2472,6 +2472,7 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
// For set command:
// 1. Create the corresponding table entries in APPL_DB
// 2. Record the table in the internal cache m_bufferProfileLookup
bool notExisted = (m_bufferProfileLookup.find(profileName) == m_bufferProfileLookup.end());
buffer_profile_t &profileApp = m_bufferProfileLookup[profileName];

profileApp.static_configured = true;
Expand All @@ -2492,18 +2493,12 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
if (!value.empty())
{
auto &poolName = value;
if (poolName.empty())
{
SWSS_LOG_ERROR("BUFFER_PROFILE: Invalid format of reference to pool: %s", value.c_str());
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_invalid_entry;
}

auto poolRef = m_bufferPoolLookup.find(poolName);
if (poolRef == m_bufferPoolLookup.end())
{
SWSS_LOG_WARN("Pool %s hasn't been configured yet, need retry", poolName.c_str());
m_bufferProfileLookup.erase(profileName);
if (notExisted)
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_need_retry;
}
profileApp.pool_name = poolName;
Expand All @@ -2520,14 +2515,16 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
profileApp.threshold_mode.c_str(),
poolName.c_str(),
threshold_mode.c_str());
m_bufferProfileLookup.erase(profileName);
if (notExisted)
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_failed;
}
}
else
{
SWSS_LOG_ERROR("Pool for BUFFER_PROFILE %s hasn't been specified", field.c_str());
m_bufferProfileLookup.erase(profileName);
if (notExisted)
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_failed;
}
}
Expand Down Expand Up @@ -2561,7 +2558,8 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
field.c_str(),
profileApp.pool_name.c_str(),
profileApp.threshold_mode.c_str());
m_bufferProfileLookup.erase(profileName);
if (notExisted)
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_failed;
}
profileApp.threshold = value;
Expand All @@ -2586,7 +2584,8 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
if (profileApp.direction != BUFFER_INGRESS)
{
SWSS_LOG_ERROR("BUFFER_PROFILE %s is ingress but referencing an egress pool %s", profileName.c_str(), profileApp.pool_name.c_str());
m_bufferProfileLookup.erase(profileName);
if (notExisted)
m_bufferProfileLookup.erase(profileName);
return task_process_status::task_failed;
}

Expand Down Expand Up @@ -2859,6 +2858,7 @@ void BufferMgrDynamic::handleDelSingleBufferObjectOnAdminDownPort(buffer_directi
task_process_status BufferMgrDynamic::handleSingleBufferPgEntry(const string &key, const string &port, const KeyOpFieldsValuesTuple &tuple)
{
string op = kfvOp(tuple);
bool notExisted = (m_portPgLookup[port].find(key) == m_portPgLookup[port].end());
buffer_pg_t &bufferPg = m_portPgLookup[port][key];
port_info_t &portInfo = m_portInfoLookup[port];

Expand Down Expand Up @@ -2894,7 +2894,8 @@ task_process_status BufferMgrDynamic::handleSingleBufferPgEntry(const string &ke
if (profileName.empty())
{
SWSS_LOG_ERROR("BUFFER_PG: Invalid format of reference to profile: %s", value.c_str());
m_portPgLookup[port].erase(key);
if (notExisted)
m_portPgLookup[port].erase(key);
return task_process_status::task_invalid_entry;
}

Expand All @@ -2903,13 +2904,21 @@ task_process_status BufferMgrDynamic::handleSingleBufferPgEntry(const string &ke
{
// In this case, we shouldn't set the dynamic calculated flag to true
// It will be updated when its profile configured.
bufferPg.dynamic_calculated = false;
if (notExisted)
m_portPgLookup[port].erase(key);
SWSS_LOG_WARN("Profile %s hasn't been configured yet, skip", profileName.c_str());
return task_process_status::task_need_retry;
}
else
{
buffer_profile_t &profileRef = searchRef->second;
if (profileRef.direction == BUFFER_EGRESS)
{
if (notExisted)
m_portPgLookup[port].erase(key);
SWSS_LOG_ERROR("Egress buffer profile configured on PG %s", key.c_str());
return task_process_status::task_failed;
}
bufferPg.dynamic_calculated = profileRef.dynamic_calculated;
bufferPg.configured_profile_name = profileName;
bufferPg.lossless = profileRef.lossless;
Expand All @@ -2921,7 +2930,8 @@ task_process_status BufferMgrDynamic::handleSingleBufferPgEntry(const string &ke
if (field != buffer_profile_field_name)
{
SWSS_LOG_ERROR("BUFFER_PG: Invalid field %s", field.c_str());
m_portPgLookup[port].erase(key);
if (notExisted)
m_portPgLookup[port].erase(key);
return task_process_status::task_invalid_entry;
}

Expand Down
103 changes: 84 additions & 19 deletions tests/mock_tests/buffermgrdyn_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ namespace buffermgrdyn_test
{
bufferPgTable.set(key,
{
{"profile", "NULL"}
{"profile", profile}
});
m_dynamicBuffer->addExistingData(&bufferPgTable);
static_cast<Orch *>(m_dynamicBuffer)->doTask();
Expand Down Expand Up @@ -682,38 +682,103 @@ namespace buffermgrdyn_test
CheckProfileList("Ethernet0", true, "ingress_lossless_profile", true);
CheckProfileList("Ethernet0", false, "egress_lossless_profile,egress_lossy_profile", true);

// Profile with wrong mode
bufferProfileTable.set("wrong_mode_profile",
{
{"pool", "ingress_lossless_pool"},
{"static_th", "100"},
{"size", "0"}
});
m_dynamicBuffer->addExistingData(&bufferProfileTable);
static_cast<Orch *>(m_dynamicBuffer)->doTask();
ASSERT_EQ(m_dynamicBuffer->m_bufferProfileLookup.find("wrongMode"), m_dynamicBuffer->m_bufferProfileLookup.end());
ASSERT_TRUE(!appBufferProfileTable.get("wrong_mode_profile", fieldValues));
// No pending notifications
m_dynamicBuffer->dumpPendingTasks(ts);
ASSERT_EQ(ts.size(), 0);

InitPort("Ethernet4");
InitPort("Ethernet6");
InitBufferQueue("Ethernet6|0-2", "egress_lossy_profile");
InitBufferProfileList("Ethernet6", "ingress_lossless_profile", bufferIngProfileListTable);

// Buffer queue/PG/profile lists with wrong direction should not overwrite the existing ones
vector<string> ingressProfiles = {"egress_lossy_profile", "ingress_profile", ""};
vector<string> portsToTest = {"Ethernet0", "Ethernet4"};
for (auto port : portsToTest)
{
for (auto ingressProfile : ingressProfiles)
{
InitBufferPg(port + "|3-4", ingressProfile);
if (port == "Ethernet0")
{
ASSERT_EQ(m_dynamicBuffer->m_portPgLookup["Ethernet0"]["Ethernet0:3-4"].running_profile_name, expectedProfile);
ASSERT_TRUE(appBufferPgTable.get("Ethernet0:3-4", fieldValues));
CheckIfVectorsMatch(fieldValues, {{"profile", expectedProfile}});
}
else
{
ASSERT_TRUE(m_dynamicBuffer->m_portPgLookup[port].find(port + ":3-4") == m_dynamicBuffer->m_portPgLookup[port].end());
ASSERT_FALSE(appBufferPgTable.get(port + ":3-4", fieldValues));
}
}
}

// Buffer queue/PG/profile lists with wrong direction
InitBufferQueue("Ethernet4|0-2", "ingress_lossless_profile");
ASSERT_TRUE(m_dynamicBuffer->m_portQueueLookup["Ethernet4"]["Ethernet0:0-2"].running_profile_name.empty());
ASSERT_TRUE(!appBufferQueueTable.get("Ethernet4:0-2", fieldValues));
ASSERT_FALSE(appBufferQueueTable.get("Ethernet4:0-2", fieldValues));
// No pending notifications
ts.clear();
m_dynamicBuffer->dumpPendingTasks(ts);
ASSERT_EQ(ts.size(), 0);

InitBufferQueue("Ethernet6|0-2", "ingress_lossless_profile");
ASSERT_EQ(m_dynamicBuffer->m_portQueueLookup["Ethernet6"]["Ethernet6:0-2"].running_profile_name, "egress_lossy_profile");
ASSERT_TRUE(appBufferQueueTable.get("Ethernet6:0-2", fieldValues));
CheckIfVectorsMatch(fieldValues, {{"profile", "egress_lossy_profile"}});
// No pending notifications
m_dynamicBuffer->dumpPendingTasks(ts);
ASSERT_EQ(ts.size(), 0);

// Wrong direction
InitBufferProfileList("Ethernet4", "egress_lossless_profile", bufferIngProfileListTable);
ASSERT_TRUE(m_dynamicBuffer->m_portProfileListLookups[BUFFER_INGRESS]["Ethernet4"].empty());
ASSERT_TRUE(!appBufferIngProfileListTable.get("Ethernet4", fieldValues));
ASSERT_FALSE(appBufferIngProfileListTable.get("Ethernet4", fieldValues));
// No pending notifications
m_dynamicBuffer->dumpPendingTasks(ts);
ASSERT_EQ(ts.size(), 0);

InitBufferProfileList("Ethernet6", "egress_lossless_profile", bufferIngProfileListTable);
ASSERT_EQ(m_dynamicBuffer->m_portProfileListLookups[BUFFER_INGRESS]["Ethernet6"], "ingress_lossless_profile");
ASSERT_TRUE(appBufferIngProfileListTable.get("Ethernet6", fieldValues));
CheckIfVectorsMatch(fieldValues, {{"profile_list", "ingress_lossless_profile"}});
// No pending notifications
m_dynamicBuffer->dumpPendingTasks(ts);
ASSERT_EQ(ts.size(), 0);

// Profile with wrong mode should not override the existing entries
vector<string> wrong_profile_names = {"ingress_lossless_profile", "wrong_param_profile"};
vector<vector<FieldValueTuple>> wrong_profile_patterns = {
// wrong threshold mode
{
{"pool", "ingress_lossless_pool"},
{"static_th", "100"},
{"size", "0"}
},
// unconfigured pool
{
{"pool", "ingress_pool"},
{"dynamic_th", "0"},
{"size", "0"}
}
};
auto expected_pending_tasks = 0;
for (auto wrong_profile_name : wrong_profile_names)
{
bool exist = (testBufferProfile.find(wrong_profile_name) != testBufferProfile.end());
for (auto wrong_profile_pattern : wrong_profile_patterns)
{
bufferProfileTable.set(wrong_profile_name, wrong_profile_pattern);
m_dynamicBuffer->addExistingData(&bufferProfileTable);
static_cast<Orch *>(m_dynamicBuffer)->doTask();
if (exist)
CheckProfile(m_dynamicBuffer->m_bufferProfileLookup[wrong_profile_name], testBufferProfile[wrong_profile_name]);
else
ASSERT_EQ(m_dynamicBuffer->m_bufferProfileLookup.find(wrong_profile_name), m_dynamicBuffer->m_bufferProfileLookup.end());
ASSERT_EQ(appBufferProfileTable.get(wrong_profile_name, fieldValues), exist);
// No pending notifications
ts.clear();
m_dynamicBuffer->dumpPendingTasks(ts);
if (get<1>(wrong_profile_pattern[0]) == "ingress_pool")
expected_pending_tasks++;
ASSERT_EQ(ts.size(), expected_pending_tasks);
}
}
}

/*
Expand Down

0 comments on commit 5013ed5

Please sign in to comment.