ctdb-recoverd: Clarify recovery master validation logic

There can be no holes in the nodemap.  Even if a node has been deleted
it will take a slot in the nodemap.  The only exception is that the
nodemap shrinks if nodes are deleted from the end.  That should never
include the master because a node should be shutdown before being
deleted, and an election should already have take place.

To avoid walking off the end of the nodemap nodes array just confirm
that the master node's PNN is a valid index into the array.  No need
to walk through the nodemap.

After this, in this section of the code j is now invalid.  So use the
master's PNN to index into the nodemap.  This is safe.

In the process, clean up some log messages to avoid saying "Force
reelection".  It's just an "election".

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 13f833fd74a7766310df0eee96f696df1c63072d..abc5d0f1e4f6854145bb8af22db5c86b6e4b358c 100644 (file)
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -3575,45 +3575,54 @@ static void main_loop(struct ctdb_context *ctdb, struct ctdb_recoverd *rec,
                return;
        }
 
-       /* verify that the recmaster node is still active */
-       for (j=0; j<nodemap->num; j++) {
-               if (nodemap->nodes[j].pnn==rec->recmaster) {
-                       break;
-               }
-       }
-
-       if (j == nodemap->num) {
-               DEBUG(DEBUG_ERR, ("Recmaster node %u not in list. Force reelection\n", rec->recmaster));
+       /* Verify that the master node has not been deleted.  This
+        * should not happen because a node should always be shutdown
+        * before being deleted, causing a new master to be elected
+        * before now.  However, if something strange has happened
+        * then checking here will ensure we don't index beyond the
+        * end of the nodemap array. */
+       if (rec->recmaster >= nodemap->num) {
+               DEBUG(DEBUG_ERR,
+                     ("Recmaster node %u has been deleted. Force election\n",
+                      rec->recmaster));
                force_election(rec, pnn, nodemap);
                return;
        }
 
-       /* if recovery master is disconnected we must elect a new recmaster */
-       if (nodemap->nodes[j].flags & NODE_FLAGS_DISCONNECTED) {
-               DEBUG(DEBUG_NOTICE, ("Recmaster node %u is disconnected. Force reelection\n", nodemap->nodes[j].pnn));
+       /* if recovery master is disconnected/deleted we must elect a new recmaster */
+       if (nodemap->nodes[rec->recmaster].flags &
+           (NODE_FLAGS_DISCONNECTED|NODE_FLAGS_DELETED)) {
+               DEBUG(DEBUG_NOTICE,
+                     ("Recmaster node %u is disconnected/deleted. Force election\n",
+                      rec->recmaster));
                force_election(rec, pnn, nodemap);
                return;
        }
 
        /* get nodemap from the recovery master to check if it is inactive */
-       ret = ctdb_ctrl_getnodemap(ctdb, CONTROL_TIMEOUT(), nodemap->nodes[j].pnn, 
+       ret = ctdb_ctrl_getnodemap(ctdb, CONTROL_TIMEOUT(), rec->recmaster,
                                   mem_ctx, &recmaster_nodemap);
        if (ret != 0) {
-               DEBUG(DEBUG_ERR, (__location__ " Unable to get nodemap from recovery master %u\n", 
-                         nodemap->nodes[j].pnn));
+               DEBUG(DEBUG_ERR,
+                     (__location__
+                      " Unable to get nodemap from recovery master %u\n",
+                         rec->recmaster));
                return;
        }
 
 
-       if ((recmaster_nodemap->nodes[j].flags & NODE_FLAGS_INACTIVE) &&
+       if ((recmaster_nodemap->nodes[rec->recmaster].flags & NODE_FLAGS_INACTIVE) &&
            (rec->node_flags & NODE_FLAGS_INACTIVE) == 0) {
-               DEBUG(DEBUG_NOTICE, ("Recmaster node %u no longer available. Force reelection\n", nodemap->nodes[j].pnn));
+               DEBUG(DEBUG_NOTICE,
+                     ("Recmaster node %u is inactive. Force election\n",
+                      rec->recmaster));
                /*
                 * update our nodemap to carry the recmaster's notion of
                 * its own flags, so that we don't keep freezing the
                 * inactive recmaster node...
                 */
-               nodemap->nodes[j].flags = recmaster_nodemap->nodes[j].flags;
+               nodemap->nodes[rec->recmaster].flags =
+                       recmaster_nodemap->nodes[rec->recmaster].flags;
                force_election(rec, pnn, nodemap);
                return;
        }