From 28566d6832274c59f27bb7b2f1f54420a3f3d822 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Thu, 9 May 2019 20:26:08 -0500 Subject: [PATCH 1/2] Refactor: libpe_status: functionize unfencing digest code more ... for readability, reusability, and avoiding unnecessary function calls or memory allocation. --- lib/pengine/utils.c | 159 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 118 insertions(+), 41 deletions(-) diff --git a/lib/pengine/utils.c b/lib/pengine/utils.c index 2f4dc1e..f80f8d4 100644 --- a/lib/pengine/utils.c +++ b/lib/pengine/utils.c @@ -2080,57 +2080,134 @@ rsc_action_digest_cmp(resource_t * rsc, xmlNode * xml_op, node_t * node, return data; } +/*! + * \internal + * \brief Create an unfencing summary for use in special node attribute + * + * Create a string combining a fence device's resource ID, agent type, and + * parameter digest (whether for all parameters or just non-private parameters). + * This can be stored in a special node attribute, allowing us to detect changes + * in either the agent type or parameters, to know whether unfencing must be + * redone or can be safely skipped when the device's history is cleaned. + * + * \param[in] rsc_id Fence device resource ID + * \param[in] agent_type Fence device agent + * \param[in] param_digest Fence device parameter digest + * + * \return Newly allocated string with unfencing digest + * \note The caller is responsible for freeing the result. + */ +static inline char * +create_unfencing_summary(const char *rsc_id, const char *agent_type, + const char *param_digest) +{ + return crm_strdup_printf("%s:%s:%s", rsc_id, agent_type, param_digest); +} + +/*! + * \internal + * \brief Check whether a node can skip unfencing + * + * Check whether a fence device's current definition matches a node's + * stored summary of when it was last unfenced by the device. + * + * \param[in] rsc_id Fence device's resource ID + * \param[in] agent Fence device's agent type + * \param[in] digest_calc Fence device's current parameter digest + * \param[in] node_summary Value of node's special unfencing node attribute + * (a comma-separated list of unfencing summaries for + * all devices that have unfenced this node) + * + * \return TRUE if digest matches, FALSE otherwise + */ +static bool +unfencing_digest_matches(const char *rsc_id, const char *agent, + const char *digest_calc, const char *node_summary) +{ + bool matches = FALSE; + + if (rsc_id && agent && digest_calc && node_summary) { + char *search_secure = create_unfencing_summary(rsc_id, agent, + digest_calc); + + /* The digest was calculated including the device ID and agent, + * so there is no risk of collision using strstr(). + */ + matches = (strstr(node_summary, search_secure) != NULL); + crm_trace("Calculated unfencing digest '%s' %sfound in '%s'", + search_secure, matches? "" : "not ", node_summary); + free(search_secure); + } + return matches; +} + +/* Magic string to use as action name for digest cache entries used for + * unfencing checks. This is not a real action name (i.e. "on"), so + * check_action_definition() won't confuse these entries with real actions. + */ #define STONITH_DIGEST_TASK "stonith-on" +/*! + * \internal + * \brief Calculate fence device digests and digest comparison result + * + * \param[in] rsc Fence device resource + * \param[in] agent Fence device's agent type + * \param[in] node Node with digest cache to use + * \param[in] data_set Cluster working set + * + * \return Node's digest cache entry + */ static op_digest_cache_t * -fencing_action_digest_cmp(resource_t * rsc, node_t * node, pe_working_set_t * data_set) +fencing_action_digest_cmp(pe_resource_t *rsc, const char *agent, + pe_node_t *node, pe_working_set_t *data_set) { - char *key = generate_op_key(rsc->id, STONITH_DIGEST_TASK, 0); - op_digest_cache_t *data = rsc_action_digest(rsc, STONITH_DIGEST_TASK, key, node, NULL, data_set); + const char *node_summary = NULL; - const char *digest_all = pe_node_attribute_raw(node, CRM_ATTR_DIGESTS_ALL); - const char *digest_secure = pe_node_attribute_raw(node, CRM_ATTR_DIGESTS_SECURE); + // Calculate device's current parameter digests + char *key = generate_op_key(rsc->id, STONITH_DIGEST_TASK, 0); + op_digest_cache_t *data = rsc_action_digest(rsc, STONITH_DIGEST_TASK, key, + node, NULL, data_set); - /* No 'reloads' for fencing device changes - * - * We use the resource id + agent + digest so that we can detect - * changes to the agent and/or the parameters used - */ - char *search_all = crm_strdup_printf("%s:%s:%s", rsc->id, (const char*)g_hash_table_lookup(rsc->meta, XML_ATTR_TYPE), data->digest_all_calc); - char *search_secure = crm_strdup_printf("%s:%s:%s", rsc->id, (const char*)g_hash_table_lookup(rsc->meta, XML_ATTR_TYPE), data->digest_secure_calc); + free(key); - data->rc = RSC_DIGEST_ALL; - if (digest_all == NULL) { - /* it is unknown what the previous op digest was */ + // Check whether node has special unfencing summary node attribute + node_summary = pe_node_attribute_raw(node, CRM_ATTR_DIGESTS_ALL); + if (node_summary == NULL) { data->rc = RSC_DIGEST_UNKNOWN; + return data; + } - } else if (strstr(digest_all, search_all)) { + // Check whether full parameter digest matches + if (unfencing_digest_matches(rsc->id, agent, data->digest_all_calc, + node_summary)) { data->rc = RSC_DIGEST_MATCH; + return data; + } - } else if(digest_secure && data->digest_secure_calc) { - if(strstr(digest_secure, search_secure)) { - if (is_set(data_set->flags, pe_flag_stdout)) { - printf("Only 'private' parameters to %s for unfencing %s changed\n", - rsc->id, node->details->uname); - } - data->rc = RSC_DIGEST_MATCH; + // Check whether secure parameter digest matches + node_summary = pe_node_attribute_raw(node, CRM_ATTR_DIGESTS_SECURE); + if (unfencing_digest_matches(rsc->id, agent, data->digest_secure_calc, + node_summary)) { + data->rc = RSC_DIGEST_MATCH; + if (is_set(data_set->flags, pe_flag_stdout)) { + printf("Only 'private' parameters to %s for unfencing %s changed\n", + rsc->id, node->details->uname); } + return data; } - if (is_set(data_set->flags, pe_flag_sanitized) - && is_set(data_set->flags, pe_flag_stdout) - && (data->rc == RSC_DIGEST_ALL) + // Parameters don't match + data->rc = RSC_DIGEST_ALL; + if (is_set(data_set->flags, (pe_flag_sanitized|pe_flag_stdout)) && data->digest_secure_calc) { - printf("Parameters to %s for unfencing %s changed, try '%s:%s:%s'\n", - rsc->id, node->details->uname, rsc->id, - (const char *) g_hash_table_lookup(rsc->meta, XML_ATTR_TYPE), - data->digest_secure_calc); - } - - free(key); - free(search_all); - free(search_secure); + char *digest = create_unfencing_summary(rsc->id, agent, + data->digest_secure_calc); + printf("Parameters to %s for unfencing %s changed, try '%s'\n", + rsc->id, node->details->uname, digest); + free(digest); + } return data; } @@ -2218,9 +2295,6 @@ pe_fence_op(node_t * node, const char *op, bool optional, const char *reason, pe * * We may do this for all nodes in the future, but for now * the check_action_definition() based stuff works fine. - * - * Use "stonith-on" to avoid creating cache entries for - * operations check_action_definition() would look for. */ long max = 1024; long digests_all_offset = 0; @@ -2232,8 +2306,11 @@ pe_fence_op(node_t * node, const char *op, bool optional, const char *reason, pe for (GListPtr gIter = matches; gIter != NULL; gIter = gIter->next) { resource_t *match = gIter->data; - op_digest_cache_t *data = fencing_action_digest_cmp(match, node, data_set); + const char *agent = g_hash_table_lookup(match->meta, + XML_ATTR_TYPE); + op_digest_cache_t *data = NULL; + data = fencing_action_digest_cmp(match, agent, node, data_set); if(data->rc == RSC_DIGEST_ALL) { optional = FALSE; crm_notice("Unfencing %s (remote): because the definition of %s changed", node->details->uname, match->id); @@ -2244,11 +2321,11 @@ pe_fence_op(node_t * node, const char *op, bool optional, const char *reason, pe digests_all_offset += snprintf( digests_all+digests_all_offset, max-digests_all_offset, - "%s:%s:%s,", match->id, (const char*)g_hash_table_lookup(match->meta, XML_ATTR_TYPE), data->digest_all_calc); + "%s:%s:%s,", match->id, agent, data->digest_all_calc); digests_secure_offset += snprintf( digests_secure+digests_secure_offset, max-digests_secure_offset, - "%s:%s:%s,", match->id, (const char*)g_hash_table_lookup(match->meta, XML_ATTR_TYPE), data->digest_secure_calc); + "%s:%s:%s,", match->id, agent, data->digest_secure_calc); } g_hash_table_insert(stonith_op->meta, strdup(XML_OP_ATTR_DIGESTS_ALL), -- 1.8.3.1 From fd6e06ff419c95f4423202163d2d4dca3f03a4c5 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 10 May 2019 11:57:31 -0500 Subject: [PATCH 2/2] Fix: libpe_status: calculate secure digests for unfencing ops The calculation of digests for detection of when unfencing is needed reused rsc_action_digest(). However that would only add secure digests when the pe_flag_sanitized flag was set, which is only set by crm_simulate, so secure digests would never be added in normal cluster operation. This led to node attributes like name="#digests-secure" value="stonith-fence_compute-fence-nova:fence_compute:(null),". Now, rsc_action_digest() takes a new argument to select whether secure digests are added, which is always set to TRUE when calculating unfencing digests. --- lib/pengine/utils.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/pengine/utils.c b/lib/pengine/utils.c index f80f8d4..5b893f7 100644 --- a/lib/pengine/utils.c +++ b/lib/pengine/utils.c @@ -1936,9 +1936,24 @@ append_versioned_params(xmlNode *versioned_params, const char *ra_version, xmlNo } #endif +/*! + * \internal + * \brief Calculate action digests and store in node's digest cache + * + * \param[in] rsc Resource that action was for + * \param[in] task Name of action performed + * \param[in] key Action's task key + * \param[in] node Node action was performed on + * \param[in] xml_op XML of operation in CIB status (if available) + * \param[in] calc_secure Whether to calculate secure digest + * \param[in] data_set Cluster working set + * + * \return Pointer to node's digest cache entry + */ static op_digest_cache_t * -rsc_action_digest(resource_t * rsc, const char *task, const char *key, - node_t * node, xmlNode * xml_op, pe_working_set_t * data_set) +rsc_action_digest(pe_resource_t *rsc, const char *task, const char *key, + pe_node_t *node, xmlNode *xml_op, bool calc_secure, + pe_working_set_t *data_set) { op_digest_cache_t *data = NULL; @@ -2007,7 +2022,7 @@ rsc_action_digest(resource_t * rsc, const char *task, const char *key, data->digest_all_calc = calculate_operation_digest(data->params_all, op_version); - if (is_set(data_set->flags, pe_flag_sanitized)) { + if (calc_secure) { data->params_secure = copy_xml(data->params_all); if(secure_list) { filter_parameters(data->params_secure, secure_list, FALSE); @@ -2053,7 +2068,9 @@ rsc_action_digest_cmp(resource_t * rsc, xmlNode * xml_op, node_t * node, interval_ms = crm_parse_ms(interval_ms_s); key = generate_op_key(rsc->id, task, interval_ms); - data = rsc_action_digest(rsc, task, key, node, xml_op, data_set); + data = rsc_action_digest(rsc, task, key, node, xml_op, + is_set(data_set->flags, pe_flag_sanitized), + data_set); data->rc = RSC_DIGEST_MATCH; if (digest_restart && data->digest_restart_calc && strcmp(data->digest_restart_calc, digest_restart) != 0) { @@ -2167,7 +2184,7 @@ fencing_action_digest_cmp(pe_resource_t *rsc, const char *agent, // Calculate device's current parameter digests char *key = generate_op_key(rsc->id, STONITH_DIGEST_TASK, 0); op_digest_cache_t *data = rsc_action_digest(rsc, STONITH_DIGEST_TASK, key, - node, NULL, data_set); + node, NULL, TRUE, data_set); free(key); -- 1.8.3.1