From 1d257b4cd8fae140774e88e269a7df479160a13e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 02:20:54 -0700 Subject: [PATCH 1/5] Refactor: fencer: Best practices in update_cib_stonith_devices() * Remove some nesting. * Use const where possible. * Improve variable names and scope. * Use a macro for repeated string literal. * Use sizeof() - 1 instead of strlen() for string literal. * Remove unnecessary escaping of single quote within double quotes. * Copy only what is needed of the xpath string, if anything. * Log assertion on malformed patchset, since we created it in cib_perform_op(). * Add a comment to clarify why we're (apparently) doing all this in the first place. Keep the watchdog_device_update() comment as-is because I haven't looked into it. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 72 +++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 887486d04e7..a10777a4494 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -232,13 +232,16 @@ cib_devices_update(void) } } +#define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" + static void update_cib_stonith_devices(const char *event, xmlNode * msg) { int format = 1; - xmlNode *wrapper = pcmk__xe_first_child(msg, PCMK__XE_CIB_UPDATE_RESULT, - NULL, NULL); - xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); + const xmlNode *wrapper = pcmk__xe_first_child(msg, + PCMK__XE_CIB_UPDATE_RESULT, + NULL, NULL); + const xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); char *reason = NULL; CRM_CHECK(patchset != NULL, return); @@ -249,12 +252,12 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) return; } - for (xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, NULL); + for (const xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, + NULL); change != NULL; change = pcmk__xe_next(change, NULL)) { const char *op = crm_element_value(change, PCMK_XA_OPERATION); const char *xpath = crm_element_value(change, PCMK_XA_PATH); - const char *shortpath = NULL; if (pcmk__str_eq(op, PCMK_VALUE_MOVE, pcmk__str_null_matches) || (strstr(xpath, "/" PCMK_XE_STATUS) != NULL)) { @@ -264,8 +267,8 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) if (pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none) && (strstr(xpath, "/" PCMK_XE_PRIMITIVE) != NULL)) { const char *rsc_id = NULL; - char *search = NULL; - char *mutable = NULL; + const char *end_quote = NULL; + char *copy = NULL; if ((strstr(xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) || (strstr(xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { @@ -274,28 +277,43 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) "resource"); break; } - mutable = pcmk__str_copy(xpath); - rsc_id = strstr(mutable, PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "=\'"); - if (rsc_id != NULL) { - rsc_id += strlen(PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "=\'"); - search = strchr(rsc_id, '\''); + + rsc_id = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); + if (rsc_id == NULL) { + continue; } - if (search != NULL) { - *search = 0; - stonith_device_remove(rsc_id, true); - /* watchdog_device_update called afterwards - to fall back to implicit definition if needed */ - } else { - crm_warn("Ignoring malformed CIB update (resource deletion)"); + + rsc_id += sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; + end_quote = strchr(rsc_id, '\''); + + CRM_LOG_ASSERT(end_quote != NULL); + if (end_quote == NULL) { + crm_err("Bug: Malformed item in Pacemaker-generated patchset"); + continue; } - free(mutable); - - } else if (strstr(xpath, "/" PCMK_XE_RESOURCES) - || strstr(xpath, "/" PCMK_XE_CONSTRAINTS) - || strstr(xpath, "/" PCMK_XE_RSC_DEFAULTS)) { - shortpath = strrchr(xpath, '/'); - pcmk__assert(shortpath != NULL); - reason = crm_strdup_printf("%s %s", op, shortpath+1); + + /* A primitive resource was removed. If it was a fencing resource, + * it's faster to remove it directly than to run the scheduler and + * update all device registrations. + */ + copy = strndup(rsc_id, end_quote - rsc_id); + pcmk__assert(copy != NULL); + stonith_device_remove(copy, true); + + /* watchdog_device_update called afterwards + to fall back to implicit definition if needed */ + + free(copy); + continue; + } + + if (strstr(xpath, "/" PCMK_XE_RESOURCES) + || strstr(xpath, "/" PCMK_XE_CONSTRAINTS) + || strstr(xpath, "/" PCMK_XE_RSC_DEFAULTS)) { + + const char *shortpath = strrchr(xpath, '/'); + + reason = crm_strdup_printf("%s %s", op, shortpath + 1); break; } } From a6df7c5feb182616271f6a3df547b433e3e652af Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 12:35:03 -0700 Subject: [PATCH 2/5] Refactor: fencer: Match primitive XPath only with ID An ID is required when schema validation is enabled anyway, and this allows us to be sure rsc_id will be non-NULL within the block. It also allows us to shorten some strstr() searches by starting later in the string, not that this will matter in practice. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index a10777a4494..cb673dfa6d6 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -258,32 +258,30 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) const char *op = crm_element_value(change, PCMK_XA_OPERATION); const char *xpath = crm_element_value(change, PCMK_XA_PATH); + const char *primitive_xpath = NULL; if (pcmk__str_eq(op, PCMK_VALUE_MOVE, pcmk__str_null_matches) || (strstr(xpath, "/" PCMK_XE_STATUS) != NULL)) { continue; } - if (pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none) - && (strstr(xpath, "/" PCMK_XE_PRIMITIVE) != NULL)) { + primitive_xpath = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); + if ((primitive_xpath != NULL) + && pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none)) { + const char *rsc_id = NULL; const char *end_quote = NULL; char *copy = NULL; - if ((strstr(xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) - || (strstr(xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { + if ((strstr(primitive_xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) + || (strstr(primitive_xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { reason = pcmk__str_copy("(meta) attribute deleted from " "resource"); break; } - rsc_id = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); - if (rsc_id == NULL) { - continue; - } - - rsc_id += sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; + rsc_id = primitive_xpath + sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; end_quote = strchr(rsc_id, '\''); CRM_LOG_ASSERT(end_quote != NULL); From 0cf23c06591a9c140927b13d4b0cd832eb1c2678 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 12:58:13 -0700 Subject: [PATCH 3/5] Refactor: fencer: Take patchset as arg in update_cib_stonith_devices() The sole caller has already done the checking, we don't need event, and we don't need msg except as a way to get the patchset. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index cb673dfa6d6..d933c996c61 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -235,23 +235,10 @@ cib_devices_update(void) #define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" static void -update_cib_stonith_devices(const char *event, xmlNode * msg) +update_cib_stonith_devices(const xmlNode *patchset) { - int format = 1; - const xmlNode *wrapper = pcmk__xe_first_child(msg, - PCMK__XE_CIB_UPDATE_RESULT, - NULL, NULL); - const xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); char *reason = NULL; - CRM_CHECK(patchset != NULL, return); - crm_element_value_int(patchset, PCMK_XA_FORMAT, &format); - - if (format != 2) { - crm_warn("Unknown patch format: %d", format); - return; - } - for (const xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, NULL); change != NULL; change = pcmk__xe_next(change, NULL)) { @@ -481,6 +468,7 @@ update_fencing_topology(const char *event, xmlNode *msg) static void update_cib_cache_cb(const char *event, xmlNode * msg) { + xmlNode *patchset = NULL; long long timeout_ms_saved = stonith_watchdog_timeout_ms; bool need_full_refresh = false; @@ -499,7 +487,6 @@ update_cib_cache_cb(const char *event, xmlNode * msg) if (local_cib != NULL) { int rc = pcmk_ok; xmlNode *wrapper = NULL; - xmlNode *patchset = NULL; crm_element_value_int(msg, PCMK__XA_CIB_RC, &rc); if (rc != pcmk_ok) { @@ -514,6 +501,11 @@ update_cib_cache_cb(const char *event, xmlNode * msg) switch (rc) { case pcmk_ok: case -pcmk_err_old_data: + /* @TODO Full refresh (with or without query) in case of + * -pcmk_err_old_data? It seems wrong to call + * stonith_device_remove() based on primitive deletion in an + * old diff. + */ break; case -pcmk_err_diff_resync: case -pcmk_err_diff_failed: @@ -548,7 +540,7 @@ update_cib_cache_cb(const char *event, xmlNode * msg) } else { // Partial refresh update_fencing_topology(event, msg); - update_cib_stonith_devices(event, msg); + update_cib_stonith_devices(patchset); } watchdog_device_update(); From 80161d5c05abbfe27cdc79f7c46a92692d22f5b0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 02:31:22 -0700 Subject: [PATCH 4/5] Low: fencer: Don't remove device if child is deleted If the child of a primitive element representing a fencing device was deleted, and we didn't catch it earlier as instance_attributes or meta_attributes, then previously we would remove the fencing device. We should remove the device only if the primitive itself was deleted. By inspection, it looks like this would unregister a fencing device if one of its operations (op elements) were deleted. If we find that only a child of the primitive was deleted, fall through to the next if block -- that is, "continue" only if the primitive itself was deleted and we call stonith_device_remove(). Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index d933c996c61..3e9f0be05da 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -258,7 +258,6 @@ update_cib_stonith_devices(const xmlNode *patchset) const char *rsc_id = NULL; const char *end_quote = NULL; - char *copy = NULL; if ((strstr(primitive_xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) || (strstr(primitive_xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { @@ -277,19 +276,22 @@ update_cib_stonith_devices(const xmlNode *patchset) continue; } - /* A primitive resource was removed. If it was a fencing resource, - * it's faster to remove it directly than to run the scheduler and - * update all device registrations. - */ - copy = strndup(rsc_id, end_quote - rsc_id); - pcmk__assert(copy != NULL); - stonith_device_remove(copy, true); + if (strchr(end_quote, '/') == NULL) { + /* The primitive element itself was deleted. If this was a + * fencing resource, it's faster to remove it directly than to + * run the scheduler and update all device registrations. + */ + char *copy = strndup(rsc_id, end_quote - rsc_id); - /* watchdog_device_update called afterwards - to fall back to implicit definition if needed */ + pcmk__assert(copy != NULL); + stonith_device_remove(copy, true); - free(copy); - continue; + /* watchdog_device_update called afterwards + to fall back to implicit definition if needed */ + + free(copy); + continue; + } } if (strstr(xpath, "/" PCMK_XE_RESOURCES) From 757cce1cbaaae60c39a41b728ab4f97f7dc6fa95 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 13:34:55 -0700 Subject: [PATCH 5/5] Fix: fencer: Refresh CIB devices on change to nodes section This fixes a regression introduced by bf7ffcd. As of that commit, the fake local node is created after all resources have been unpacked. So it doesn't get added to resources' allowed_nodes tables. This prevents registration of fencing devices when the fencer receives a CIB diff that removes the local node. For example, the user may have replaced the CIB with a boilerplate configuration that has an empty nodes section. Registering a fencing device requires that the local node be in the resource's allowed nodes table. One option would be to add the fake local node to all resources' allowed nodes tables immediately after creating it. However, it shouldn't necessarily be an allowed node for all resources. For example, if symmetric-cluster=false, then a node should not be placed in a resource's allowed nodes table by default. It's difficult to ensure correctness of the allowed nodes tables when a fake local node is involved. It may involve repeated code or a fragile and confusing dependence on the order of unpacking. Since the fake local node is a hack in the first place, it seems better to avoid using it where possible. Currently the only code that even sets the local_node_name member of scheduler->priv is in: * the fencer * crm_mon when showing the "times" section This commit works as follows. If the fencer receives a CIB diff notification that contains the nodes section, it triggers a full refresh of fencing device registrations. In our example above, where a user has replaced the CIB with a configuration that has an empty nodes section, this means all fencing device registrations will be removed. However, the controller also has a CIB diff notification callback: do_cib_updated(). The controller's callback repopulates the nodes section with up-to-date information from the cluster layer (or its node cache) if it finds that an untrusted client like cibadmin has sent a modified the nodes section. Then it updates the CIB accordingly. The fencer will receive this updated CIB and refresh fencing device registrations again, re-registering the fencing devices that were just removed. Note that in the common case, we're not doing all this wasted work. The "remove and then re-register" sequence should happen only when a user has modified the CIB in a sloppy way (for example, by deleting nodes from the CIB's nodes section that have not been removed from the cluster). In short, it seems better to rely on the controller's maintenance of the CIB's node list, than to rely on a "fake local node" hack in the scheduler. See the following pull requests from Hideo Yamauchi and their discussions: https://github.com/ClusterLabs/pacemaker/pull/3849 https://github.com/ClusterLabs/pacemaker/pull/3852 Thanks to Hideo for the report and finding the cause. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 18 +++++++++++++++- daemons/fenced/fenced_scheduler.c | 36 ++++--------------------------- daemons/fenced/pacemaker-fenced.c | 21 ++++++++++++++++-- daemons/fenced/pacemaker-fenced.h | 6 +++--- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 3e9f0be05da..f719babe114 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -294,7 +294,23 @@ update_cib_stonith_devices(const xmlNode *patchset) } } - if (strstr(xpath, "/" PCMK_XE_RESOURCES) + /* @FIXME Suppose a node gets deleted in one CIB diff and then re-added + * in another. For example, this can happen if a client deletes a node + * from the CIB when it's still part of the cluster, and then the + * controller's diff callback repopulates the nodes section. If + * responding to the deletion here causes a stonith device to get + * unregistered, then the next monitor will fail. This is because when + * the device gets re-registered after nodes are repopulated, the + * api_registered flag doesn't get set. Only the cib_registered flag + * gets set. + * + * See commit message in eaee0fc for some more info. + * + * The fencer should not refuse to run a monitor action for a resource + * that the controller knows is started. + */ + if (strstr(xpath, "/" PCMK_XE_NODES) + || strstr(xpath, "/" PCMK_XE_RESOURCES) || strstr(xpath, "/" PCMK_XE_CONSTRAINTS) || strstr(xpath, "/" PCMK_XE_RSC_DEFAULTS)) { diff --git a/daemons/fenced/fenced_scheduler.c b/daemons/fenced/fenced_scheduler.c index 46d74320fcc..34e39b89884 100644 --- a/daemons/fenced/fenced_scheduler.c +++ b/daemons/fenced/fenced_scheduler.c @@ -52,35 +52,6 @@ fenced_scheduler_init(void) return pcmk_rc_ok; } -/*! - * \internal - * \brief Set the local node name for scheduling purposes - * - * \param[in] node_name Name to set as local node name - */ -void -fenced_set_local_node(const char *node_name) -{ - pcmk__assert(scheduler != NULL); - - scheduler->priv->local_node_name = pcmk__str_copy(node_name); -} - -/*! - * \internal - * \brief Get the local node name - * - * \return Local node name - */ -const char * -fenced_get_local_node(void) -{ - if (scheduler == NULL) { - return NULL; - } - return scheduler->priv->local_node_name; -} - /*! * \internal * \brief Free all scheduler-related resources @@ -112,14 +83,15 @@ fenced_scheduler_cleanup(void) static pcmk_node_t * local_node_allowed_for(const pcmk_resource_t *rsc) { - if ((rsc != NULL) && (scheduler->priv->local_node_name != NULL)) { + const char *local_node = fenced_get_local_node(); + + if ((rsc != NULL) && (local_node != NULL)) { GHashTableIter iter; pcmk_node_t *node = NULL; g_hash_table_iter_init(&iter, rsc->priv->allowed_nodes); while (g_hash_table_iter_next(&iter, NULL, (void **) &node)) { - if (pcmk__str_eq(node->priv->name, scheduler->priv->local_node_name, - pcmk__str_casei)) { + if (pcmk__str_eq(node->priv->name, local_node, pcmk__str_casei)) { return node; } } diff --git a/daemons/fenced/pacemaker-fenced.c b/daemons/fenced/pacemaker-fenced.c index 32a3f2bf562..e323d59c8ad 100644 --- a/daemons/fenced/pacemaker-fenced.c +++ b/daemons/fenced/pacemaker-fenced.c @@ -1,5 +1,5 @@ /* - * Copyright 2009-2024 the Pacemaker project contributors + * Copyright 2009-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -40,6 +40,9 @@ #define SUMMARY "daemon for executing fencing devices in a Pacemaker cluster" +//! Local node's name in cluster +static char *local_node_name = NULL; + // @TODO This should be guint long long stonith_watchdog_timeout_ms = 0; @@ -68,6 +71,18 @@ crm_exit_t exit_code = CRM_EX_OK; static void stonith_cleanup(void); +/*! + * \internal + * \brief Get the local node name + * + * \return Local node name + */ +const char * +fenced_get_local_node(void) +{ + return local_node_name; +} + static int32_t st_ipc_accept(qb_ipcs_connection_t * c, uid_t uid, gid_t gid) { @@ -628,7 +643,7 @@ main(int argc, char **argv) crm_crit("Cannot sign in to the cluster... terminating"); goto done; } - fenced_set_local_node(cluster->priv->node_name); + local_node_name = pcmk__str_copy(cluster->priv->node_name); if (!options.stand_alone) { setup_cib(); @@ -654,6 +669,8 @@ main(int argc, char **argv) pcmk_cluster_free(cluster); fenced_scheduler_cleanup(); + free(local_node_name); + pcmk__output_and_clear_error(&error, out); if (out != NULL) { diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 04aadedc39e..a22e3ac7e56 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2024 the Pacemaker project contributors + * Copyright 2009-2025 the Pacemaker project contributors * * This source code is licensed under the GNU General Public License version 2 * or later (GPLv2+) WITHOUT ANY WARRANTY. @@ -13,6 +13,8 @@ #include #include +const char *fenced_get_local_node(void); + /*! * \internal * \brief Check whether target has already been fenced recently @@ -296,8 +298,6 @@ void setup_cib(void); void fenced_cib_cleanup(void); int fenced_scheduler_init(void); -void fenced_set_local_node(const char *node_name); -const char *fenced_get_local_node(void); void fenced_scheduler_cleanup(void); void fenced_scheduler_run(xmlNode *cib);