fix: bound sentinel and reboot command execution by mvanhorn · Pull Request #1423 · kubereboot/kured

mvanhorn · 2026-09-28T09:53:18Z

Add --sentinel-command-timeout and --reboot-command-timeout duration flags, each defaulting to 60s, validate positive values before cluster initialization, and thread them through internal.NewRebootChecker and internal.NewRebooter into timeout fields on the concrete command types. Both CommandChecker.RebootRequired() and CommandRebooter.Reboot() execute user-configured commands without a deadline.

Checker commands exiting 0, 1, and an unexpected nonzero code retain their true/false and warning behavior; missing executables retain the existing start-error behavior. Successful reboot commands return nil and nonzero/start errors retain captured diagnostics; Use short test deadlines and controlled helper subprocesses to verify a sleeping checker returns false with a timeout warning and a sleeping rebooter returns a timeout error within deadline plus drain grace and a generous scheduling tolerance. Never run an actual reboot or require privileged nsenter in unit tests.

Fixes #1412

evrardjp

I like the idea of it, not so sure about the implementation.

Here are a few points for conversation before we go ahead

evrardjp · 2026-09-29T08:07:45Z

 	flag.StringVar(&rebootSentinelCommand, "reboot-sentinel-command", "",
 		"command for which a zero return code will trigger a reboot command")
+	flag.DurationVar(&sentinelCommandTimeout, "sentinel-command-timeout", 60*time.Second,
+		"positive maximum duration for one reboot sentinel command (default 60s); after cancellation or process exit, waiting for inherited stdout/stderr pipes is bounded by a 1s drain grace")


"waiting for inherited stdout/stderr pipes is bounded by a 1s drain grace" is confusing.

But outside the wording, I am not so fond of the 1 sec that's actively set in constants below.
I would rather we have a generic check to match for expected behaviour instead.

If it's not fire and forget anymore, so I would say we need to check the successes/failures, and extend capabilities...

Here, when the deadline is exceeded, you consider the node as not in need of reboot, which I have a tendency to disagree. We do not know whether the reboot is required or not.
Logging and warning is not enough for me. Cause if people do not read their logs, the system is starting to misbehave silently. Should we crash instead?

next to that, there is no validation of the value compared to the regular period.

@dharsanb any opinion?

I reworded both help strings in 16b3323, along with the manifest and code comments that repeated them. They now say the command is killed at the timeout, and that kured then waits at most 1s more for any child process still holding its output. That commit is wording only, so the rest of your comment still stands at this head:

A sentinel that hits the deadline still returns false from CommandChecker.RebootRequired(), so the node reads as not needing a reboot. I agree that's wrong. It only returns a bool, so the options are crashing, as you suggested, or returning an error, as in my Sep 30 comment. Crashing on the first timeout is the smaller change, but the call can happen while kured holds the lock (the annotateNodes check in rebootAsRequired).

The 1s is still a constant.

The timeouts are still not checked against --period. A plain check has a catch: make dev-manifest sets --period=20s for the e2e tests, so with the 60s flag default kured would refuse to start there, and for anyone else with a period under 60s. The default would have to change with it, or the check would apply only when the flag is set.

Which do you want for the deadline: exit on the first timeout, or the error return?

gofmt, go vet, go build, golangci-lint and go test -short pass on macOS with Go 1.26.8. I did not run the e2e tests.

evrardjp · 2026-09-29T08:15:01Z

 	flag.StringVar(&rebootCommand, "reboot-command", "/bin/systemctl reboot",
 		"command to run when a reboot is required")
+	flag.DurationVar(&rebootCommandTimeout, "reboot-command-timeout", 60*time.Second,
+		"positive maximum duration for one reboot command (default 60s); after cancellation or process exit, waiting for inherited stdout/stderr pipes is bounded by a 1s drain grace")


see my comments above sentinel command timeout, just adapt a bit for the reboot command:

