Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[orchagent] Add separate next hop table and orch #1702

Merged
merged 51 commits into from
Oct 15, 2021

Conversation

TACappleman
Copy link
Contributor

@TACappleman TACappleman commented Apr 9, 2021

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
Copy link

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

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Apr 27, 2021

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

@TACappleman
Copy link
Contributor Author

@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 Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this addressed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@prsunny
Copy link
Collaborator

prsunny commented May 19, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor

rlhui commented May 24, 2021

@smaheshm - FYI

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

@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
Copy link
Collaborator

prsunny commented May 25, 2021

@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".

Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments.

@TACappleman
Copy link
Contributor Author

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
Copy link

lgtm-com bot commented Aug 11, 2021

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
Copy link

lgtm-com bot commented Aug 11, 2021

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
Copy link

lgtm-com bot commented Aug 11, 2021

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
Copy link

lgtm-com bot commented Aug 11, 2021

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

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Oct 8, 2021

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
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

smaheshm
smaheshm previously approved these changes Oct 11, 2021
Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -137,7 +168,7 @@ struct LabelRouteBulkContext
class RouteOrch : public Orch, public Subject
{
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls fix typo

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

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

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@@ -638,7 +708,7 @@ bool RouteOrch::addLabelRoutePost(const LabelRouteBulkContext& ctx, const NextHo
}
}
/* The route is pointing to a next hop group */
else
else if (nextHops.getSize() > 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why > ?. Please keep it as 'else'

gNhgOrch->incNhgRefCount(ctx.nhg_index);
}

SWSS_LOG_INFO("Create label route %u with next hop(s) %s",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the alignment as before

@@ -731,10 +825,10 @@ bool RouteOrch::addLabelRoutePost(const LabelRouteBulkContext& ctx, const NextHo
}

SWSS_LOG_INFO("Post set label %u with next hop(s) %s",
label, nextHops.to_string().c_str());
label, nextHops.to_string().c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix alignment

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

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

fixed alerts:

  • 1 for Unnecessary 'else' clause in loop

@smaheshm smaheshm merged commit f248e26 into sonic-net:master Oct 15, 2021
@abanu-ms abanu-ms deleted the nh_split_pr branch December 3, 2021 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.