Virtual Router Redundancy Protocol Adaptation HLD

[philo-micas]

Hld for VRRP adaptation to SONiC, as mentioned in issue: #1366:

Repo	PR title	State
sonic-buildimage	sonic-net/sonic-buildimage#18617
sonic-swss	sonic-net/sonic-swss#3106, sonic-net/sonic-swss#3313, sonic-net/sonic-swss#3315
sonic-swss-common	sonic-net/sonic-swss-common#813
sonic-utilities	sonic-net/sonic-utilities#2949

[nser77]

Hi all, adding the VRRP protocol implementation in SONiC would be a great improvement for me; really good job.

While reading the HLD, I left some comments (which for GitHub looks like a review); I hope those are not off-topics.

[philo-micas]

Hi all, adding the VRRP protocol implementation in SONiC would be a great improvement for me; really good job.

While reading the HLD, I left some comments (which for GitHub looks like a review); I hope those are not off-topics.

Thanks! The comments are helpful, wish for more reviewing comments.

[nser77]

@philo-micas, many thanks for your answers.

Just want you to know that I'm pointing to another scenario that we may need to add in the #restrictionslimitations section (or even fix it), maybe a possible security scenario.

Using your diagram doc/vrrp/images/VRRP_Tracking_Interface_Scenarios_Diagram.png as base, my concern is connected to the interface where vrrp_advs are sent, but let me investigate and elaborate it a bit more.

[prvattem]

@philo-micas There was a review request covering VRRP functionality earlier.
#475
Please see if it can be combined with your proposal.

[philo-micas]

@philo-micas There was a review request covering VRRP functionality earlier. #475 Please see if it can be combined with your proposal.

@prvattem From the aspect of RFC, two proposals have already combined, please check the CLI and Functionality session, but from up layer service, one is based on keepactived, the other is based on FRR, they can't be combined, the adaptation exit some difference.

[zhangyanzhao]

SONiC community review recording https://zoom.us/rec/share/_PaY38c6vF8N6iAWALtMg1Rt0d9Ou8Guf4_ZRNayRYobIPq0jryZv2AsRE-OlLx5.3O_GA7L8vNPjWhkQ

[zhangyanzhao]

BRCM register as a reviewer. @adyeung

[zhangyanzhao]

@philo-micas can you please help to check the code PRs? Will Micas target 202405 release for this feature or move to future release? Thanks.

[philo-micas]

@philo-micas can you please help to check the code PRs? Will Micas target 202405 release for this feature or move to future release? Thanks.

Code change will be ready soon, and will have a revivew at 3/28 Routing WG meeting. Keep target 202405. Thanks.

[adyeung]

@prvattem @nser77 @madhupalu @vvbrcm @lguohan @venkatmahalingam pls help review the latest change, signoff if you agree with the design, we are tracking this for 202405. The HLD has also been presented and reviewed in the Routing WG on 4/11/24, recording link https://lists.sonicfoundation.dev/g/sonic-wg-routing/wiki/36530

[nser77]

Hi all, many thanks for that and for your work.

I just have one major concern regarding the actual and the future architecture: why is vrrpmgr adding and deleting VIPs into VMAC interfaces? This is not done by FRR/vrrpd? I don't know if the code is valid or ever triggered, but I don't see any reason to manage that part from the vrrpmgr and to put that code there.

Apart of that, and as also other users suggested, I also believe that warm reboot can be covered by this HLD.

Many thanks and regards.

[philo-micas]

Hi all, many thanks for that and for your work.

I just have one major concern regarding the actual and the future architecture: why is vrrpmgr adding and deleting VIPs into VMAC interfaces? This is not done by FRR/vrrpd? I don't know if the code is valid or ever triggered, but I don't see any reason to manage that part from the vrrpmgr and to put that code there.

Apart of that, and as also other users suggested, I also believe that warm reboot can be covered by this HLD.

Many thanks and regards.

Since we need to transmit vip and vmac to OA, appl db need them.
and the whole proposal support cli configuration, so a mgrd is needed to do the data transmission from config db to appl db
(since other protocol like keepactive will config function direct to kernel, and kernel send data to vrrpsyncd which will transmit data into appl db), And why we not do it in vrrpd, since it will need some system tool to set vip or vmac, like iproute2, but other OS may support ifupdown2, as FRR is open source, will be much more compatible not do it in vrrpd.

