Skip to content

Commit

Permalink
[portsorch] Avoid orchagent crash when set invalid interface types to…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@Junchao-Mellanox
Junchao-Mellanox committed Sep 20, 2021
1 parent 025032f commit 86b4ede
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 58 deletions.
28 changes: 24 additions & 4 deletions orchagent/orch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
124 changes: 75 additions & 49 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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<sai_uint32_t>& speed_list)
task_process_status PortsOrch::setPortAdvSpeeds(sai_object_id_t port_id, std::vector<sai_uint32_t>& speed_list)
{
SWSS_LOG_ENTER();
sai_attribute_t attr;
Expand All @@ -1984,11 +1980,15 @@ bool PortsOrch::setPortAdvSpeeds(sai_object_id_t port_id, std::vector<sai_uint32
attr.value.u32list.count = static_cast<uint32_t>(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;
Expand All @@ -1998,11 +1998,15 @@ bool PortsOrch::setPortInterfaceType(sai_object_id_t port_id, sai_port_interface
attr.value.u32 = static_cast<uint32_t>(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<uint32_t> &interface_types)
task_process_status PortsOrch::setPortAdvInterfaceTypes(sai_object_id_t port_id, std::vector<uint32_t> &interface_types)
{
SWSS_LOG_ENTER();
sai_attribute_t attr;
Expand All @@ -2015,14 +2019,10 @@ bool PortsOrch::setPortAdvInterfaceTypes(sai_object_id_t port_id, std::vector<ui
status = sai_port_api->set_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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
10 changes: 5 additions & 5 deletions orchagent/portsorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<sai_uint32_t>& speed_list);
task_process_status setPortAdvSpeeds(sai_object_id_t port_id, std::vector<sai_uint32_t>& speed_list);

bool getQueueTypeAndIndex(sai_object_id_t queue_id, string &type, uint8_t &index);

Expand All @@ -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<uint32_t> &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<uint32_t> &interface_types);

bool getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const;
void updatePortOperStatus(Port &port, sai_port_oper_status_t status);
Expand Down

0 comments on commit 86b4ede

Please sign in to comment.