diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 666c908734..91d7b8040a 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -825,7 +825,10 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) } else { - getPort(itr->second, port); + if (!getPort(itr->second, port)) + { + SWSS_LOG_THROW("Inconsistent saiOidToAlias map and m_portList map: oid=%" PRIx64, id); + } return true; } @@ -1066,9 +1069,9 @@ void PortsOrch::getCpuPort(Port &port) port = m_cpuPort; } -/* - * Create host_tx_ready field in PORT_TABLE of STATE-DB - * and set the field to false by default for the +/* + * Create host_tx_ready field in PORT_TABLE of STATE-DB + * and set the field to false by default for the * front port. */ void PortsOrch::initHostTxReadyState(Port &port) @@ -1093,7 +1096,7 @@ void PortsOrch::initHostTxReadyState(Port &port) if (hostTxReady.empty()) { m_portStateTable.hset(port.m_alias, "host_tx_ready", "false"); - SWSS_LOG_INFO("initalize hostTxReady %s with status %s", + SWSS_LOG_INFO("initalize hostTxReady %s with status %s", port.m_alias.c_str(), hostTxReady.c_str()); } } @@ -1107,13 +1110,13 @@ bool PortsOrch::setPortAdminStatus(Port &port, bool state) attr.value.booldata = state; /* Update the host_tx_ready to false before setting admin_state, when admin state is false */ - if (!state) + if (!state) { m_portStateTable.hset(port.m_alias, "host_tx_ready", "false"); SWSS_LOG_INFO("Set admin status DOWN host_tx_ready to false to port pid:%" PRIx64, port.m_port_id); } - + sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr); if (status != SAI_STATUS_SUCCESS) { @@ -1132,14 +1135,14 @@ bool PortsOrch::setPortAdminStatus(Port &port, bool state) { m_portStateTable.hset(port.m_alias, "host_tx_ready", "false"); } - + /* Update the state table for host_tx_ready*/ if (state && (gbstatus == true) && (status == SAI_STATUS_SUCCESS) ) { m_portStateTable.hset(port.m_alias, "host_tx_ready", "true"); SWSS_LOG_INFO("Set admin status UP host_tx_ready to true to port pid:%" PRIx64, port.m_port_id); - } + } return true; } @@ -1373,9 +1376,9 @@ bool PortsOrch::setPortPfcWatchdogStatus(sai_object_id_t portId, uint8_t pfcwd_b SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, portId); return false; } - + p.m_pfcwd_sw_bitmask = pfcwd_bitmask; - + m_portList[p.m_alias] = p; SWSS_LOG_INFO("Set PFC watchdog port id=0x%" PRIx64 ", bitmast=0x%x", portId, pfcwd_bitmask); @@ -1393,9 +1396,9 @@ bool PortsOrch::getPortPfcWatchdogStatus(sai_object_id_t portId, uint8_t *pfcwd_ SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, portId); return false; } - + *pfcwd_bitmask = p.m_pfcwd_sw_bitmask; - + return true; } @@ -3772,10 +3775,10 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } } - + /* create host_tx_ready field in state-db */ initHostTxReadyState(p); - + /* Last step set port admin status */ if (!admin_status.empty() && (p.m_admin_state_up != (admin_status == "up"))) { @@ -3853,6 +3856,7 @@ void PortsOrch::doPortTask(Consumer &consumer) /* Delete port from port list */ m_portList.erase(alias); + saiOidToAlias.erase(port_id); } else { @@ -6277,7 +6281,7 @@ void PortsOrch::doTask(NotificationConsumer &consumer) if (!getPort(id, port)) { - SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, id); + SWSS_LOG_NOTICE("Got port state change for port id 0x%" PRIx64 " which does not exist, possibly outdated event", id); continue; } diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 6425dca20f..78982072e7 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -82,16 +82,16 @@ namespace portsorch_test { sai_port_api = pold_sai_port_api; } - + sai_queue_api_t ut_sai_queue_api; sai_queue_api_t *pold_sai_queue_api; int _sai_set_queue_attr_count = 0; - + sai_status_t _ut_stub_sai_set_queue_attribute(sai_object_id_t queue_id, const sai_attribute_t *attr) { if(attr->id == SAI_QUEUE_ATTR_PFC_DLR_INIT) { - if(attr->value.booldata == true) + if(attr->value.booldata == true) { _sai_set_queue_attr_count++; } @@ -269,6 +269,46 @@ namespace portsorch_test }; + /** + * Test that verifies PortsOrch::getPort() on a port that has been deleted + */ + TEST_F(PortsOrchTest, GetPortTest) + { + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + std::deque entries; + + // Get SAI default ports to populate DB + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + + // Set PortConfigDone + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + + // refill consumer + gPortsOrch->addExistingData(&portTable); + + // Apply configuration : + // create ports + static_cast(gPortsOrch)->doTask(); + + Port port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); + ASSERT_NE(port.m_port_id, SAI_NULL_OBJECT_ID); + + // Delete port + entries.push_back({"Ethernet0", "DEL", {}}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + entries.clear(); + + ASSERT_FALSE(gPortsOrch->getPort(port.m_port_id, port)); + } + TEST_F(PortsOrchTest, PortSupportedFecModes) { _hook_sai_port_api(); @@ -892,7 +932,7 @@ namespace portsorch_test * updated to DB. */ TEST_F(PortsOrchTest, PortOperStatusIsUpAndOperSpeedIsZero) - { + { Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); // Get SAI default ports to populate DB @@ -917,7 +957,7 @@ namespace portsorch_test Port port; gPortsOrch->getPort("Ethernet0", port); ASSERT_TRUE(port.m_oper_status != SAI_PORT_OPER_STATUS_UP); - + // save original api since we will spy auto orig_port_api = sai_port_api; sai_port_api = new sai_port_api_t(); @@ -935,14 +975,14 @@ namespace portsorch_test // Return 0 for port operational speed attrs[0].value.u32 = 0; } - + return (sai_status_t)SAI_STATUS_SUCCESS; } ); auto exec = static_cast(gPortsOrch->getExecutor("PORT_STATUS_NOTIFICATIONS")); auto consumer = exec->getNotificationConsumer(); - + // mock a redis reply for notification, it notifies that Ehernet0 is going to up mockReply = (redisReply *)calloc(sizeof(redisReply), 1); mockReply->type = REDIS_REPLY_ARRAY; @@ -964,7 +1004,7 @@ namespace portsorch_test // trigger the notification consumer->readData(); gPortsOrch->doTask(*consumer); - mockReply = nullptr; + mockReply = nullptr; gPortsOrch->getPort("Ethernet0", port); ASSERT_TRUE(port.m_oper_status == SAI_PORT_OPER_STATUS_UP);