ctdb-daemon: Mark tunable VerifyRecoveryLock as obsolete

It is pointless having a recovery lock but not sanity checking that it
is working.  Also, the logic that uses this tunable is confusing.  In
some places the recovery lock is released unnecessarily because the
tunable isn't set.

Simplify the logic by assuming that if a recovery lock is specified
then it should be verified.

Update documentation that references this tunable.

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

6 files changed, 37 insertions, 36 deletions

diff --git a/ctdb/doc/ctdb-tunables.7.xml b/ctdb/doc/ctdb-tunables.7.xml
index 456e856355..b029fdb75a 100644
--- a/ctdb/doc/ctdb-tunables.7.xml
+++ b/ctdb/doc/ctdb-tunables.7.xml
@@ -448,15 +448,6 @@
     </refsect2>
 
     <refsect2>
-      <title>VerifyRecoveryLock</title>
-      <para>Default: 1</para>
-      <para>
-	Should we take a fcntl() lock on the reclock file to verify that we are the
-	sole recovery master node on the cluster or not.
-      </para>
-    </refsect2>
-
-    <refsect2>
       <title>VacuumInterval</title>
       <para>Default: 10</para>
       <para>
diff --git a/ctdb/doc/ctdb.1.xml b/ctdb/doc/ctdb.1.xml
index 07df43bd76..1a1ae40fbe 100644
--- a/ctdb/doc/ctdb.1.xml
+++ b/ctdb/doc/ctdb.1.xml
@@ -682,7 +682,6 @@ RecdFailCount           = 10
 LogLatencyMs            = 0
 RecLockLatencyMs        = 1000
 RecoveryDropAllIPs      = 120
-VerifyRecoveryLock      = 1
 VacuumInterval          = 10
 VacuumMaxRunTime        = 30
 RepackLimit             = 10000
@@ -883,30 +882,51 @@ DB Statistics: locking.tdb
     <refsect2>
       <title>getreclock</title>
       <para>
-	This command is used to show the filename of the reclock file that is used.
+	Show the name of the recovery lock file, if any.
       </para>
 
       <para>
 	Example output:
       </para>
       <screen>
-	Reclock file:/gpfs/.ctdb/shared
+	Reclock file:/clusterfs/.ctdb/recovery.lock
       </screen>
 
     </refsect2>
 
     <refsect2>
-      <title>setreclock [filename]</title>
+      <title>
+	setreclock <optional><parameter>FILE</parameter></optional>
+      </title>
+
       <para>
-	This command is used to modify, or clear, the file that is used as the reclock file at runtime. When this command is used, the reclock file checks are disabled. To re-enable the checks the administrator needs to activate the "VerifyRecoveryLock" tunable using "ctdb setvar".
+	FILE specifies the name of the recovery lock file.  If the
+	recovery lock file is changed at run-time then this will cause
+	a recovery, which in turn causes the recovery lock to be
+	retaken.
       </para>
 
       <para>
-	If run with no parameter this will remove the reclock file completely. If run with a parameter the parameter specifies the new filename to use for the recovery lock.
+	If no FILE is specified then a recovery lock file will no
+	longer be used.
       </para>
 
       <para>
-	This command only affects the runtime settings of a ctdb node and will be lost when ctdb is restarted. For persistent changes to the reclock file setting you must edit /etc/sysconfig/ctdb.
+	This command only affects the run-time setting of a single
+	CTDB node.  This setting <emphasis>must</emphasis> be changed
+	on all nodes simultaneously by specifying <option>-n
+	all</option> (or similar).  For information about configuring
+	the recovery lock file please see the
+	<citetitle>CTDB_RECOVERY_LOCK</citetitle> entry in
+	<citerefentry><refentrytitle>ctdbd.conf</refentrytitle>
+	<manvolnum>5</manvolnum></citerefentry> and the
+	<citetitle>--reclock</citetitle> entry in
+	<citerefentry><refentrytitle>ctdbd</refentrytitle>
+	<manvolnum>1</manvolnum></citerefentry>.  For information
+	about the recovery lock please see the <citetitle>RECOVERY
+	LOCK</citetitle> section in
+	<citerefentry><refentrytitle>ctdb</refentrytitle>
+	<manvolnum>7</manvolnum></citerefentry>.
       </para>
     </refsect2>
 
diff --git a/ctdb/server/ctdb_recover.c b/ctdb/server/ctdb_recover.c
index 10d088b6d0..cd77418728 100644
--- a/ctdb/server/ctdb_recover.c
+++ b/ctdb/server/ctdb_recover.c
@@ -640,8 +640,8 @@ int32_t ctdb_control_set_recmode(struct ctdb_context *ctdb,
 		ctdb_process_deferred_attach(ctdb);
 	}
 
