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

[fpmsyncd] Add support for SRv6 #2515

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cscarpitta
Copy link
Contributor

The dplane_fpm_nl module in FRR allows SONiC to learn the forwarding information computed by FRR. FRR already supports many SRv6 functionalities. There is an open PR (FRRouting/frr/pull/12301) to extend the dplane_fpm_nl module of FRR to export the required SRv6 information to SONiC.

OrchAgent (specifically, RouteOrch and SRv6Orch) in SONiC SWSS can already extract the SRv6 information from the APP_DB and push it towards the ASIC_DB (sonic-net/sonic-swss/pull/1964).

However, the fpmsyncd in SONiC SWSS does not process the SRv6 information received from FRR. This PR extends fpmsyncd to parse the SRv6 information received from FRR and push it toward the APP_DB.

What I did

I added support for SRv6 to fpmsyncd:

  • I added a handler onSrv6RouteMsg() which is able to parse Netlink messages containing an SRv6 Steering Route and create an entry in the ROUTE_TABLE of APP_DB.
  • I added a handler onSrv6LocalSidMsg() which is able to parse Netlink messages containing an SRv6 Local SID and create an entry in the SRV6_MY_SID_TABLE of APP_DB.
  • I changed onMsgRaw() to pass the Netlink messages to the appropriate handler depending on the encapsulation type.
  • I added the unit tests to verify that fpmsyncd processes the Netlink messages and updates the APP_DB correctly.

Why I did it

FRR supports many SRv6 functionalities. There is an open PR (FRRouting/frr/pull/12301) to extend the dplane_fpm_nl module of FRR to export the required SRv6 information to SONiC.

However, the fpmsyncd in SONiC SWSS did not process the SRv6 information received from FRR.

How I verified it

I added new unit tests in tests/mock_tests/fpmsyncd/receive_srv6_localsids_ut.cpp and tests/mock_tests/fpmsyncd/receive_srv6_steer_routes_ut.cpp.

Details if related
SRv6 HLD: https://github.com/sonic-net/SONiC/blob/master/doc/srv6/srv6_hld.md
SRv6 uSID HLD: https://github.com/sonic-net/SONiC/blob/master/doc/srv6/SRv6_uSID.md

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@cscarpitta cscarpitta force-pushed the feature/fpmsyncd-srv6-support branch 2 times, most recently from aed277f to a0920f2 Compare November 14, 2022 10:36
@reshmaintel
Copy link

@svshah-intel Hi Shitanshu, could you please help review this PR. Thanks.

}

vector<FieldValueTuple> fvVectorRoute;
if (!vpn_sid_str.empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not required since this check is already performed few lines above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it, thanks!

fvVector.push_back(adj);
}

m_srv6LocalSidTable.set(my_sid_table_key, fvVector);
Copy link
Contributor

Choose a reason for hiding this comment

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

srv6orch.cpp says
/* Key for mySid : block_len:node_len:function_len:args_len:sid-ip */

here looks like only sid-ip is used as my_sid_table_key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! I fixed it.

onSrv6LocalSidMsg(h, len);
break;
default:
onEvpnRouteMsg(h, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

it may be good to have a comment here to mention why default case handles onEvpnRouteMsg. I understand you are mainting functionality as before switch case, but for future reading it may be confusing without supporting comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment, thanks for the suggestion!


/* Duplicated delete as we do not know if it is a seg6 route, seg6local
* route or regular route */
m_srv6LocalSidTable.del(destipaddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. don't you need to check ip type to be v6 first?
  2. How about handling warmRestartInProgress check just like is handled for normal route delete?
  3. Same as earlier comments - srv6orch.cpp seems to be expecting more than just ip address as a key value

Copy link
Contributor Author

@cscarpitta cscarpitta May 11, 2023

Choose a reason for hiding this comment

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

I moved the delete code to onSrv6LocalSidMsg() and changed to use the correct key value.

* Adding the ability to pass the routes to the appropriate handler
depending on the encap type (getEncapType())
* Adding a handler onSrv6RouteMsg() for parsing the routes containing
SRv6 steer route nexthops
(i.e., routes with encap type NH_ENCAP_SRV6_ROUTE)
* Adding the ability to parse SRv6 steer route nexthops
(getSrv6SteerRouteNextHop())
* Adding the ability to decode a Netlink message containing an SRv6
steer route nexthop (parseEncapSrv6SteerRoute())
* Adding the ability to include the SRv6 information in the entries
pushed to the ROUTE_TABLE

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
* Adding a handler onSrv6LocalSidRouteMsg() for parsing the routes
containing SRv6 Local SID nexthops (i.e., routes with encap type
NH_ENCAP_SRV6_LOCAL_SID)
* Adding the ability to parse SRv6 Local SID nexthops
(getSrv6LocalSidNextHop())
* Adding the ability to decode a Netlink message containing an SRv6
Local SID nexthop (parseEncapSrv6LocalSid())
* Adding the ability to decode a Netlink message containing the format
of an SRv6 Local SID (parseEncapSrv6LocalSidFormat())
* Adding the ability to write an SRv6 Local SID entry to APP_DB
* Adding the ability to delete SRv6 Local SIDs from APP_DB

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
* Added an helper function (`create_srv6_steer_route_nlmsg()`) to build
a Netlink request message for adding an SRv6 steering route.
* Added an helper function (`create_srv6_localsid_nlmsg()`) to build a
Netlink request message for adding an SRv6 local SID.
* Add several helper functions for adding Netlink attributes to an existing Netlink message.
* Add a mock function (`__wrap_rtnl_link_i2name()`) that simulates calls
to `rtnl_link_i2name()` to get the name of an interface.

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
@cscarpitta cscarpitta force-pushed the feature/fpmsyncd-srv6-support branch from a0920f2 to 697a940 Compare May 11, 2023 07:00
@cscarpitta
Copy link
Contributor Author

@svshah-intel Many thanks for your review! I addressed all your comments. Could you please take another look?

* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 steer route with an IPv4 destination prefix
(`RecevingSRv6SteerRoutesWithIPv4Prefix`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 steer route with an IPv6 destination prefix
(`RecevingSRv6SteerRoutesWithIPv6Prefix`).

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the End.DT4 behavior
(`RecevingRouteWithSRv6LocalSIDEndDT4`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the End.DT6 behavior
(`RecevingRouteWithSRv6LocalSIDEndDT6`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the End.DT46 behavior
(`RecevingRouteWithSRv6LocalSIDEndDT46`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the uDT4 behavior
(`RecevingRouteWithSRv6LocalSIDUDT4`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the uDT6 behavior
(`RecevingRouteWithSRv6LocalSIDUDT6`).
* Added a unit test to check that fpmsyncd updates the APP_DB correctly
when it receives an SRv6 Local SID bound to the uDT46 behavior
(`RecevingRouteWithSRv6LocalSIDUDT46`).

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
* Ignore the `tests_fpmsyncd` binary

Signed-off-by: Carmine Scarpitta <carmine.scarpitta@uniroma2.it>
@cscarpitta cscarpitta force-pushed the feature/fpmsyncd-srv6-support branch from 697a940 to 6513fc5 Compare May 16, 2023 15:42
liuyuefengcn added a commit to liuyuefengcn/sonic-swss that referenced this pull request Apr 15, 2024
liuyuefengcn added a commit to ubinexus/sonic-swss that referenced this pull request Sep 24, 2024
…net#2515 · sonic-net/sonic-swss (github.com)

2.Del flavor: SAI_MY_SID_ENTRY_ATTR_ENDPOINT_BEHAVIOR_FLAVOR
3.Fix createUpdateSidList bug
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.

3 participants