add vrrpmgrd to support FRR-VRRP configuration #3106
Conversation
Signed-off-by: philo <[email protected]>
|
@vvbrcm @madhupalu please help review |
cfgmgr/Makefile.am
Outdated
| @@ -5,7 +5,7 @@ LIBNL_LIBS = -lnl-genl-3 -lnl-route-3 -lnl-3 | |||
| SAIMETA_LIBS = -lsaimeta -lsaimetadata -lzmq | |||
| COMMON_LIBS = -lswsscommon -lpthread | |||
|
|
|||
| bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd sflowmgrd natmgrd coppmgrd tunnelmgrd macsecmgrd fabricmgrd | |||
| bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd sflowmgrd natmgrd coppmgrd tunnelmgrd macsecmgrd fabricmgrd macvlanmgrd | |||
There was a problem hiding this comment.
I would prefer vrrpmgrd, looks like we used macvlanmgrd in the HLD, please make the changes if possible.
There was a problem hiding this comment.
sonic-net/SONiC#1446 (comment)
As suggested by @nser77, renaming vrrpmgrd to macvlanmgrd would enhance its generality within the whole SONiC architecture, making it more convenient for future functional components to use the macvlan interface type.
There was a problem hiding this comment.
What future functional components you are referring to here? the whole code is written for VRRP, wondering how you would use this macvlanmgr for other features.
There was a problem hiding this comment.
In VRRP, macvlanmgrd manages the configurations of adding, deleting, and addressing for macvlan devices based on VRRP configurations. Although at present only vrrp is using macvlan devices, as long as there is a similar function like VRRP which needs to use macvlan devices, you can subscribe to the corresponding table entries in macvlanmagrd, and then a simple adaptation can realize the unified management of macvlan devices.
There was a problem hiding this comment.
I'm not convinced with your answer, I don't see any mention of future functional components you are referring to here. You are trying to make the code changes with the assumption that some feature will make use of this macvlanmgrd.
If you really want to have a generic macvlanmgrd, remove VRRP name usage in the files and use it as a generic library so that any future module can use it, today VRRP feature can make use of generic MACVLAN mgr.
All I'm trying to say here is, either have the generic MACVLAN mgr and use it for the VRRP for now or specific VRRP mgr but not mixed.
There was a problem hiding this comment.
Yes，you are right. We will use vrrpmgrd and have also submitted the PR for HLD sonic-net/SONiC#1834. Could you please review it. Thanks.
| string res; | ||
|
|
||
| // link add vrrp type macvlan | ||
| cmd << IP_CMD << " link add " << shellquote(vrrp_name) << " link " << shellquote(intf_alias) << " type macvlan mode bridge && "; |
There was a problem hiding this comment.
I believe it will fail if there are some problems while loading the macvlan module - which looks like is done by iproute2 in this case.
Would you prefer to check if the module can be loaded into the kernel before any operations or even manage it apart?
There was a problem hiding this comment.
This is a great suggestion. For now, we haven't encountered any issues during our normal validation process. Other components, such as intfmgr and vlanmgr, currently also do not check whether the module is loaded before execution. We can start a new task to implement this code reinforcement for all components in the future.
There was a problem hiding this comment.
Hey @philo-micas , thanks for your feedback and point of view.
I haven't checked other modules - really big effort - and yes, I agree that we may want to make a general consideration and evaluation here if also other SONiC's features are relaying over iprouter2 for module load and unload.
Just to expanding a bit more the topic, those operations require the CAP_SYS_MODULE capability. I believe there is an on-going work regarding this - sonic-net/SONiC#1364 - but not sure if this dependency has been identified yet (I see swss container has been retained).
We can start a new task to implement this code reinforcement for all components in the future.
Ok keep me posted plz I'm interested on it and happy to collaborate if possible.
| // del vrrp intf | ||
| if (vrrp.isValid()) | ||
| { | ||
| if (!delVirtualInterface(intf_alias, vrrp.getVrrpName())) |
There was a problem hiding this comment.
IMO, MAC Vlan interface removal needs to be avoided when the VIP list is empty as it is getting removed as soon as the assigned VIPs are removed from the VRRP interface.
Signed-off-by: philo <[email protected]>
|
/azp run |
|
Pull request contains merge conflicts. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,209 @@ | |||
| import time | |||
| import json | |||
Check notice
Code scanning / CodeQL
Unused import Note test
| @@ -0,0 +1,209 @@ | |||
| import time | |||
| import json | |||
| import pytest | |||
Check notice
Code scanning / CodeQL
Unused import Note test
hld:sonic-net/SONiC#1446
depends-on:sonic-net/sonic-swss-common#813