[muxorch] handling multiple mux nexthops for route by Ndancejic · Pull Request #2656 · sonic-net/sonic-swss

Ndancejic · 2023-02-07T21:08:27Z

What I did: added logic to handle when a route points to a nexthop
group with mux neighbors. In this case, only one active neighbor, or the
tunnel nexthop will be programmed to the ASIC.

Why I did it: having a route with multiple mux neighbors caused a data
loop which lead to packet loss when different neighbors were in
different states.

How I did it: added logic to update routes when a route is changed, a
mux neighbor is changed, or there is a mux state change.

HLD: sonic-net/SONiC#1256

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

orchagent/nexthopgroupkey.h

prsunny · 2023-02-15T01:18:55Z

Please provide the Description with all details as per the template

Ndancejic · 2023-02-15T17:23:11Z

Please provide the Description with all details as per the template

Ah my bad, that usually auto generates with my commit message but I had a draft PR open before and it didn't update. I'll fix that

orchagent/routeorch.cpp

orchagent/muxorch.cpp

orchagent/routeorch.cpp

Ndancejic · 2023-02-28T21:29:31Z

/azp run

azure-pipelines · 2023-02-28T21:29:37Z

Commenter does not have sufficient privileges for PR 2656 in repo sonic-net/sonic-swss

Ndancejic · 2023-02-28T21:29:46Z

/azpw run

mssonicbld · 2023-02-28T21:29:48Z

/AzurePipelines run

azure-pipelines · 2023-02-28T21:29:55Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

What I did: added tests/dualtor/test_multi_mux_nexthop_route.py which tests use case of multiple nexthop neighbors across different MUX ports. DEPENDS ON: sonic-net/sonic-swss#2656 Why I did it: test coverage of this scenario How I did it: test creates route to 2 different interfaces and neighbors, then validates that traffic is recieved from the expected ports.

What I did: added logic to handle when a route points to a nexthop group with mux neighbors. In this case, only one active neighbor, or the tunnel nexthop will be programmed to the ASIC. Why I did it: having a route with multiple mux neighbors caused a data loop which lead to packet loss when different neighbors were in different states. How I did it: added logic to update routes when a route is changed, a mux neighbor is changed, or there is a mux state change. HLD: sonic-net/SONiC#1256 Signed-off-by: Nikola Dancejic <[email protected]>

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

theasianpianist · 2023-03-10T03:30:42Z

orchagent/muxorch.cpp

