ctdbd: Stop takeovers and releases from colliding in mid-air

There's a race here where release and takeover events for an IP can
run at the same time.  For example, a "ctdb deleteip" and a takeover
initiated by the recovery daemon.  The timeline is as follows:

1. The release code registers a callback to update the VNN.  The
   callback is executed *after* the eventscripts run the releaseip
   event.

2. The release code calls the eventscripts for the releaseip event,
   removing IP from its interface.

   The takeover code "updates" the VNN saying that IP is on some
   iface.... even if/though the address is already there.

3. The release callback runs, removing the iface associated with IP in
   the VNN.

   The takeover code calls the eventscripts for the takeip event,
   adding IP to an interface.

As a result, CTDB doesn't think it should be hosting IP but IP is on
an interface.  The recovery daemon fixes this later... but it
shouldn't happen.

This patch can cause some additional noise in the logs:

  Release of IP 10.0.2.133/24 on interface eth2  node:2
  recoverd:We are still serving a public address '10.0.2.133' that we should not be serving. Removing it.
  Release of IP 10.0.2.133/24 rejected update for this IP already in flight
  recoverd:client/ctdb_client.c:2455 ctdb_control for release_ip failed
  recoverd:Failed to release local ip address

In this case the node has started releasing an IP when the recovery
daemon notices the addresses is still hosted and initiates another
release.  This noise is harmless but annoying.

Signed-off-by: Martin Schwenke <martin@meltin.net>
(cherry picked from commit bfe16cf69bf2eee93c0d831f76d88bba0c2b96c2)

diff --git a/include/ctdb_private.h b/include/ctdb_private.h
index aceeb8e9c23da3022f806d674e327af857da909f..e0b39eecf97ec72bf53e1a4a4408ab2c38fa44ec 100644 (file)
--- a/include/ctdb_private.h
+++ b/include/ctdb_private.h
@@ -209,6 +209,10 @@ struct ctdb_vnn {
        TALLOC_CTX *takeover_ctx;
 
        struct ctdb_kill_tcp *killtcp;
+
+       /* Set to true any time an update to this VNN is in flight.
+          This helps to avoid races. */
+       bool update_in_flight;
 };
 
 /*

diff --git a/server/ctdb_takeover.c b/server/ctdb_takeover.c
index 9826d632a21b4425ccc0ee200932682599278438..19ed670989c663a5f3236694364ab4da76f34bcb 100644 (file)
--- a/server/ctdb_takeover.c
+++ b/server/ctdb_takeover.c
@@ -369,6 +369,12 @@ static void ctdb_do_takeip_callback(struct ctdb_context *ctdb, int status,
        return;
 }
 
+static int ctdb_takeip_destructor(struct ctdb_do_takeip_state *state)
+{
+       state->vnn->update_in_flight = false;
+       return 0;
+}
+
 /*
   take over an ip address
  */
@@ -379,6 +385,14 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb,
        int ret;
        struct ctdb_do_takeip_state *state;
 
