From 6c529bb624ad548f66ce6ef1fa80b77c688918f4 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 22 Nov 2019 16:39:54 -0600 Subject: [PATCH 1/4] Refactor: controller: rename struct recurring_op_s to active_op_t ... because it holds both recurring and pending non-recurring actions, and the name was confusing --- daemons/controld/controld_execd.c | 18 +++++++++--------- daemons/controld/controld_execd_state.c | 4 ++-- daemons/controld/controld_lrm.h | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c index 9e8dd36..48f35dd 100644 --- a/daemons/controld/controld_execd.c +++ b/daemons/controld/controld_execd.c @@ -403,7 +403,7 @@ lrm_state_verify_stopped(lrm_state_t * lrm_state, enum crmd_fsa_state cur_state, GHashTableIter gIter; const char *key = NULL; rsc_history_t *entry = NULL; - struct recurring_op_s *pending = NULL; + active_op_t *pending = NULL; crm_debug("Checking for active resources before exit"); @@ -909,7 +909,7 @@ static gboolean lrm_remove_deleted_op(gpointer key, gpointer value, gpointer user_data) { const char *rsc = user_data; - struct recurring_op_s *pending = value; + active_op_t *pending = value; if (crm_str_eq(rsc, pending->rsc_id, TRUE)) { crm_info("Removing op %s:%d for deleted resource %s", @@ -1137,7 +1137,7 @@ cancel_op(lrm_state_t * lrm_state, const char *rsc_id, const char *key, int op, { int rc = pcmk_ok; char *local_key = NULL; - struct recurring_op_s *pending = NULL; + active_op_t *pending = NULL; CRM_CHECK(op != 0, return FALSE); CRM_CHECK(rsc_id != NULL, return FALSE); @@ -1203,7 +1203,7 @@ cancel_action_by_key(gpointer key, gpointer value, gpointer user_data) { gboolean remove = FALSE; struct cancel_data *data = user_data; - struct recurring_op_s *op = (struct recurring_op_s *)value; + active_op_t *op = value; if (crm_str_eq(op->op_key, data->key, TRUE)) { data->done = TRUE; @@ -2107,7 +2107,7 @@ stop_recurring_action_by_rsc(gpointer key, gpointer value, gpointer user_data) { gboolean remove = FALSE; struct stop_recurring_action_s *event = user_data; - struct recurring_op_s *op = (struct recurring_op_s *)value; + active_op_t *op = value; if ((op->interval_ms != 0) && crm_str_eq(op->rsc_id, event->rsc->id, TRUE)) { @@ -2124,7 +2124,7 @@ stop_recurring_actions(gpointer key, gpointer value, gpointer user_data) { gboolean remove = FALSE; lrm_state_t *lrm_state = user_data; - struct recurring_op_s *op = (struct recurring_op_s *)value; + active_op_t *op = value; if (op->interval_ms != 0) { crm_info("Cancelling op %d for %s (%s)", op->call_id, op->rsc_id, @@ -2297,9 +2297,9 @@ do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operat * for them to complete during shutdown */ char *call_id_s = make_stop_id(rsc->id, call_id); - struct recurring_op_s *pending = NULL; + active_op_t *pending = NULL; - pending = calloc(1, sizeof(struct recurring_op_s)); + pending = calloc(1, sizeof(active_op_t)); crm_trace("Recording pending op: %d - %s %s", call_id, op_id, call_id_s); pending->call_id = call_id; @@ -2517,7 +2517,7 @@ did_lrm_rsc_op_fail(lrm_state_t *lrm_state, const char * rsc_id, void process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op, - struct recurring_op_s *pending, xmlNode *action_xml) + active_op_t *pending, xmlNode *action_xml) { char *op_id = NULL; char *op_key = NULL; diff --git a/daemons/controld/controld_execd_state.c b/daemons/controld/controld_execd_state.c index 0e21d18..473da97 100644 --- a/daemons/controld/controld_execd_state.c +++ b/daemons/controld/controld_execd_state.c @@ -44,7 +44,7 @@ free_deletion_op(gpointer value) static void free_recurring_op(gpointer value) { - struct recurring_op_s *op = (struct recurring_op_s *)value; + active_op_t *op = value; free(op->user_data); free(op->rsc_id); @@ -61,7 +61,7 @@ fail_pending_op(gpointer key, gpointer value, gpointer user_data) { lrmd_event_data_t event = { 0, }; lrm_state_t *lrm_state = user_data; - struct recurring_op_s *op = (struct recurring_op_s *)value; + active_op_t *op = value; crm_trace("Pre-emptively failing " CRM_OP_FMT " on %s (call=%s, %s)", op->rsc_id, op->op_type, op->interval_ms, diff --git a/daemons/controld/controld_lrm.h b/daemons/controld/controld_lrm.h index 598682b..27df5d7 100644 --- a/daemons/controld/controld_lrm.h +++ b/daemons/controld/controld_lrm.h @@ -33,8 +33,8 @@ typedef struct resource_history_s { void history_free(gpointer data); -/* TODO - Replace this with lrmd_event_data_t */ -struct recurring_op_s { +// In-flight action (recurring or pending) +typedef struct active_op_s { guint interval_ms; int call_id; gboolean remove; @@ -45,7 +45,7 @@ struct recurring_op_s { char *op_key; char *user_data; GHashTable *params; -}; +} active_op_t; typedef struct lrm_state_s { const char *node_name; @@ -164,4 +164,4 @@ void remote_ra_process_maintenance_nodes(xmlNode *xml); gboolean remote_ra_controlling_guest(lrm_state_t * lrm_state); void process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op, - struct recurring_op_s *pending, xmlNode *action_xml); + active_op_t *pending, xmlNode *action_xml); -- 1.8.3.1 From 93a59f1df8fe11d365032d75f10cb4189ad2f1f8 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 22 Nov 2019 16:45:31 -0600 Subject: [PATCH 2/4] Refactor: controller: convert active_op_t booleans to bitmask --- daemons/controld/controld_execd.c | 11 +++++------ daemons/controld/controld_lrm.h | 8 ++++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c index 48f35dd..2c9d9c0 100644 --- a/daemons/controld/controld_execd.c +++ b/daemons/controld/controld_execd.c @@ -1148,18 +1148,17 @@ cancel_op(lrm_state_t * lrm_state, const char *rsc_id, const char *key, int op, pending = g_hash_table_lookup(lrm_state->pending_ops, key); if (pending) { - if (remove && pending->remove == FALSE) { - pending->remove = TRUE; + if (remove && is_not_set(pending->flags, active_op_remove)) { + set_bit(pending->flags, active_op_remove); crm_debug("Scheduling %s for removal", key); } - if (pending->cancelled) { + if (is_set(pending->flags, active_op_cancelled)) { crm_debug("Operation %s already cancelled", key); free(local_key); return FALSE; } - - pending->cancelled = TRUE; + set_bit(pending->flags, active_op_cancelled); } else { crm_info("No pending op found for %s", key); @@ -2652,7 +2651,7 @@ process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op, crm_err("Recurring operation %s was cancelled without transition information", op_key); - } else if (pending->remove) { + } else if (is_set(pending->flags, active_op_remove)) { /* This recurring operation was cancelled (by us) and pending, and we * have been waiting for it to finish. */ diff --git a/daemons/controld/controld_lrm.h b/daemons/controld/controld_lrm.h index 27df5d7..3ab7048 100644 --- a/daemons/controld/controld_lrm.h +++ b/daemons/controld/controld_lrm.h @@ -33,12 +33,16 @@ typedef struct resource_history_s { void history_free(gpointer data); +enum active_op_e { + active_op_remove = (1 << 0), + active_op_cancelled = (1 << 1), +}; + // In-flight action (recurring or pending) typedef struct active_op_s { guint interval_ms; int call_id; - gboolean remove; - gboolean cancelled; + uint32_t flags; // bitmask of active_op_e time_t start_time; char *rsc_id; char *op_type; -- 1.8.3.1 From 4d087d021d325e26b41a9b36b5b190dc7b25334c Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 22 Nov 2019 16:58:25 -0600 Subject: [PATCH 3/4] Refactor: controller: remove unused argument --- daemons/controld/controld_execd.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c index 2c9d9c0..46c1958 100644 --- a/daemons/controld/controld_execd.c +++ b/daemons/controld/controld_execd.c @@ -43,8 +43,8 @@ static int delete_rsc_status(lrm_state_t * lrm_state, const char *rsc_id, int ca static lrmd_event_data_t *construct_op(lrm_state_t * lrm_state, xmlNode * rsc_op, const char *rsc_id, const char *operation); -static void do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operation, - xmlNode * msg, xmlNode * request); +static void do_lrm_rsc_op(lrm_state_t *lrm_state, lrmd_rsc_info_t *rsc, + const char *operation, xmlNode *msg); void send_direct_ack(const char *to_host, const char *to_sys, lrmd_rsc_info_t * rsc, lrmd_event_data_t * op, const char *rsc_id); @@ -1858,7 +1858,7 @@ do_lrm_invoke(long long action, crm_rsc_delete, user_name); } else { - do_lrm_rsc_op(lrm_state, rsc, operation, input->xml, input->msg); + do_lrm_rsc_op(lrm_state, rsc, operation, input->xml); } lrmd_free_rsc_info(rsc); @@ -2170,8 +2170,8 @@ record_pending_op(const char *node_name, lrmd_rsc_info_t *rsc, lrmd_event_data_t } static void -do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operation, xmlNode * msg, - xmlNode * request) +do_lrm_rsc_op(lrm_state_t *lrm_state, lrmd_rsc_info_t *rsc, + const char *operation, xmlNode *msg) { int call_id = 0; char *op_id = NULL; -- 1.8.3.1 From 356b417274918b7da6cdd9c72c036c923160b318 Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Fri, 6 Dec 2019 12:15:05 -0600 Subject: [PATCH 4/4] Refactor: scheduler: combine two "if" statements ... for readability, and ease of adding another block later --- lib/pacemaker/pcmk_sched_graph.c | 120 +++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 60 deletions(-) diff --git a/lib/pacemaker/pcmk_sched_graph.c b/lib/pacemaker/pcmk_sched_graph.c index e5a8a01..a6967fe 100644 --- a/lib/pacemaker/pcmk_sched_graph.c +++ b/lib/pacemaker/pcmk_sched_graph.c @@ -1088,71 +1088,71 @@ action2xml(action_t * action, gboolean as_input, pe_working_set_t *data_set) return action_xml; } - /* List affected resource */ - if (action->rsc) { - if (is_set(action->flags, pe_action_pseudo) == FALSE) { - int lpc = 0; - - xmlNode *rsc_xml = create_xml_node(action_xml, crm_element_name(action->rsc->xml)); - - const char *attr_list[] = { - XML_AGENT_ATTR_CLASS, - XML_AGENT_ATTR_PROVIDER, - XML_ATTR_TYPE - }; - - if (is_set(action->rsc->flags, pe_rsc_orphan) && action->rsc->clone_name) { - /* Do not use the 'instance free' name here as that - * might interfere with the instance we plan to keep. - * Ie. if there are more than two named /anonymous/ - * instances on a given node, we need to make sure the - * command goes to the right one. - * - * Keep this block, even when everyone is using - * 'instance free' anonymous clone names - it means - * we'll do the right thing if anyone toggles the - * unique flag to 'off' - */ - crm_debug("Using orphan clone name %s instead of %s", action->rsc->id, - action->rsc->clone_name); - crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->clone_name); - crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id); + if (action->rsc && is_not_set(action->flags, pe_action_pseudo)) { + int lpc = 0; + xmlNode *rsc_xml = NULL; + const char *attr_list[] = { + XML_AGENT_ATTR_CLASS, + XML_AGENT_ATTR_PROVIDER, + XML_ATTR_TYPE + }; + + // List affected resource + + rsc_xml = create_xml_node(action_xml, + crm_element_name(action->rsc->xml)); + if (is_set(action->rsc->flags, pe_rsc_orphan) + && action->rsc->clone_name) { + /* Do not use the 'instance free' name here as that + * might interfere with the instance we plan to keep. + * Ie. if there are more than two named /anonymous/ + * instances on a given node, we need to make sure the + * command goes to the right one. + * + * Keep this block, even when everyone is using + * 'instance free' anonymous clone names - it means + * we'll do the right thing if anyone toggles the + * unique flag to 'off' + */ + crm_debug("Using orphan clone name %s instead of %s", action->rsc->id, + action->rsc->clone_name); + crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->clone_name); + crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id); - } else if (is_not_set(action->rsc->flags, pe_rsc_unique)) { - const char *xml_id = ID(action->rsc->xml); - - crm_debug("Using anonymous clone name %s for %s (aka. %s)", xml_id, action->rsc->id, - action->rsc->clone_name); - - /* ID is what we'd like client to use - * ID_LONG is what they might know it as instead - * - * ID_LONG is only strictly needed /here/ during the - * transition period until all nodes in the cluster - * are running the new software /and/ have rebooted - * once (meaning that they've only ever spoken to a DC - * supporting this feature). - * - * If anyone toggles the unique flag to 'on', the - * 'instance free' name will correspond to an orphan - * and fall into the clause above instead - */ - crm_xml_add(rsc_xml, XML_ATTR_ID, xml_id); - if (action->rsc->clone_name && safe_str_neq(xml_id, action->rsc->clone_name)) { - crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->clone_name); - } else { - crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id); - } + } else if (is_not_set(action->rsc->flags, pe_rsc_unique)) { + const char *xml_id = ID(action->rsc->xml); + + crm_debug("Using anonymous clone name %s for %s (aka. %s)", xml_id, action->rsc->id, + action->rsc->clone_name); + /* ID is what we'd like client to use + * ID_LONG is what they might know it as instead + * + * ID_LONG is only strictly needed /here/ during the + * transition period until all nodes in the cluster + * are running the new software /and/ have rebooted + * once (meaning that they've only ever spoken to a DC + * supporting this feature). + * + * If anyone toggles the unique flag to 'on', the + * 'instance free' name will correspond to an orphan + * and fall into the clause above instead + */ + crm_xml_add(rsc_xml, XML_ATTR_ID, xml_id); + if (action->rsc->clone_name && safe_str_neq(xml_id, action->rsc->clone_name)) { + crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->clone_name); } else { - CRM_ASSERT(action->rsc->clone_name == NULL); - crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->id); + crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id); } - for (lpc = 0; lpc < DIMOF(attr_list); lpc++) { - crm_xml_add(rsc_xml, attr_list[lpc], - g_hash_table_lookup(action->rsc->meta, attr_list[lpc])); - } + } else { + CRM_ASSERT(action->rsc->clone_name == NULL); + crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->id); + } + + for (lpc = 0; lpc < DIMOF(attr_list); lpc++) { + crm_xml_add(rsc_xml, attr_list[lpc], + g_hash_table_lookup(action->rsc->meta, attr_list[lpc])); } } -- 1.8.3.1