ctdb-recoverd: Drop some sanity checking in local IP verification

The recovery start/end times used in the checks at the top of
verify_local_ip_allocation() are set by the START_RECOVERY and
END_RECOVERY controls.  A couple of takeover runs escape the checks
because they were added later and are not surrounded by these
controls.

Recovery and IP allocation need to be untangled from each other, so
recovery-related events should not be relied on for IP allocation.
This means the solution is not to add these where they are "missing".

The concern that the checks are addressing is to avoid local IP
verification when IP addresses are in a state of flux.  Takeover runs
on non-master nodes are already disabled while a takeover run is in
progress, so local IP verification is already skipped in that case.
The other case is the master node, which will be busy with the
takeover run, rather than running main_loop().

The other issue is races.  verify_local_ip_allocation() takes a
non-zero amount of time to fetch IP addresses from the local CTDB
daemon and during this time a recovery or takeover run can start, but
a takeover run can still be triggered.  The current tests do not stop
this.

Apart from all of this, with most reasonable public IP address
configurations, an extra takeover run will be a no-op so is not a
cause for concern.

It is safe to drop these checks.

Signed-off-by: Martin Schwenke <martin@meltin.net>
Reviewed-by: Amitay Isaacs <amitay@gmail.com>

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index fd134f9ca65fe2bafb08101aeedcfea6488de09c..409aaf154927a89fd7eaae3d1de75821b3477007 100644 (file)
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -3310,19 +3310,9 @@ static bool interfaces_have_changed(struct ctdb_context *ctdb,
 static int verify_local_ip_allocation(struct ctdb_context *ctdb, struct ctdb_recoverd *rec, uint32_t pnn, struct ctdb_node_map_old *nodemap)
 {
        TALLOC_CTX *mem_ctx = talloc_new(NULL);
-       struct ctdb_uptime *uptime1 = NULL;
-       struct ctdb_uptime *uptime2 = NULL;
        int ret, j;
        bool need_takeover_run = false;
 
-       ret = ctdb_ctrl_uptime(ctdb, mem_ctx, CONTROL_TIMEOUT(),
-                               CTDB_CURRENT_NODE, &uptime1);
-       if (ret != 0) {
-               DEBUG(DEBUG_ERR, ("Unable to get uptime from local node %u\n", pnn));
-               talloc_free(mem_ctx);
-               return -1;
-       }
-
        if (interfaces_have_changed(ctdb, rec)) {
                DEBUG(DEBUG_NOTICE, ("The interfaces status has changed on "
                                     "local node %u - force takeover run\n",
@@ -3330,39 +3320,6 @@ static int verify_local_ip_allocation(struct ctdb_context *ctdb, struct ctdb_rec
                need_takeover_run = true;
        }
 
-       ret = ctdb_ctrl_uptime(ctdb, mem_ctx, CONTROL_TIMEOUT(),
-                               CTDB_CURRENT_NODE, &uptime2);
-       if (ret != 0) {
-               DEBUG(DEBUG_ERR, ("Unable to get uptime from local node %u\n", pnn));
-               talloc_free(mem_ctx);
-               return -1;
-       }
-
-       /* skip the check if the startrecovery time has changed */
-       if (timeval_compare(&uptime1->last_recovery_started,
-                           &uptime2->last_recovery_started) != 0) {
-               DEBUG(DEBUG_NOTICE, (__location__ " last recovery time changed while we read the public ip list. skipping public ip address check\n"));
-               talloc_free(mem_ctx);
-               return 0;
-       }
-
-       /* skip the check if the endrecovery time has changed */
-       if (timeval_compare(&uptime1->last_recovery_finished,
-                           &uptime2->last_recovery_finished) != 0) {
-               DEBUG(DEBUG_NOTICE, (__location__ " last recovery time changed while we read the public ip list. skipping public ip address check\n"));
-               talloc_free(mem_ctx);
-               return 0;
-       }
-
-       /* skip the check if we have started but not finished recovery */
-       if (timeval_compare(&uptime1->last_recovery_finished,
-                           &uptime1->last_recovery_started) != 1) {
-               DEBUG(DEBUG_INFO, (__location__ " in the middle of recovery or ip reallocation. skipping public ip address check\n"));
-               talloc_free(mem_ctx);
-
-               return 0;
-       }
-
        /* verify that we have the ip addresses we should have
           and we don't have ones we shouldnt have.
           if we find an inconsistency we set recmode to