From 0cd2c19d11f38f492214626a14bbfc2f1dcfb4ad Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 12:23:29 +0200 Subject: [PATCH 1/6] Feature: scheduler: priority-fencing-delay defaults to 0 meaning disabled This commit also documents the upcoming new behavior as discussed from: https://github.com/ClusterLabs/pacemaker/pull/2012 Any static/random delays that are introduced by `pcmk_delay_base/max` configured for the corresponding fencing resources will be added to this delay. This delay should be significantly greater than, safely twice, the maximum `pcmk_delay_base/max`. By default, priority fencing delay is disabled. --- doc/Pacemaker_Explained/en-US/Ch-Options.txt | 16 ++++++++-------- include/crm/pengine/pe_types.h | 2 +- lib/pengine/common.c | 18 +++++++++--------- lib/pengine/unpack.c | 2 -- 4 files changed, 18 insertions(+), 20 deletions(-) diff --git a/doc/Pacemaker_Explained/en-US/Ch-Options.txt b/doc/Pacemaker_Explained/en-US/Ch-Options.txt index 58080b15572..faefe7c8887 100644 --- a/doc/Pacemaker_Explained/en-US/Ch-Options.txt +++ b/doc/Pacemaker_Explained/en-US/Ch-Options.txt @@ -292,21 +292,21 @@ are +stop+ to attempt to immediately stop pacemaker and stay stopped, or on failure. The default is likely to be changed to +panic+ in a future release. '(since 2.0.3)' -| priority-fencing-delay | | +| priority-fencing-delay | 0 | indexterm:[priority-fencing-delay,Cluster Option] indexterm:[Cluster,Option,priority-fencing-delay] -Enforce specified delay for the fencings that are targeting the lost +Apply specified delay for the fencings that are targeting the lost nodes with the highest total resource priority in case we don't have the majority of the nodes in our cluster partition, so that the more significant nodes potentially win any fencing match, which is especially meaningful under split-brain of 2-node cluster. A promoted resource instance takes the base priority + 1 -on calculation if the base priority is not 0. If all the nodes -have equal priority, then any pcmk_delay_base/max configured for -the corresponding fencing resources will be applied. Otherwise as -long as it's set, even if to 0, it takes precedence over any -configured pcmk_delay_base/max. By default, priority fencing -delay is disabled. '(since 2.0.4)' +on calculation if the base priority is not 0. Any static/random +delays that are introduced by `pcmk_delay_base/max` configured +for the corresponding fencing resources will be added to this +delay. This delay should be significantly greater than, safely +twice, the maximum `pcmk_delay_base/max`. By default, priority +fencing delay is disabled. '(since 2.0.4)' | cluster-delay | 60s | indexterm:[cluster-delay,Cluster Option] diff --git a/include/crm/pengine/pe_types.h b/include/crm/pengine/pe_types.h index a8974f8fd0c..ba8849181f9 100644 --- a/include/crm/pengine/pe_types.h +++ b/include/crm/pengine/pe_types.h @@ -176,7 +176,7 @@ struct pe_working_set_s { time_t recheck_by; // Hint to controller to re-run scheduler by this time int ninstances; // Total number of resource instances guint shutdown_lock;// How long (seconds) to lock resources to shutdown node - int priority_fencing_delay; // Enforced priority fencing delay + int priority_fencing_delay; // Priority fencing delay }; enum pe_check_parameters { diff --git a/lib/pengine/common.c b/lib/pengine/common.c index 7326916a00d..ded6df80bbc 100644 --- a/lib/pengine/common.c +++ b/lib/pengine/common.c @@ -160,20 +160,20 @@ static pcmk__cluster_option_t pe_opts[] = { }, { XML_CONFIG_ATTR_PRIORITY_FENCING_DELAY, NULL, "time", NULL, - NULL, pcmk__valid_interval_spec, - "Enforced fencing delay targeting the lost nodes with the highest total resource priority", - "Enforce specified delay for the fencings that are targeting the lost " + "0", pcmk__valid_interval_spec, + "Apply fencing delay targeting the lost nodes with the highest total resource priority", + "Apply specified delay for the fencings that are targeting the lost " "nodes with the highest total resource priority in case we don't " "have the majority of the nodes in our cluster partition, so that " "the more significant nodes potentially win any fencing match, " "which is especially meaningful under split-brain of 2-node " "cluster. A promoted resource instance takes the base priority + 1 " - "on calculation if the base priority is not 0. If all the nodes " - "have equal priority, then any pcmk_delay_base/max configured for " - "the corresponding fencing resources will be applied. Otherwise as " - "long as it's set, even if to 0, it takes precedence over any " - "configured pcmk_delay_base/max. By default, priority fencing " - "delay is disabled." + "on calculation if the base priority is not 0. Any static/random " + "delays that are introduced by `pcmk_delay_base/max` configured " + "for the corresponding fencing resources will be added to this " + "delay. This delay should be significantly greater than, safely " + "twice, the maximum `pcmk_delay_base/max`. By default, priority " + "fencing delay is disabled." }, { diff --git a/lib/pengine/unpack.c b/lib/pengine/unpack.c index 374bc901c0d..532a3e6fd86 100644 --- a/lib/pengine/unpack.c +++ b/lib/pengine/unpack.c @@ -233,8 +233,6 @@ unpack_config(xmlNode * config, pe_working_set_t * data_set) crm_debug("Concurrent fencing is %s", is_set(data_set->flags, pe_flag_concurrent_fencing) ? "enabled" : "disabled"); - // Default value -1 means `priority-fencing-delay` is disabled - data_set->priority_fencing_delay = -1; value = pe_pref(data_set->config_hash, XML_CONFIG_ATTR_PRIORITY_FENCING_DELAY); if (value) { From f65fc05b42a9009c812896aa6435d0e8262d5991 Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 12:55:51 +0200 Subject: [PATCH 2/6] Feature: scheduler: do not differentiate the case where all the nodes have equal priority In any cases, priority-fencing-delay won't take precedence over any configured pcmk_delay_base/max. --- lib/pengine/utils.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/pengine/utils.c b/lib/pengine/utils.c index d6a18401417..923c557a25c 100644 --- a/lib/pengine/utils.c +++ b/lib/pengine/utils.c @@ -2337,17 +2337,17 @@ node_priority_fencing_delay(pe_node_t * node, pe_working_set_t * data_set) GListPtr gIter = NULL; // `priority-fencing-delay` is disabled - if (data_set->priority_fencing_delay < 0) { - return -1; + if (data_set->priority_fencing_delay <= 0) { + return 0; } - /* No need to delay fencing if the fencing target is not a normal cluster + /* No need to request a delay if the fencing target is not a normal cluster * member, for example if it's a remote node or a guest node. */ if (node->details->type != node_member) { return 0; } - // No need to delay fencing if the fencing target is in our partition + // No need to request a delay if the fencing target is in our partition if (node->details->online) { return 0; } @@ -2384,7 +2384,7 @@ node_priority_fencing_delay(pe_node_t * node, pe_working_set_t * data_set) /* All the nodes have equal priority. * Any configured corresponding `pcmk_delay_base/max` will be applied. */ if (lowest_priority == top_priority) { - return -1; + return 0; } if (node->details->priority < top_priority) { @@ -2468,7 +2468,7 @@ pe_fence_op(pe_node_t * node, const char *op, bool optional, const char *reason, free(op_key); } - if (data_set->priority_fencing_delay >= 0 + if (data_set->priority_fencing_delay > 0 /* It's a suitable case where `priority-fencing-delay` applies. * At least add `priority-fencing-delay` field as an indicator. */ From a0cbd527aacc35fd8e4312865597333680326296 Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 13:12:29 +0200 Subject: [PATCH 3/6] Feature: fencer: any delays from pcmk_delay_base/max are added to requested fencing delay Requested fencing delay doesn't take precedence over any configured pcmk_delay_base/max. A delay value -1 now means disable also any static/random fencing delays from pcmk_delay_base/max. It's not used by any consumers for now. --- daemons/fenced/fenced_commands.c | 54 +++++++++++++++---------------- daemons/fenced/fenced_remote.c | 13 +++----- daemons/fenced/pacemaker-fenced.h | 4 +-- include/crm/stonith-ng.h | 5 +-- lib/fencing/st_client.c | 7 ++-- 5 files changed, 38 insertions(+), 45 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index aa224c59671..88a78544d13 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -241,8 +241,7 @@ create_async_command(xmlNode * msg) crm_element_value_int(msg, F_STONITH_CALLOPTS, &(cmd->options)); crm_element_value_int(msg, F_STONITH_TIMEOUT, &(cmd->default_timeout)); cmd->timeout = cmd->default_timeout; - // Default value -1 means no enforced fencing delay - cmd->start_delay = -1; + // Value -1 means disable any static/random fencing delays crm_element_value_int(msg, F_STONITH_DELAY, &(cmd->start_delay)); cmd->origin = crm_element_value_copy(msg, F_ORIG); @@ -465,7 +464,7 @@ schedule_stonith_command(async_command_t * cmd, stonith_device_t * device) { int delay_max = 0; int delay_base = 0; - bool delay_enforced = (cmd->start_delay >= 0); + int requested_delay = cmd->start_delay; CRM_CHECK(cmd != NULL, return); CRM_CHECK(device != NULL, return); @@ -498,35 +497,36 @@ schedule_stonith_command(async_command_t * cmd, stonith_device_t * device) device->pending_ops = g_list_append(device->pending_ops, cmd); mainloop_set_trigger(device->work); - // No enforced fencing delay - if (delay_enforced == FALSE) { - delay_max = get_action_delay_max(device, cmd->action); - delay_base = get_action_delay_base(device, cmd->action); - if (delay_max == 0) { - delay_max = delay_base; - } - if (delay_max < delay_base) { - crm_warn("Base-delay (%ds) is larger than max-delay (%ds) " - "for %s on %s - limiting to max-delay", - delay_base, delay_max, cmd->action, device->id); - delay_base = delay_max; - } - if (delay_max > 0) { - // coverity[dont_call] We're not using rand() for security - cmd->start_delay = - ((delay_max != delay_base)?(rand() % (delay_max - delay_base)):0) - + delay_base; - } + // Value -1 means disable any static/random fencing delays + if (requested_delay < 0) { + return; + } + + delay_max = get_action_delay_max(device, cmd->action); + delay_base = get_action_delay_base(device, cmd->action); + if (delay_max == 0) { + delay_max = delay_base; + } + if (delay_max < delay_base) { + crm_warn("Base-delay (%ds) is larger than max-delay (%ds) " + "for %s on %s - limiting to max-delay", + delay_base, delay_max, cmd->action, device->id); + delay_base = delay_max; + } + if (delay_max > 0) { + // coverity[dont_call] We're not using rand() for security + cmd->start_delay += + ((delay_max != delay_base)?(rand() % (delay_max - delay_base)):0) + + delay_base; } if (cmd->start_delay > 0) { - crm_notice("Delaying '%s' action%s%s on %s for %s%ds (timeout=%ds, base=%ds, " - "max=%ds)", + crm_notice("Delaying '%s' action%s%s on %s for %ds (timeout=%ds, " + "requested_delay=%ds, base=%ds, max=%ds)", cmd->action, cmd->victim ? " targeting " : "", cmd->victim ? cmd->victim : "", - device->id, delay_enforced ? "enforced " : "", - cmd->start_delay, cmd->timeout, - delay_base, delay_max); + device->id, cmd->start_delay, cmd->timeout, + requested_delay, delay_base, delay_max); cmd->delay_id = g_timeout_add_seconds(cmd->start_delay, start_delay_helper, cmd); } diff --git a/daemons/fenced/fenced_remote.c b/daemons/fenced/fenced_remote.c index ac9bcb06923..b89c40abd9a 100644 --- a/daemons/fenced/fenced_remote.c +++ b/daemons/fenced/fenced_remote.c @@ -842,7 +842,7 @@ stonith_topology_next(remote_fencing_op_t * op) op->client_name, op->originator, op->id); set_op_device_list(op, tp->levels[op->level]); - // The enforced delay has been applied for the first fencing level + // The requested delay has been applied for the first fencing level if (op->level > 1 && op->delay > 0) { op->delay = 0; } @@ -1004,9 +1004,7 @@ create_remote_stonith_op(const char *client, xmlNode * request, gboolean peer) op = calloc(1, sizeof(remote_fencing_op_t)); crm_element_value_int(request, F_STONITH_TIMEOUT, &(op->base_timeout)); - - // Default value -1 means no enforced fencing delay - op->delay = -1; + // Value -1 means disable any static/random fencing delays crm_element_value_int(request, F_STONITH_DELAY, &(op->delay)); if (peer && dev) { @@ -1458,7 +1456,7 @@ advance_op_topology(remote_fencing_op_t *op, const char *device, xmlNode *msg, crm_trace("Next targeting %s on behalf of %s@%s (rc was %d)", op->target, op->originator, op->client_name, rc); - // The enforced delay has been applied for the first device + // The requested delay has been applied for the first device if (op->delay > 0) { op->delay = 0; } @@ -1517,10 +1515,7 @@ call_remote_stonith(remote_fencing_op_t * op, st_query_result_t * peer) crm_xml_add(remote_op, F_STONITH_CLIENTNAME, op->client_name); crm_xml_add_int(remote_op, F_STONITH_TIMEOUT, timeout); crm_xml_add_int(remote_op, F_STONITH_CALLOPTS, op->call_options); - - if (op->delay >= 0) { - crm_xml_add_int(remote_op, F_STONITH_DELAY, op->delay); - } + crm_xml_add_int(remote_op, F_STONITH_DELAY, op->delay); if (device) { timeout_one = TIMEOUT_MULTIPLY_FACTOR * diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 73cd28e340f..02a8c0a8e59 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -113,8 +113,8 @@ typedef struct remote_fencing_op_s { * values associated with the devices this fencing operation may call */ gint total_timeout; - /*! Enforced fencing delay. - * Default value -1 means no enforced fencing delay. */ + /*! Requested fencing delay. + * Value -1 means disable any static/random fencing delays. */ int delay; /*! Delegate is the node being asked to perform a fencing action diff --git a/include/crm/stonith-ng.h b/include/crm/stonith-ng.h index c46956a2142..08587e4dbfc 100644 --- a/include/crm/stonith-ng.h +++ b/include/crm/stonith-ng.h @@ -393,7 +393,7 @@ typedef struct stonith_api_operations_s char **error_output); /*! - * \brief Issue a fencing action against a node with enforced fencing delay. + * \brief Issue a fencing action against a node with requested fencing delay. * * \note Possible actions are, 'on', 'off', and 'reboot'. * @@ -403,7 +403,8 @@ typedef struct stonith_api_operations_s * \param action, The fencing action to take * \param timeout, The default per device timeout to use with each device * capable of fencing the target. - * \param delay, Any enforced fencing delay. -1 to disable + * \param delay, Apply a fencing delay. Value -1 means disable also any + * static/random fencing delays from pcmk_delay_base/max * * \retval 0 success * \retval negative error code on failure. diff --git a/lib/fencing/st_client.c b/lib/fencing/st_client.c index 0806c1333a6..1c7de52c73f 100644 --- a/lib/fencing/st_client.c +++ b/lib/fencing/st_client.c @@ -1080,10 +1080,7 @@ stonith_api_fence_with_delay(stonith_t * stonith, int call_options, const char * crm_xml_add(data, F_STONITH_ACTION, action); crm_xml_add_int(data, F_STONITH_TIMEOUT, timeout); crm_xml_add_int(data, F_STONITH_TOLERANCE, tolerance); - - if (delay >= 0) { - crm_xml_add_int(data, F_STONITH_DELAY, delay); - } + crm_xml_add_int(data, F_STONITH_DELAY, delay); rc = stonith_send_command(stonith, STONITH_OP_FENCE, data, NULL, call_options, timeout); free_xml(data); @@ -1096,7 +1093,7 @@ stonith_api_fence(stonith_t * stonith, int call_options, const char *node, const int timeout, int tolerance) { return stonith_api_fence_with_delay(stonith, call_options, node, action, - timeout, tolerance, -1); + timeout, tolerance, 0); } static int From d375eccaf0f43352d20f906d98c292758b7c394c Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 15:04:34 +0200 Subject: [PATCH 4/6] Feature: stonith_admin: --delay option defaults to 0 This commit also documents the current behavior in the help: - Any static/random delays from pcmk_delay_base/max will be added to requested fencing delay. - A delay value -1 now means disable also any static/random fencing delays from pcmk_delay_base/max. --- include/pcmki/pcmki_fence.h | 3 ++- tools/stonith_admin.c | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/pcmki/pcmki_fence.h b/include/pcmki/pcmki_fence.h index e091a171045..5f4757a8179 100644 --- a/include/pcmki/pcmki_fence.h +++ b/include/pcmki/pcmki_fence.h @@ -26,7 +26,8 @@ * \param[in] tolerance If a successful action for \p target happened within * this many ms, return 0 without performing the action * again. - * \param[in] delay Enforce a fencing delay. Value -1 means disabled. + * \param[in] delay Apply a fencing delay. Value -1 means disable also any + * static/random fencing delays from pcmk_delay_base/max * * \return Standard Pacemaker return code */ diff --git a/tools/stonith_admin.c b/tools/stonith_admin.c index a50688e748b..4a6b49f7543 100644 --- a/tools/stonith_admin.c +++ b/tools/stonith_admin.c @@ -71,7 +71,7 @@ struct { char *unregister_level; } options = { .timeout = 120, - .delay = -1 + .delay = 0 }; gboolean add_env_params(const gchar *option_name, const gchar *optarg, gpointer data, GError **error); @@ -208,8 +208,10 @@ static GOptionEntry addl_entries[] = { INDENT "used with most commands).", "SECONDS" }, { "delay", 'y', 0, G_OPTION_ARG_INT, &options.delay, - "Enforced fencing delay in seconds (default -1 (disabled);\n" - INDENT "with --fence, --reboot, --unfence).", + "Apply a fencing delay in seconds. Any static/random delays from\n" + INDENT "pcmk_delay_base/max will be added, otherwise all\n" + INDENT "disabled with the value -1\n" + INDENT "(default 0; with --fence, --reboot, --unfence).", "SECONDS" }, { "as-node-id", 'n', 0, G_OPTION_ARG_NONE, &options.as_nodeid, "(Advanced) The supplied node is the corosync node ID\n" From 0653b86d1f7767f378f2c8e608718fe6fc080f56 Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 15:52:50 +0200 Subject: [PATCH 5/6] Feature: controller: requested priority fencing delay defaults to 0 --- daemons/controld/controld_fencing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemons/controld/controld_fencing.c b/daemons/controld/controld_fencing.c index e871326426e..9a1d0945918 100644 --- a/daemons/controld/controld_fencing.c +++ b/daemons/controld/controld_fencing.c @@ -848,7 +848,7 @@ te_fence_node(crm_graph_t *graph, crm_action_t *action) rc = stonith_api->cmds->fence_with_delay(stonith_api, options, target, type, (int) (transition_graph->stonith_timeout / 1000), - 0, crm_atoi(priority_delay, "-1")); + 0, crm_atoi(priority_delay, "0")); transition_key = pcmk__transition_key(transition_graph->id, action->id, 0, te_uuid), From 6ab6d380b2f8df97e51cbb494a69dfc87272888a Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Wed, 1 Apr 2020 19:17:48 +0200 Subject: [PATCH 6/6] Test: fencer: update cpg_topology_delay test to also verify pcmk_delay_base is added This commit also updates log patterns for the log changes. --- cts/cts-fencing.in | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/cts/cts-fencing.in b/cts/cts-fencing.in index 239319cb169..4444024380e 100644 --- a/cts/cts-fencing.in +++ b/cts/cts-fencing.in @@ -1126,22 +1126,23 @@ class Tests(object): test.add_stonith_neg_log_pattern("does not advertise support for 'reboot', performing 'off'") test.add_stonith_log_pattern("with device 'true1' returned: 0 (OK)") - # make sure enforced fencing delay is applied only for the first device in the first level + # make sure requested fencing delay is applied only for the first device in the first level + # make sure static delay from pcmk_delay_base is added for test_type in test_types: if test_type["use_cpg"] == 0: continue test = self.new_test("%s_topology_delay" % test_type["prefix"], - "Verify enforced fencing delay is applied only for the first device in the first level.", + "Verify requested fencing delay is applied only for the first device in the first level and pcmk_delay_base is added.", test_type["use_cpg"]) test.add_cmd("stonith_admin", - "--output-as=xml -R true1 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\"") + "--output-as=xml -R true1 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\" -o \"pcmk_delay_base=1\"") test.add_cmd("stonith_admin", - "--output-as=xml -R false1 -a fence_dummy -o \"mode=fail\" -o \"pcmk_host_list=node1 node2 node3\"") + "--output-as=xml -R false1 -a fence_dummy -o \"mode=fail\" -o \"pcmk_host_list=node1 node2 node3\" -o \"pcmk_delay_base=1\"") test.add_cmd("stonith_admin", - "--output-as=xml -R true2 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\"") + "--output-as=xml -R true2 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\"") test.add_cmd("stonith_admin", - "--output-as=xml -R true3 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\"") + "--output-as=xml -R true3 -a fence_dummy -o \"mode=pass\" -o \"pcmk_host_list=node1 node2 node3\"") test.add_cmd("stonith_admin", "--output-as=xml -r node3 -i 1 -v true1") test.add_cmd("stonith_admin", "--output-as=xml -r node3 -i 1 -v false1") @@ -1150,8 +1151,8 @@ class Tests(object): test.add_cmd("stonith_admin", "--output-as=xml -F node3 --delay 1") - test.add_stonith_log_pattern("Delaying 'off' action targeting node3 on true1 for enforced 1s") - test.add_stonith_neg_log_pattern("Delaying 'off' action targeting node3 on false1") + test.add_stonith_log_pattern("Delaying 'off' action targeting node3 on true1 for 2s (timeout=120s, requested_delay=1s, base=1s, max=1s)") + test.add_stonith_log_pattern("Delaying 'off' action targeting node3 on false1 for 1s (timeout=120s, requested_delay=0s, base=1s, max=1s)") test.add_stonith_neg_log_pattern("Delaying 'off' action targeting node3 on true2") test.add_stonith_neg_log_pattern("Delaying 'off' action targeting node3 on true3")