Changes for handling recursive NHG entries by nkelapur · Pull Request #1636 · sonic-net/SONiC

nkelapur · 2024-03-14T16:10:38Z

Changes to handle recursive NHG entries

Repo	Title	PR
sonic-swss	Recursive nexthop group enhancement	sonic-net/sonic-swss#3105

nkelapur · 2024-04-03T06:26:19Z

This change is part of #1594

adyeung · 2024-04-03T19:43:26Z

@nakano-omw please help review the change in nexthop group

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.

prsunny · 2024-04-10T23:20:00Z

@nkelapur , @utpalkantpintoo, is this discussed in Sonic community meeting?

adyeung · 2024-04-10T23:25:00Z

@prsunny the proposal was presented and reviewed in the Routing WG meetings, this is a HLD submission based on the WG discussion

prsunny · 2024-04-11T00:06:23Z

@prsunny the proposal was presented and reviewed in the Routing WG meetings, this is a HLD submission based on the WG discussion

ack

hasan-brcm · 2024-04-16T22:22:34Z

doc/pic/hld_fpmsyncd.md


 This `Fpmsyncd extension` will modify `fpmsyncd` to handle  `RTM_NEWNEXTHOP` and `RTM_DELNEXTHOP` as below.

+This featured introduces new orchagent `NhgOrch` to handle `RTM_NEWNEXTHOP` events and update `NEXTHOP_GROUP_TABLE` entries accordingly


The "NhgOrch" already exists. The HLD/PR enhances the NhgOrch to handle recursive nexthop groups.

hasan-brcm · 2024-04-16T22:26:27Z

doc/pic/hld_fpmsyncd.md


+#### Orchestration Agent Changes to handle NEXTHOP_GROUP
+
+A new orchestration agent `NhgOrch` will be written to handle the new NEXTHOP_GROUP_TABLE in APP_DB. For adding or updating an entry in the NEXTHOP_GROUP_TABLE, programming will depend on whether the group is configured with one or multiple next hops or is a recursive nexthop group i.e. it contains one or more nexthop group.


Same as previous comment. NhgOrch is extended to handle recursive NEXTHOP_GROUP_TABLE entries in app-db.

hasan-brcm · 2024-04-16T22:30:30Z

doc/pic/hld_fpmsyncd.md

+    nexthop       = *prefix,           ; IP addresses separated
+                     “,” (empty indicates no gateway)
+    ifname        = *PORT_TABLE.key,   ; zero or more separated 
+                     by “,” (zero indicates no interface)


The above fields already exist as part of the NEXT_HOP_GROUP_TABLE:

SONiC/doc/ip/next_hop_group_hld.md

Line 85 in f799bf2

### NEXT_HOP_GROUP_TABLE

Please mark "nexthop_group" field as newly added by this HLD.

This table is new in this HLD. Added "existing" and "new" fields labels for the "ROUTE_TABLE"

hasan-brcm · 2024-04-16T22:52:08Z

doc/pic/hld_fpmsyncd.md

+```
+

 ### Warmboot and Fastboot Design Impact  


Nexthop group ID reconciliation as part of the warm-boot is not currently supported.

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.

Updated with review comments

adyeung · 2024-05-03T17:57:12Z

@arlakshm the HLD has been reviewed in the Routing WG, let us know if you have further comment, team is looking to close next week

hasan-brcm · 2024-05-03T21:16:13Z

doc/pic/hld_fpmsyncd.md

@@ -484,6 +564,7 @@ Mention whether this feature/enhancement has got any requirements/dependencies/i

 - When the feature is disabled, there should be no impact to Warmboot and Fastboot.
 - When the feature is enabled, there will be no warmboot nor fastboot support.


Fast boot can be supported. Only WB will have limitations due to reconciliation of nhids.

hasan-brcm · 2024-05-03T21:17:38Z

doc/pic/hld_fpmsyncd.md

 ```
+#### APPL DB Enhancements  
+
+This feature adds a new NEXT_HOP_GROUP_TABLE, to store next hop group information to be used by one or more routes. Recursive/ecmp routes are represented by referencing a list of nexthop group ids in the nexthop_group field. When this field is used, other fields will not be present.


