[portsorch] Avoid orchagent crash when set invalid interface types to port #1906
Conversation
| @@ -2836,7 +2844,7 @@ void PortsOrch::doPortTask(Consumer &consumer) | |||
| else | |||
| { | |||
| SWSS_LOG_ERROR("Failed to set port %s AN from %d to %d", alias.c_str(), p.m_autoneg, an); | |||
| it++; | |||
There was a problem hiding this comment.
Do we need to make this change?
| @@ -2872,7 +2880,7 @@ void PortsOrch::doPortTask(Consumer &consumer) | |||
| if (!setPortSpeed(p, speed)) | |||
| { | |||
| SWSS_LOG_ERROR("Failed to set port %s speed from %u to %u", alias.c_str(), p.m_speed, speed); | |||
| it++; | |||
There was a problem hiding this comment.
Do we need to make this change?
| @@ -2920,7 +2928,7 @@ void PortsOrch::doPortTask(Consumer &consumer) | |||
| 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++; | |||
There was a problem hiding this comment.
Do we need to make this change?
| @@ -2960,7 +2968,7 @@ void PortsOrch::doPortTask(Consumer &consumer) | |||
| if (!setPortInterfaceType(p.m_port_id, interface_type)) | |||
| { | |||
| SWSS_LOG_ERROR("Failed to set port %s interface type to %s", alias.c_str(), interface_type_str.c_str()); | |||
| it++; | |||
There was a problem hiding this comment.
Do we need to make this change?
| @@ -2999,7 +3007,7 @@ void PortsOrch::doPortTask(Consumer &consumer) | |||
| if (!setPortAdvInterfaceTypes(p.m_port_id, adv_interface_types)) | |||
| { | |||
| SWSS_LOG_ERROR("Failed to set port %s advertised interface type to %s", alias.c_str(), adv_interface_types_str.c_str()); | |||
| it++; | |||
There was a problem hiding this comment.
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.
|
@shi-su I have fixed all comments, could you please review again? |
|
The test failure is not related to the PR. |
|
Updated description with error message |
|
Hi @judyjoseph, could you help cherry-pick to 202106? |
… 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
… port (sonic-net#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
What I did get_port_config was modified to return different set number of arguments in PR: sonic-net/sonic-buildimage#4222. Changes in this PR is: To address the different set of return values To get the ports from all namespaces To add unit-test How I did it Modify port2alias to read ports from all namespaces and also add unit-test How to verify it Test on single and multi-asic platforms. unit-test passed.
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. Error log see:
How I verified it
Manual test
Details if related