From d110fe231849d76ecb83378c934627dc64b74c72 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Tue, 9 Dec 2014 13:47:42 +1100 Subject: [PATCH] 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 Reviewed-by: Amitay Isaacs --- ctdb/doc/ctdb-tunables.7.xml | 9 --------- ctdb/doc/ctdb.1.xml | 34 +++++++++++++++++++++++++++------- ctdb/server/ctdb_recover.c | 4 ++-- ctdb/server/ctdb_recoverd.c | 12 +++++------- ctdb/server/ctdb_server.c | 1 - ctdb/server/ctdb_tunables.c | 13 +++---------- 6 files changed, 37 insertions(+), 36 deletions(-) diff --git a/ctdb/doc/ctdb-tunables.7.xml b/ctdb/doc/ctdb-tunables.7.xml index 456e856355c..b029fdb75a8 100644 --- a/ctdb/doc/ctdb-tunables.7.xml +++ b/ctdb/doc/ctdb-tunables.7.xml @@ -448,15 +448,6 @@ - VerifyRecoveryLock - Default: 1 - - 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. - - - - VacuumInterval Default: 10 diff --git a/ctdb/doc/ctdb.1.xml b/ctdb/doc/ctdb.1.xml index 07df43bd761..1a1ae40fbec 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 getreclock - 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. Example output: - Reclock file:/gpfs/.ctdb/shared + Reclock file:/clusterfs/.ctdb/recovery.lock - setreclock [filename] + + setreclock <optional><parameter>FILE</parameter></optional> + + - 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. - 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. - 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 must be changed + on all nodes simultaneously by specifying (or similar). For information about configuring + the recovery lock file please see the + CTDB_RECOVERY_LOCK entry in + ctdbd.conf + 5 and the + --reclock entry in + ctdbd + 1. For information + about the recovery lock please see the RECOVERY + LOCK section in + ctdb + 7. diff --git a/ctdb/server/ctdb_recover.c b/ctdb/server/ctdb_recover.c index 10d088b6d0b..cd774187284 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 f86f57e570d..6ea381dbc6e 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 1e3fde497f6..2598c4cb5b3 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 069958653f1..f5713f8d416 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; } -- 2.11.4.GIT