[orchagent] Add separate next hop table and orch by TACappleman · Pull Request #1702 · sonic-net/sonic-swss

TACappleman · 2021-04-09T16:20:12Z

What I did
Added a new next hop group table to APP_DB, and orchagent support
to use this as an alternative to including next hop information in
the route table

Why I did it
Improves performance and occupancy usage when using the new table

How I verified it
Extended SWSS unit tests to cover new code, and ran existing tests

Additional details
The first 2 commits in this PR are part of Juniper's MPLS enhancement, which should be merged to master first. The commit to review for this PR is just the latest commit.

anshuv-mfst · 2021-04-12T15:27:15Z

Hi @adamyeung - could you please add BRCM team to review this PR, thanks.

qiluo-msft · 2021-04-27T18:21:51Z

Your change should not depend on "Juniper's MPLS enhancement". Could you only separate your changes into a standalone PR?

TACappleman · 2021-04-28T08:24:47Z

@qiluo-msft We discussed this with Anshu and agreed this as a good approach to begin the review process. The code we intend to merge includes MPLS throughout, and so will only ever go in once Juniper's MPLS code is in. Once their PR has been merged we will rework this to just include the top commit.

doc/swss-schema.md

orchagent/orchdaemon.cpp

LaveenBrcm · 2021-05-17T20:22:28Z

orchagent/nexthopkey.h

+
+    NextHopKey ipKey() const
+    {
+        return NextHopKey(ip_address, alias);


In case of EVPN/Vxlan, for the same ip & alias there can be multiple nexhtops with different vni and mac_address. If ipKey is used to compare the nexthop objects then the results might not be valid for EVPN/Vxlan.

This method is specifically to get the underlying IP/interface pair to check if neighorch has learned this neighbor and has programmed the basic next hop for the pair. We don't want to create e.g. a labelled next hop for the pair if the basic next hop doesn't exist yet. I'll add a comment explaining the purpose of this function in my next update.

I don't think this has to be added as part of the common class for a specific use-case. Please revert and use as pointed by Ann's comment below.

I don't see this addressed

@prsunny We are using this in several places and I think it would be good to keep here, but maybe a different name would make its purpose clearer? getNeighNhKey()? getArpNhKey()?

@prsunny Or it could be pushed to NeighOrch... NeighOrch::isNeighborResolved(nexthop)?

Fixed - I've taken Ann's second solution of adding a NeighOrch method

prsunny · 2021-05-19T01:04:07Z

@qiluo-msft We discussed this with Anshu and agreed this as a good approach to begin the review process. The code we intend to merge includes MPLS throughout, and so will only ever go in once Juniper's MPLS code is in. Once their PR has been merged we will rework this to just include the top commit.

This is a critical change and its hard to review if mpls changes are present. Agree with @qiluo-msft that the code should be separated for better review.

TACappleman · 2021-05-19T08:25:21Z

@qiluo-msft We discussed this with Anshu and agreed this as a good approach to begin the review process. The code we intend to merge includes MPLS throughout, and so will only ever go in once Juniper's MPLS code is in. Once their PR has been merged we will rework this to just include the top commit.

This is a critical change and its hard to review if mpls changes are present. Agree with @qiluo-msft that the code should be separated for better review.

If we were to separate the code, then only some of what we are planning to merge would get reviewed - we need this to go in with the MPLS changes included. This commit (23da0d2) shows our proposed changes and nothing else so is the best place to review this. I can attempt to create the subset of that that doesn't depend on Juniper's code if absolutely necessary, but that subset would never actually be intended to be merged on its own.

rlhui · 2021-05-24T15:30:33Z

@smaheshm - FYI

TACappleman · 2021-05-25T13:18:38Z

@LaveenBrcm @prsunny @qiluo-msft @smaheshm I've rebased our changes on top of Juniper's latest commit. ed1f164 is now the commit to review for this enhancement.

prsunny · 2021-05-25T22:04:53Z

@LaveenBrcm @prsunny @qiluo-msft @smaheshm I've rebased our changes on top of Juniper's latest commit. ed1f164 is now the commit to review for this enhancement.

MPLS changes are reviewed separately in another PR. Recommend splitting the PR to only have next hop table related changes. Else please rebase after mpls changes are merged and then mark this PR "ready for review".

prsunny

As comments.

TACappleman · 2021-08-11T13:04:22Z

Hi all, @prsunny @qiluo-msft @smaheshm @LaveenBrcm our code for this PR is now on top of the latest master, as a single commit with no dependencies on Juniper's changes.

lgtm-com · 2021-08-11T13:05:14Z

This pull request introduces 14 alerts and fixes 1 when merging 2da3ab3 into 0217b66 - view on LGTM.com

new alerts:

7 for Variable defined multiple times
6 for Unused local variable
1 for Except block handles 'BaseException'

fixed alerts:

1 for Unnecessary 'else' clause in loop

lgtm-com · 2021-08-11T13:38:00Z

This pull request introduces 1 alert and fixes 1 when merging 1ef160e into d8ca31c - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

fixed alerts:

1 for Unnecessary 'else' clause in loop

lgtm-com · 2021-08-11T14:03:11Z

This pull request fixes 1 alert when merging 71d7692 into d8ca31c - view on LGTM.com

fixed alerts:

1 for Unnecessary 'else' clause in loop

lgtm-com · 2021-08-11T14:23:59Z

This pull request fixes 1 alert when merging fb54b25 into d8ca31c - view on LGTM.com

fixed alerts:

1 for Unnecessary 'else' clause in loop

This reverts commit 0aac13f.

mssonicbld · 2021-10-08T14:50:25Z

/AzurePipelines run

azure-pipelines · 2021-10-08T14:51:29Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2021-10-08T14:51:42Z

/AzurePipelines run

azure-pipelines · 2021-10-08T14:51:47Z

Comment was made before the most recent commit for PR 1702 in repo Azure/sonic-swss

azure-pipelines · 2021-10-08T14:52:00Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2021-10-08T15:54:37Z

This pull request fixes 1 alert when merging 54234b4 into 94d2d44 - view on LGTM.com

fixed alerts:

1 for Unnecessary 'else' clause in loop

TACappleman · 2021-10-08T19:56:28Z

/azpw run

mssonicbld · 2021-10-08T19:56:29Z

/AzurePipelines run

azure-pipelines · 2021-10-08T19:56:40Z

Azure Pipelines successfully started running 1 pipeline(s).

smaheshm

prsunny · 2021-10-11T22:32:21Z

orchagent/mplsrouteorch.cpp

-    }
-    if (nextHops.getSize() == 1)
+    /* Check that the next hop group is not owned by NhgOrch. */
+    if (ctx.nhg_index.empty())