+       if (vnn->update_in_flight) {
+               DEBUG(DEBUG_NOTICE,("Takeover of IP %s/%u rejected "
+                                   "update for this IP already in flight\n",
+                                   ctdb_addr_to_str(&vnn->public_address),
+                                   vnn->public_netmask_bits));
+               return -1;
+       }
+
        ret = ctdb_vnn_assign_iface(ctdb, vnn);
        if (ret != 0) {
                DEBUG(DEBUG_ERR,("Takeover of IP %s/%u failed to "
@@ -394,6 +408,9 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb,
        state->c = talloc_steal(ctdb, c);
        state->vnn   = vnn;
 
+       vnn->update_in_flight = true;
+       talloc_set_destructor(state, ctdb_takeip_destructor);
+
        DEBUG(DEBUG_NOTICE,("Takeover of IP %s/%u on interface %s\n",
                            ctdb_addr_to_str(&vnn->public_address),
                            vnn->public_netmask_bits,
@@ -476,6 +493,12 @@ static void ctdb_do_updateip_callback(struct ctdb_context *ctdb, int status,
        return;
 }
 
+static int ctdb_updateip_destructor(struct ctdb_do_updateip_state *state)
+{
+       state->vnn->update_in_flight = false;
+       return 0;
+}
+
 /*
   update (move) an ip address
  */
@@ -487,6 +510,14 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb,
        struct ctdb_do_updateip_state *state;
        struct ctdb_iface *old = vnn->iface;
 
+       if (vnn->update_in_flight) {
+               DEBUG(DEBUG_NOTICE,("Update of IP %s/%u rejected "
+                                   "update for this IP already in flight\n",
+                                   ctdb_addr_to_str(&vnn->public_address),
+                                   vnn->public_netmask_bits));
+               return -1;
+       }
+
        ctdb_vnn_unassign_iface(ctdb, vnn);
        ret = ctdb_vnn_assign_iface(ctdb, vnn);
        if (ret != 0) {
@@ -514,6 +545,9 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb,
        state->old = old;
        state->vnn = vnn;
 
+       vnn->update_in_flight = true;
+       talloc_set_destructor(state, ctdb_updateip_destructor);
+
        DEBUG(DEBUG_NOTICE,("Update of IP %s/%u from "
                            "interface %s to %s\n",
                            ctdb_addr_to_str(&vnn->public_address),
@@ -776,6 +810,12 @@ static void release_ip_callback(struct ctdb_context *ctdb, int status,
        talloc_free(state);
 }
 
+static int ctdb_releaseip_destructor(struct takeover_callback_state *state)
+{
+       state->vnn->update_in_flight = false;
+       return 0;
+}
+
 /*
   release an ip address
  */
@@ -803,8 +843,12 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
        talloc_free(vnn->takeover_ctx);
        vnn->takeover_ctx = NULL;
 
+       /* Some ctdb tool commands (e.g. moveip, rebalanceip) send
+        * lazy multicast to drop an IP from any node that isn't the
+        * intended new node.  The following causes makes ctdbd ignore
+        * a release for any address it doesn't host.
+        */
        if (ctdb->do_checkpublicip) {
-
                if (!ctdb_sys_have_ip(&pip->addr)) {
                        DEBUG(DEBUG_DEBUG,("Redundant release of IP %s/%u on interface %s (ip not held)\n",
                                ctdb_addr_to_str(&pip->addr),
@@ -813,12 +857,6 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
                        ctdb_vnn_unassign_iface(ctdb, vnn);
                        return 0;
                }
-
-               iface = ctdb_sys_find_ifname(&pip->addr);
-               if (iface == NULL) {
-                       DEBUG(DEBUG_ERR, ("Could not find which interface the ip address is hosted on. can not release it\n"));
-                       return 0;
-               }
        } else {
                if (vnn->iface == NULL) {
                        DEBUG(DEBUG_DEBUG,("Redundant release of IP %s/%u (ip not held)\n",
@@ -826,6 +864,28 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
                                           vnn->public_netmask_bits));
                        return 0;
                }
+       }
+
+       /* There is a potential race between take_ip and us because we
+        * update the VNN via a callback that run when the
+        * eventscripts have been run.  Avoid the race by allowing one
+        * update to be in flight at a time.
+        */
+       if (vnn->update_in_flight) {
+               DEBUG(DEBUG_NOTICE,("Release of IP %s/%u rejected "
+                                   "update for this IP already in flight\n",
+                                   ctdb_addr_to_str(&vnn->public_address),
+                                   vnn->public_netmask_bits));
+               return -1;
+       }
+
+       if (ctdb->do_checkpublicip) {
+               iface = ctdb_sys_find_ifname(&pip->addr);
+               if (iface == NULL) {
+                       DEBUG(DEBUG_ERR, ("Could not find which interface the ip address is hosted on. can not release it\n"));
+                       return 0;
+               }
+       } else {
                iface = strdup(ctdb_vnn_iface_string(vnn));
        }
 
@@ -844,6 +904,9 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
        *state->addr = pip->addr;
        state->vnn   = vnn;
 
+       vnn->update_in_flight = true;
+       talloc_set_destructor(state, ctdb_releaseip_destructor);
+
        ret = ctdb_event_script_callback(ctdb, 
                                         state, release_ip_callback, state,
                                         false,