diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index fd95de1d..cd2e3a97 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4723,44 +4723,47 @@ void PortsOrch::doPortTask(Consumer &consumer) } if (p.m_cap_an < 1) { - SWSS_LOG_ERROR("%s: autoneg is not supported (cap=%d)", p.m_alias.c_str(), p.m_cap_an); - // autoneg is not supported, don't retry - it = taskMap.erase(it); - continue; + /* autoneg not supported: log and continue applying remaining config (disable is a no-op; enable cannot be honored) */ + SWSS_LOG_WARN("%s: autoneg is not supported (cap=%d), requested=%s", + p.m_alias.c_str(), p.m_cap_an, + pCfg.autoneg.value ? "on" : "off"); } - if (p.m_admin_state_up) + else { - /* Bring port down before applying speed */ - if (!setPortAdminStatus(p, false)) + if (p.m_admin_state_up) { - SWSS_LOG_ERROR( - "Failed to set port %s admin status DOWN to set port autoneg mode", - p.m_alias.c_str() - ); - it++; - continue; - } - - p.m_admin_state_up = false; - m_portList[p.m_alias] = p; - } + /* Bring port down before changing autoneg mode */ + if (!setPortAdminStatus(p, false)) + { + SWSS_LOG_ERROR( + "Failed to set port %s admin status DOWN to set port autoneg mode", + p.m_alias.c_str() + ); + it++; + continue; + } - auto status = setPortAutoNeg(p, pCfg.autoneg.value); - if (status != task_success) - { - SWSS_LOG_ERROR( - "Failed to set port %s AN from %d to %d", - p.m_alias.c_str(), p.m_autoneg, pCfg.autoneg.value - ); - if (status == task_need_retry) - { - it++; + p.m_admin_state_up = false; + m_portList[p.m_alias] = p; } - else + + auto status = setPortAutoNeg(p, pCfg.autoneg.value); + if (status != task_success) { - it = taskMap.erase(it); + SWSS_LOG_ERROR( + "Failed to set port %s AN from %d to %d", + p.m_alias.c_str(), p.m_autoneg, pCfg.autoneg.value + ); + if (status == task_need_retry) + { + it++; + } + else + { + it = taskMap.erase(it); + } + continue; } - continue; } p.m_autoneg = pCfg.autoneg.value; diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index d8d1c876..9506bfb0 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -42,6 +42,8 @@ namespace portsorch_test bool not_support_fetching_fec; uint32_t _sai_set_port_fec_count; uint32_t _sai_set_port_auto_neg_count; + bool mock_autoneg_cap_supported = true; + bool set_autoneg_need_retry = false; uint32_t _sai_set_port_tpid_count; int32_t _sai_port_fec_mode; vector mock_port_fec_modes = {SAI_PORT_FEC_MODE_RS, SAI_PORT_FEC_MODE_FC}; @@ -110,6 +112,11 @@ namespace portsorch_test status = SAI_STATUS_SUCCESS; } } + else if (attr_count == 1 && attr_list[0].id == SAI_PORT_ATTR_SUPPORTED_AUTO_NEG_MODE) + { + attr_list[0].value.booldata = mock_autoneg_cap_supported; + status = SAI_STATUS_SUCCESS; + } else { status = pold_sai_port_api->get_port_attribute(port_id, attr_count, attr_list); @@ -153,6 +160,10 @@ namespace portsorch_test else if (attr[0].id == SAI_PORT_ATTR_AUTO_NEG_MODE) { _sai_set_port_auto_neg_count++; + if (set_autoneg_need_retry) + { + return SAI_STATUS_INSUFFICIENT_RESOURCES; + } /* Simulating failure case */ return SAI_STATUS_FAILURE; } @@ -3511,6 +3522,149 @@ namespace portsorch_test _unhook_sai_switch_api(); } + TEST_F(PortsOrchTest, AutonegOnUnsupportedPort) + { + // Configuring autoneg on a port that doesn't support it (either on or off) + // must skip the ASIC call, mark the port configured so we don't retry, and + // not drop the rest of the task's config. The requested value is mirrored + // in cache. + _hook_sai_port_api(); + mock_autoneg_cap_supported = false; + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + gPortsOrch->addExistingData(&portTable); + static_cast(gPortsOrch)->doTask(); + + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + + for (const auto &requested : {std::string("off"), std::string("on")}) + { + const auto an_call_count = _sai_set_port_auto_neg_count; + + // Reset the port's AN-configured flag so each iteration re-enters the AN block + Port pre; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", pre)); + pre.m_an_cfg = false; + gPortsOrch->m_portList["Ethernet0"] = pre; + + std::deque entries; + entries.push_back({"Ethernet0", SET_COMMAND, { { "autoneg", requested } }}); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + + // No SAI set call should have been made + EXPECT_EQ(_sai_set_port_auto_neg_count, an_call_count) << "requested=" << requested; + + // Port marked configured; requested value reflected in cache + Port p; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p)); + EXPECT_TRUE(p.m_an_cfg) << "requested=" << requested; + EXPECT_EQ(p.m_autoneg, requested == "on") << "requested=" << requested; + + // Task consumed (no pending) — sibling config in the same task is no longer dropped + vector ts; + gPortsOrch->dumpPendingTasks(ts); + EXPECT_TRUE(ts.empty()) << "requested=" << requested; + } + + mock_autoneg_cap_supported = true; + _unhook_sai_port_api(); + } + + TEST_F(PortsOrchTest, AutonegAdminDownFailure) + { + // When autoneg is configured on a port that is admin UP and bringing the + // port DOWN fails, the task must be retried (it++, not erased). + _hook_sai_port_api(); + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + gPortsOrch->addExistingData(&portTable); + static_cast(gPortsOrch)->doTask(); + + // Force Ethernet0 into admin UP state + Port p; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p)); + p.m_admin_state_up = true; + gPortsOrch->m_portList["Ethernet0"] = p; + + set_admin_status_fail = true; + + std::deque entries; + entries.push_back({"Ethernet0", SET_COMMAND, + { + { "autoneg", "on" } + }}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + + EXPECT_EQ(set_admin_status_failures, 1); + + // Task should remain queued for retry + vector ts; + gPortsOrch->dumpPendingTasks(ts); + EXPECT_FALSE(ts.empty()); + + set_admin_status_fail = false; + set_admin_status_failures = 0; + _unhook_sai_port_api(); + } + + TEST_F(PortsOrchTest, AutonegSetNeedsRetry) + { + // When setPortAutoNeg returns task_need_retry (SAI_STATUS_INSUFFICIENT_RESOURCES), + // the task must stay queued (it++) rather than being dropped. + _hook_sai_port_api(); + set_autoneg_need_retry = true; + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + gPortsOrch->addExistingData(&portTable); + static_cast(gPortsOrch)->doTask(); + + const auto an_call_count = _sai_set_port_auto_neg_count; + + std::deque entries; + entries.push_back({"Ethernet0", SET_COMMAND, + { + { "autoneg", "on" } + }}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + + // SAI set was attempted + EXPECT_GT(_sai_set_port_auto_neg_count, an_call_count); + + // Task should remain queued for retry + vector ts; + gPortsOrch->dumpPendingTasks(ts); + EXPECT_FALSE(ts.empty()); + + set_autoneg_need_retry = false; + _unhook_sai_port_api(); + } + TEST_F(PortsOrchTest, PortReadinessColdBoot) { Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);