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.

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

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

Conversation

nrwahl2
Copy link
Contributor

@nrwahl2 nrwahl2 commented Oct 27, 2023

The executor uses pcmk_monitor_timeout, but the controller considers a recurring monitor to have timed out after its op timeout expires. If pcmk_monitor_timeout is very long (for example, 240 seconds), a stonith stop action can fail. In this situation, the monitor is declared as timed out before the pcmk_monitor_timeout expires, the stop action is requested, and its timer begins counting down. However, the stop action can't begin until after the monitor finishes or pcmk_monitor_timeout expires.

This also makes special handling in controld_execd.c unnecessary. pcmk__unpack_action_meta() has already replaced the meta timeout with the pcmk_monitor_timeout.

Closes RHEL-14826 (JIRA).

The executor uses pcmk_monitor_timeout, but the controller considers a
recurring monitor to have timed out after its op timeout expires. If
pcmk_monitor_timeout is very long (for example, 240 seconds), a stonith
stop action can fail. In this situation, the monitor is declared as
timed out before the pcmk_monitor_timeout expires, the stop action is
requested, and its timer begins counting down. However, the stop action
can't begin until after the monitor finishes or pcmk_monitor_timeout
expires.

This also makes special handling in controld_execd.c unnecessary.
pcmk__unpack_action_meta() has already replaced the meta timeout with
the pcmk_monitor_timeout.

Closes RHEL-14826 (JIRA).

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
&& (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants