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

Introduce a SONiC-specific communication channel between FRR and SONiC #18715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce a SONiC-specific communication channel between FRR and SONiC #18715

wants to merge 1 commit into from
+2,310 −2

Conversation

cscarpitta
Copy link

@cscarpitta cscarpitta commented Apr 18, 2024 •
edited
Loading

Why I did it

Introduce a SONiC-specific communication channel between FRR and SONiC.
HLD: sonic-net/SONiC#1620

How I did it

Introduce a SONiC-specific communication channel between FRR and SONiC (FPM SONiC module).

How to verify it

By switching to the new FPM SONiC module, all existing test cases for all features are successfully passed.

Description for the changelog

  • Add new FPM SONiC module
  • Add a patch file "build-dplane-fpm-sonic-module.patch" to compile FRR with the new FPM SONiC module
  • Change supervisord.conf.j2 of the docker-fpm-frr container to load the new FPM SONiC module on FRR startup

@mssonicbld
Copy link
Collaborator

@StormLiangMS,@liushilongbuaa PR: #18715 is conflict with MS internal repo
Please push fix commit to sonicbld/precheck/head/18715 and approve
https://msazure.visualstudio.com/One/_git/Networking-acs-buildimage/pullrequest/9922845
After ms PR is merged, comment "/azpw ms_conflict" to rerun PR checker.

@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

@cscarpitta cscarpitta changed the title Enhance communication channel between FRR and SONiC Introduce new FRR-SONiC communication channel (FPM SONiC module) Apr 24, 2024
@cscarpitta cscarpitta changed the title Introduce new FRR-SONiC communication channel (FPM SONiC module) Introduce SONiC-specific communication channel between FRR and SONiC Apr 24, 2024
@cscarpitta cscarpitta changed the title Introduce SONiC-specific communication channel between FRR and SONiC Introduce a SONiC-specific communication channel between FRR and SONiC Apr 24, 2024
@cscarpitta cscarpitta mentioned this pull request Apr 24, 2024
Merged
@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict

lguohan
lguohan reviewed May 12, 2024
View reviewed changes
platform/vs/docker-sonic-vs/supervisord.conf.j2
@@ -164,7 +164,7 @@ environment=ASAN_OPTIONS="log_path=/var/log/asan/teammgrd-asan.log{{ asan_extra_
{% endif %}

[program:zebra]
command=/usr/lib/frr/zebra -A 127.0.0.1 -s 90000000 -M dplane_fpm_nl --asic-offload=notify_on_offload
command=/usr/lib/frr/zebra -A 127.0.0.1 -s 90000000 -M dplane_fpm_sonic --asic-offload=notify_on_offload
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better if we do not change it by default, it is better to first introduce this as option and then wait till it becomes stable.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @lguohan

Many thanks for the review.

I added a compile-time option ENABLE_DPLANE_FPM_SONIC.

When this option is set, SONiC uses the new FPM module.
When this option is not set, SONiC uses the old FPM module and behaves exactly as it does today.

By default, this option is not set.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i am fine that we build them into image by default, but do not enable it by default. if you can do it at runtime, it will be easier for you, and i am fine with that.

does this dplane_fpm_sonic support all features as dplane_fpm_nl?

Copy link
Author

Choose a reason for hiding this comment

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

@lguohan

Yes, the dplane_fpm_sonic supports all features as dplane_fpm_nl. It is a copy of the old dplane_fpm_nl.

Indeed, it is easier to compile this by default and keep the old dplane_fpm_nl running by default.
I reverted the compile option and changed the supervisord config file to keep dplane_fpm_nl as default.

@cscarpitta cscarpitta force-pushed the dplane_fpm_sonic_master branch 3 times, most recently from ce8dd1f to 4831515 Compare May 17, 2024 06:40
@lguohan
Copy link
Collaborator

lguohan commented May 18, 2024

@cscarpitta , have we sync with frr community to see if we can add this plugin into frr repo and get maintainer there? I think this can be facilitated by the routing group? @kperumalbfn , and @eddieruan-alibaba ?

if we cannot get into frr repo, for this plugin, can we maintain as a patch format to fpm_nl? so that we can still keep track of upstream changes. What if there is some new changes to dplane_fpm_nl and we need those changes into dplane_fpm_sonic?

@eddieruan-alibaba
Copy link
Collaborator

@cscarpitta , have we sync with frr community to see if we can add this plugin into frr repo and get maintainer there? I think this can be facilitated by the routing group? @kperumalbfn , and @eddieruan-alibaba ?

if we cannot get into frr repo, for this plugin, can we maintain as a patch format to fpm_nl? so that we can still keep track of upstream changes. What if there is some new changes to dplane_fpm_nl and we need those changes into dplane_fpm_sonic?

