Skip to content

[portsorch] Avoid orchagent crash when set invalid interface types to port #1906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shi-su merged 4 commits into from
Sep 20, 2021
Merged

[portsorch] Avoid orchagent crash when set invalid interface types to port #1906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9acc77
[portsorch] Avoid orchagent crash when set invalid interface types to…
Junchao-Mellanox Sep 15, 2021
89f5532
Fix review comment
Junchao-Mellanox Sep 16, 2021
1b109dc
Fix review comments
Junchao-Mellanox Sep 17, 2021
d508e02
Fix issue found in test
Junchao-Mellanox Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions orchagent/orch.cpp
View file
Open in desktop
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
Open in desktop
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++;
Copy link
Copy Markdown
Contributor

@shi-su shi-su Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this change?

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++;
Copy link
Copy Markdown
Contributor

@shi-su shi-su Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this change?

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++;
Copy link
Copy Markdown
Contributor

@shi-su shi-su Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this change?

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++;
Copy link
Copy Markdown
Contributor

@shi-su shi-su Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this change?

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++;
Copy link
Copy Markdown
Contributor

@shi-su shi-su Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the reason to make this change is to avoid retrying. However, I think the problem is we typically treat false return value as need retry and this behavior deviate from the expected one. I am not sure what would be the best solution, I think a reasonable one could be making the return value type of setPortAdvInterfaceTypes to task_process_status. And adding the status of SAI_STATUS_INVALID_ATTR_VALUE_0 for SAI_API_PORT as a special case to return task_failed in the failure handling function like #1815. Then we can make the return value of task_process_status type from handleSaiSetStatus as the return value of setPortAdvInterfaceTypes in case of SAI failure. It follows that we can choose whether to retry here.

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
Open in desktop
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