Adding vrrpsyncd module #3313
Adding vrrpsyncd module #3313
Conversation
|
@philo-micas @madhupalu please help review |
|
@vvbrcm can you please confirm if you are planning to extend this PR only to accomodate vrrporch related changes too? |
|
@philo-micas vrrpsyncd needs to be spawned from the supervisord.conf of swss container. Thus, are you planning to add this in PR#18617 itself? |
vrrpsyncd/vrrpsync.cpp
Outdated
| } | ||
| else | ||
| { | ||
| family = "ipv4"; |
There was a problem hiding this comment.
This variable doesn't seem to be used while creating a key for VRRP_TABLE as per HLD.
There was a problem hiding this comment.
Will remove the unused variable 'family'.
|
@vvbrcm I quickly tested this implementation but it seems something is still missing as the VRRP_TABLE is not getting created in the APPL_DB even though vrrpsyncd received the system MAC. |
| if (afi == AF_INET6) | ||
| { | ||
| family = "ipv6"; | ||
| len = "/128"; |
There was a problem hiding this comment.
What is the purpose of this len variable ? Shouldn't it represent IP interface subnet ?
There was a problem hiding this comment.
The primary interface on which VRRP is running will add the interface address with interface subnet. VRRP VIP on the macvlan interface will be an IP address with mask /32(IPv4) or /128 (IPv6).
vrrpsyncd/Makefile.am
Outdated
| if DEBUG | ||
| DBGFLAGS = -ggdb -DDEBUG | ||
| else | ||
| DBGFLAGS = -ggdb -DDEBUG |
There was a problem hiding this comment.
This probably should be "DBGFLAGS = -g"
vrrpsyncd/vrrpsync.cpp
Outdated
| } | ||
|
|
||
|
|
||
| void VrrpSync::VrrpUpdateNetdevFlags(string &ifname, int afi, bool up) |
vrrpsyncd/vrrpsyncd.cpp
Outdated
| Selectable *temps; | ||
| int ret; | ||
|
|
||
| using namespace std::chrono; |
vrrpsyncd/vrrpsync.cpp
Outdated
| if ((nlmsg_type == RTM_DELLINK) || (nlmsg_type == RTM_DELADDR)) | ||
| delete_key = true; | ||
|
|
||
| delete_key = delete_key; |
vrrpsyncd/vrrpsync.cpp
Outdated
| if (!ifname) | ||
| return false; | ||
|
|
||
| if (strncmp(ifname, "vrrp", 4)) |
There was a problem hiding this comment.
It must be "Vrrp" as the interface name will be
"Vrrp-<af>-<vrrp_instance_number>".
There was a problem hiding this comment.
From FRR the macvlan interface name was thought to be of the format vrrp-afi-instance ex vrrp4-2-1.
@philo-micas, can you please check.
There was a problem hiding this comment.
The Macvlan device name starting with 'Vrrp4-' or 'Vrrp6-', ex Vrrp4-5 and Vrrp6-5.
vrrpsyncd/vrrpsync.cpp
Outdated
|
|
||
| VrrpDbUpdate(ifname, key, m_vrrpinfo[key].parent_ifname, afi, vip, m_vrrpinfo[key].vmac, false); | ||
|
|
||
| VrrpUpdateVipNbr(ifname, m_vrrpinfo[key].parent_ifname, afi, vip, true); |
There was a problem hiding this comment.
Check the indent here please, and there are also same issues below.
vrrpsyncd/vrrpsync.cpp
Outdated
| } | ||
|
|
||
|
|
||
| void VrrpSync::VrrpUpdateNetdevFlags(string &ifname, int afi, bool up) |
There was a problem hiding this comment.
Could you please explain why it is implemented in vrrpsyncd instead of macvlanmgrd?
There was a problem hiding this comment.
This is for vrrp parent interface. Is this same logic present in macvlanmgrd? Is so, it could be removed.
vrrpsyncd/vrrpsync.h
Outdated
| struct vrrpmacip | ||
| { | ||
| unsigned int if_state; | ||
| std::string ifname; |
| int ret; | ||
| string afi_str = (afi == AF_INET6)? "-6": ""; | ||
|
|
||
| cmd = "ip " + afi_str + " neigh flush " + vip + " dev " + ifname; |
There was a problem hiding this comment.
I guess it needs to be done only for op = "del".
There was a problem hiding this comment.
Yes, had written it generically to del/replace. Currently only del is used.
| } | ||
|
|
||
|
|
||
| void VrrpSync::VrrpUpdateNbr(string &ifname, int afi, string &vip, string &op) |
There was a problem hiding this comment.
It seems that the op parameter has not been used.
There was a problem hiding this comment.
Yes, had written it generically to del/replace. Currently only del is used.
has done. Thanks. |
| m_cfgDeviceMetadataTable(cfgDb, CFG_DEVICE_METADATA_TABLE_NAME) | ||
| { | ||
| m_nl_sock = nl_socket_alloc(); | ||
| nl_connect(m_nl_sock, NETLINK_ROUTE); |
There was a problem hiding this comment.
I'm not sure if this is required because vrrpsync needs to directly handle the netlink socket and if somenthing is missing from swss-common side my suggestion is to focus the effort in such library (netdispatcher and friends).
Anyway, if my previous comment is not correct, I think we want to:
- Manage the case where
m_nl_sockis NULL - Allocate a
rxandtxbuffer or specify 0,0 to enforce the default values.
There was a problem hiding this comment.
Other matches into sonic-swss repo:
cfgmgr/nbrmgr.cpp:54: m_nl_sock = nl_socket_alloc();
cfgmgr/nbrmgr.cpp:59: else if ((err = nl_connect(m_nl_sock, NETLINK_ROUTE)) < 0)
fpmsyncd/routesync.cpp:86: m_nl_sock = nl_socket_alloc();
fpmsyncd/routesync.cpp:87: nl_connect(m_nl_sock, NETLINK_ROUTE);
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
No pipelines are associated with this pull request. |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
What I did
Add module vrrpsyncd to program VRRP entities (VIP and VMAC) from kernel to APP_DB
Why I did it
Support VRRP component
How I verified it
UT
Details if related