What happens if you start the trigger and stop it?
IMO cancelling should NOT be the default case, especially with a 60 seconds timeout and 1 sec post command.

This will break users with large clusters.

Agreed, cancelling should not be the default. In 9ec3ff0, --reboot-command-timeout now defaults to 0, which means no deadline, so by default kured waits for the reboot command and its output for as long as they take, as it does on main. The 1s output bound only applies when a timeout is set, and the 60s package fallback for the rebooter is gone. Negative values are still rejected at startup. The manifest comment is updated too.

To your question about starting the trigger and stopping it: with the default that can't happen now. If someone sets a timeout, the command is killed at the deadline, Reboot() returns an error, and rebootAsRequired hits log.Fatalf, so kured exits and the pod restarts. I have not tested what a killed reboot command leaves behind on a real node, for example whether systemctl reboot carries on once it has been accepted.

This covers the reboot command only. --sentinel-command-timeout still defaults to 60s and still returns false at the deadline, and that decision is still open in your thread above.

gofmt, go vet, go build, golangci-lint and go test -short pass on macOS with Go 1.26.8. I did not run the e2e tests.

evrardjp · 2026-09-29T08:17:47Z


 	log.Infof("Reboot method: %s", rebootMethod)
-	rebooter, err := internal.NewRebooter(rebootMethod, rebootCommand, rebootSignal)
+	rebooter, err := internal.NewRebooter(rebootMethod, rebootCommand, rebootSignal, rebootCommandTimeout)


while I am not fond of the extension here (maybe this NewRebooter should go away and we will rely on config parsing in v2), I think it's the cleanest so far. You get my 👍

evrardjp · 2026-09-29T08:20:26Z

+	}
+	if rebootCommandTimeout <= 0 {
+		log.Fatalf("reboot-command-timeout must be positive, got %v", rebootCommandTimeout)
+	}


In this part of the loading of configuration, we should check for consistency between those timeouts and period.

Added in 4fa51c8. When a sentinel command is set, kured now checks --sentinel-command-timeout against --period at startup and exits if an explicit value (flag or env) is not shorter than the period.

Two choices you may want to weigh in on:

The 60s default only logs a warning. A sentinel command with --period=30s works today and never set this flag, so failing at startup would break that setup on upgrade.

--reboot-command-timeout is not compared with the period. rebootAsRequired calls rebooter.Reboot() once, then ends in log.Fatalf or the Waiting for reboot loop, so there is no next tick for it to overlap. Rejecting it would stop someone setting a long timeout with a short period for no gain.

A sentinel file has no deadline, so it is not checked.

gofmt, golangci-lint, go vet, go build and go test -short for cmd, internal and pkg pass locally. The kind e2e tests were not run, and the PR workflows are waiting for approval.

evrardjp · 2026-09-29T08:24:47Z

+	defaultCommandTimeout = 60 * time.Second
+	// commandWaitDelay bounds draining of inherited stdout/stderr pipes after
+	// the process exits or its deadline fires. It does not terminate
+	// descendant processes. The CLI help text documents this 1s drain grace.
+	commandWaitDelay = time.Second


Not so fond of setting default values. How do you know they are good here?

I don't know that they are good. 60s is a judgment call, not a measured value, and I have no data on how long real sentinel or reboot commands take. It also changes behavior: these commands have no deadline today, so a sentinel that runs past 60s is now killed and reads as no reboot needed, and a reboot command past 60s is killed and returns an error.

This constant covers callers that build the structs directly. It matches the 60s default on --sentinel-command-timeout and --reboot-command-timeout, and main.go refuses zero or negative values, so nobody can turn the deadline off today.

My Sep 30 comment proposed dropping the package-level defaults, keeping defaults only in the flags, and having no reboot timeout by default. None of that is in c75066c. Does that direction work for you, or would you rather have no default at all, so a deadline only applies when someone sets one?