"adds a new .." => "extends the existing .."

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.

Updated as per review 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.

venkatmahalingam · 2024-05-13T05:05:37Z

doc/pic/hld_fpmsyncd.md

+                    application.
+    nexthop       = *prefix,           ; IP addresses separated
+                     “,” (empty indicates no gateway)
+    ifname        = *PORT_TABLE.key,   ; zero or more separated 


Why just PORT_TABLE? how about portchannel, vlan and sub-interfaces..etc

There is no change in this DB Table. This is as is from https://github.com/sonic-net/SONiC/blob/master/doc/ip/next_hop_group_hld.md

Do we allow NH interfaces other than PORT_TABLE?

Yes, as I understand all possible existing NH interface would be allowed

Just mentioning PORT_TABLE alone the schema is weird, please fix it.

Changed to "INTF_TABLE" so that all types on interfaces are covered

venkatmahalingam · 2024-05-13T05:07:04Z

doc/pic/hld_fpmsyncd.md

+                     “,” (empty indicates no gateway)
+    ifname        = *PORT_TABLE.key,   ; zero or more separated 
+                     by “,” (zero indicates no interface)
+    nexthop_group = NEXT_HOP_GROUP_TABLE:key, ; index within the


can you add an example for the recursive routes?

The DB entry for recursive NHG is same as ECMP NHGs. In that for recursive NHG may contain only one NH and hence the NHA_GROUP will contain only one NHID

venkatmahalingam · 2024-05-13T05:08:27Z

doc/pic/hld_fpmsyncd.md

- When `fpmsyncd` receive the `RTM_NEWROUTE` on sequence 4, the process will write the NextHop Group with ID 118 to the `NEXTHOP_GROUP_TABLE` using the information of gateway and interface from the NextHop Group events with IDs 116 and 117.
- Then `fpmsyncd` will create a new route entry to `ROUTE_TABLE` with a `nexthop_group` field with value `ID118`.
- When `fpmsyncd` receives the last `RTM_NEWROUTE` on sequence 5, the process will create a new route entry (but no NextHop Group entry) in `ROUTE_TABLE` with `nexthop_group` field with value `ID118`. (Note: This NextHop Group entry was created when the `fpmsyncd` received the event sequence 4.)
+- When receiving `RTM_NEWNEXTHOP` events on sequence 1 and 2, `fpmsyncd` will write the information in `NEXTHOP_GROUP_TABLE` with NextHop Group `ID125` and `ID126` and with information of gateway and interface respectively. For sequence 3  multi path NextHop Group `ID127` the information in `NEXTHOP_GROUP_TABLE` will contain the list of IDs 125 and 126


What changes are expected towards kernel programming? please capture those details if not captured in a separate document.

No new changes are required from frr/kernel side for this feature. When NHG is enabled in FRR, it already programs routes to kernel using reference to the NHID. This implementation in fact make the FPM programming similar to how the routes are being programmed in kernel already

Gokulnath-Raja · 2024-05-14T05:42:18Z

doc/pic/hld_fpmsyncd.md

+

 #### Example of entries in ASIC_DB



Please capture ASIC_DB update for NHG group with multipath

Added diagram to show the updated ASIC_DB entry

Gokulnath-Raja · 2024-05-14T06:25:06Z

doc/pic/hld_fpmsyncd.md


+#### Orchestration Agent Changes to handle NEXTHOP_GROUP
+
+Orchestration agent `NhgOrch` will be enhanced to handle the new NEXTHOP_GROUP_TABLE in APP_DB. For adding or updating an entry in the NEXTHOP_GROUP_TABLE, programming will depend on whether the group is configured with one or multiple next hops or is a recursive nexthop group i.e. it contains one or more nexthop group.


Could see code related to handling of Loopback interface and NHID added as CPU port, please capture here if required.

This change has been removed from the code PR now

Gokulnath-Raja · 2024-05-14T06:32:40Z

doc/pic/hld_fpmsyncd.md

 ![fig3](images_fpmsyncd/fig3.svg)

+#### Orchestration Agent Changes to handle NEXTHOP_GROUP
+


Please capture the case NH as Interface?