Discussed this with Donald Sharp in Routing WG. The conclusion was that Donald didn't want to take it into FRR community since it is SONiC specific. This proposal was proposed via Mark Stepp from Cisco who worked in FRR fpm before and supported by Donald who owns FRR. The discussion meeting minutes could be found in https://lists.sonicfoundation.dev/g/sonic-wg-routing/wiki/36598.

The basic idea is to let SONiC team own sonic fpm. The zebra/fpm apis have already been changed with versioning by Mark S. Since we own sonic fpm, I feel it is simple to keep dplane_fpm_sonic.c as a file instead of as a diff.

@cscarpitta
Copy link
Author

@cscarpitta , have we sync with frr community to see if we can add this plugin into frr repo and get maintainer there? I think this can be facilitated by the routing group? @kperumalbfn , and @eddieruan-alibaba ?

if we cannot get into frr repo, for this plugin, can we maintain as a patch format to fpm_nl? so that we can still keep track of upstream changes. What if there is some new changes to dplane_fpm_nl and we need those changes into dplane_fpm_sonic?

@lguohan As Eddie mentioned, the option of adding the new changes as a patch was discussed in the Routing WG. However there were some concerns on this approach and the consensus was that this will be maintained as a separate module under the sonic-buildimage repository

Regarding the second point, in case of new features or bug fixes or improvements of dplane_fpm_nl, as part of the FRR upgrade process we will make sure that all the changes will be reflected in the dplane_fpm_sonic.

@eddieruan-alibaba
Copy link
Collaborator

For the platform which wants to use dplane_fpm_sonic.so, how does this .so get picked up? Do you want to add a README file for that?

@cscarpitta cscarpitta force-pushed the dplane_fpm_sonic_master branch 2 times, most recently from d79ee30 to 2590bc9 Compare May 29, 2024 15:14
@cscarpitta
Copy link
Author

For the platform which wants to use dplane_fpm_sonic.so, how does this .so get picked up? Do you want to add a README file for that?

Hi @eddieruan-alibaba Many thanks for the review.

I added a README file to explain how to enable the dplane_fpm_sonic module.

@cscarpitta
Copy link
Author

/azpw ms_conflict

@cscarpitta
Copy link
Author

@liushilongbuaa @StormLiangMS Can you please help fix the ms_conflict?

@cscarpitta
Copy link
Author

@lguohan

The HLD for this feature has been merged: sonic-net/SONiC#1620

We addressed all comments on this PR. Can you please take a look again and help merge?

@liushilongbuaa
Copy link
Contributor

/azpw ms_conflict -f

@eddieruan-alibaba
Copy link
Collaborator

For the platform which wants to use dplane_fpm_sonic.so, how does this .so get picked up? Do you want to add a README file for that?

Hi @eddieruan-alibaba Many thanks for the review.

I added a README file to explain how to enable the dplane_fpm_sonic module.

Thanks Carmine. The readme explains the changes needed. As part of Phoenix wing project, we could change the .j2 based on the supported platform type.

@eddieruan-alibaba
Copy link
Collaborator

@lguohan currently, he just commit the changes needed as infra. It would not be enabled on the current platforms. As part of phoenix wing project, we could enable them on the platforms verified in that project.

@ysj-alibaba
Copy link

@cscarpitta I don't understand what does DPLAINE_OP_STARTUP_STAGE mean and it seems that there is no definition about this flag. Thanks.

@eddieruan-alibaba
Copy link
Collaborator

This PR's changes are cherry picked to Phoenixwing Repro and get validated.

@cscarpitta cscarpitta force-pushed the dplane_fpm_sonic_master branch 2 times, most recently from 1861641 to 8cac5df Compare September 15, 2024 17:05
@kperumalbfn
Copy link
Contributor

@cscarpitta could you check the buildimage failures and VS test failurs?

@cscarpitta
Introduce new FRR-SONiC communication channel (FPM SONiC module).
293af24 
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@kperumalbfn
Copy link
Contributor

/azpw ms_conflict -f

kperumalbfn
kperumalbfn approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@kperumalbfn kperumalbfn left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge after resolving the conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lguohan lguohan lguohan left review comments

@kperumalbfn kperumalbfn kperumalbfn approved these changes

@ahsalam ahsalam ahsalam approved these changes

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

@qiluo-msft qiluo-msft Awaiting requested review from qiluo-msft

@xumia xumia Awaiting requested review from xumia

Assignees
No one assigned
Labels
FRR 🚥
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cscarpitta @mssonicbld @liushilongbuaa @lguohan @eddieruan-alibaba @ysj-alibaba @kperumalbfn @ahsalam