-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Supports FRR-VRRP configuration #18617
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
…28-support-vrrp6-commands-and-tracking-interface.patch
…30-support-vrrp6-commands-and-tracking-interface.patch
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
PROXY_SERVER_ADDR = '/etc/frr/vrrpd_client_sock' | ||
ALL_DAEMONS = ['vrrpd'] | ||
TABLE_DAEMON = { | ||
'VRRP': ['vrrpd'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe, we discussed to have VRRP config handler for FRR as part of frrcfgd itself, what are we doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have also implemented this functionality in frrcfgd. This provides an alternative for usage when frrcfgd is not enabled. Some users may need to use it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vrrcfgd is totally a duplicate code, I would like to better understand in which case vrrpcfgd will be enabled but not frrcfgd. Keeping both the code will be hard to manage, I would recommend using frrcfgd only for VRRP feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vrrcfgd is totally a duplicate code, I would like to better understand in which case vrrpcfgd will be enabled but not frrcfgd. Keeping both the code will be hard to manage, I would recommend using frrcfgd only for VRRP feature.
OK, vrrpcfgd has been removed, thanks.
…54-support-vrrp6-commands-and-tracking-interface.patch
Signed-off-by: philo <[email protected]>
@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? |
Signed-off-by: philo <[email protected]>
@vvbrcm @madhupalu please help review |
elif d_address != "": | ||
d_addr = d_address.split('/') | ||
try: | ||
ip_address = ipaddress.ip_interface(d_addr[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import ipaddress is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has added, thanks.
Please check the CPU utilisation of frrcfgd when VRRP is enabled. I found it to be constant to 100%. |
- VRRP_IP_STR) | ||
+ "Add tracking interface\n" | ||
+ VRRP_TRACK_INTF_STR | ||
+ VRRP_PRIORITY_DECREMENT_STR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need VRRP_PRIORITY_DECREMENT_STR here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be removed, thanks.
+ "Add tracking interface\n" | ||
+ VRRP6_TRACK_INTF_STR | ||
+ VRRP6_PRIORITY_DECREMENT_STR | ||
+ "Priority decrement value\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VRRP6_PRIORITY_DECREMENT_STR needed in this 'no' command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be removed, thanks.
Signed-off-by: philo <[email protected]>
Do you know why it is constant 100% CPU utilization? adding some context would help e.g in which scenario you got into the issue. |
No @venkatmahalingam yet to root cause the issue. But found that even if the VRRP is not configured, frrcfgd continuously consuming high CPU. Same was the case with vrrpcfgd which is now removed. Logs:
|
I tried to reproduce it through the following configuration, but the result was normal:
Perhaps I missed some details, could you please provide your testing steps? Thanks. |
@philo-micas Following are configs that I have used.
And If neither vrrpcfgd nor ffrcfgd is running, I saw that "bufferM+" process is consuming all of the CPU.
|
@@ -28,7 +28,7 @@ SONIC_DOCKER_IMAGES += $(DOCKER_FPM_FRR) | |||
SONIC_DOCKER_DBG_IMAGES += $(DOCKER_FPM_FRR_DBG) | |||
|
|||
$(DOCKER_FPM_FRR)_CONTAINER_NAME = bgp | |||
$(DOCKER_FPM_FRR)_RUN_OPT += -t --cap-add=NET_ADMIN --cap-add=SYS_ADMIN | |||
$(DOCKER_FPM_FRR)_RUN_OPT += -t --cap-add=NET_ADMIN --cap-add=SYS_ADMIN --cap-add=CAP_NET_ADMIN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if CAP_NET_ADMIN
is the right capability.
In docker
, I believe It should be NET_ADMIN
but in that case looks duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to limit privileged flag for bgp container
#14932, NET-ADMIN
is required to use the iprouter2 command in vrrpmgr. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iprouter2 command in vrrpmgr
Thanks double checked
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
We tried to verify again, but the result is still normal. Perhaps there are other factors affecting your environment. After this PR is merged, you can try using the community version for verification. Thanks. |
Why I did it
Work item tracking
How I did it
hld:sonic-net/SONiC#1446
How to verify it
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)