diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 9b84b619b3..4719f5633c 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -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; @@ -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; @@ -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; } } @@ -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; @@ -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; } @@ -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]; @@ -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; } @@ -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; @@ -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; } diff --git a/tests/mock_tests/buffermgrdyn_ut.cpp b/tests/mock_tests/buffermgrdyn_ut.cpp index a3d3eeab26..fd9a74553d 100644 --- a/tests/mock_tests/buffermgrdyn_ut.cpp +++ b/tests/mock_tests/buffermgrdyn_ut.cpp @@ -291,7 +291,7 @@ namespace buffermgrdyn_test { bufferPgTable.set(key, { - {"profile", "NULL"} + {"profile", profile} }); m_dynamicBuffer->addExistingData(&bufferPgTable); static_cast(m_dynamicBuffer)->doTask(); @@ -682,27 +682,45 @@ 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(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 ingressProfiles = {"egress_lossy_profile", "ingress_profile", ""}; + vector 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); @@ -710,10 +728,57 @@ namespace buffermgrdyn_test // 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 wrong_profile_names = {"ingress_lossless_profile", "wrong_param_profile"}; + vector> 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(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); + } + } } /*