+        if (it != mux_multi_nh_route_tb.end())
+        {
+            MuxCable* cable = findMuxCableInSubnet(it->second.ip_address);
+            if (cable == nullptr || cable->isActive())


If I'm reading this correctly, if our route nexthop is not one of the configured mux cable IPs, we take no action. This seems a bit optimistic to me, unless we can 100% guarantee that the multi-mux-nexthop scenario only occurs with nexthop IPs that are configured as mux cable IPs?

From my understanding from discussions with Prince we currently handle neighbors on the Vlan that aren't tied to a muxcable as active. maybe we can have a discussion on this?

i think findMuxCableInSubnet returns if its directly connected server IP but not all learned neighbors (and is in config_db). But i think we have to handle it if the nexthop belongs to any mux cable.

theasianpianist · 2023-03-10T03:32:20Z

orchagent/muxorch.cpp

+        {
+            NextHopKey nexthop = *it;
+            MuxCable* cable = findMuxCableInSubnet(nexthop.ip_address);
+            if (cable == nullptr || (cable->isActive() && !active_found))


same as above comment - if the nexthop IP isn't a mux cable IP, it seems safer to assume it's standby

same as above

tests/test_mux.py

theasianpianist · 2023-03-10T03:39:48Z

tests/test_mux.py

+                # move neighbor 2 back
+                self.add_neighbor(dvs, neighbors[0], macs[i])
+
+                self.del_route(dvs, route)


Would prefer if this test case were parameterized so that we could use a fixture to do setup/teardown in case something goes wrong in the middle of the test. Depending on urgency, we can merge as-is and discuss this change in the future.

yeah I agree with this. currently I think the try,finally block will teardown resources that I'm using in this test, but I can definitely rework it in that way.

orchagent/routeorch.cpp

orchagent/routeorch.h

prsunny · 2023-03-14T23:24:14Z

orchagent/routeorch.cpp

+    /* Check if nexthop is mux nexthop */
+    MuxOrch* mux_orch = gDirectory.get<MuxOrch*>();
+    NextHopGroupKey nhg_key;
+    if (inNextHopGroup(nextHop, nhg_key) && mux_orch->isMuxNexthops(nhg_key))


What happens if a nexthop is part of NHG but also another route points to this single nexthop?
For e.g like NH1 below:

R1 -> NHG1 (NH1, NH2)
R2 -> NH1

orchagent/routeorch.cpp

orchagent/muxorch.h

orchagent/muxorch.cpp

prsunny · 2023-03-14T23:43:52Z

orchagent/muxorch.cpp

+        if (it != mux_multi_nh_route_tb.end())
+        {
+            MuxCable* cable = findMuxCableInSubnet(it->second.ip_address);
+            if (cable == nullptr || cable->isActive())


i think findMuxCableInSubnet returns if its directly connected server IP but not all learned neighbors (and is in config_db). But i think we have to handle it if the nexthop belongs to any mux cable.

orchagent/muxorch.cpp

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

StormLiangMS · 2023-04-13T11:51:33Z

@Ndancejic cherry pick conflict, to 202211, could you raise separate PR for 202211 branch?

@prsunny for vis.

* [muxorch] handling multiple mux nexthops for route What I did: added logic to handle when a route points to a nexthop group with mux neighbors. In this case, only one active neighbor, or the tunnel nexthop will be programmed to the ASIC. Why I did it: having a route with multiple mux neighbors caused a data loop which lead to packet loss when different neighbors were in different states. How I did it: added logic to update routes when a route is changed, a mux neighbor is changed, or there is a mux state change. HLD: sonic-net/SONiC#1256 Signed-off-by: Nikola Dancejic <[email protected]>

Ndancejic force-pushed the multi-nexthop branch 2 times, most recently from 266395b to 312f41a Compare February 14, 2023 23:20

Ndancejic changed the title ~~multi-nexthop routes draft~~ [muxorch] handling multiple mux nexthops for route Feb 14, 2023

Ndancejic marked this pull request as ready for review February 14, 2023 23:20

Ndancejic requested a review from prsunny as a code owner February 14, 2023 23:20

prsunny reviewed Feb 15, 2023

View reviewed changes

orchagent/nexthopgroupkey.h Outdated Show resolved Hide resolved

prsunny requested a review from theasianpianist February 15, 2023 01:07

prsunny reviewed Feb 23, 2023

View reviewed changes

Ndancejic force-pushed the multi-nexthop branch 3 times, most recently from 01ee74d to adb5268 Compare February 25, 2023 06:33

This was referenced Feb 28, 2023

[dualtor] Adding test for multiple nexthop routes sonic-net/sonic-mgmt#7599

Closed

[dualtor] Adding test for multiple nexthop routes sonic-net/sonic-mgmt#7600

Closed

Ndancejic force-pushed the multi-nexthop branch from 7f57b63 to 10c946d Compare March 7, 2023 22:30

Ndancejic force-pushed the multi-nexthop branch from 10c946d to 4c8c0ef Compare March 7, 2023 22:34

Ndancejic requested review from prsunny and removed request for theasianpianist March 7, 2023 23:32

Ndancejic and others added 2 commits March 7, 2023 15:41

Merge branch 'master' into multi-nexthop

02f452f

fixing merge issues

a795c4d

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

Ndancejic force-pushed the multi-nexthop branch from f7a6dfa to a795c4d Compare March 8, 2023 00:06

fixing test_NH failures due to changes in test_route

1318314

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

theasianpianist reviewed Mar 10, 2023

View reviewed changes

tests/test_mux.py Outdated Show resolved Hide resolved

theasianpianist reviewed Mar 10, 2023

View reviewed changes

Ndancejic mentioned this pull request Mar 14, 2023

[202012][muxorch] handling multiple mux nexthops for route #2702

Merged

prsunny reviewed Mar 14, 2023

View reviewed changes

Addressing PR comments

6a727d9

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

prsunny added Request for 202205 Branch Request for 202211 Branch labels Apr 10, 2023

prsunny approved these changes Apr 10, 2023

View reviewed changes

prsunny merged commit 32ecc2a into sonic-net:master Apr 10, 2023

StormLiangMS added the Cherry Pick Conflict_202211 label Apr 13, 2023

yxieca added the Included in 202205 Branch label Apr 19, 2023

Conversation

Ndancejic commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

prsunny commented Feb 15, 2023

Uh oh!

Ndancejic commented Feb 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ndancejic commented Feb 28, 2023

Uh oh!

azure-pipelines bot commented Feb 28, 2023

Uh oh!

Ndancejic commented Feb 28, 2023

Uh oh!

mssonicbld commented Feb 28, 2023

Uh oh!

azure-pipelines bot commented Feb 28, 2023

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StormLiangMS commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Ndancejic commented Feb 7, 2023 •

edited

Loading