Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: libpe_status: Use pcmk_monitor_timeout for recurring monitors #3246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: libpe_status: Use pcmk_monitor_timeout for recurring monitors #3246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions daemons/controld/controld_execd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1608,9 +1608,6 @@ construct_op(const lrm_state_t *lrm_state, const xmlNode *rsc_op,
const char *op_timeout = NULL;
GHashTable *params = NULL;

xmlNode *primitive = NULL;
const char *class = NULL;

const char *transition = NULL;

CRM_ASSERT(rsc_id && operation);
Expand Down Expand Up @@ -1652,21 +1649,6 @@ construct_op(const lrm_state_t *lrm_state, const xmlNode *rsc_op,
op->interval_ms = 0;
}

/* Use pcmk_monitor_timeout instead of meta timeout for stonith
recurring monitor, if set */
primitive = find_xml_node(rsc_op, XML_CIB_TAG_RESOURCE, FALSE);
class = crm_element_value(primitive, XML_AGENT_ATTR_CLASS);

if (pcmk_is_set(pcmk_get_ra_caps(class), pcmk_ra_cap_fence_params)
&& pcmk__str_eq(operation, PCMK_ACTION_MONITOR, pcmk__str_casei)
&& (op->interval_ms > 0)) {

op_timeout = g_hash_table_lookup(params, "pcmk_monitor_timeout");
if (op_timeout != NULL) {
op->timeout = crm_get_msec(op_timeout);
}
}

if (!pcmk__str_eq(operation, PCMK_ACTION_STOP, pcmk__str_casei)) {
op->params = params;

Expand Down
9 changes: 4 additions & 5 deletions lib/pengine/pe_actions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,9 +773,8 @@ pcmk__unpack_action_meta(pcmk_resource_t *rsc, const pcmk_node_t *node,
}

/* Timeout order of precedence (highest to lowest):
* 1. pcmk_monitor_timeout resource parameter (only for starts and probes
* when rsc has pcmk_ra_cap_fence_params; this gets used for recurring
* monitors via the executor instead)
* 1. pcmk_monitor_timeout resource parameter (only for starts and
* monitors when rsc has pcmk_ra_cap_fence_params)
* 2. timeout configured in <op> (with <op timeout> taking precedence over
* <op> <meta_attributes>)
* 3. timeout configured in <op_defaults> <meta_attributes>
Expand All @@ -785,8 +784,8 @@ pcmk__unpack_action_meta(pcmk_resource_t *rsc, const pcmk_node_t *node,
// Check for pcmk_monitor_timeout
if (pcmk_is_set(pcmk_get_ra_caps(rsc_rule_data.standard),
pcmk_ra_cap_fence_params)
&& (pcmk__str_eq(action_name, PCMK_ACTION_START, pcmk__str_none)
|| pcmk_is_probe(action_name, interval_ms))) {
&& pcmk__str_any_of(action_name, PCMK_ACTION_START, PCMK_ACTION_MONITOR,
NULL)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See e44a6d4 commit message -- will the controller think the action configuration changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. The commit message is long and I missed that while refreshing for this "easy fix"...

Sigh, almost certainly. This is probably going to be a CANTFIX, unless we decide the digest change is acceptable in order to avoid a failed stonith stop action.

If it would make any difference in our decision, I'll double-check whether this can happen with any timed-out recurring monitor (with long pcmk_monitor_timeout) or if it's specific to the first one somehow. If it can happen with any (which it looks like should be possible), I'm surprised no one has hit and reported this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That, or try to find a different way to update the controller's expected timeout, which may be considerably more complicated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do already have a mechanism in the fencer for updating the controller's expected timeout for fencing actions, so it could be modeled on that, but it would still likely be pretty intrusive.

Doesn't the controller have both timeout and pcmk_monitor_timeout in the graph? Maybe it could just do the override itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the controller have both timeout and pcmk_monitor_timeout in the graph? Maybe it could just do the override itself.

IIRC we strip out pcmk_* options from the transition graph. I suspect a fairly straightforward fix would be to add pcmk_monitor_timeout to the graph when needed, as a special XML attribute rather than with the rest of the resource parameters (to avoid breaking the hash). The controller can then pull it out and use it instead of the usual timeout.


GHashTable *params = pe_rsc_params(rsc, node, rsc->cluster);

Expand Down