I think you must address this similar to routeorch. No need to move the whole code under this if.

prsunny · 2021-10-12T05:15:21Z

orchagent/routeorch.h

 {
 public:
-    RouteOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames, SwitchOrch *switchOrch, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch, FgNhgOrch *fgNhgOrch);
+    RouteOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames, SwitchOrch *switchOrc, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch, FgNhgOrch *fgNhgOrch);


pls fix typo

lgtm-com · 2021-10-12T09:27:40Z

This pull request fixes 1 alert when merging 792a4bd into ef6b5d4 - view on LGTM.com

fixed alerts:

1 for Unnecessary 'else' clause in loop

prsunny · 2021-10-12T15:55:50Z

orchagent/mplsrouteorch.cpp

    }
    /* The route is pointing to a next hop group */
-    else
+    else if (nextHops.getSize() > 1)


why > ?. Please keep it as 'else'

prsunny · 2021-10-12T15:56:41Z

orchagent/mplsrouteorch.cpp

+            gNhgOrch->incNhgRefCount(ctx.nhg_index);
+        }
+
+        SWSS_LOG_INFO("Create label route %u with next hop(s) %s",


Fix the alignment as before

prsunny · 2021-10-12T15:57:05Z

orchagent/mplsrouteorch.cpp


        SWSS_LOG_INFO("Post set label %u with next hop(s) %s",
-                      label, nextHops.to_string().c_str());
+                label, nextHops.to_string().c_str());


Fix alignment

lgtm-com · 2021-10-12T17:43:44Z

This pull request fixes 1 alert when merging a0c982e into fd0cafe - view on LGTM.com

fixed alerts:

1 for Unnecessary 'else' clause in loop

Added a new next hop group table to APP_DB, and orchagent support to use this as an alternative to including next hop information in the route table. Added a new "nghOrch" that subscribes to next hop group table events to program next hop groups into SAI layer. Improves performance and occupancy usage when using the new table Extended SWSS unit tests to cover new code, and ran existing tests