"NH as Interface" will be same as any other NHG. The application ( zebra) will create a NHID for the NHG with NH as interface and the route will use this NHID as reference to the NH

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.

Asic DB Update Diagram

Updated with review 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.

adyeung · 2024-05-16T16:18:21Z

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

* 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

nkelapur added 2 commits March 13, 2024 21:04

Changes to handle recursive NHG entries

cb1ba9f

typo correction

3a690fc

nkelapur marked this pull request as ready for review March 14, 2024 16:11

nkelapur mentioned this pull request Apr 3, 2024

Changes to handling of single path and multi path NextHOP Group msg #1602

Closed

nkelapur changed the title ~~Changes to handle recursive NHG entries~~ Changes for handling recursive NHG entries Apr 3, 2024

utpalkantpintoo mentioned this pull request Apr 7, 2024

[Orchagent] Recursive nexthop group enhancement sonic-net/sonic-swss#3105

Merged

prsunny requested a review from arlakshm April 11, 2024 00:06

hasan-brcm reviewed Apr 17, 2024

View reviewed changes

Update hld_fpmsyncd.md

e869889

Updated with review comments

hasan-brcm reviewed May 3, 2024

View reviewed changes

Update hld_fpmsyncd.md

481cffc

Updated as per review comments

hasan-brcm approved these changes May 6, 2024

View reviewed changes

venkatmahalingam reviewed May 13, 2024

View reviewed changes

Gokulnath-Raja reviewed May 14, 2024

View reviewed changes

nkelapur added 3 commits May 14, 2024 15:37

ASIC DB Update Diagram

1066333

Asic DB Update Diagram

Update hld_fpmsyncd.md

a1230bb

Updated with review comments

Update hld_fpmsyncd.md

5b11b67

Updated with review comments

Gokulnath-Raja approved these changes May 17, 2024

View reviewed changes

venkatmahalingam approved these changes May 17, 2024

View reviewed changes

adyeung approved these changes May 17, 2024

View reviewed changes

adyeung merged commit 867237c into sonic-net:master May 17, 2024


		This `Fpmsyncd extension` will modify `fpmsyncd` to handle `RTM_NEWNEXTHOP` and `RTM_DELNEXTHOP` as below.

		This featured introduces new orchagent `NhgOrch` to handle `RTM_NEWNEXTHOP` events and update `NEXTHOP_GROUP_TABLE` entries accordingly


		#### Orchestration Agent Changes to handle NEXTHOP_GROUP

		A new orchestration agent `NhgOrch` will be written to handle the new NEXTHOP_GROUP_TABLE in APP_DB. For adding or updating an entry in the NEXTHOP_GROUP_TABLE, programming will depend on whether the group is configured with one or multiple next hops or is a recursive nexthop group i.e. it contains one or more nexthop group.

		@@ -484,6 +564,7 @@ Mention whether this feature/enhancement has got any requirements/dependencies/i

		- When the feature is disabled, there should be no impact to Warmboot and Fastboot.
		- When the feature is enabled, there will be no warmboot nor fastboot support.


		#### Orchestration Agent Changes to handle NEXTHOP_GROUP

		Orchestration agent `NhgOrch` will be enhanced to handle the new NEXTHOP_GROUP_TABLE in APP_DB. For adding or updating an entry in the NEXTHOP_GROUP_TABLE, programming will depend on whether the group is configured with one or multiple next hops or is a recursive nexthop group i.e. it contains one or more nexthop group.

		![fig3](images_fpmsyncd/fig3.svg)

		#### Orchestration Agent Changes to handle NEXTHOP_GROUP

Conversation

nkelapur commented Mar 14, 2024 • edited by adyeung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkelapur commented Apr 3, 2024

Uh oh!

adyeung commented Apr 3, 2024

Uh oh!

prsunny commented Apr 10, 2024

Uh oh!

adyeung commented Apr 10, 2024

Uh oh!

prsunny commented Apr 11, 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!

adyeung commented May 3, 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!

nkelapur May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

nkelapur commented Mar 14, 2024 •

edited by adyeung

Loading

nkelapur May 14, 2024 •

edited

Loading