Skip to content

Fix: Check for watchdog-based self-fencing device only if "fencing-watchdog-timeout" is enabled #4146

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

Open
gao-yan wants to merge 4 commits into
base: main
Choose a base branch
from
+33 −32
Open

Fix: Check for watchdog-based self-fencing device only if "fencing-watchdog-timeout" is enabled #4146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39b828f
Fix: controller: Check for watchdog-based self-fencing device only if…
gao-yan Jul 7, 2026
a69067b
Fix: liblrmd: Check watchdog-based self-fencing device for remote nod…
gao-yan Jul 8, 2026
5ec2059
Refactor: libcrmcommon: Functionize parsing "fencing-watchdog-timeout"
gao-yan Jul 8, 2026
cf58cca
Refactor: libcrmcommon: Make pcmk__auto_fencing_watchdog_timeout() st…
gao-yan Jul 8, 2026
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
8 changes: 7 additions & 1 deletion daemons/controld/controld_fencing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,13 +995,19 @@ void
controld_validate_fencing_watchdog_timeout(const char *value)
{
const char *our_nodename = controld_globals.cluster->priv->node_name;
long long timeout_ms = 0;

@nrwahl2 nrwahl2 Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the regression cause only misleading log messages, or does it cause real misbehavior?

IMO, it seems that the log message should be at debug level instead of notice level anyway. And that would avoid the whole "confusing log" issue.

@gao-yan gao-yan Jul 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It mainly causes the misleading messages. But of course the checks with stonith__watchdog_fencing_enabled_for_node_api(), which cost IPCs, are totally meaningless when watchdog-based fencing is not even configured/enabled.

About the logging level of the message, there are some comments around the code about some "race". So I'm not sure what "-ENODEV" is supposed to indicate if "fencing-watchdog-timeout" is actually enabled.


timeout_ms = pcmk__parse_fencing_watchdog_timeout(value);
if (timeout_ms == 0) {

@nrwahl2 nrwahl2 Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now, if the timeout fails to parse, pcmk__parse_fencing_watchdog_timeout() will return 0. Then we'll return early.

Previously, if the timeout failed to parse, we would continue to the next block and call pcmk__valid_fencing_watchdog_timeout() (if watchdog fencing were enabled for the node, etc.).

Wouldn't we want to call pcmk__valid_fencing_watchdog_timeout() on parse failure?

Edit: Previously, I guess we would have passed 0 to pcmk__valid_fencing_watchdog_timeout() upon parse failure. So it would have validated fine. And in both cases, it would not log any warning except the nonspecific one in pcmk__parse_ms().

This doesn't look like a meaningful change in behavior after all. The only difference is that now we will no longer call stonith__watchdog_fencing_enabled_for_node_api() after a parse failure.

Edit:

This doesn't look like a meaningful change in behavior after all.

Actually, the "previous" behavior was only introduced by your first commit lol ;) So it really doesn't seem to matter.

@gao-yan gao-yan Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After all, it's about what behavior we should expect from a misconfiguration. And it should be consistent wherever it's parsed.

return;
}

// Validate only if the timeout will be used
if ((fencer_api != NULL) && (fencer_api->state != stonith_disconnected)
&& stonith__watchdog_fencing_enabled_for_node_api(fencer_api,
our_nodename)) {

pcmk__valid_fencing_watchdog_timeout(value);
pcmk__valid_fencing_watchdog_timeout(timeout_ms);
}
}

Expand Down
6 changes: 3 additions & 3 deletions daemons/execd/execd_messages.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ handle_check_request(pcmk__request_t *request)
pcmk__client_privileged);
xmlNode *wrapper = NULL;
xmlNode *data = NULL;
const char *timeout = NULL;
long long timeout_ms = 0;

