Skip to content

Add HLD for FRR-SONiC Communication Channel Enhancements - #1620

Merged
balajib-cisco merged 8 commits into
sonic-net:masterfrom
cscarpitta:frr-sonic-communication-channel-enhancements
May 29, 2024
Merged

Add HLD for FRR-SONiC Communication Channel Enhancements#1620
balajib-cisco merged 8 commits into
sonic-net:masterfrom
cscarpitta:frr-sonic-communication-channel-enhancements

Conversation

@cscarpitta

@cscarpitta cscarpitta commented Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

@cscarpitta
cscarpitta force-pushed the frr-sonic-communication-channel-enhancements branch 3 times, most recently from ad47b95 to dc8e158 Compare February 21, 2024 18:26
@cscarpitta
Add HLD for FRR-SONiC Protobuf Communication Channel
82033e7 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
cscarpitta force-pushed the frr-sonic-communication-channel-enhancements branch from dc8e158 to 82033e7 Compare February 21, 2024 18:29
zice312963205
zice312963205 approved these changes Feb 22, 2024
View reviewed changes

@zice312963205 zice312963205 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good

@prsunny

prsunny commented Feb 23, 2024

Copy link
Copy Markdown
Contributor

@cscarpitta , can you please check the following?

  1. Add a section in HLD for Warmboot implications.
  2. Any modifications to existing APP_DB schema with this proposal?
  3. Any sequencing issues causing dependency since now some are coming via netlink and some via protobuf?

kperumalbfn
kperumalbfn reviewed Feb 27, 2024
View reviewed changes
Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
* The FRR zebra daemon must load the FPM Protobuf module during startup.

To implement these modifications, a PR has been submitted to the FRR repository and is currently under review: https://github.com/FRRouting/frr/pull/14173.

@kperumalbfn kperumalbfn Feb 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These changes are not yet merged to FRR. Could you provide the targeted FRR release with protobuf change?

@cscarpitta cscarpitta Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As per the last SONIC Routing WG call, we have updated this HLD.
Now there is no dependency on FRR, because we are introducing a new FPM SONiC module under SONiC repo and we are moving to this module.

Could you please review the updated HLD?

Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
1. Generate an FPM message containing an SRv6 SID encoded using Protobuf.
2. Call `FpmLink::processFpmMessage()` with the generated FPM message.
3. Verify the delivery of the FPM message to RouteSync by checking if `RouteSync.onMsgRaw()` is invoked.
4. Ensure that the route is added to the SRV6_MY_SID_TABLE of APPL_DB.

@kperumalbfn kperumalbfn Feb 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any out of sequence issue when some information of MY_SID_TABLE uses NL and other information like SRV6 SID uses Protobuf?

@cscarpitta cscarpitta Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ditto. In the updated HLD, we are no longer using Protobuf. So the issue is resolved.

@cscarpitta
Update frr_sonic_protobuf_channel HLD to address review comments
c6adf44 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
arlakshm
arlakshm reviewed Feb 29, 2024
View reviewed changes
Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
Comment thread doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated
@cscarpitta cscarpitta changed the title Add HLD for FRR-SONiC Protobuf Communication Channel Add HLD for FRR-SONiC Communication Channel Enhancements Apr 18, 2024
@cscarpitta
Update HLD to address review comments
69c3599 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
cscarpitta force-pushed the frr-sonic-communication-channel-enhancements branch from abdda4b to 69c3599 Compare April 18, 2024 15:57
@cscarpitta
Update FPM SONiC HLD according to review comments
755e398 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
Update HLD to address review comments
7aa8b64 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
Add pointers to code PRs
cb5eb66 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta
Update HLD to address review comments
1f7f404 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
This was referenced Apr 24, 2024
Merged
Merged
@cscarpitta
Update HLD to address review comments
61a8732 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@cscarpitta

cscarpitta commented May 4, 2024

Copy link
Copy Markdown
Contributor Author

@cscarpitta , can you please check the following?

  1. Add a section in HLD for Warmboot implications.
  2. Any modifications to existing APP_DB schema with this proposal?
  3. Any sequencing issues causing dependency since now some are coming via netlink and some via protobuf?

@prsunny

Many thanks for the review.

  1. There is no impact on warmboot.
  2. No APP_DB schema modifications.
  3. We reviewed this proposal in the Routing WG meeting, and I updated the proposal based on the feedback of the community. Now Protobuf is no longer used. All messages are now sent via Netlink.

eddieruan-alibaba
eddieruan-alibaba approved these changes May 27, 2024
View reviewed changes

@eddieruan-alibaba eddieruan-alibaba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This approach has been discussed in routing WG.

@ahsalam

ahsalam commented May 28, 2024 •
edited
Loading

Copy link
Copy Markdown

@zhangyanzhao @balajib-cisco @kperumalbfn, this HLD has been discussed and approved in the SONiC Routing WG.
Eddie has approved the PR.
Could you please check and help to merge?
Thanks!

@eddieruan-alibaba

eddieruan-alibaba commented May 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

I could help to merge if no one has any other comments. We need this changes for Routing WG's PhoenixWing project, which upstreaming and validate SRv6 codes in 202411 release on various hardware platforms. The detail info for this project could be found at https://lists.sonicfoundation.dev/g/sonic-wg-routing/wiki/36798

@balajib-cisco
balajib-cisco merged commit ff26eeb into sonic-net:master May 29, 2024
@ahsalam ahsalam mentioned this pull request Oct 23, 2024
Closed
@zhangyanzhao
zhangyanzhao requested a review from r12f November 11, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kperumalbfn kperumalbfn kperumalbfn left review comments
@arlakshm arlakshm arlakshm left review comments
@eddieruan-alibaba eddieruan-alibaba eddieruan-alibaba approved these changes
@r12f r12f Awaiting requested review from r12f
+1 more reviewer
@zice312963205 zice312963205 zice312963205 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

SONiC 202411 Release
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@cscarpitta @prsunny @ahsalam @eddieruan-alibaba @kperumalbfn @zice312963205 @arlakshm @balajib-cisco