acassen / keepalived

Keepalived
https://www.keepalived.org
GNU General Public License v2.0
3.95k stars 736 forks source link

BUG when reload after vip moving from one vrrp instance to another #1854

Closed g6x closed 3 years ago

g6x commented 3 years ago

Describe the bug BUG possibly triggers when we execute SIGHUP reloading after changing one VIP from belonging to one vrrp instance to another.

To Reproduce To Reproduce, we change the configuration in a special way. First, in old configuration file, there were 2 vrrp instances, V_1 and V_2, and V_2 was defined below V_1 in configuration file, and there was one VIP in V_2; Then, in new configuration file, we removed V_2, and moved the VIP into V_1; After that, we sent SIGHUP and reloading functioned.

Expected behavior If we were MASTER before and after reloading, we were expected to own the VIP after reloading. But actually, the VIP despaired after reloading.

Coding error At reload, Keepalived compares the new and the old configuration, and do updates according to the Diff. Updates includes removing and adding operation, and the problem is the order of these two types of operation. Anyway, I found the mistake in fuction clear_diff_vrrp(), where operations order is same as vrrp instance list order in configuration file. Maybe, a better way is to adjust the order, to do the vrrp instance removing operation in preference to other operations.

Keepalived version

2.1.5
g6x commented 3 years ago

I tried to fix in PR #1855 , need review.

pqarmitage commented 3 years ago

@g6x Many thanks for your patch which I have now reviewed and merged.

This still leaves one scenario where there is a similar problem. If the VRRP instance is not deleted, but the VIP (or virtual route or rule) is move from one VRRP instance to another, and both instances are in MASTER state, and the VIP etc is added to the earlier instance in the configuration, then the VIP etc will end up not being configured.

I do not intend to attempt to fix this highly specific problem, and so I will add the following to the keepalived(8) man page to warn of this:

Note: If a virtual_ipaddress, virtual_route or virtual_rule is being moved from one VRRP instance to another one, two reloads will be necessary, the first to remove the virtual ipaddress/route/rule, and the second reload to add it to the VRRP instance it is now to be configured on. Failing to do this can result in the ipaddress/route/rule not being configured on the new instance if both the old and new instances are in master state. It will usually work with a single reload, however, if either of the VRRP instances is not in MASTER state or if the VRRP instance the ipaddress/route/rule the VRRP instance is being added to is later in the original configuration file than the instance it is being removed from.

g6x commented 3 years ago

@pqarmitage Thank you for the information about the scenarios. It's much clearer now.