[mux]: Implement rollback for failed mux switchovers by theasianpianist · Pull Request #2714 · sonic-net/sonic-swss

theasianpianist · 2023-03-24T18:20:59Z

Depends on sonic-net/sonic-sairedis#1224 being merged first

What I did

Make all SAI API operations needed for switchover idempotent
Implement rollback when a switchover fails

Why I did it

How I verified it

Run new mux_rollback mock_tests.

Details if related

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

- Make all switchover-related SAI operations idempotent (create and remove for ACL, next hop, neighbor, and route) - Implement rollback when a switchover fails Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

orchagent/muxorch.cpp

orchagent/neighorch.cpp

prsunny · 2023-03-27T23:49:27Z

orchagent/muxorch.cpp

+            // Check at the start of the function means we will never reach here
+            SWSS_LOG_ERROR("[%s] Rollback to %s not supported", mux_name_.c_str(),
+                                muxStateValToString.at(prev_state_).c_str());
+            return;


This will leave st_chg_in_progress_ as true, right?

Yes, but we will never get here since the check on the first line of this function will return early for FAILED and PENDING states

orchagent/muxorch.cpp

prsunny · 2023-03-27T23:53:07Z

orchagent/muxorch.h

    void bindAllPorts(AclTable &acl_table);

-    // class shared dict: ACL table name -> ACL table
-    static std::map<std::string, AclTable> acl_table_;


why is this removed? i think its changing the overall logic.

based on my understanding, this was used to cache if the ACL table had already been created in hardware. I changed the behavior to delegate this check to AclOrch instead, since the static scope of this map was preventing the mock_tests from passing. The MuxAclHandler now always asks AclOrch to check if the table already exists, instead of storing that info in MuxAclHandler.

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny · 2023-04-03T20:18:10Z

orchagent/aclorch.cpp

    status = sai_acl_api->create_acl_entry(&m_ruleOid, gSwitchId, (uint32_t)rule_attrs.size(), rule_attrs.data());
    if (status != SAI_STATUS_SUCCESS)
    {
+        if (status == SAI_STATUS_ITEM_ALREADY_EXISTS)


@theasianpianist, is this change required for mux idempotency/rollback? @bingwang-ms , can you please review?

If the ACL entry already exists, the SAI API will return this status, when we rollback we might hit this scenario so want to continue normally if the entry does already exist.

prsunny · 2023-04-03T20:21:28Z

orchagent/neighorch.cpp

        {
            /* When next hop is not found, we continue to remove neighbor entry. */
-            if (status == SAI_STATUS_ITEM_NOT_FOUND)
+            if (status == SAI_STATUS_ITEM_NOT_FOUND || status == SAI_STATUS_INVALID_PARAMETER)


This seems to be changing existing flow. Are we getting SAI_STATUS_INVALID_PARAMETER here for non-existent entries?

Yes https://github.com/sonic-net/sonic-sairedis/blob/60fa682fcce68ef9100bb161a05b19cb5bed0c82/meta/Meta.cpp#L3261

…lback

prsunny · 2023-04-05T22:13:57Z

orchagent/aclorch.cpp

    auto status = sai_acl_api->remove_acl_entry(m_ruleOid);
    if (status != SAI_STATUS_SUCCESS)
    {
+        if (status == SAI_STATUS_ITEM_NOT_FOUND || status == SAI_STATUS_INVALID_PARAMETER)


Let's get the sairedis merged and revisit this section

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny · 2023-04-24T21:13:46Z

/azp run

azure-pipelines · 2023-04-24T21:13:56Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2023-04-26T02:54:51Z

Need to fix build failure

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny

lgtm

yxieca · 2023-05-04T02:11:21Z

@theasianpianist can you create separate PR for 202205 branch?

- Make all SAI API operations needed for switchover idempotent - Implement rollback when a switchover fails Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist · 2023-05-04T18:11:42Z

@theasianpianist can you create separate PR for 202205 branch?

PR here: #2761

- Make all SAI API operations needed for switchover idempotent - Implement rollback when a switchover fails Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist added 4 commits March 17, 2023 22:15

Ignore mock test logs and results

d410ab2

[muxorch]: Add tests to verify idempotency

c7d3e13

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

[muxorch]: Rollback to previous mux state

fdf2725

- Make all switchover-related SAI operations idempotent (create and remove for ACL, next hop, neighbor, and route) - Implement rollback when a switchover fails Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

[muxorch]: Add more rollback tests

eda687a

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist requested a review from prsunny as a code owner March 24, 2023 18:20

theasianpianist added 2 commits March 24, 2023 19:52

Init NhgOrch during mock tests

ee1f47d

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

remove garbage

bbe6677

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny reviewed Mar 27, 2023

View reviewed changes

make compatible with googletest 1.8.1

c28207c

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist force-pushed the mux-rollback branch from c78cc21 to c28207c Compare March 28, 2023 00:55

theasianpianist added 2 commits March 28, 2023 01:04

address review comments

99104d0

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

[azp]: Remove sonic-eventd when building DVS image

7e3fcb7

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist mentioned this pull request Mar 28, 2023

[202012][mux]: Implement rollback for failed mux switchovers #2716

Merged

theasianpianist added 3 commits March 28, 2023 19:41

Remove python2 swsscommon package

d1de878

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

Merge branch 'master' into mux-rollback

13b1f68

Merge branch 'master' into mux-rollback

0724b52

theasianpianist requested a review from prsunny March 31, 2023 19:32

prsunny requested a review from Ndancejic April 3, 2023 20:15

prsunny reviewed Apr 3, 2023

View reviewed changes

Merge branch 'master' of github.com:sonic-net/sonic-swss into mux-rol…

29db8a2

…lback

prsunny reviewed Apr 5, 2023

View reviewed changes

theasianpianist added the Request for 202205 Branch label Apr 18, 2023

Don't use SAI_STATUS_INVALID_PARAMETER

7e7d956

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prabhataravind requested a review from prsunny April 29, 2023 04:03

remove invalid param rollback UTs

c0c91f2

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny approved these changes May 2, 2023

View reviewed changes

prsunny added the Request for 202211 Branch label May 2, 2023

theasianpianist merged commit bc4062b into sonic-net:master May 2, 2023

yxieca added the Cherry Pick Conflict_202205 label May 4, 2023

theasianpianist mentioned this pull request May 4, 2023

Use SAI_STATUS_ITEM_NOT_FOUND when key not found sonic-net/sonic-sairedis#1224

Merged

yxieca added the Included in 202205 Branch label May 11, 2023

yxieca added the Included in 202211 Branch label Aug 29, 2023

Conversation

theasianpianist commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Apr 24, 2023

Uh oh!

azure-pipelines bot commented Apr 24, 2023

Uh oh!

prsunny commented Apr 26, 2023

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

yxieca commented May 4, 2023

Uh oh!

theasianpianist commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

theasianpianist commented Mar 24, 2023 •

edited

Loading