From 86b4ede8818b3917958318de6f0116399a47d6e7 Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Tue, 21 Sep 2021 02:24:13 +0800 Subject: [PATCH] [portsorch] Avoid orchagent crash when set invalid interface types to port (#1906) What I did When setting invalid interface types to port, do not call handleSaiSetStatus to avoid orchagent exit. Why I did it Currently, when setting invalid interface types to port, orchagent would crash, this is not necessary since user still has chance to correct it --- orchagent/orch.cpp | 28 +++++++-- orchagent/portsorch.cpp | 124 ++++++++++++++++++++++++---------------- orchagent/portsorch.h | 10 ++-- 3 files changed, 104 insertions(+), 58 deletions(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index b4ef24666238..5cc9ac03351c 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -748,16 +748,36 @@ task_process_status Orch::handleSaiSetStatus(sai_api_t api, sai_status_t status, * in each orch. * 3. Take the type of sai api into consideration. */ - switch (status) + if (status == SAI_STATUS_SUCCESS) { - case SAI_STATUS_SUCCESS: - SWSS_LOG_WARN("SAI_STATUS_SUCCESS is not expected in handleSaiSetStatus"); - return task_success; + SWSS_LOG_WARN("SAI_STATUS_SUCCESS is not expected in handleSaiSetStatus"); + return task_success; + } + + switch (api) + { + case SAI_API_PORT: + switch (status) + { + case SAI_STATUS_INVALID_ATTR_VALUE_0: + /* + * If user gives an invalid attribute value, no need to retry or exit orchagent, just fail the current task + * and let user correct the configuration. + */ + SWSS_LOG_ERROR("Encountered SAI_STATUS_INVALID_ATTR_VALUE_0 in set operation, task failed, SAI API: %s, status: %s", + sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); + return task_failed; + default: + SWSS_LOG_ERROR("Encountered failure in set operation, exiting orchagent, SAI API: %s, status: %s", + sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); + exit(EXIT_FAILURE); + } default: SWSS_LOG_ERROR("Encountered failure in set operation, exiting orchagent, SAI API: %s, status: %s", sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); exit(EXIT_FAILURE); } + return task_need_retry; } diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 76babdf24d20..68028e2c219d 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -1919,7 +1919,7 @@ bool PortsOrch::setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_p return true; } -bool PortsOrch::setPortSpeed(Port &port, sai_uint32_t speed) +task_process_status PortsOrch::setPortSpeed(Port &port, sai_uint32_t speed) { sai_attribute_t attr; sai_status_t status; @@ -1932,15 +1932,11 @@ bool PortsOrch::setPortSpeed(Port &port, sai_uint32_t speed) status = sai_port_api->set_port_attribute(port.m_port_id, &attr); if (status != SAI_STATUS_SUCCESS) { - task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + return handleSaiSetStatus(SAI_API_PORT, status); } setGearboxPortsAttr(port, SAI_PORT_ATTR_SPEED, &speed); - return true; + return task_success; } bool PortsOrch::getPortSpeed(sai_object_id_t id, sai_uint32_t &speed) @@ -1973,7 +1969,7 @@ bool PortsOrch::getPortSpeed(sai_object_id_t id, sai_uint32_t &speed) return true; } -bool PortsOrch::setPortAdvSpeeds(sai_object_id_t port_id, std::vector& speed_list) +task_process_status PortsOrch::setPortAdvSpeeds(sai_object_id_t port_id, std::vector& speed_list) { SWSS_LOG_ENTER(); sai_attribute_t attr; @@ -1984,11 +1980,15 @@ bool PortsOrch::setPortAdvSpeeds(sai_object_id_t port_id, std::vector(speed_list.size()); status = sai_port_api->set_port_attribute(port_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + return handleSaiSetStatus(SAI_API_PORT, status); + } - return status == SAI_STATUS_SUCCESS; + return task_success; } -bool PortsOrch::setPortInterfaceType(sai_object_id_t port_id, sai_port_interface_type_t interface_type) +task_process_status PortsOrch::setPortInterfaceType(sai_object_id_t port_id, sai_port_interface_type_t interface_type) { SWSS_LOG_ENTER(); sai_attribute_t attr; @@ -1998,11 +1998,15 @@ bool PortsOrch::setPortInterfaceType(sai_object_id_t port_id, sai_port_interface attr.value.u32 = static_cast(interface_type); status = sai_port_api->set_port_attribute(port_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + return handleSaiSetStatus(SAI_API_PORT, status); + } - return status == SAI_STATUS_SUCCESS; + return task_success; } -bool PortsOrch::setPortAdvInterfaceTypes(sai_object_id_t port_id, std::vector &interface_types) +task_process_status PortsOrch::setPortAdvInterfaceTypes(sai_object_id_t port_id, std::vector &interface_types) { SWSS_LOG_ENTER(); sai_attribute_t attr; @@ -2015,14 +2019,10 @@ bool PortsOrch::setPortAdvInterfaceTypes(sai_object_id_t port_id, std::vectorset_port_attribute(port_id, &attr); if (status != SAI_STATUS_SUCCESS) { - task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + return handleSaiSetStatus(SAI_API_PORT, status); } - return true; + return task_success; } bool PortsOrch::getQueueTypeAndIndex(sai_object_id_t queue_id, string &type, uint8_t &index) @@ -2065,33 +2065,22 @@ bool PortsOrch::getQueueTypeAndIndex(sai_object_id_t queue_id, string &type, uin return true; } -bool PortsOrch::setPortAutoNeg(sai_object_id_t id, int an) +task_process_status PortsOrch::setPortAutoNeg(sai_object_id_t id, int an) { SWSS_LOG_ENTER(); sai_attribute_t attr; attr.id = SAI_PORT_ATTR_AUTO_NEG_MODE; - switch(an) { - case 1: - attr.value.booldata = true; - break; - default: - attr.value.booldata = false; - break; - } + attr.value.booldata = (an == 1 ? true : false); sai_status_t status = sai_port_api->set_port_attribute(id, &attr); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set AutoNeg %u to port pid:%" PRIx64, attr.value.booldata, id); - task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + return handleSaiSetStatus(SAI_API_PORT, status); } SWSS_LOG_INFO("Set AutoNeg %u to port pid:%" PRIx64, attr.value.booldata, id); - return true; + return task_success; } bool PortsOrch::setHostIntfsOperStatus(const Port& port, bool isUp) const @@ -2827,18 +2816,23 @@ void PortsOrch::doPortTask(Consumer &consumer) m_portList[alias] = p; } - if (setPortAutoNeg(p.m_port_id, an)) - { - SWSS_LOG_NOTICE("Set port %s AutoNeg from %d to %d", alias.c_str(), p.m_autoneg, an); - p.m_autoneg = an; - m_portList[alias] = p; - } - else + auto status = setPortAutoNeg(p.m_port_id, an); + if (status != task_success) { SWSS_LOG_ERROR("Failed to set port %s AN from %d to %d", alias.c_str(), p.m_autoneg, an); - it++; + if (status == task_need_retry) + { + it++; + } + else + { + it = consumer.m_toSync.erase(it); + } continue; } + SWSS_LOG_NOTICE("Set port %s AutoNeg from %d to %d", alias.c_str(), p.m_autoneg, an); + p.m_autoneg = an; + m_portList[alias] = p; } } @@ -2869,10 +2863,18 @@ void PortsOrch::doPortTask(Consumer &consumer) m_portList[alias] = p; } - if (!setPortSpeed(p, speed)) + auto status = setPortSpeed(p, speed); + if (status != task_success) { SWSS_LOG_ERROR("Failed to set port %s speed from %u to %u", alias.c_str(), p.m_speed, speed); - it++; + if (status == task_need_retry) + { + it++; + } + else + { + it = consumer.m_toSync.erase(it); + } continue; } @@ -2914,13 +2916,21 @@ void PortsOrch::doPortTask(Consumer &consumer) } auto ori_adv_speeds = swss::join(',', p.m_adv_speeds.begin(), p.m_adv_speeds.end()); - if (!setPortAdvSpeeds(p.m_port_id, adv_speeds)) + auto status = setPortAdvSpeeds(p.m_port_id, adv_speeds); + if (status != task_success) { SWSS_LOG_ERROR("Failed to set port %s advertised speed from %s to %s", alias.c_str(), ori_adv_speeds.c_str(), adv_speeds_str.c_str()); - it++; + if (status == task_need_retry) + { + it++; + } + else + { + it = consumer.m_toSync.erase(it); + } continue; } SWSS_LOG_NOTICE("Set port %s advertised speed from %s to %s", alias.c_str(), @@ -2957,10 +2967,18 @@ void PortsOrch::doPortTask(Consumer &consumer) m_portList[alias] = p; } - if (!setPortInterfaceType(p.m_port_id, interface_type)) + auto status = setPortInterfaceType(p.m_port_id, interface_type); + if (status != task_success) { SWSS_LOG_ERROR("Failed to set port %s interface type to %s", alias.c_str(), interface_type_str.c_str()); - it++; + if (status == task_need_retry) + { + it++; + } + else + { + it = consumer.m_toSync.erase(it); + } continue; } @@ -2996,10 +3014,18 @@ void PortsOrch::doPortTask(Consumer &consumer) m_portList[alias] = p; } - if (!setPortAdvInterfaceTypes(p.m_port_id, adv_interface_types)) + auto status = setPortAdvInterfaceTypes(p.m_port_id, adv_interface_types); + if (status != task_success) { SWSS_LOG_ERROR("Failed to set port %s advertised interface type to %s", alias.c_str(), adv_interface_types_str.c_str()); - it++; + if (status == task_need_retry) + { + it++; + } + else + { + it = consumer.m_toSync.erase(it); + } continue; } diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 22efce3561dd..9ae26242ce6c 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -278,12 +278,12 @@ 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); - bool setPortSpeed(Port &port, sai_uint32_t speed); + 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); bool setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value); - bool setPortAdvSpeeds(sai_object_id_t port_id, std::vector& speed_list); + task_process_status setPortAdvSpeeds(sai_object_id_t port_id, std::vector& speed_list); bool getQueueTypeAndIndex(sai_object_id_t queue_id, string &type, uint8_t &index); @@ -296,10 +296,10 @@ class PortsOrch : public Orch, public Subject bool m_isPortCounterMapGenerated = false; bool m_isPortBufferDropCounterMapGenerated = false; - bool setPortAutoNeg(sai_object_id_t id, int an); + task_process_status setPortAutoNeg(sai_object_id_t id, int an); bool setPortFecMode(sai_object_id_t id, int fec); - bool setPortInterfaceType(sai_object_id_t id, sai_port_interface_type_t interface_type); - bool setPortAdvInterfaceTypes(sai_object_id_t id, std::vector &interface_types); + task_process_status setPortInterfaceType(sai_object_id_t id, sai_port_interface_type_t interface_type); + task_process_status setPortAdvInterfaceTypes(sai_object_id_t id, std::vector &interface_types); bool getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const; void updatePortOperStatus(Port &port, sai_port_oper_status_t status);