evrardjp · 2026-09-29T08:25:12Z

+	// Timeout is the deadline for a single RebootRequired execution.
+	// The zero value falls back to 60s. Each call creates its own context
+	// and output buffers; nothing about a previous run is retained.
+	Timeout time.Duration


As hinted before, maybe this should be made more generic.

Agreed, and this PR has not done that. Timeout is still only on CommandChecker, so FileRebootChecker has no deadline.

What I would do is in the internal/validators.go thread: RebootRequired returns an error, and one wrapper applies the deadline to any checker. Your answer there on whether that goes in this PR or the sentinel follow-up decides this one too, so I have not changed anything here.

evrardjp · 2026-09-29T08:25:30Z

+// commandTimeout returns the deadline for one execution.
+func (rc CommandChecker) commandTimeout() time.Duration {
+	if rc.Timeout == 0 {
+		return defaultCommandTimeout


this actively changes the current behaviour

Agreed, and this fallback was the part with no reason to exist. In aef5f2d a zero Timeout on CommandChecker means no deadline, the same as CommandRebooter, so code that builds the type directly is not cancelled, as on main. The 1s output bound now only applies when a timeout is set, and defaultCommandTimeout is gone.

That does not cover the flag. --sentinel-command-timeout still defaults to 60s and still rejects 0, so a kured sentinel command that runs past 60s is killed and reads as no reboot needed, which main does not do. Whether that default stays is the open question in your Not so fond of setting default values thread on this file, so I have not changed it and I am leaving this thread open.

The PR description is also out of date on this: it still says both flags default to 60s, while --reboot-command-timeout now defaults to 0.

gofmt, golangci-lint, go vet, go build and go test -short for cmd, internal and pkg pass on macOS with Go 1.26.8. I did not run the e2e tests.

evrardjp · 2026-09-29T08:26:15Z

+	defaultCommandTimeout = 60 * time.Second
+	// commandWaitDelay bounds draining of inherited stdout/stderr pipes after
+	// the process exits or its deadline fires. It does not terminate
+	// descendant processes. The CLI help text documents this 1s drain grace.
+	commandWaitDelay = time.Second


I read this as the same point as your comment on commandWaitDelay in pkg/checkers/checker.go: how do I know 1s is good here. I don't. It is a judgment call, not a measured value, and there is no flag to change it.

It sets cmd.WaitDelay, which needs some bound. With no WaitDelay, cmd.Run() keeps waiting on a descendant that holds stdout or stderr open, even after the deadline kills the command. TestCommandRebooterDescendantDoesNotHang covers that, but it only shows the bound works, not that 1s is the right size.

It also has a cost on the reboot path. If the command exits 0 and leaves a child holding those pipes for more than 1s, Reboot() returns an ErrWaitDelay error and cmd/kured/main.go hits log.Fatalf("Unable to reboot node: %v", err). Before this PR that command would just wait for the pipes to close, so this is a new failure that can happen well inside the 60s deadline.

My Sep 30 comment proposed dropping the hard-coded 1s. That is not in c75066c, and it is not a plain delete, because whatever replaces it still has to bound that wait. I have not changed anything here, so I am leaving this open.

evrardjp · 2026-09-29T08:26:29Z

+	// Timeout is the deadline for a single Reboot execution.
+	// The zero value falls back to 60s. Each call creates its own context
+	// and output buffers; nothing about a previous run is retained.
+	Timeout time.Duration
+}
+
+// commandTimeout returns the deadline for one execution.
+func (c CommandRebooter) commandTimeout() time.Duration {
+	if c.Timeout == 0 {
+		return defaultCommandTimeout
+	}
+	return c.Timeout


This fallback is gone. 9ec3ff0 removed it, so a zero Timeout on CommandRebooter now means no deadline and Reboot() is never cancelled, as on main. --reboot-command-timeout also defaults to 0. TestCommandRebooterZeroTimeoutHasNoDeadline covers this.

I read "ditto" as your comment that this actively changes the current behaviour, on the same fallback in pkg/checkers/checker.go. If you meant something else here, tell me. No new commit for this thread, and I am leaving it open for you.

go test -short ./pkg/reboot/ passes at aef5f2d on macOS with Go 1.26.8. I did not run the e2e tests.

mvanhorn · 2026-09-30T07:17:43Z

Thanks for the thorough pass. Agreed on the main point: a timed-out sentinel means we don't know, not "no reboot needed". Here's what I'd change before touching code again:

Sentinel timeout returns an error instead of false. The loop treats it as unknown: no reboot, no lock release, previous state kept, and a failure metric bumped. To avoid the silent-misbehavior problem from Sentinel/reboot command exec has no timeout - hang wedges kured silently, undetectable via liveness probe #1412, after N consecutive timeouts kured exits non-zero so it shows up as CrashLoopBackOff rather than a log line nobody reads. Open to crashing on the first one if you prefer.
No timeout on the reboot command by default (0 = disabled, today's behavior), so large clusters aren't affected unless someone opts in.
Drop the hard-coded 1s drain grace and the package-level defaults. Defaults live only in the flags, and the pkg types take the timeout through their constructors (NewCommandRebooter(cmd, timeout), same for the checkers).
Make it generic: one timeout-aware execution path shared by the command and file checkers, and for the signal rebooter the timeout means "node didn't go down in time", reported the same way as a command failure.
Validate at config load that the sentinel timeout is shorter than --period, and fail fast otherwise.

If that direction works for you, I'd split it: the sentinel side first since that's what #1412 hit, then rebooters in a follow-up. Does that sound right?

Fixes kubereboot#1412 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

NewCommandRebooter and NewCommandChecker now take the timeout as an argument, so NewRebooter and NewRebootChecker no longer set the field after construction. A zero timeout still falls back to 60s. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

The help for --sentinel-command-timeout and --reboot-command-timeout called the wait for inherited pipes a "drain grace", which reviewers found confusing. Say what happens instead: the command is killed at the deadline, then kured waits at most 1s more for any child process still holding its output. The help also stated the default by hand, next to the one the flag library prints. Drop the hand-written copy, and use the same wording in the manifest comments and the code comments that point at the help. No behavior change. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

A reboot command that takes longer than 60s was killed by default, which can break large clusters. --reboot-command-timeout now defaults to 0, no deadline, which keeps today's behavior: kured waits for the command and its output for as long as they take. The 1s output drain bound only applies when a timeout is set, and the package-level 60s fallback is gone. Negative values are still rejected at startup. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

A sentinel command runs inline on each --period tick, so a deadline of one period or more lets a stuck command take up the next check. Reject an explicit --sentinel-command-timeout that is not shorter than --period when a sentinel command is set. The 60s default only warns: a shorter --period with a sentinel command works today, and an upgrade should not stop that daemon for a value the user never set. The reboot command timeout is not compared, because the reboot runs once and then ends in a fatal or the wait-for-reboot loop. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

CommandChecker gave a zero Timeout a 60s deadline, so anyone building the type directly had a limit they never asked for. Zero now means no deadline, the same as CommandRebooter, and the 1s output bound applies only when a timeout is set. The --sentinel-command-timeout flag and its 60s default are unchanged. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

evrardjp reviewed Sep 29, 2026

View reviewed changes

mvanhorn added 2 commits October 30, 2026 23:44

fix: bound sentinel and reboot command execution

c9cff92

Fixes kubereboot#1412 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

mvanhorn force-pushed the fix/1412-kured-command-timeouts branch from ddcb019 to c75066c Compare October 31, 2026 06:44

mvanhorn added 4 commits October 31, 2026 09:55

Conversation

mvanhorn commented Sep 28, 2026

Uh oh!

evrardjp 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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

mvanhorn commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants