[muxorch] Using bulker to program routes/neighbors during switchover #3148

Ndancejic · 2024-05-13T23:37:19Z

Uses entity bulker to program routes and neighbors during mux
switchover. Mux switchover performance suffers when switching over with
a large number of neighbors on the mux port. This uses the optimization
of programming the neighbors and routes in bulk to avoid sequentially
programming each.

What I did
Changed mux switchover logic to use neighbor and bulk switchover instead of programming neighbors sequentially.

Why I did it
Testing shows this improves switchover time by an average of 30% at 128 neighbors.

How I verified it
added a test to vstests to test functionality of code, and tested performance on a dualtor lab testbed.

Details if related
ado:#25072867

bulk_switchover_test_results.xlsx

Uses entity bulker to program routes and neighbors during mux switchover. Mux switchover performance suffers when switching over with a large number of neighbors on the mux port. This uses the optimization of programming the neighbors and routes in bulk to avoid sequentially programming each. Signed-off-by: Nikola Dancejic <[email protected]>

Ndancejic · 2024-05-14T18:44:31Z

/azpw run

mssonicbld · 2024-05-14T18:44:33Z

/AzurePipelines run

azure-pipelines · 2024-05-14T18:44:43Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2024-05-21T21:11:11Z

orchagent/neighorch.cpp

+ * @brief Creates a neighbor add entry and adds it to bulker.
+ * @param ctx NeighborBulkContext contains neighbor information and list of object statuses.
+ */
+bool NeighOrch::addBulkNeighbor(NeighborBulkContext& ctx)


does it use bulk neighbor only for mux scenario or even for the existing path? I think we should enable it only for mux flow.

it's only for mux flow for this PR

prsunny · 2024-05-21T21:17:16Z

@dgsudharsan for viz

Signed-off-by: Nikola Dancejic <[email protected]>

linux-foundation-easycla · 2024-05-25T01:31:29Z

The committers listed above are authorized under a signed CLA.

✅ login: Ndancejic / name: Nikola Dancejic (f259759, 4fd3503, 11e5cce, d0d3fe9, d4db44e, 9a675bf, 2b3240b, 31e0dd9, e318676, 1cfcb4a, c823cb2, 411da0f)
✅ login: prsunny / name: Prince Sunny (811f5a9, 7c2bcbb, fc9f317, 11f1674, 934b566)

bulk switchover feature chages the sai_api calls in switchover flow. Because of this mux_rollback_ut tests fail. This PR adds bulk operations to mock_sai_neighbor and mock_sai_api, and adjusts the expected function calls in mux_rollback_ut. Signed-off-by: Nikola Dancejic <[email protected]>

prsunny · 2024-05-30T16:30:55Z

Dependant on PR sonic-net/sonic-sairedis#1373

Ndancejic · 2024-05-31T03:12:09Z

still pulling from wrong sairedis pipeline, running again:
Download from the specified build: #558894

Ndancejic · 2024-05-31T03:12:20Z

/azpw run

mssonicbld · 2024-05-31T03:12:22Z

/AzurePipelines run

azure-pipelines · 2024-05-31T03:12:32Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Nikola Dancejic <[email protected]>

Ndancejic · 2024-05-31T09:54:09Z

Last run was still failing because the correct syncd was not installed for some reason. Ran mux tests locally with syncd package pulled from https://dev.azure.com/mssonic/build/_build/results?buildId=559999&view=results and only had one test_multi_nexthop test fail due to a small bug I was able to resolve. Tested with latest commit and all tests are passing.

=================================================================
conftest.py:1880
/data/sonic-swss/tests/conftest.py:1880: PytestDeprecationWarning: @pytest.yield_fixture is deprecated.
Use @pytest.fixture instead; they are the same.
@pytest.yield_fixture(scope="module")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================================== 31 passed, 1 warning in 1031.63s (0:17:11) =====

Ndancejic · 2024-05-31T15:23:59Z

/azpw run

mssonicbld · 2024-05-31T15:24:01Z

/AzurePipelines run

