Fix: Check for watchdog-based self-fencing device only if "fencing-watchdog-timeout" is enabled by gao-yan · Pull Request #4146 · ClusterLabs/pacemaker

gao-yan · 2026-07-08T18:46:35Z

With 14e9b3e, stonith__watchdog_fencing_enabled_for_node_api() was
called only if "stonith-watchdog-timeout" was enabled (non-zero). But
with a4f2db3, the function is again always called even if the cluster
option is not enabled, so that it again keeps logging the confusing
notice "Cluster does not have watchdog fencing".

Besides, in liblrmd, stonith__watchdog_fencing_enabled_for_node_api()
has always been called for remote nodes even if "fencing-watchdog-timeout"
is disabled.

This PR fixes the issues by ensuring the watchdog-based self-fencing device
is only checked when "fencing-watchdog-timeout" is actually enabled.

… "fencing-watchdog-timeout" is enabled With 14e9b3e, stonith__watchdog_fencing_enabled_for_node_api() was called only if "stonith-watchdog-timeout" was enabled (non-zero). But with a4f2db3, the function is again always called even if the cluster option is not enabled, so that it again keeps logging the confusing notice "Cluster does not have watchdog fencing". This commit fixes the regression by anyway parsing the value of the cluster option first and avoiding calling stonith__watchdog_fencing_enabled_for_node_api() if the cluster option is not even enabled.

…es only if "fencing-watchdog-timeout" is enabled Previously, stonith__watchdog_fencing_enabled_for_node_api() was always called even when "fencing-watchdog-timeout" was disabled. This resulted in a constant, confusing log notice: "Cluster does not have watchdog fencing device". This commit fixes it by ensuring the watchdog-based self-fencing device for remote nodes is only checked when the cluster option is actually enabled.

nrwahl2

Thanks!

nrwahl2 · 2026-07-24T23:12:49Z

 // 0 <= return value <= min(LONG_MAX, (2 * SBD timeout))
-long
+static long
 pcmk__auto_fencing_watchdog_timeout(void)


Let's change the name to auto_fencing_watchdog_timeout(). As a convention, we usually reserve pcmk__ for symbols that are internal but not static.

nrwahl2 · 2026-07-25T01:13:17Z

 controld_validate_fencing_watchdog_timeout(const char *value)
 {
    const char *our_nodename = controld_globals.cluster->priv->node_name;
+    long long timeout_ms = 0;


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.

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.

nrwahl2 · 2026-07-26T01:23:51Z

        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 (%lldms)",


We could just drop the (%lldms) and st_timeout here. It would always say "(0ms)".

nrwahl2 · 2026-07-26T01:37:04Z

-    }

-    return pcmk__auto_fencing_watchdog_timeout();
+    return pcmk__parse_fencing_watchdog_timeout(value);;


Drop the extra semicolon character please :)

This is a slight change in behavior. Previously, if the value failed to parse, we would return the auto timeout. Now, if the value fails to parse, we will return 0.

However, I think the new behavior is correct.

Obviously it matches pcmk__parse_fencing_watchdog_timeout(). Consistency seems correct.

I think I introduced the old (incorrect) behavior with commit 215ad4f.

nrwahl2 · 2026-07-26T01:43:39Z

-    if ((pcmk__parse_ms(value, &timeout_ms) == pcmk_rc_ok)
-        && (timeout_ms == 0)) {
+    timeout_ms = pcmk__parse_fencing_watchdog_timeout(value);
+    if (timeout_ms == 0) {


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.

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

nrwahl2 · 2026-07-26T01:50:10Z

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


I think we should use pcmk__xe_set_ll(data, PCMK__XA_LRMD_WATCHDOG, timeout_ms) to avoid having to parse the value on both ends. See also the comment I'm adding to execd_messages.c.

nrwahl2 · 2026-07-26T01:50:43Z

I think we can just have

pcmk__xe_get_ll(data, PCMK__XA_LRMD_WATCHDOG, &timeout_ms); pcmk__valid_fencing_watchdog_timeout(timeout_ms);

lrmd__validate_remote_settings(), which is the only thing that sets PCMK__XA_LRMD_WATCHDOG, has already parsed the timeout to a long long.

Makes sense.

nrwahl2 · 2026-07-26T01:51:46Z

-    if ((value) &&
-        (pcmk__parse_ms(value, &timeout_ms) == pcmk_rc_ok) &&
-        (timeout_ms != 0) &&
+    timeout_ms = pcmk__parse_fencing_watchdog_timeout(value);


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.

…atic And rename to auto_fencing_watchdog_timeout(). Now it's only used within the file.

gao-yan

Thanks for reviewing, @nrwahl2 !

gao-yan · 2026-07-26T09:25:37Z

 controld_validate_fencing_watchdog_timeout(const char *value)
 {
    const char *our_nodename = controld_globals.cluster->priv->node_name;
+    long long timeout_ms = 0;


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.

gao-yan · 2026-07-26T09:38:19Z

Makes sense.

gao-yan · 2026-07-26T18:41:14Z

-    if ((pcmk__parse_ms(value, &timeout_ms) == pcmk_rc_ok)
-        && (timeout_ms == 0)) {
+    timeout_ms = pcmk__parse_fencing_watchdog_timeout(value);
+    if (timeout_ms == 0) {


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

gao-yan added 2 commits July 8, 2026 14:48

nrwahl2 added the review: in progress PRs that are currently being reviewed label Jul 26, 2026

nrwahl2 requested changes Jul 26, 2026

View reviewed changes

gao-yan added 2 commits July 26, 2026 14:13

Refactor: libcrmcommon: Functionize parsing "fencing-watchdog-timeout"

5ec2059

Refactor: libcrmcommon: Make pcmk__auto_fencing_watchdog_timeout() st…

cf58cca

…atic And rename to auto_fencing_watchdog_timeout(). Now it's only used within the file.

gao-yan commented Jul 26, 2026

View reviewed changes

gao-yan force-pushed the check-watchdog-fencing-device branch from 78301d5 to cf58cca Compare July 26, 2026 18:47

Uh oh!

Conversation

gao-yan commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrwahl2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gao-yan Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gao-yan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gao-yan Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gao-yan commented Jul 8, 2026 •

edited

Loading

gao-yan Jul 26, 2026 •

edited

Loading

gao-yan Jul 26, 2026 •

edited

Loading