diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 5017ad9d1b..5b2d5a0d17 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -3110,8 +3110,7 @@ task_process_status BufferMgrDynamic::handleSingleBufferQueueEntry(const string if (op == SET_COMMAND) { - auto &portQueue = m_portQueueLookup[port][queues]; - + bool successful = false; SWSS_LOG_INFO("Inserting entry BUFFER_QUEUE_TABLE:%s to APPL_DB", key.c_str()); for (auto i : kfvFieldsValues(tuple)) @@ -3122,8 +3121,10 @@ task_process_status BufferMgrDynamic::handleSingleBufferQueueEntry(const string auto rc = checkBufferProfileDirection(fvValue(i), BUFFER_EGRESS); if (rc != task_process_status::task_success) return rc; - portQueue.running_profile_name = fvValue(i); + + m_portQueueLookup[port][queues].running_profile_name = fvValue(i); SWSS_LOG_NOTICE("Queue %s has been configured on the system, referencing profile %s", key.c_str(), fvValue(i).c_str()); + successful = true; } else { @@ -3134,8 +3135,15 @@ task_process_status BufferMgrDynamic::handleSingleBufferQueueEntry(const string SWSS_LOG_INFO("Inserting field %s value %s", fvField(i).c_str(), fvValue(i).c_str()); } + if (!successful) + { + SWSS_LOG_ERROR("Invalid BUFFER_QUEUE configuration on %s: no profile configured", key.c_str()); + return task_process_status::task_failed; + } + // TODO: check overlap. Currently, assume there is no overlap + auto &portQueue = m_portQueueLookup[port][queues]; if (PORT_ADMIN_DOWN == portInfo.state) { handleSetSingleBufferObjectOnAdminDownPort(BUFFER_QUEUE, port, key, portQueue.running_profile_name); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index f04a438a5d..8a4049583a 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -393,7 +393,7 @@ ref_resolve_status Orch::resolveFieldRefValue( { return ref_resolve_status::not_resolved; } - else if (ref_type_name.empty() && object_name.empty()) + else if (object_name.empty()) { return ref_resolve_status::empty; } diff --git a/tests/mock_tests/buffermgrdyn_ut.cpp b/tests/mock_tests/buffermgrdyn_ut.cpp index 9dd17a5da8..bf9946ff9b 100644 --- a/tests/mock_tests/buffermgrdyn_ut.cpp +++ b/tests/mock_tests/buffermgrdyn_ut.cpp @@ -667,6 +667,13 @@ namespace buffermgrdyn_test CheckProfileList("Ethernet0", true, "ingress_lossless_profile", false); CheckProfileList("Ethernet0", false, "egress_lossless_profile,egress_lossy_profile", false); + // Initialize a port with all profiles undefined + InitPort("Ethernet8"); + InitBufferPg("Ethernet8|0", "ingress_not_defined_profile"); + InitBufferQueue("Ethernet8|0", "egress_not_defined_profile"); + InitBufferProfileList("Ethernet8", "egress_not_defined_profile", bufferEgrProfileListTable); + InitBufferProfileList("Ethernet8", "ingress_not_defined_profile", bufferIngProfileListTable); + // All default buffer profiles should be generated and pushed into BUFFER_PROFILE_TABLE static_cast(m_dynamicBuffer)->doTask(); @@ -686,6 +693,36 @@ namespace buffermgrdyn_test CheckProfileList("Ethernet0", true, "ingress_lossless_profile", true); CheckProfileList("Ethernet0", false, "egress_lossless_profile,egress_lossy_profile", true); + // Check no items applied on port Ethernet8 + ASSERT_EQ(appBufferPgTable.get("Ethernet8:0", fieldValues), false); + CheckQueue("Ethernet8", "Ethernet8:0", "", false); + CheckProfileList("Ethernet8", true, "", false); + CheckProfileList("Ethernet8", false, "", false); + + // Configure the missing buffer profiles + bufferProfileTable.set("ingress_not_defined_profile", + { + {"pool", "ingress_lossless_pool"}, + {"dynamic_th", "0"}, + {"size", "0"} + }); + bufferProfileTable.set("egress_not_defined_profile", + { + {"pool", "egress_lossless_pool"}, + {"dynamic_th", "0"}, + {"size", "0"} + }); + m_dynamicBuffer->addExistingData(&bufferProfileTable); + // For buffer profile + static_cast(m_dynamicBuffer)->doTask(); + // For all other items + static_cast(m_dynamicBuffer)->doTask(); + ASSERT_EQ(appBufferPgTable.get("Ethernet8:0", fieldValues), true); + ASSERT_EQ(fvValue(fieldValues[0]), "ingress_not_defined_profile"); + CheckQueue("Ethernet8", "Ethernet8:0", "egress_not_defined_profile", true); + CheckProfileList("Ethernet8", true, "ingress_not_defined_profile", true); + CheckProfileList("Ethernet8", false, "egress_not_defined_profile", true); + InitPort("Ethernet4"); InitPort("Ethernet6"); InitBufferQueue("Ethernet6|0-2", "egress_lossy_profile"); diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index 13454cee56..e26b60c549 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -1337,4 +1337,35 @@ namespace qosorch_test testing_wred_thresholds = false; } + + /* + * Make sure empty fields won't cause orchagent crash + */ + TEST_F(QosOrchTest, QosOrchTestEmptyField) + { + // Create a new dscp to tc map + std::deque entries; + entries.push_back({"Ethernet0", "SET", + { + {"dscp_to_tc_map", ""} + }}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_PORT_QOS_MAP_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + + entries.push_back({"Ethernet0|3", "SET", + { + {"scheduler", ""} + }}); + entries.push_back({"Ethernet0|4", "SET", + { + {"wred_profile", ""} + }}); + consumer = dynamic_cast(gQosOrch->getExecutor(CFG_QUEUE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + + // Drain DSCP_TO_TC_MAP and PORT_QOS_MAP table + static_cast(gQosOrch)->doTask(); + } }