[Orchagent] Recursive nexthop group enhancement by utpalkantpintoo · Pull Request #3105 · sonic-net/sonic-swss

utpalkantpintoo · 2024-04-07T09:12:10Z

What I did

Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in:
sonic-net/SONiC#1636

Why I did it

These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects.

How I verified it

Details if related

linux-foundation-easycla · 2024-04-07T09:12:15Z

The committers listed above are authorized under a signed CLA.

✅ login: utpalkantpintoo / name: Utpal (ff2112e, 3ae044a, 6776a5f, 0788fc1, fe7f488)

utpalkantpintoo · 2024-05-02T09:46:57Z

/azpw run Azure.sonic-swss

mssonicbld · 2024-05-02T09:47:00Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2024-05-02T09:47:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Gokulnath-Raja · 2024-05-14T05:03:08Z

orchagent/nhgorch.cpp

+                }
+
+                /* Form nexthopgroup key with the nexthopgroup keys of available members */
+                nhgv = tokenize(nhgs, ',');


Can we use NHG_DELIMITER instead of ","

Gokulnath-Raja · 2024-05-14T05:03:14Z

orchagent/nhgorch.cpp

+                    if (nhgs.empty())
+                        nhgs = nhgm;
+                    else
+                        nhgs += ',' + nhgm;


Can we use NHG_DELIMITER instead of ","

Gokulnath-Raja · 2024-05-14T05:12:34Z

orchagent/nhgorch.cpp

    SWSS_LOG_ENTER();

+    /* This method should not be called for single-membered non-recursive nexthop groups */
+    assert(isRecursive() || (m_members.size() > 1));


Instead of assert can we handle as exception?

This is in-line with similar error handling elsewhere in the code. We do not have a way to gracefully handle once we end up here for a non-recursive singleton nhg. There are necessary checks in caller fn.

Gokulnath-Raja · 2024-05-14T05:27:38Z

orchagent/nhgorch.cpp

-     */
-    if (m_is_temp)
+    /* If the group is non-recursive with single member, the group ID will be the only member's NH ID */
+    if (!isRecursive() && (m_members.size() == 1))


Do we need m_is_temp check?

That'll be redundant as the current check will be true for "temp" nhgs as well.

Gokulnath-Raja · 2024-05-14T05:32:01Z

orchagent/nhgorch.cpp

+    {
+        nh_id = gIntfsOrch->getRouterIntfsId(m_key.alias);
+
+        if ((nh_id == SAI_NULL_OBJECT_ID) &&


We don't consider loopback as nexthop and trap packet to CPU during route add https://github.com/sonic-net/sonic-swss/blob/master/orchagent/routeorch.cpp#L1789, so do we need to add this CPU port for NHG?

Will make the NhgOrch behavior in-line with RouteOrch handling.
But I see an issue where if we receive such a msg, we'll end up holding the msgs in the corres. m_toSync queue(s) till the route is withdrawn.

We install IP2ME for loopback IPs, right? I think, we can safely ignore the message with error log if we get a NHG with loopback interface.

If the NHG is not created, the route msg will be retained waiting for the NHG. This may require creating a dummy NHG with just the key so that route msg can also be ignored.
This should be addressed in both the NhgOrch and RouteOrch as a defect fix.

venkatmahalingam · 2024-05-14T06:43:33Z

orchagent/nhgorch.cpp

-            for (uint32_t i = 0; i < ipv.size(); i++)
+            /* Keeps track of any non-existing member of a recursive nexthop group */
+            bool non_existent_member = false;
+


Do we need to throw a warning if ipv.size() is non-zero when is_recursive is set?

venkatmahalingam · 2024-05-14T06:47:25Z

orchagent/nhgorch.cpp

                }
-                nhg_str += ipv[i] + NH_DELIMITER + alsv[i];
            }



Can we add a SWSS_LOG_DEBUG for nhg_str?

adyeung · 2024-05-16T16:17:32Z

@venkatmahalingam @Gokulnath-Raja pls signoff if you have no further comments

Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects.

Gokulnath-Raja · 2024-05-17T04:49:20Z

orchagent/nhgorch.cpp