TACappleman force-pushed the nh_split_pr branch from df41c2c to 23da0d2 Compare April 13, 2021 12:30

lguohan requested a review from qiluo-msft April 24, 2021 19:53

preetham-singh reviewed May 17, 2021

View reviewed changes

doc/swss-schema.md Outdated Show resolved Hide resolved

LaveenBrcm reviewed May 17, 2021

View reviewed changes

orchagent/orchdaemon.cpp Outdated Show resolved Hide resolved

LaveenBrcm reviewed May 17, 2021

View reviewed changes

TACappleman force-pushed the nh_split_pr branch 4 times, most recently from b433ea3 to ed1f164 Compare May 25, 2021 13:16

prsunny requested changes May 25, 2021

View reviewed changes

TACappleman force-pushed the nh_split_pr branch from ed1f164 to 8432992 Compare May 26, 2021 09:45

TACappleman force-pushed the nh_split_pr branch 2 times, most recently from ed1f164 to 2da3ab3 Compare August 11, 2021 12:59

TACappleman force-pushed the nh_split_pr branch from 2da3ab3 to 1ef160e Compare August 11, 2021 13:29

TACappleman force-pushed the nh_split_pr branch from 1ef160e to 71d7692 Compare August 11, 2021 13:56

TACappleman force-pushed the nh_split_pr branch from 71d7692 to fb54b25 Compare August 11, 2021 14:13

TACappleman mentioned this pull request Aug 24, 2021

Updating sonic-swss-common submodule sonic-net/sonic-buildimage#8566

Closed

5 tasks

Revert "Pipeline fix"

54234b4

This reverts commit 0aac13f.

smaheshm previously approved these changes Oct 11, 2021

View reviewed changes

prsunny reviewed Oct 11, 2021

View reviewed changes

prsunny reviewed Oct 12, 2021

View reviewed changes

Fixes

2b0a533

TACappleman dismissed smaheshm’s stale review via 2b0a533 October 12, 2021 08:19

Fixes

792a4bd

prsunny reviewed Oct 12, 2021

View reviewed changes

TACappleman added 6 commits October 12, 2021 17:14

Fixes

660bc90

Fixes

dbd96f4

Fixes

979d349

Fixes

13ef025

Fixes

3789b5b

Fixes

a0c982e

prsunny approved these changes Oct 15, 2021

View reviewed changes

smaheshm approved these changes Oct 15, 2021

View reviewed changes

smaheshm merged commit f248e26 into sonic-net:master Oct 15, 2021

abanu-ms deleted the nh_split_pr branch December 3, 2021 09:55

Conversation

TACappleman commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anshuv-mfst commented Apr 12, 2021

Uh oh!

qiluo-msft commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TACappleman commented Apr 28, 2021

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

prsunny commented May 19, 2021

Uh oh!

TACappleman commented May 19, 2021

Uh oh!

rlhui commented May 24, 2021

Uh oh!

TACappleman commented May 25, 2021

Uh oh!

prsunny commented May 25, 2021

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

TACappleman commented Aug 11, 2021

Uh oh!

lgtm-com bot commented Aug 11, 2021

Uh oh!

lgtm-com bot commented Aug 11, 2021

Uh oh!

lgtm-com bot commented Aug 11, 2021

Uh oh!

lgtm-com bot commented Aug 11, 2021

Uh oh!

mssonicbld commented Oct 8, 2021

Uh oh!

azure-pipelines bot commented Oct 8, 2021

Uh oh!

mssonicbld commented Oct 8, 2021

Uh oh!

azure-pipelines bot commented Oct 8, 2021

Uh oh!

azure-pipelines bot commented Oct 8, 2021

Uh oh!

lgtm-com bot commented Oct 8, 2021

Uh oh!

TACappleman commented Oct 8, 2021

Uh oh!

mssonicbld commented Oct 8, 2021

Uh oh!

azure-pipelines bot commented Oct 8, 2021

Uh oh!

smaheshm left a comment

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!

lgtm-com bot commented Oct 12, 2021

Uh oh!

Choose a reason for hiding this comment

TACappleman commented Apr 9, 2021 •

edited

Loading

qiluo-msft commented Apr 27, 2021 •

edited

Loading