azure-pipelines · 2024-05-31T15:24:10Z

Azure Pipelines successfully started running 1 pipeline(s).

Ndancejic · 2024-05-31T15:28:57Z

/azpw run

mssonicbld · 2024-05-31T15:28:59Z

/AzurePipelines run

azure-pipelines · 2024-05-31T15:29:06Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny

can you post the dualttor sonic-mgmt test results?

prsunny · 2024-05-31T18:33:30Z

orchagent/muxorch.cpp

+        while (it != neighbors_.end())
+        {
+            NextHopKey nh_key = NextHopKey(it->first, alias_);
+            if (update_rt)


Why is update_rt checked here? Its already done above.

good catch, will fix that

prsunny · 2024-05-31T18:36:56Z

orchagent/muxorch.cpp

+        }
+
+        it = neighbors_.begin();
+        while (it != neighbors_.end())


We've introduced three loops now in the new code. Can we optimize it?

I took the loop out from the comment below, I believe the two left over are necessary, we need the neighbors enabled before we update routes to point to their next hops

prsunny · 2024-05-31T18:38:30Z

orchagent/muxorch.cpp

+            NextHopKey nh_key = NextHopKey(it->first, alias_);
+            if (update_rt)
+            {
+                updateTunnelRoute(nh_key, false);


Why don't we move this to original section and do this loop only if Bulk api fails. So we don't have to do it everytime

I see what you're saying, I think I just wanted to make sure the order was maintained. I'll fix that

I think it's safe to take out this loop entirely then, if bulk api fails we will fall back, which will remove the tunnel routes anyways

prsunny · 2024-05-31T19:20:02Z

orchagent/neighorch.cpp

+    PortsOrch* ports_orch = gDirectory.get<PortsOrch*>();
+    auto vlan_ports = ports_orch->getAllVlans();
+
+    for (auto vlan_port: vlan_ports)


This is a lot of repeated code from existing addNeighbor. I think we should only add the bulk function as discussed offline.

Reduced the amount of repeated code in latest commit

- using existing add/removeNeighbor functions to do bulker operations. - Created addNeighborPost and removeNeighborPost - Created addRoutes and removeRoutes to deal with bulk operations - Created enableNeighbors and disableNeighbors to handle bulk switchover. Signed-off-by: Nikola Dancejic <[email protected]>

Signed-off-by: Nikola Dancejic <[email protected]>

prsunny · 2024-06-04T16:44:27Z

orchagent/muxorch.cpp

        }

+        updateTunnelRoute(nh_key, false);


should this be inside the update_rt check?

yes, I'll move that back

prsunny · 2024-06-04T16:45:10Z

orchagent/muxorch.cpp

+        return false;
+    }
+
+    gRouteBulker.clear();


should we clear this even if its returned false above

Took a look at this, it's not needed since we call clear from removeRoutes

prsunny · 2024-06-04T17:01:16Z

orchagent/neighorch.cpp

@@ -1060,6 +1081,7 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress
    }
    else if (isHwConfigured(neighborEntry))
    {
+        ctx.set_neigh_attr_count = (int)neighbor_attrs.size();


This can be removed

Ndancejic · 2024-06-04T17:09:45Z

orchagent/muxorch.cpp

        }

+        updateTunnelRoute(nh_key, false);


yes, I'll move that back

Ndancejic · 2024-06-04T17:10:16Z

orchagent/muxorch.cpp

+        return false;
+    }
+
+    gRouteBulker.clear();


Took a look at this, it's not needed since we call clear from removeRoutes

Ndancejic · 2024-06-04T17:20:54Z

orchagent/neighorch.cpp

+
+    SWSS_LOG_INFO("Checking neighbor remove entry status %s on %s.", ip_address.to_string().c_str(), m_syncdNeighbors[neighborEntry].mac.to_string().c_str());
+
+    if (isHwConfigured(neighborEntry))


Took a loot at this, we still need the HwConfigured check since this isn't checked for in the disableNeighbors patch

Signed-off-by: Nikola Dancejic <[email protected]>