+            /* A NHG should not have both regular(ip/alias) and recursive fields */
+            if (is_recursive && (!ips.empty() || !aliases.empty()))
+            {
+                SWSS_LOG_ERROR("Nexthop group %s has both regular(ip/alias) and recursive fields", index.c_str());


Can we move this to info log ?

Since the nexthop-group is not created due to this check, it's better to log this as error.

goomadao · 2024-05-24T08:29:10Z

Besides, there might be two issues that:

If a route points to a singleton NEXTHOP_GROUP and it becomes non-singleton later, the route will still refer to the SAI ID of the NEXTHOP instead of the newly created NEXTHOP_GROUP
For recursive situations, how to update NEXTHOP_GROUP recursively? For example, NEXTHOP_GROUP_TABLE:ID1 points to ID2 & ID3, ID2 refers to NEXTHOP NHa & NHb. When ID2 is updated to refer to NHa, NHb & NHc, how can we add member NHc to NEXTHOP_GROUP_TABLE:ID1 simultaneously?

utpalkantpintoo · 2024-05-29T09:47:45Z

Hi @goomadao

Thanks for the comments. Please find below my response:

For any nexthop-group update which changes its SAI-ID, it is expected that the routing app update the associated routes as well. This is expected behavior with FRR.
Handling it in Orchagent (NhgOrch/RouteOrch) will require maintaining a nexthop-group to route entries data-structure.
With recursive nexthop-group scheme, it is expected that the routing app resolve the recursive nexthop-groups and provide a recursive nexthop-group with ultimate nexthop member(s) which are not recursive themselves. This is expected behavior with FRR.

goomadao · 2024-06-06T07:54:24Z

@utpalkantpintoo Thanks for your explanation. You're right, I missed that there is an upper application where the update could be triggered.

Could you please take a look at the pending review I raised before? I still can't figure out any situation to get into those two conditions.

* What I did: Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects. * What I did: Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects. * What I did: Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects. * What I did: Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects. * Addressed review comments

utpalkantpintoo requested a review from prsunny as a code owner April 7, 2024 09:12

nkelapur mentioned this pull request Apr 10, 2024

Changes for handling recursive NHG entries sonic-net/SONiC#1636

Merged

utpalkantpintoo force-pushed the recursive_nhg branch from f5d145d to 69c204f Compare May 2, 2024 09:45

hasan-brcm approved these changes May 6, 2024

View reviewed changes

adyeung mentioned this pull request May 7, 2024

NextHop Group Table Enhancement sonic-net/SONiC#1594

Closed

utpalkantpintoo force-pushed the recursive_nhg branch from d54cd50 to 11379cf Compare May 8, 2024 06:45

Gokulnath-Raja reviewed May 14, 2024

View reviewed changes

venkatmahalingam reviewed May 14, 2024

View reviewed changes

utpalkantpintoo force-pushed the recursive_nhg branch 2 times, most recently from 4f972b0 to 166785d Compare May 15, 2024 09:13

utpalkantpintoo added 5 commits May 17, 2024 09:21

Addressed review comments

0788fc1

utpalkantpintoo force-pushed the recursive_nhg branch from 166785d to 0788fc1 Compare May 17, 2024 03:51

Gokulnath-Raja reviewed May 17, 2024

View reviewed changes

Gokulnath-Raja approved these changes May 17, 2024

View reviewed changes

venkatmahalingam approved these changes May 17, 2024

View reviewed changes

srj102 approved these changes May 18, 2024

View reviewed changes

srj102 merged commit 536f43a into sonic-net:master May 18, 2024

utpalkantpintoo deleted the recursive_nhg branch May 18, 2024 16:40

eddieruan-alibaba mentioned this pull request Sep 29, 2024

[fpmsyncd] Fpmsyncd Next Hop Table Enhancement #2919

Merged

Conversation

utpalkantpintoo commented Apr 7, 2024

Uh oh!

linux-foundation-easycla bot commented Apr 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

utpalkantpintoo commented May 2, 2024

Uh oh!

mssonicbld commented May 2, 2024

Uh oh!

azure-pipelines bot commented May 2, 2024

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!

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!

adyeung commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goomadao commented May 24, 2024

Uh oh!

utpalkantpintoo commented May 29, 2024

Uh oh!

goomadao commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

linux-foundation-easycla bot commented Apr 7, 2024 •

edited

Loading

goomadao commented Jun 6, 2024 •

edited

Loading