Virtual Router Redundancy Protocol Adaptation HLD

[philo-micas]

Hld for VRRP adaptation to SONiC, as mentioned in issue: https://github.com/sonic-net/SONiC/issues/1366:

Repo	PR title	State
sonic-buildimage	https://github.com/sonic-net/sonic-buildimage/pull/18617
sonic-swss	https://github.com/sonic-net/sonic-swss/pull/3106
sonic-swss-common	https://github.com/sonic-net/sonic-swss-common/pull/813
sonic-utilities	https://github.com/sonic-net/sonic-utilities/pull/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. https://github.com/sonic-net/SONiC/pull/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

[nser77]

Hi all, sorry to spam again, but I need to do it because it affects some of my previous comments (not reviews); I am only just aware that VRRP has a RFC YANG data model.

I found these sections that may cover other questions as well:

[...]

  /*  identity and its derivatives. */
  identity vrrp-state-type {
    description
      "Indicates the state of a virtual router.";
  }
  identity initialize {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is waiting
       for a startup event.";
  }
  identity backup {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is monitoring the
       availability of the master router.";
  }
  identity master {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is forwarding
       packets for IP addresses that are associated with
       this virtual router.";
  }

[...]

    leaf priority {
      type uint8 {
        range "1..254";
      }
      default 100;
      description
        "Configures the VRRP election priority for the backup
         virtual router.";
    }

    leaf is-owner {
      type boolean;
      config false;
      description
        "Set to 'true' if this virtual router is the owner.";
    }

       leaf state {
         type identityref {
           base vrrp:vrrp-state-type;
         }
         config false;
         description
           "Operational state.";
       }

[...]

I think that In the case of preempt and priority, @philo-micas's suggestions were certainly correct.

I also think we should try to follow the RFC 8347 as much as possible.

---

GitHub source: https://github.com/YangModels/yang/blob/main/standard/ietf/RFC/ietf-vrrp%402018-03-13.yang

[prvattem]

Hi all, sorry to spam again, but I need to do it because it affects some of my previous comments (not reviews); I am only just aware that VRRP has a RFC YANG data model.

I found these sections that may cover other questions as well:

[...]

  /*  identity and its derivatives. */
  identity vrrp-state-type {
    description
      "Indicates the state of a virtual router.";
  }
  identity initialize {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is waiting
       for a startup event.";
  }
  identity backup {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is monitoring the
       availability of the master router.";
  }
  identity master {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is forwarding
       packets for IP addresses that are associated with
       this virtual router.";
  }

[...]

    leaf priority {
      type uint8 {
        range "1..254";
      }
      default 100;
      description
        "Configures the VRRP election priority for the backup
         virtual router.";
    }

    leaf is-owner {
      type boolean;
      config false;
      description
        "Set to 'true' if this virtual router is the owner.";
    }

       leaf state {
         type identityref {
           base vrrp:vrrp-state-type;
         }
         config false;
         description
           "Operational state.";
       }

[...]

I think that In the case of preempt and priority, @philo-micas's suggestions were certainly correct.

I also think we should try to follow the RFC 8347 as much as possible.

GitHub source: https://github.com/YangModels/yang/blob/main/standard/ietf/RFC/ietf-vrrp%402018-03-13.yang

A similar Yang model is also available in OpenConfig Yang, which can be taken as reference too: https://github.com/openconfig/public/blob/5ef541dd3d468239f07d69fb05a4dbc9e3f7476b/release/models/interfaces/openconfig-if-ip.yang#L896

[philo-micas]

https://github.com/openconfig/public/blob/5ef541dd3d468239f07d69fb05a4dbc9e3f7476b/release/models/interfaces/openconfig-if-ip.yang#L896

OK

[philo-micas]

Hi all, sorry to spam again, but I need to do it because it affects some of my previous comments (not reviews); I am only just aware that VRRP has a RFC YANG data model.

I found these sections that may cover other questions as well:

[...]

  /*  identity and its derivatives. */
  identity vrrp-state-type {
    description
      "Indicates the state of a virtual router.";
  }
  identity initialize {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is waiting
       for a startup event.";
  }
  identity backup {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is monitoring the
       availability of the master router.";
  }
  identity master {
    base vrrp-state-type;
    description
      "Indicates that the virtual router is forwarding
       packets for IP addresses that are associated with
       this virtual router.";
  }

[...]

    leaf priority {
      type uint8 {
        range "1..254";
      }
      default 100;
      description
        "Configures the VRRP election priority for the backup
         virtual router.";
    }

    leaf is-owner {
      type boolean;
      config false;
      description
        "Set to 'true' if this virtual router is the owner.";
    }

       leaf state {
         type identityref {
           base vrrp:vrrp-state-type;
         }
         config false;
         description
           "Operational state.";
       }

[...]

I think that In the case of preempt and priority, @philo-micas's suggestions were certainly correct.

I also think we should try to follow the RFC 8347 as much as possible.

GitHub source: https://github.com/YangModels/yang/blob/main/standard/ietf/RFC/ietf-vrrp%402018-03-13.yang

OK

[zhangyanzhao]

Thanks reviewers for the comments. Please help to approve this PR after your review. Thanks.

[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!

[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