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.

Sign up for GitHub

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

Jump to bottom

Add HLD for FRR-SONiC Communication Channel Enhancements #1620

Merged
merged 8 commits into from
May 29, 2024
Merged

Add HLD for FRR-SONiC Communication Channel Enhancements #1620

merged 8 commits into from
May 29, 2024

Conversation

cscarpitta
Copy link
Contributor

@cscarpitta cscarpitta commented Feb 21, 2024 •
edited
Loading

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

@zice312963205 zice312963205 left a comment

Choose a reason for hiding this comment

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

Looks good

@prsunny
Copy link
Contributor

prsunny commented Feb 23, 2024

@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
doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated Show resolved Hide resolved
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.

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

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

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

Copy link
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
doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated Show resolved Hide resolved
doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated Show resolved Hide resolved
doc/fpmsyncd/frr_sonic_protobuf_channel.md Outdated Show resolved Hide resolved
@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
Copy link
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
Copy link
Contributor

@eddieruan-alibaba eddieruan-alibaba left a comment

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

ahsalam commented May 28, 2024 •
edited
Loading

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

eddieruan-alibaba commented May 28, 2024 •
edited
Loading

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
1 check passed
@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

@arlakshm arlakshm arlakshm left review comments

@kperumalbfn kperumalbfn kperumalbfn left review comments

@eddieruan-alibaba eddieruan-alibaba eddieruan-alibaba approved these changes

@zice312963205 zice312963205 zice312963205 approved these changes

@r12f r12f Awaiting requested review from r12f

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