-	if (ctdb->tunable.verify_recovery_lock == 0) {
-		/* dont need to verify the reclock file */
+	if (ctdb->recovery_lock_file == NULL) {
+		/* Not using recovery lock file */
 		ctdb->recovery_mode = recmode;
 		return 0;
 	}
diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index f86f57e570..6ea381dbc6 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1808,7 +1808,7 @@ static int do_recovery(struct ctdb_recoverd *rec,
 		return -1;
 	}
 
-        if (ctdb->tunable.verify_recovery_lock != 0) {
+        if (ctdb->recovery_lock_file != NULL) {
 		DEBUG(DEBUG_ERR,("Taking out recovery lock from recovery daemon\n"));
 		start_time = timeval_current();
 		if (!ctdb_recovery_lock(ctdb, true)) {
@@ -2684,8 +2684,8 @@ static void election_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	talloc_free(rec->send_election_te);
 	rec->send_election_te = NULL;
 
-        if (ctdb->tunable.verify_recovery_lock != 0) {
-		/* release the recmaster lock */
+        if (ctdb->recovery_lock_file != NULL) {
+		/* Release the recovery lock file */
 		if (em->pnn != ctdb->pnn &&
 		    ctdb->recovery_lock_fd != -1) {
 			DEBUG(DEBUG_NOTICE, ("Release the recovery lock\n"));
@@ -3483,7 +3483,6 @@ static int update_recovery_lock_file(struct ctdb_context *ctdb)
 				ctdb->recovery_lock_fd = -1;
 			}
 		}
-		ctdb->tunable.verify_recovery_lock = 0;
 		talloc_free(tmp_ctx);
 		return 0;
 	}
@@ -3506,7 +3505,6 @@ static int update_recovery_lock_file(struct ctdb_context *ctdb)
 
 	talloc_free(ctdb->recovery_lock_file);
 	ctdb->recovery_lock_file = talloc_strdup(ctdb, reclockfile);
-	ctdb->tunable.verify_recovery_lock = 0;
 	if (ctdb->recovery_lock_fd != -1) {
 		close(ctdb->recovery_lock_fd);
 		ctdb->recovery_lock_fd = -1;
@@ -3576,7 +3574,7 @@ static void main_loop(struct ctdb_context *ctdb, struct ctdb_recoverd *rec,
 	/* Make sure that if recovery lock verification becomes disabled when
 	   we close the file
 	*/
-        if (ctdb->tunable.verify_recovery_lock == 0) {
+        if (ctdb->recovery_lock_file == NULL) {
 		if (ctdb->recovery_lock_fd != -1) {
 			close(ctdb->recovery_lock_fd);
 			ctdb->recovery_lock_fd = -1;
@@ -3841,7 +3839,7 @@ static void main_loop(struct ctdb_context *ctdb, struct ctdb_recoverd *rec,
 	}
 
 
-        if (ctdb->tunable.verify_recovery_lock != 0) {
+        if (ctdb->recovery_lock_file != NULL) {
 		/* we should have the reclock - check its not stale */
 		ret = check_recovery_lock(ctdb);
 		if (ret != 0) {
diff --git a/ctdb/server/ctdb_server.c b/ctdb/server/ctdb_server.c
index 1e3fde497f..2598c4cb5b 100644
--- a/ctdb/server/ctdb_server.c
+++ b/ctdb/server/ctdb_server.c
@@ -67,7 +67,6 @@ int ctdb_set_recovery_lock_file(struct ctdb_context *ctdb, const char *file)
 
 	if (file == NULL) {
 		DEBUG(DEBUG_ALERT,("Recovery lock file set to \"\". Disabling recovery lock checking\n"));
-		ctdb->tunable.verify_recovery_lock = 0;
 		return 0;
 	}
 
diff --git a/ctdb/server/ctdb_tunables.c b/ctdb/server/ctdb_tunables.c
index 069958653f..f5713f8d41 100644
--- a/ctdb/server/ctdb_tunables.c
+++ b/ctdb/server/ctdb_tunables.c
@@ -57,7 +57,7 @@ static const struct {
 	{ "LogLatencyMs",         0,  offsetof(struct ctdb_tunable, log_latency_ms), false },
 	{ "RecLockLatencyMs",  1000,  offsetof(struct ctdb_tunable, reclock_latency_ms), false },
 	{ "RecoveryDropAllIPs", 120,  offsetof(struct ctdb_tunable, recovery_drop_all_ips), false },
-	{ "VerifyRecoveryLock",   1,  offsetof(struct ctdb_tunable, verify_recovery_lock), false },
+	{ "VerifyRecoveryLock",   1,  offsetof(struct ctdb_tunable, verify_recovery_lock), true },
 	{ "VacuumInterval",   10,  offsetof(struct ctdb_tunable, vacuum_interval), false },
 	{ "VacuumMaxRunTime",     120,  offsetof(struct ctdb_tunable, vacuum_max_run_time), false },
 	{ "RepackLimit",      10000,  offsetof(struct ctdb_tunable, repack_limit), false },
@@ -145,7 +145,7 @@ int32_t ctdb_control_get_tunable(struct ctdb_context *ctdb, TDB_DATA indata,
  */
 int32_t ctdb_control_set_tunable(struct ctdb_context *ctdb, TDB_DATA indata)
 {
-	struct ctdb_control_set_tunable *t = 
+	struct ctdb_control_set_tunable *t =
 		(struct ctdb_control_set_tunable *)indata.dptr;
 	char *name;
 	int i;
@@ -163,15 +163,8 @@ int32_t ctdb_control_set_tunable(struct ctdb_context *ctdb, TDB_DATA indata)
 		if (strcasecmp(name, tunable_map[i].name) == 0) break;
 	}
 
-	if (!strcmp(name, "VerifyRecoveryLock") && t->value != 0
-	&& ctdb->recovery_lock_file == NULL) {
-		DEBUG(DEBUG_ERR,("Can not activate tunable \"VerifyRecoveryLock\" since there is no recovery lock file set.\n"));
-		talloc_free(name);
-		return -1;
-	}
-
 	talloc_free(name);
-	
+
 	if (i == ARRAY_SIZE(tunable_map)) {
 		return -1;
 	}