From dc8bc1c40f647cc2acf2c21431c4f58c6ccd8ff3 Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Sun, 24 Jul 2022 15:20:05 +0800 Subject: [PATCH] [portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it (#2333) - What I did Expose supported FEC modes to STATE_DB.PORT_TABLE|.supported_fecs. The orchagent calls get_port_attribute to get attribute SAI_PORT_ATTR_SUPPORTED_FEC_MODE during initialization and then records it into internal data. 1. By default, the supported FEC modes will be returned by SAI and exposed to STATE_DB. Eg. rs,none means only rs and none is supported on the port. The orchagent will check whether the FEC mode is supported on the port before calling the SAI API to set it. The CLI will check whether the FEC mode is in supported_fecs before setting FEC mode on a port to the CONFIG_DB 2. In case the SAI API does not support any FEC mode on the port, N/A will be exposed to STATE_DB The orchagent will deny any FEC setting and prints log. The CLI will deny FEC setting. 3. In case the SAI API get_port_attribute returns Not implemented No check will be performed before setting a FEC mode to SAI on the port. The CLI will check whether the FEC mode is defined before setting it to CONFIG_DB. - Why I did it It is not supported to set FEC mode on some platforms. To avoid error, we need to expose the supported FEC list. - How I verified it Manually test and mock test. --- orchagent/portsorch.cpp | 119 ++++++++++++++-- orchagent/portsorch.h | 8 +- tests/mock_tests/portsorch_ut.cpp | 227 ++++++++++++++++++++++++++++++ 3 files changed, 342 insertions(+), 12 deletions(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index e9a3afdc1e80..fd96d6a3c204 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -77,6 +77,13 @@ static map fec_mode_map = { "fc", SAI_PORT_FEC_MODE_FC } }; +static map fec_mode_reverse_map = +{ + { SAI_PORT_FEC_MODE_NONE, "none" }, + { SAI_PORT_FEC_MODE_RS, "rs" }, + { SAI_PORT_FEC_MODE_FC, "fc" } +}; + static map pfc_asym_map = { { "on", SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE }, @@ -1188,19 +1195,31 @@ bool PortsOrch::setPortTpid(sai_object_id_t id, sai_uint16_t tpid) return true; } - -bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode) +bool PortsOrch::setPortFec(Port &port, string &mode) { SWSS_LOG_ENTER(); + auto searchRef = m_portSupportedFecModes.find(port.m_port_id); + if (searchRef != m_portSupportedFecModes.end()) + { + auto &supportedFecModes = searchRef->second; + if (!supportedFecModes.empty() && (supportedFecModes.find(mode) == supportedFecModes.end())) + { + SWSS_LOG_ERROR("Unsupported mode %s on port %s", mode.c_str(), port.m_alias.c_str()); + // We return true becase the caller will keep the item in m_toSync and retry it later if we return false + // As the FEC mode is not supported it doesn't make sense to retry. + return true; + } + } + sai_attribute_t attr; attr.id = SAI_PORT_ATTR_FEC_MODE; - attr.value.s32 = mode; + attr.value.s32 = port.m_fec_mode; sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id); + SWSS_LOG_ERROR("Failed to set FEC mode %s to port %s", mode.c_str(), port.m_alias.c_str()); task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status); if (handle_status != task_success) { @@ -1208,7 +1227,7 @@ bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode) } } - SWSS_LOG_INFO("Set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id); + SWSS_LOG_NOTICE("Set port %s FEC mode %s", port.m_alias.c_str(), mode.c_str()); setGearboxPortsAttr(port, SAI_PORT_ATTR_FEC_MODE, &mode); @@ -1985,6 +2004,87 @@ void PortsOrch::initPortSupportedSpeeds(const std::string& alias, sai_object_id_ m_portStateTable.set(alias, v); } +void PortsOrch::getPortSupportedFecModes(const std::string& alias, sai_object_id_t port_id, PortSupportedFecModes &supported_fecmodes) +{ + sai_attribute_t attr; + sai_status_t status; + vector fecModes(fec_mode_reverse_map.size()); + + attr.id = SAI_PORT_ATTR_SUPPORTED_FEC_MODE; + attr.value.s32list.count = static_cast(fecModes.size()); + attr.value.s32list.list = fecModes.data(); + + status = sai_port_api->get_port_attribute(port_id, 1, &attr); + fecModes.resize(attr.value.s32list.count); + if (status == SAI_STATUS_SUCCESS) + { + if (fecModes.empty()) + { + supported_fecmodes.insert("N/A"); + } + else + { + for(auto fecMode : fecModes) + { + supported_fecmodes.insert(fec_mode_reverse_map[static_cast(fecMode)]); + } + } + } + else + { + if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || + SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) || + status == SAI_STATUS_NOT_IMPLEMENTED) + { + // unable to validate FEC mode if attribute is not supported on platform + SWSS_LOG_NOTICE("Unable to validate FEC mode for port %s id=%" PRIx64 " due to unsupported by platform", + alias.c_str(), port_id); + } + else + { + SWSS_LOG_ERROR("Failed to get a list of supported FEC modes for port %s id=%" PRIx64 ". Error=%d", + alias.c_str(), port_id, status); + } + + supported_fecmodes.clear(); // return empty + } +} + +void PortsOrch::initPortSupportedFecModes(const std::string& alias, sai_object_id_t port_id) +{ + // If port supported speeds map already contains the information, save the SAI call + if (m_portSupportedFecModes.count(port_id)) + { + return; + } + PortSupportedFecModes supported_fec_modes; + getPortSupportedFecModes(alias, port_id, supported_fec_modes); + m_portSupportedFecModes[port_id] = supported_fec_modes; + + if (supported_fec_modes.empty()) + { + // Do not expose "supported_fecs" in case fetching FEC modes is not supported by the vendor + SWSS_LOG_INFO("No supported_fecs exposed to STATE_DB for port %s since fetching supported FEC modes is not supported by the vendor", + alias.c_str()); + return; + } + + vector v; + std::string supported_fec_modes_str; + bool first = true; + for(auto fec : supported_fec_modes) + { + if (first) + first = false; + else + supported_fec_modes_str += ','; + supported_fec_modes_str += fec; + } + + v.emplace_back(std::make_pair("supported_fecs", supported_fec_modes_str)); + m_portStateTable.set(alias, v); +} + /* * If Gearbox is enabled and this is a Gearbox port then set the attributes accordingly. */ @@ -2978,6 +3078,7 @@ void PortsOrch::doPortTask(Consumer &consumer) } initPortSupportedSpeeds(get<0>(it->second), m_portListLaneMap[it->first]); + initPortSupportedFecModes(get<0>(it->second), m_portListLaneMap[it->first]); it++; } @@ -3326,14 +3427,12 @@ void PortsOrch::doPortTask(Consumer &consumer) p.m_fec_mode = fec_mode_map[fec_mode]; p.m_fec_cfg = true; - if (setPortFec(p, p.m_fec_mode)) + if (setPortFec(p, fec_mode)) { m_portList[alias] = p; - SWSS_LOG_NOTICE("Set port %s fec to %s", alias.c_str(), fec_mode.c_str()); } else { - SWSS_LOG_ERROR("Failed to set port %s fec to %s", alias.c_str(), fec_mode.c_str()); it++; continue; } @@ -3343,14 +3442,12 @@ void PortsOrch::doPortTask(Consumer &consumer) /* Port is already down, setting fec mode*/ p.m_fec_mode = fec_mode_map[fec_mode]; p.m_fec_cfg = true; - if (setPortFec(p, p.m_fec_mode)) + if (setPortFec(p, fec_mode)) { m_portList[alias] = p; - SWSS_LOG_NOTICE("Set port %s fec to %s", alias.c_str(), fec_mode.c_str()); } else { - SWSS_LOG_ERROR("Failed to set port %s fec to %s", alias.c_str(), fec_mode.c_str()); it++; continue; } diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 0fd3552e1982..a3413790b14c 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -27,6 +27,7 @@ #define PG_DROP_STAT_COUNTER_FLEX_COUNTER_GROUP "PG_DROP_STAT_COUNTER" typedef std::vector PortSupportedSpeeds; +typedef std::set PortSupportedFecModes; static const map oper_status_strings = { @@ -209,6 +210,8 @@ class PortsOrch : public Orch, public Subject unique_ptr m_gbcounterTable; std::map m_portSupportedSpeeds; + // Supported FEC modes on the system side. + std::map m_portSupportedFecModes; bool m_initDone = false; Port m_cpuPort; @@ -305,7 +308,7 @@ class PortsOrch : public Orch, public Subject bool setPortTpid(sai_object_id_t id, sai_uint16_t tpid); bool setPortPvid (Port &port, sai_uint32_t pvid); bool getPortPvid(Port &port, sai_uint32_t &pvid); - bool setPortFec(Port &port, sai_port_fec_mode_t mode); + bool setPortFec(Port &port, std::string &mode); bool setPortPfcAsym(Port &port, string pfc_asym); bool getDestPortId(sai_object_id_t src_port_id, dest_port_type_t port_type, sai_object_id_t &des_port_id); @@ -314,6 +317,9 @@ class PortsOrch : public Orch, public Subject bool isSpeedSupported(const std::string& alias, sai_object_id_t port_id, sai_uint32_t speed); void getPortSupportedSpeeds(const std::string& alias, sai_object_id_t port_id, PortSupportedSpeeds &supported_speeds); void initPortSupportedSpeeds(const std::string& alias, sai_object_id_t port_id); + // Get supported FEC modes on system side + void getPortSupportedFecModes(const std::string& alias, sai_object_id_t port_id, PortSupportedFecModes &supported_fecmodes); + void initPortSupportedFecModes(const std::string& alias, sai_object_id_t port_id); task_process_status setPortSpeed(Port &port, sai_uint32_t speed); bool getPortSpeed(sai_object_id_t id, sai_uint32_t &speed); bool setGearboxPortsAttr(Port &port, sai_port_attr_t id, void *value); diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 78c633d4a1cf..93e73d904117 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -20,6 +20,70 @@ namespace portsorch_test using namespace std; + sai_port_api_t ut_sai_port_api; + sai_port_api_t *pold_sai_port_api; + + bool not_support_fetching_fec; + vector mock_port_fec_modes = {SAI_PORT_FEC_MODE_RS, SAI_PORT_FEC_MODE_FC}; + + sai_status_t _ut_stub_sai_get_port_attribute( + _In_ sai_object_id_t port_id, + _In_ uint32_t attr_count, + _Inout_ sai_attribute_t *attr_list) + { + sai_status_t status; + if (attr_count == 1 && attr_list[0].id == SAI_PORT_ATTR_SUPPORTED_FEC_MODE) + { + if (not_support_fetching_fec) + { + status = SAI_STATUS_NOT_IMPLEMENTED; + } + else + { + uint32_t i; + for (i = 0; i < attr_list[0].value.s32list.count && i < mock_port_fec_modes.size(); i++) + { + attr_list[0].value.s32list.list[i] = mock_port_fec_modes[i]; + } + attr_list[0].value.s32list.count = i; + status = SAI_STATUS_SUCCESS; + } + } + else + { + status = pold_sai_port_api->get_port_attribute(port_id, attr_count, attr_list); + } + return status; + } + + uint32_t _sai_set_port_fec_count; + int32_t _sai_port_fec_mode; + sai_status_t _ut_stub_sai_set_port_attribute( + _In_ sai_object_id_t port_id, + _In_ const sai_attribute_t *attr) + { + if (attr[0].id == SAI_PORT_ATTR_FEC_MODE) + { + _sai_set_port_fec_count++; + _sai_port_fec_mode = attr[0].value.s32; + } + return pold_sai_port_api->set_port_attribute(port_id, attr); + } + + void _hook_sai_port_api() + { + ut_sai_port_api = *sai_port_api; + pold_sai_port_api = sai_port_api; + ut_sai_port_api.get_port_attribute = _ut_stub_sai_get_port_attribute; + ut_sai_port_api.set_port_attribute = _ut_stub_sai_set_port_attribute; + sai_port_api = &ut_sai_port_api; + } + + void _unhook_sai_port_api() + { + sai_port_api = pold_sai_port_api; + } + struct PortsOrchTest : public ::testing::Test { shared_ptr m_app_db; @@ -173,6 +237,169 @@ namespace portsorch_test }; + TEST_F(PortsOrchTest, PortSupportedFecModes) + { + _hook_sai_port_api(); + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + std::deque entries; + + not_support_fetching_fec = false; + // 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(); + + uint32_t current_sai_api_call_count = _sai_set_port_fec_count; + + entries.push_back({"Ethernet0", "SET", + { + {"fec", "rs"} + }}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + entries.clear(); + + ASSERT_EQ(_sai_set_port_fec_count, ++current_sai_api_call_count); + ASSERT_EQ(_sai_port_fec_mode, SAI_PORT_FEC_MODE_RS); + + vector ts; + + gPortsOrch->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + + entries.push_back({"Ethernet0", "SET", + { + {"fec", "none"} + }}); + consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + + ASSERT_EQ(_sai_set_port_fec_count, current_sai_api_call_count); + ASSERT_EQ(_sai_port_fec_mode, SAI_PORT_FEC_MODE_RS); + + gPortsOrch->dumpPendingTasks(ts); + ASSERT_EQ(ts.size(), 0); + + _unhook_sai_port_api(); + } + + /* + * Test case: SAI_PORT_ATTR_SUPPORTED_FEC_MODE is not supported by vendor + **/ + TEST_F(PortsOrchTest, PortNotSupportedFecModes) + { + _hook_sai_port_api(); + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + std::deque entries; + + not_support_fetching_fec = true; + // 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(); + + uint32_t current_sai_api_call_count = _sai_set_port_fec_count; + + entries.push_back({"Ethernet0", "SET", + { + {"fec", "rs"} + }}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + entries.clear(); + + ASSERT_EQ(_sai_set_port_fec_count, ++current_sai_api_call_count); + ASSERT_EQ(_sai_port_fec_mode, SAI_PORT_FEC_MODE_RS); + + vector ts; + + gPortsOrch->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + + _unhook_sai_port_api(); + } + + /* + * Test case: Fetching SAI_PORT_ATTR_SUPPORTED_FEC_MODE is supported but no FEC mode is supported on the port + **/ + TEST_F(PortsOrchTest, PortSupportNoFecModes) + { + _hook_sai_port_api(); + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + std::deque entries; + + not_support_fetching_fec = false; + auto old_mock_port_fec_modes = mock_port_fec_modes; + mock_port_fec_modes.clear(); + // 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(); + + uint32_t current_sai_api_call_count = _sai_set_port_fec_count; + + entries.push_back({"Ethernet0", "SET", + { + {"fec", "rs"} + }}); + auto consumer = dynamic_cast(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gPortsOrch)->doTask(); + entries.clear(); + + ASSERT_EQ(_sai_set_port_fec_count, current_sai_api_call_count); + + vector ts; + + gPortsOrch->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + + mock_port_fec_modes = old_mock_port_fec_modes; + _unhook_sai_port_api(); + } + TEST_F(PortsOrchTest, PortReadinessColdBoot) { Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);