Ndancejic added 2 commits May 20, 2024 15:11

Merge branch 'master' into bulk_switchover

d0d3fe9

Merge branch 'master' into bulk_switchover

4fd3503

Ndancejic marked this pull request as ready for review May 21, 2024 20:49

Ndancejic requested a review from prsunny as a code owner May 21, 2024 20:49

prsunny reviewed May 21, 2024

View reviewed changes

Ndancejic added 2 commits May 23, 2024 06:03

fixing aclorch_ut

11e5cce

Signed-off-by: Nikola Dancejic <[email protected]>

skip MuxRollbackTests until create_neighbor_entries api is supported

2b3240b

Signed-off-by: Nikola Dancejic <[email protected]>

Ndancejic force-pushed the bulk_switchover branch from 397f058 to 87281fb Compare May 25, 2024 01:32

Ndancejic force-pushed the bulk_switchover branch from 87281fb to f259759 Compare May 27, 2024 18:04

prsunny added 4 commits May 30, 2024 09:30

Merge branch 'master' into bulk_switchover

7c2bcbb

Merge branch 'master' into bulk_switchover

11f1674

Merge branch 'master' into bulk_switchover

811f5a9

Merge branch 'master' into bulk_switchover

934b566

Ndancejic and others added 2 commits May 31, 2024 09:44

[muxorch] Removing INFO logging from tests and ensuring neigh is set

e318676

Signed-off-by: Nikola Dancejic <[email protected]>

Merge branch 'master' into bulk_switchover

9a675bf

Merge branch 'master' into bulk_switchover

fc9f317

prsunny approved these changes May 31, 2024

View reviewed changes

prsunny requested changes May 31, 2024

View reviewed changes

prsunny reviewed May 31, 2024

View reviewed changes

Ndancejic added 3 commits June 3, 2024 19:33

Addressing PR comments

31e0dd9

Signed-off-by: Nikola Dancejic <[email protected]>

Removing un-needed code and expanding test coverage

c823cb2

Signed-off-by: Nikola Dancejic <[email protected]>

prsunny reviewed Jun 4, 2024

View reviewed changes

Ndancejic commented Jun 4, 2024

View reviewed changes

addressing PR comments

1cfcb4a

Signed-off-by: Nikola Dancejic <[email protected]>

prsunny approved these changes Jun 4, 2024

View reviewed changes

prsunny merged commit b3ebfc4 into sonic-net:master Jun 4, 2024
17 checks passed

nazariig added the Request for 202405 Branch label Aug 6, 2024

mssonicbld added the Cherry Pick Conflict_202405 label Aug 6, 2024

nazariig added the Included in 202405 Branch label Aug 6, 2024

vkjammala-arista mentioned this pull request Sep 24, 2024

[202405][dualtor] Orchagent is going down during switchover #3298

Open


		SWSS_LOG_INFO("Checking neighbor remove entry status %s on %s.", ip_address.to_string().c_str(), m_syncdNeighbors[neighborEntry].mac.to_string().c_str());

		if (isHwConfigured(neighborEntry))

[muxorch] Using bulker to program routes/neighbors during switchover #3148

[muxorch] Using bulker to program routes/neighbors during switchover #3148

Conversation

Ndancejic commented May 13, 2024

Ndancejic commented May 14, 2024

mssonicbld commented May 14, 2024

azure-pipelines bot commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented May 21, 2024

linux-foundation-easycla bot commented May 25, 2024 • edited Loading

prsunny commented May 30, 2024

Ndancejic commented May 31, 2024

Ndancejic commented May 31, 2024

mssonicbld commented May 31, 2024

azure-pipelines bot commented May 31, 2024

Ndancejic commented May 31, 2024

Ndancejic commented May 31, 2024

mssonicbld commented May 31, 2024

azure-pipelines bot commented May 31, 2024

Ndancejic commented May 31, 2024

mssonicbld commented May 31, 2024

azure-pipelines bot commented May 31, 2024

prsunny left a comment • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented May 25, 2024 •

edited

Loading

prsunny left a comment •

edited

Loading