Add failure notification for orchagent #1665

shi-su · 2021-03-08T21:20:02Z

What I did
Add SAI failure handling functions in aclorch, bufferorch, copporch, dtelorch, fdborch, fgnhgorch, intfsorch, mirrororch, natorch, policerorch, macsecorch, portsorch, qosorch, sfloworch, switchorch, tunneldecaporch, vrforch.

Why I did it
Failure notification mechanism to ensure enough notifications in the presence of SAI failures and avoid running switches with unhandled failures.

How I verified it

Details if related

lgtm-com · 2021-03-08T22:30:40Z

This pull request fixes 2 alerts when merging 5de44a6 into 03a0e21 - view on LGTM.com

fixed alerts:

2 for FIXME comment

lgtm-com · 2021-03-10T22:57:01Z

This pull request fixes 2 alerts when merging eaed258 into 9e30abb - view on LGTM.com

fixed alerts:

2 for FIXME comment

lgtm-com · 2021-03-12T20:03:20Z

This pull request fixes 2 alerts when merging 625dfbb into 1951365 - view on LGTM.com

fixed alerts:

2 for FIXME comment

qiluo-msft · 2021-03-16T23:30:02Z

orchagent/aclorch.cpp

@@ -3596,6 +3596,7 @@ sai_status_t AclOrch::createDTelWatchListTables()
    if (status != SAI_STATUS_SUCCESS)
    {
        SWSS_LOG_ERROR("Failed to create table %s", flowWLTable.description.c_str());
+        handleSaiCreateStatus(SAI_API_ACL, status);


handleSaiCreateStatus [](start = 8, length = 21)

handleSaiCreateStatus [](start = 8, length = 21)

Check the return value? If successfully handled, you treat it as success? #Closed

There are a few SAI calls that do not take any action in the case of SAI failures. For these ones, since our goal is to make it on-par with async mode at this stage, I am not checking the return value for now so that it has the same behavior as before.

Let's assume this function magically retry and fix the issue, so the return value is task_success. You should return SAI_STATUS_SUCCESS after this function.

In reply to: 595609272 [](ancestors = 595609272)

Agree, updated the logic to check if the return value is task_success, and proceed when it is.

qiluo-msft · 2021-03-16T23:32:57Z

orchagent/copporch.cpp

@@ -125,7 +125,7 @@ void CoppOrch::initDefaultHostIntfTable()
    if (status != SAI_STATUS_SUCCESS)
    {
        SWSS_LOG_ERROR("Failed to create default host interface table, rv:%d", status);
-        throw "CoppOrch initialization failure";
+        handleSaiCreateStatus(SAI_API_HOSTIF, status);


handleSaiCreateStatus [](start = 8, length = 21)

handleSaiCreateStatus [](start = 8, length = 21)

Do you want to keep old behavior even with your new handle function? #Closed

I think in the old behavior (async mode), this throw should not even be called since syncd should already trigger an swss restart before it reaches this throw. To mimic the old behavior close enough, I think a method could be running this handling function before this throw and keep this throw regardless of the return value of the handling function. I updated the code as such.

You also need to check return value and conditional throw.

In reply to: 595610233 [](ancestors = 595610233)

Updated the logic to check return value before throwing.

qiluo-msft · 2021-03-16T23:39:35Z

orchagent/copporch.cpp

@@ -489,6 +489,7 @@ task_process_status CoppOrch::processCoppRule(Consumer& consumer)
                if (sai_status != SAI_STATUS_SUCCESS)
                {
                    SWSS_LOG_ERROR("Failed to apply attribute:%d to trap group:%" PRIx64 ", name:%s, error:%d\n", trap_gr_attr.id, m_trap_group_map[trap_group_name], trap_group_name.c_str(), sai_status);
+                    handleSaiSetStatus(SAI_API_HOSTIF, sai_status);


handleSaiSetStatus [](start = 20, length = 18)

handleSaiSetStatus [](start = 20, length = 18)

Check the return value? #Closed

There are a few SAI calls that do not take any action in the case of SAI failures. For these ones, since our goal is to make it on-par with async mode at this stage, I am not checking the return value for now so that it has the same behavior as before.

Added a check for the return value of the handling function and return only when it is not task_success

qiluo-msft · 2021-03-16T23:51:53Z

orchagent/dtelorch.cpp

+                    }
+                    else
+                    {
+                        it++;


it++; [](start = 24, length = 5)

I think you change the behavior. Old code will always erase(it) no matter success or not. #Closed

Currently, the handling function always exits orchagent to achieve on-par handling with async mode. So this change will not be activated at the moment and the behavior should be the same as async mode. Once we have a better logic for the failure handling functions, we can fully control whether it should retry or not.

qiluo-msft · 2021-03-16T23:54:41Z

orchagent/fdborch.cpp

@@ -1097,7 +1098,7 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name,
            SWSS_LOG_ERROR("Failed to create %s FDB %s in %s on %s, rv:%d",
                    fdbData.type.c_str(), entry.mac.to_string().c_str(),
                    vlan.m_alias.c_str(), port_name.c_str(), status);
-            return false; //FIXME: it should be based on status. Some could be retried, some not


FIXME [](start = 28, length = 5)

Did you truly fixed the FIXME? #Closed

I did not, added the FIXME back.

qiluo-msft · 2021-03-16T23:54:53Z

orchagent/fdborch.cpp

@@ -1212,7 +1213,7 @@ bool FdbOrch::removeFdbEntry(const FdbEntry& entry, FdbOrigin origin)
    {
        SWSS_LOG_ERROR("FdbOrch RemoveFDBEntry: Failed to remove FDB entry. mac=%s, bv_id=0x%" PRIx64,
                       entry.mac.to_string().c_str(), entry.bv_id);
-        return true; //FIXME: it should be based on status. Some could be retried. some not


FIXME [](start = 23, length = 5)

The same. #Closed

Added the FIXME back.

qiluo-msft · 2021-03-17T00:46:42Z

orchagent/intfsorch.cpp

@@ -1056,7 +1061,7 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
    {
        SWSS_LOG_ERROR("Failed to create router interface %s, rv:%d",
                port.m_alias.c_str(), status);
-        throw runtime_error("Failed to create router interface.");
+        return handleSaiCreateStatus(SAI_API_ROUTER_INTERFACE, status);


handleSaiCreateStatus [](start = 15, length = 21)

handleSaiCreateStatus [](start = 15, length = 21)

To exactly mimic old behavior, you can derive the handleSaiCreateStatus function in this class, and throw inside the function. #Closed

I think in the old behavior (async mode), this throw should not even be called since syncd should already trigger an swss restart before it reaches this throw. To mimic the old behavior close enough, I think a method could be running this handling function before this throw and keep this throw regardless of the return value of the handling function. I updated the code as such.

Added a check for the return value of the handling function and throw exception only when it did not successfully handle the failure.

What I did Add SAI failure handling functions in aclorch, bufferorch, copporch, dtelorch, fdborch, fgnhgorch, intfsorch, mirrororch, natorch, policerorch, macsecorch, portsorch, qosorch, sfloworch, switchorch, tunneldecaporch, vrforch. Why I did it Failure notification mechanism to ensure enough notifications in the presence of SAI failures and avoid running switches with unhandled failures.

What I did Backport SAI failure handling related commits into the 202012 branch. The following is a list of backported commits: 941875a Deactivate mirror session only when session status is true in updateLagMember (#1666) be12482 Ignore ALREADY_EXIST error in FDB creation (#1815) c9c1aa2 Add failure handling for SAI get operations (#1768) 47b4276 [BufferOrch] Don't call SAI API for BUFFER_POOL/PROFILE handling in case the op is DEL and the SAI OID is NULL (#1786) db9238f Add failure notification for orchagent (#1665) fc8e43f [synchronous mode] Add failure notification for SAI failures in synchronous mode (#1596) Why I did it 202012 image needs to include failure handling mechanism for enough notification in the presence of SAI failures.

What I did Add SAI failure handling functions in aclorch, bufferorch, copporch, dtelorch, fdborch, fgnhgorch, intfsorch, mirrororch, natorch, policerorch, macsecorch, portsorch, qosorch, sfloworch, switchorch, tunneldecaporch, vrforch. Why I did it Failure notification mechanism to ensure enough notifications in the presence of SAI failures and avoid running switches with unhandled failures.

shi-su changed the base branch from master to bmtor March 12, 2021 18:54

shi-su changed the base branch from bmtor to master March 12, 2021 18:54

shi-su changed the title ~~Add failure notification for multiple orch~~ Add failure notification for orchagent Mar 15, 2021

shi-su requested a review from qiluo-msft March 15, 2021 16:53

qiluo-msft reviewed Mar 16, 2021

View reviewed changes

qiluo-msft reviewed Mar 17, 2021

View reviewed changes

shi-su added 6 commits March 22, 2021 22:24

Add failure notification for orchs

d4819cd

fix a typo

69f04dc

Add failure notification for other orchs

d91f9b4

undo a few changes

7e33a6d

use task_process_status as return value

b0e8db3

fix identation

2463004

shi-su force-pushed the on_par branch from 8d27970 to 2463004 Compare March 22, 2021 22:25

shi-su added 2 commits March 24, 2021 17:15

Add handling functions before throw

b369718

remove returns

3258527

shi-su requested a review from qiluo-msft March 24, 2021 17:25

address review comments

662acef

qiluo-msft approved these changes Mar 31, 2021

View reviewed changes

shi-su marked this pull request as ready for review March 31, 2021 03:28

shi-su merged commit fa983d2 into sonic-net:master Mar 31, 2021

Blueve mentioned this pull request Apr 9, 2021

[HLX] Regression: syncd crash sonic-net/sonic-buildimage#7276

Closed

sanmalho-git mentioned this pull request May 27, 2021

test_config_interface_speed causes orchagent to exit on DUT that doesn't support 10G. sonic-net/sonic-mgmt#3550

Closed

Junchao-Mellanox mentioned this pull request Jun 1, 2021

Orchagent exit after configuring incorrect advertised interface type value sonic-net/sonic-buildimage#7768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add failure notification for orchagent #1665

Add failure notification for orchagent #1665

shi-su commented Mar 8, 2021 •

edited

Loading

lgtm-com bot commented Mar 8, 2021

lgtm-com bot commented Mar 10, 2021

lgtm-com bot commented Mar 12, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

qiluo-msft Mar 27, 2021

shi-su Mar 29, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

qiluo-msft Mar 27, 2021

shi-su Mar 29, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

shi-su Mar 30, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

qiluo-msft Mar 16, 2021 •

edited

Loading

shi-su Mar 24, 2021

qiluo-msft Mar 17, 2021 •

edited

Loading

shi-su Mar 24, 2021

shi-su Mar 30, 2021

Add failure notification for orchagent #1665

Add failure notification for orchagent #1665

Conversation

shi-su commented Mar 8, 2021 • edited Loading

lgtm-com bot commented Mar 8, 2021

lgtm-com bot commented Mar 10, 2021

lgtm-com bot commented Mar 12, 2021

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shi-su commented Mar 8, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 16, 2021 •

edited

Loading

qiluo-msft Mar 17, 2021 •

edited

Loading