[nser77]

Hi all, many thanks for that and for your work.
I just have one major concern regarding the actual and the future architecture: why is vrrpmgr adding and deleting VIPs into VMAC interfaces? This is not done by FRR/vrrpd? I don't know if the code is valid or ever triggered, but I don't see any reason to manage that part from the vrrpmgr and to put that code there.
Apart of that, and as also other users suggested, I also believe that warm reboot can be covered by this HLD.
Many thanks and regards.

Since we need to transmit vip and vmac to OA, appl db need them. and the whole proposal support cli configuration, so a mgrd is needed to do the data transmission from config db to appl db (since other protocol like keepactive will config function direct to kernel, and kernel send data to vrrpsyncd which will transmit data into appl db), And why we not do it in vrrpd, since it will need some system tool to set vip or vmac, like iproute2, but other OS may support ifupdown2, as FRR is open source, will be much more compatible not do it in vrrpd.

Hi @philo-micas, I understand your point of view and I will certainly take the time to dig deeper - many thanks for your answer and for your input.

I'm ok with vrrpsyncd, I agree that it's necessary.

Regarding my concern, and maybe due a lack of knowledge, I'm still not sure how vrrpmgr will be able to add and delete VIPs into VMAC interfaces in a efficient way without knowing the VRRP's machine state, which is managed by FRR/vrrpd through the protocol implementation.

Maybe, renaming vrrpmgr into macvlanmgr and sharing it across the whole SONiC architecture should be evaluated and as also other users pointed in the past it could help to bring more features in the future.

Regards,

[philo-micas]

Hi all, many thanks for that and for your work.
I just have one major concern regarding the actual and the future architecture: why is vrrpmgr adding and deleting VIPs into VMAC interfaces? This is not done by FRR/vrrpd? I don't know if the code is valid or ever triggered, but I don't see any reason to manage that part from the vrrpmgr and to put that code there.
Apart of that, and as also other users suggested, I also believe that warm reboot can be covered by this HLD.
Many thanks and regards.

Since we need to transmit vip and vmac to OA, appl db need them. and the whole proposal support cli configuration, so a mgrd is needed to do the data transmission from config db to appl db (since other protocol like keepactive will config function direct to kernel, and kernel send data to vrrpsyncd which will transmit data into appl db), And why we not do it in vrrpd, since it will need some system tool to set vip or vmac, like iproute2, but other OS may support ifupdown2, as FRR is open source, will be much more compatible not do it in vrrpd.

Hi @philo-micas, I understand your point of view and I will certainly take the time to dig deeper - many thanks for your answer and for your input.

I'm ok with vrrpsyncd, I agree that it's necessary.

Regarding my concern, and maybe due a lack of knowledge, I'm still not sure how vrrpmgr will be able to add and delete VIPs into VMAC interfaces in a efficient way without knowing the VRRP's machine state, which is managed by FRR/vrrpd through the protocol implementation.

Maybe, renaming vrrpmgr into macvlanmgr and sharing it across the whole SONiC architecture should be evaluated and as also other users pointed in the past it could help to bring more features in the future.

Regards,

@nser77 Thanks, it is a good idea, I have accepted and updated HLD. @adyeung help forward it, thanks!

[vvbrcm]

These changes look fine. Approved.

[vvbrcm]

Changes look fine. Approved.

[philo-micas]

@prsunny @lguohan @adyeung Please check if more reviewers are needed for this PR, otherwise, please help merge it. Thanks!

[adyeung]

This HLD has been reviewed in Routing WG, and SONiC Community call. I will merge the HLD if there is no further comments by the end of the week

[nmoray]

@philo-micas As per the HLD, I am unable to find the changes related to vrrpsyncd and vrrporch in any of your PRs. Can you please point me to the respective PR?
cc: @venkatmahalingam

[philo-micas]

@philo-micas As per the HLD, I am unable to find the changes related to vrrpsyncd and vrrporch in any of your PRs. Can you please point me to the respective PR? cc: @venkatmahalingam

@nmoray The vrrporch and vrrpsyncd sessions have already been included in this HLD. As for the code PR related to them, let's move to the SWSS code PR: sonic-net/sonic-swss#3106. Thanks!