From e4ab7ea9c5f57029b289b0d66c27d16ab309adb4 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Wed, 12 Sep 2018 14:18:00 +1000 Subject: [PATCH 01/10] ctdb-cluster-mutex: Reset SIGTERM handler in cluster mutex child If SIGTERM is received and the tevent signal handler setup in the recovery daemon is still enabled then the signal is handled and a corresponding event is queued. The child never runs an event loop so the signal is effectively ignored. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit 5a6b139884f08ee2ee10f9d16fe56ad8fb5352a6) --- ctdb/server/ctdb_cluster_mutex.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ctdb/server/ctdb_cluster_mutex.c b/ctdb/server/ctdb_cluster_mutex.c index 804c6d5dd8c..e8c75debfb8 100644 --- a/ctdb/server/ctdb_cluster_mutex.c +++ b/ctdb/server/ctdb_cluster_mutex.c @@ -234,6 +234,16 @@ ctdb_cluster_mutex(TALLOC_CTX *mem_ctx, } if (h->child == 0) { + struct sigaction sa = { + .sa_handler = SIG_DFL, + }; + + ret = sigaction(SIGTERM, &sa, NULL); + if (ret != 0) { + DBG_WARNING("Failed to reset signal handler (%d)\n", + errno); + } + /* Make stdout point to the pipe */ close(STDOUT_FILENO); dup2(h->fd[1], STDOUT_FILENO); -- 2.18.0 From bfedc04b1528efb40973dc1bb28755d01abcb1ee Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Wed, 12 Sep 2018 17:51:47 +1000 Subject: [PATCH 02/10] ctdb-cluster-mutex: Block signals around fork If SIGTERM is received and the tevent signal handler setup in the recovery daemon is still enabled then the signal is handled and a corresponding event is queued. The child never runs an event loop so the signal is effectively ignored. Resetting the SIGTERM handler isn't enough. A signal can arrive before that. Block SIGTERM before forking and then immediately unblock it in the parent. In the child, unblock SIGTERM after the signal handler is reset. An explicit unblock is needed because according to sigprocmask(2) "the signal mask is preserved across execve(2)". BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit e789d0da57fc3fc6d22bfa00577a2e65034ca27a) --- ctdb/server/ctdb_cluster_mutex.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/ctdb/server/ctdb_cluster_mutex.c b/ctdb/server/ctdb_cluster_mutex.c index e8c75debfb8..330d5fd1d90 100644 --- a/ctdb/server/ctdb_cluster_mutex.c +++ b/ctdb/server/ctdb_cluster_mutex.c @@ -196,6 +196,7 @@ ctdb_cluster_mutex(TALLOC_CTX *mem_ctx, { struct ctdb_cluster_mutex_handle *h; char **args; + sigset_t sigset_term; int ret; h = talloc(mem_ctx, struct ctdb_cluster_mutex_handle); @@ -225,11 +226,22 @@ ctdb_cluster_mutex(TALLOC_CTX *mem_ctx, return NULL; } + sigemptyset(&sigset_term); + sigaddset(&sigset_term, SIGTERM); + ret = sigprocmask(SIG_BLOCK, &sigset_term, NULL); + if (ret != 0) { + DBG_WARNING("Failed to block SIGTERM (%d)\n", errno); + } + h->child = ctdb_fork(ctdb); if (h->child == (pid_t)-1) { close(h->fd[0]); close(h->fd[1]); talloc_free(h); + ret = sigprocmask(SIG_UNBLOCK, &sigset_term, NULL); + if (ret != 0) { + DBG_WARNING("Failed to unblock SIGTERM (%d)\n", errno); + } return NULL; } @@ -244,6 +256,11 @@ ctdb_cluster_mutex(TALLOC_CTX *mem_ctx, errno); } + ret = sigprocmask(SIG_UNBLOCK, &sigset_term, NULL); + if (ret != 0) { + DBG_WARNING("Failed to unblock SIGTERM (%d)\n", errno); + } + /* Make stdout point to the pipe */ close(STDOUT_FILENO); dup2(h->fd[1], STDOUT_FILENO); @@ -258,6 +275,11 @@ ctdb_cluster_mutex(TALLOC_CTX *mem_ctx, /* Parent */ + ret = sigprocmask(SIG_UNBLOCK, &sigset_term, NULL); + if (ret != 0) { + DBG_WARNING("Failed to unblock SIGTERM (%d)\n", errno); + } + DEBUG(DEBUG_DEBUG, (__location__ " Created PIPE FD:%d\n", h->fd[0])); set_close_on_exec(h->fd[0]); -- 2.18.0 From c39f9ffad2a2e2dc09e7632f25b8b241f6c37667 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Sun, 9 Sep 2018 08:27:46 +1000 Subject: [PATCH 03/10] ctdb-recoverd: Clean up taking of recovery lock No functional changes, just coding style cleanups and debug message tweaks. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit 59fc01646c7d65ba90b0a1a34c3795ff842351c5) --- ctdb/server/ctdb_recoverd.c | 41 ++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index 3e85186f35a..d64ab8e858a 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -1315,31 +1315,38 @@ static int do_recovery(struct ctdb_recoverd *rec, goto fail; } - if (ctdb->recovery_lock != NULL) { + if (ctdb->recovery_lock != NULL) { if (ctdb_recovery_have_lock(rec)) { - DEBUG(DEBUG_NOTICE, ("Already holding recovery lock\n")); + D_NOTICE("Already holding recovery lock\n"); } else { - DEBUG(DEBUG_NOTICE, ("Attempting to take recovery lock (%s)\n", - ctdb->recovery_lock)); - if (!ctdb_recovery_lock(rec)) { - if (ctdb->runstate == CTDB_RUNSTATE_FIRST_RECOVERY) { - /* If ctdb is trying first recovery, it's - * possible that current node does not know - * yet who the recmaster is. + bool ok; + + D_NOTICE("Attempting to take recovery lock (%s)\n", + ctdb->recovery_lock); + + ok = ctdb_recovery_lock(rec); + if (! ok) { + D_ERR("Unable to take recovery lock\n"); + if (ctdb->runstate == + CTDB_RUNSTATE_FIRST_RECOVERY) { + /* + * First recovery? Perhaps + * current node does not yet + * know who the recmaster is. */ - DEBUG(DEBUG_ERR, ("Unable to get recovery lock" - " - retrying recovery\n")); + D_ERR("Retrying recovery\n"); goto fail; } - DEBUG(DEBUG_ERR,("Unable to get recovery lock - aborting recovery " - "and ban ourself for %u seconds\n", - ctdb->tunable.recovery_ban_period)); - ctdb_ban_node(rec, pnn, ctdb->tunable.recovery_ban_period); + D_ERR("Abort recovery, " + "ban this node for %u seconds\n", + ctdb->tunable.recovery_ban_period); + ctdb_ban_node(rec, + pnn, + ctdb->tunable.recovery_ban_period); goto fail; } - DEBUG(DEBUG_NOTICE, - ("Recovery lock taken successfully by recovery daemon\n")); + D_NOTICE("Recovery lock taken successfully\n"); } } -- 2.18.0 From 9690adbc8b40538fabe5ca596770fcc1230f80bb Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Sun, 9 Sep 2018 08:30:50 +1000 Subject: [PATCH 04/10] ctdb-recoverd: Re-check master on failure to take recovery lock If the master changed while trying to take the lock then fail gracefully. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit c516e58ce92c420dc993bd9b7f1433641bd764bd) --- ctdb/server/ctdb_recoverd.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index d64ab8e858a..3578ccd3a8b 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -1327,6 +1327,15 @@ static int do_recovery(struct ctdb_recoverd *rec, ok = ctdb_recovery_lock(rec); if (! ok) { D_ERR("Unable to take recovery lock\n"); + + if (pnn != rec->recmaster) { + D_NOTICE("Recovery master changed to %u," + " aborting recovery\n", + rec->recmaster); + rec->need_recovery = false; + goto fail; + } + if (ctdb->runstate == CTDB_RUNSTATE_FIRST_RECOVERY) { /* -- 2.18.0 From d4be6518dcdd285647d441e2e4ab912b3b2e7b78 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Mon, 3 Sep 2018 11:30:06 +1000 Subject: [PATCH 05/10] ctdb-recoverd: Rename hold_reclock_state to ctdb_recovery_lock_handle This will be a longer lived structure. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit af22f03dbe9040f5f743eb85bb50d411269bbab4) --- ctdb/server/ctdb_recoverd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index 3578ccd3a8b..c61fec7cce8 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -881,7 +881,7 @@ static bool ctdb_recovery_have_lock(struct ctdb_recoverd *rec) return (rec->recovery_lock_handle != NULL); } -struct hold_reclock_state { +struct ctdb_recovery_lock_handle { bool done; bool locked; double latency; @@ -891,8 +891,8 @@ static void take_reclock_handler(char status, double latency, void *private_data) { - struct hold_reclock_state *s = - (struct hold_reclock_state *) private_data; + struct ctdb_recovery_lock_handle *s = + (struct ctdb_recovery_lock_handle *) private_data; switch (status) { case '0': @@ -932,7 +932,7 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) { struct ctdb_context *ctdb = rec->ctdb; struct ctdb_cluster_mutex_handle *h; - struct hold_reclock_state s = { + struct ctdb_recovery_lock_handle s = { .done = false, .locked = false, .latency = 0, -- 2.18.0 From aa4f3129a98bb217de0376ee308da1e687c4aabc Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Mon, 3 Sep 2018 11:43:44 +1000 Subject: [PATCH 06/10] ctdb-recoverd: Use talloc() to allocate recovery lock handle At the moment this is still local and is freed after the mutex is successfully taken. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit a53b264aee7d620ee8ecf9114b0014c5bb678484) --- ctdb/server/ctdb_recoverd.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index c61fec7cce8..4655dcc496a 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -932,31 +932,43 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) { struct ctdb_context *ctdb = rec->ctdb; struct ctdb_cluster_mutex_handle *h; - struct ctdb_recovery_lock_handle s = { - .done = false, - .locked = false, - .latency = 0, + struct ctdb_recovery_lock_handle *s; + + s = talloc_zero(rec, struct ctdb_recovery_lock_handle); + if (s == NULL) { + DBG_ERR("Memory allocation error\n"); + return false; }; - h = ctdb_cluster_mutex(rec, ctdb, ctdb->recovery_lock, 0, - take_reclock_handler, &s, - lost_reclock_handler, rec); + h = ctdb_cluster_mutex(rec, + ctdb, + ctdb->recovery_lock, + 0, + take_reclock_handler, + s, + lost_reclock_handler, + rec); if (h == NULL) { + talloc_free(s); return false; } - while (!s.done) { + while (! s->done) { tevent_loop_once(ctdb->ev); } - if (! s.locked) { + if (! s->locked) { + talloc_free(s); talloc_free(h); return false; } rec->recovery_lock_handle = h; - ctdb_ctrl_report_recd_lock_latency(ctdb, CONTROL_TIMEOUT(), - s.latency); + ctdb_ctrl_report_recd_lock_latency(ctdb, + CONTROL_TIMEOUT(), + s->latency); + + talloc_free(s); return true; } -- 2.18.0 From c49f3bd4f186d0257aa27ca9c1cd90812b96e53d Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Mon, 3 Sep 2018 12:39:32 +1000 Subject: [PATCH 07/10] ctdb-recoverd: Store recovery lock handle ... not just cluster mutex handle. This makes the recovery lock handle long-lived and with allow the releasing code to cancel an in-progress attempt to take the recovery lock. The cluster mutex handle is now allocated off the recovery lock handle. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit c52216740bd81b68876de06e104822bbbca86df9) --- ctdb/server/ctdb_recoverd.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index 4655dcc496a..46357b6ed69 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -239,6 +239,8 @@ struct ctdb_banning_state { struct timeval last_reported_time; }; +struct ctdb_recovery_lock_handle; + /* private state of recovery daemon */ @@ -260,7 +262,7 @@ struct ctdb_recoverd { uint32_t *force_rebalance_nodes; struct ctdb_node_capabilities *caps; bool frozen_on_inactive; - struct ctdb_cluster_mutex_handle *recovery_lock_handle; + struct ctdb_recovery_lock_handle *recovery_lock_handle; }; #define CONTROL_TIMEOUT() timeval_current_ofs(ctdb->tunable.recover_timeout, 0) @@ -885,6 +887,7 @@ struct ctdb_recovery_lock_handle { bool done; bool locked; double latency; + struct ctdb_cluster_mutex_handle *h; }; static void take_reclock_handler(char status, @@ -940,7 +943,7 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) return false; }; - h = ctdb_cluster_mutex(rec, + h = ctdb_cluster_mutex(s, ctdb, ctdb->recovery_lock, 0, @@ -959,17 +962,15 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) if (! s->locked) { talloc_free(s); - talloc_free(h); return false; } - rec->recovery_lock_handle = h; + rec->recovery_lock_handle = s; + s->h = h; ctdb_ctrl_report_recd_lock_latency(ctdb, CONTROL_TIMEOUT(), s->latency); - talloc_free(s); - return true; } -- 2.18.0 From 3c6c5638bf8c0b2c11f9839f64c42954826b9525 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Tue, 11 Sep 2018 15:05:19 +1000 Subject: [PATCH 08/10] ctdb-recoverd: Return early when the recovery lock is not held This makes upcoming changes simpler. Update to modern debug macro while touching relevant line. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit a755d060c13b65dfb6d73979aaf111c489882bfb) --- ctdb/server/ctdb_recoverd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index 46357b6ed69..6a021101535 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -976,10 +976,12 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) static void ctdb_recovery_unlock(struct ctdb_recoverd *rec) { - if (rec->recovery_lock_handle != NULL) { - DEBUG(DEBUG_NOTICE, ("Releasing recovery lock\n")); - TALLOC_FREE(rec->recovery_lock_handle); + if (rec->recovery_lock_handle == NULL) { + return; } + + D_NOTICE("Releasing recovery lock\n"); + TALLOC_FREE(rec->recovery_lock_handle); } static void ban_misbehaving_nodes(struct ctdb_recoverd *rec, bool *self_ban) -- 2.18.0 From 3df8ae880d7cc69273e64c7a635ca3c65354861c Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Mon, 3 Sep 2018 13:01:19 +1000 Subject: [PATCH 09/10] ctdb-recoverd: Handle cancellation when releasing recovery lock If the recovery lock is in the process of being taken then free the cluster mutex handle but leave the recovery lock handle in place. This allows ctdb_recovery_lock() to fail. Note that this isn't yet live because rec->recovery_lock_handle is still only set at the completion of the attempt to take the lock. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit b1dc5687844e90b0e3c39cb46a1116c86118fbf4) --- ctdb/server/ctdb_recoverd.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index 6a021101535..d0eceee62f9 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -980,6 +980,20 @@ static void ctdb_recovery_unlock(struct ctdb_recoverd *rec) return; } + if (! rec->recovery_lock_handle->done) { + /* + * Taking of recovery lock still in progress. Free + * the cluster mutex handle to release it but leave + * the recovery lock handle in place to allow taking + * of the lock to fail. + */ + D_NOTICE("Cancelling recovery lock\n"); + TALLOC_FREE(rec->recovery_lock_handle->h); + rec->recovery_lock_handle->done = true; + rec->recovery_lock_handle->locked = false; + return; + } + D_NOTICE("Releasing recovery lock\n"); TALLOC_FREE(rec->recovery_lock_handle); } -- 2.18.0 From a7dd4c61fed940a7c65dcbdc365f2acc4057eac0 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Mon, 3 Sep 2018 13:30:57 +1000 Subject: [PATCH 10/10] ctdb-recoverd: Set recovery lock handle at start of attempt This allows the attempt to be cancelled if an election is lost and an unlock is done before the attempt is completed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13617 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs Autobuild-User(master): Martin Schwenke Autobuild-Date(master): Tue Sep 18 02:18:30 CEST 2018 on sn-devel-144 (cherry picked from commit 486022ef8f43251258f255ffa15f1a01bc6aa2b7) --- ctdb/server/ctdb_recoverd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c index d0eceee62f9..673c99c3d34 100644 --- a/ctdb/server/ctdb_recoverd.c +++ b/ctdb/server/ctdb_recoverd.c @@ -956,17 +956,18 @@ static bool ctdb_recovery_lock(struct ctdb_recoverd *rec) return false; } + rec->recovery_lock_handle = s; + s->h = h; + while (! s->done) { tevent_loop_once(ctdb->ev); } if (! s->locked) { - talloc_free(s); + TALLOC_FREE(rec->recovery_lock_handle); return false; } - rec->recovery_lock_handle = s; - s->h = h; ctdb_ctrl_report_recd_lock_latency(ctdb, CONTROL_TIMEOUT(), s->latency); -- 2.18.0