if (!allowed) {
pcmk__set_result(&request->result, CRM_EX_INSUFFICIENT_PRIV,
Expand All @@ -154,11 +154,11 @@ handle_check_request(pcmk__request_t *request)
return NULL;
}

timeout = pcmk__xe_get(data, PCMK__XA_LRMD_WATCHDOG);
pcmk__xe_get_ll(data, PCMK__XA_LRMD_WATCHDOG, &timeout_ms);
/* FIXME: This just exits on certain conditions, which seems like a pretty
* extreme reaction for a daemon to take.
*/
pcmk__valid_fencing_watchdog_timeout(timeout);
pcmk__valid_fencing_watchdog_timeout(timeout_ms);

pcmk__set_result(&request->result, CRM_EX_OK, PCMK_EXEC_DONE, NULL);
return NULL;
Expand Down
12 changes: 1 addition & 11 deletions daemons/fenced/fenced_cib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,6 @@ get_fencing_watchdog_timeout(xmlNode *cib)
{
xmlNode *stonith_watchdog_xml = NULL;
const char *value = NULL;
int rc = pcmk_rc_ok;
long long timeout_ms = 0;

// @TODO An XPath search can't handle multiple instances or rules
stonith_watchdog_xml = pcmk__xpath_find_one(cib->doc,
Expand All @@ -182,16 +180,8 @@ get_fencing_watchdog_timeout(xmlNode *cib)
}

value = pcmk__xe_get(stonith_watchdog_xml, PCMK_XA_VALUE);
if (value == NULL) {
return 0;
}

rc = pcmk__parse_ms(value, &timeout_ms);
if ((rc == pcmk_rc_ok) && (timeout_ms >= 0)) {
return timeout_ms;
}

return pcmk__auto_fencing_watchdog_timeout();
return pcmk__parse_fencing_watchdog_timeout(value);
}

/*!
Expand Down
4 changes: 2 additions & 2 deletions include/crm/common/options_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ bool pcmk__valid_placement_strategy(const char *value);
// from watchdog.c
long pcmk__get_sbd_watchdog_timeout(void);
bool pcmk__get_sbd_sync_resource_startup(void);
long pcmk__auto_fencing_watchdog_timeout(void);
bool pcmk__valid_fencing_watchdog_timeout(const char *value);
long long pcmk__parse_fencing_watchdog_timeout(const char *value);
bool pcmk__valid_fencing_watchdog_timeout(long long st_timeout);

// @COMPAT Deprecated cluster options
#define PCMK__OPT_CANCEL_REMOVED_ACTIONS "cancel-removed-actions"
Expand Down
29 changes: 16 additions & 13 deletions lib/common/watchdog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,17 +262,17 @@ pcmk__get_sbd_sync_resource_startup(void)
}

// 0 <= return value <= min(LONG_MAX, (2 * SBD timeout))
long
pcmk__auto_fencing_watchdog_timeout(void)
static long
auto_fencing_watchdog_timeout(void)
{
long sbd_timeout = pcmk__get_sbd_watchdog_timeout();
long long st_timeout = 2 * (long long) sbd_timeout;

return (long) QB_MIN(st_timeout, LONG_MAX);
}

bool
pcmk__valid_fencing_watchdog_timeout(const char *value)
long long
pcmk__parse_fencing_watchdog_timeout(const char *value)
{
/* @COMPAT At a compatibility break, accept either negative values or a
* specific string like "auto" (but not both) to mean "auto-calculate the
Expand All @@ -285,23 +285,27 @@ pcmk__valid_fencing_watchdog_timeout(const char *value)
}

if (st_timeout < 0) {
st_timeout = pcmk__auto_fencing_watchdog_timeout();
st_timeout = auto_fencing_watchdog_timeout();

// At this point, 0 <= sbd_timeout <= st_timeout
pcmk__debug("Using calculated value %lld for "
PCMK_OPT_FENCING_WATCHDOG_TIMEOUT " (%s)",
st_timeout, value);
}

return st_timeout;
}

bool
pcmk__valid_fencing_watchdog_timeout(long long st_timeout)
{
if (st_timeout == 0) {
pcmk__debug("Watchdog may be enabled but "
PCMK_OPT_FENCING_WATCHDOG_TIMEOUT " is disabled (%s)",
pcmk__s(value, "default"));
PCMK_OPT_FENCING_WATCHDOG_TIMEOUT " is disabled (0ms)");

} else if (pcmk__locate_sbd() == 0) {
pcmk__emerg("Shutting down: " PCMK_OPT_FENCING_WATCHDOG_TIMEOUT
" configured (%s) but SBD not active",
pcmk__s(value, "auto"));
" configured (%lldms) but SBD not active", st_timeout);
crm_exit(CRM_EX_FATAL);
return false;

Expand All @@ -313,14 +317,13 @@ pcmk__valid_fencing_watchdog_timeout(const char *value)
* parsable, positive, and less than the SBD_WATCHDOG_TIMEOUT
*/
pcmk__emerg("Shutting down: " PCMK_OPT_FENCING_WATCHDOG_TIMEOUT
" (%s) too short (must be >%ldms)",
value, sbd_timeout);
" (%lldms) too short (must be >%ldms)",
st_timeout, sbd_timeout);
crm_exit(CRM_EX_FATAL);
return false;
}
pcmk__info("Watchdog configured with " PCMK_OPT_FENCING_WATCHDOG_TIMEOUT
" %s and SBD timeout %ldms",
value, sbd_timeout);
" %lldms and SBD timeout %ldms", st_timeout, sbd_timeout);
}
return true;
}
6 changes: 4 additions & 2 deletions lib/lrmd/lrmd_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,13 +1029,15 @@ lrmd__validate_remote_settings(lrmd_t *lrmd, GHashTable *hash)
const char *value;
lrmd_private_t *native = lrmd->lrmd_private;
xmlNode *data = pcmk__xe_create(NULL, PCMK__XA_LRMD_OP);
long long timeout_ms = 0;

pcmk__xe_set(data, PCMK__XA_LRMD_ORIGIN, __func__);

value = pcmk__cluster_option(hash, PCMK_OPT_FENCING_WATCHDOG_TIMEOUT);
if ((value) &&
timeout_ms = pcmk__parse_fencing_watchdog_timeout(value);

@nrwahl2 nrwahl2 Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previously, if we parsed a negative value, we would set it directly as PCMK__XA_LRMD_WATCHDOG.

Now, if we parse a negative value, pcmk__parse_fencing_watchdog_timeout() will return pcmk__auto_fencing_watchdog_timeout(), and we will set PCMK__XA_LRMD_WATCHDOG to the auto value.

I think that's fine. The receiving end (in execd_messages.c) was calling pcmk__parse_fencing_watchdog_timeout() again. So I think the end result is the same.

if ((timeout_ms != 0) &&
(stonith__watchdog_fencing_enabled_for_node(native->remote_nodename))) {
pcmk__xe_set(data, PCMK__XA_LRMD_WATCHDOG, value);
pcmk__xe_set_ll(data, PCMK__XA_LRMD_WATCHDOG, timeout_ms);
}

rc = lrmd_send_command(lrmd, LRMD_OP_CHECK, data, NULL, 0, 0,
Expand Down