feat: make VM reboot timeout configurable via --vm-reboot-timeout - #414
Merged
Conversation
ovi-ionos
force-pushed
the
feat/ICNAS-875
branch
2 times, most recently
from
August 28, 2026 11:44
7703748 to
89e3d56
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the VM reboot wait (used by the ServerSet state map / failover orchestration path) configurable via CLI flags, and wires that configuration through provider options into the ServerSet controller so the controller’s reconcile timeout can optionally be widened to accommodate longer reboot waits.
Changes:
- Add
--vm-reboot-timeoutand--extend-serverset-timeout-for-vm-rebootflags and plumb them intoConfigurationOptions. - Pass the configured VM reboot timeout into the ServerSet controller’s connector/external logic and use it in
kube.WaitForResourcecalls. - Remove the hard-coded
VMRebootTimeoutconstant and clarify ServerSet ready timeout usage in comments.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/kube/kube.go | Removes the fixed VM reboot timeout constant and updates timeout-related comments. |
| internal/utils/configuration.go | Extends provider configuration options with VM reboot timeout + “extend ServerSet timeout” toggle and exposes getters. |
| internal/controller/serverset/setup.go | Computes a ServerSet-specific reconcile timeout and passes VM reboot timeout into the connector. |
| internal/controller/serverset/serverset.go | Uses the configured VM reboot timeout for post-reboot readiness waits instead of a constant. |
| cmd/provider/main.go | Defines the new CLI flags, warns about ineffective combinations, and constructs configuration options with the new values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ovi-ionos
force-pushed
the
feat/ICNAS-875
branch
from
August 28, 2026 12:55
89e3d56 to
c182994
Compare
7 tasks
ovi-ionos
force-pushed
the
feat/ICNAS-875
branch
from
August 28, 2026 13:08
c182994 to
af6c054
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
cmd/provider/main.go:97
- The kingpin.Fatalf format string uses %s (expects a stringer like time.Duration), but the arguments passed are *time.Duration pointers (vmRebootTimeout, timeout). This will print as a fmt error (e.g. %!s(*time.Duration=...)) instead of the actual duration values.
kingpin.Fatalf("--vm-reboot-timeout (%s) exceeds --timeout (%s) but --extend-serverset-timeout-for-vm-reboot is not set; "+
"the VM reboot wait would be silently capped at ~--timeout and have no effect. "+
"Either lower --vm-reboot-timeout or pass --extend-serverset-timeout-for-vm-reboot",
vmRebootTimeout, timeout)
cristiGuranIonos
previously approved these changes
Sep 1, 2026
cristiGuranIonos
left a comment
cristiGuranIonos
left a comment
Member
There was a problem hiding this comment.
might be a little better to put a per-resource timeout, but this should be fine since it will only ever be set in these conditions
Replaces the hardcoded VMRebootTimeout constant with a provider flag (default 120m, matching the prior behavior) so it can be configured in each deployment. In some situations our VMs might need be expected to take longer to reestablish a safe state between themselves, which means we'd need to configure a higher value here. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
--vm-reboot-timeout alone had no effect once raised past --timeout: the wait already runs inside a context bounded by the ServerSet controller's own managed.WithTimeout(opts.GetTimeout()), and context.WithTimeout can only shrink a deadline, never extend it. Add --extend-serverset-timeout-for-vm-reboot (default false, opt-in) to widen only the ServerSet controller's own reconcile-timeout ceiling to max(--timeout, --vm-reboot-timeout) when explicitly enabled. Every other resource kind keeps using --timeout unchanged. Also log a non-fatal warning at startup if --vm-reboot-timeout exceeds --timeout without the new flag set, since that combination is silently capped. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
There's no legitimate reason to explicitly set --vm-reboot-timeout above --timeout without also passing --extend-serverset-timeout-for-vm-reboot: without the latter, the value has zero effect (silently capped). This check only fires when the user actually set the flag. So this is intended to explicitly catch misconfigurations at startup.
It doesn't: StatefulServerSet's Ensure() only waits on this timeout in the not-found (first creation) branch; every later Update() pushes the spec change and returns without blocking on readiness. I wanted to have this documented here, because it's non-obvious from the name of the constant, and one might think this would also override the VMRebootTimeout like the global controller timeout does. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
The poll callback discarded the context wait.PollUntilContextTimeout actually passes it (which is bound to the per-call timeout) and closed over the outer, unbounded one instead, so every fn call - across all 19 call sites, not just the new vm-reboot-timeout one - ran with a looser deadline than the timeout it was supposed to respect. Impact is narrow in practice (a cached client Get is normally fast).
ovi-ionos
force-pushed
the
feat/ICNAS-875
branch
from
September 2, 2026 12:22
3c81447 to
3d40b70
Compare
Matches how --poll-jitter-percentage/--poll-not-ready were wired in, and leaves NewConfigurationOptions' signature untouched.
|
cristiGuranIonos
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
This PR adds:
--vm-reboot-timeoutflag to be able to configure the amount of time waited after a VM has been rebooted when using thestatemapfeature--extend-serverset-timeout-for-vm-rebootto optionally be able to extend the timeout of theServerSetcontroller, without needing to change the global timeout for all other controllers. This is necessary, otherwise a--vm-reboot-timeoutlonger than the global--timeoutwould not actually be applied... This is a compromise, to avoid having to raise--timeoutfor all controllers to unnecessarily high values.--vm-reboot-timeoutis configured manually beyond the--timeoutwithout also setting--extend-serverset-timeout-for-vm-reboot. This is intended to guard against unintentional misconfiguration, and will only fire if the user manually specifies the--vm-reboot-timeoutflag, and never with the default configuration (which already has --timeout lower than --vm-reboot-timeout). One could instead log a warning, but I think it's likely that would not be noticed and hence not stop accidental misconfiguration...WaitForResourcein 3c81447. The calculated deadline wasn't actually properly passed to the function callback, it always received the overall outer context without the more specific deadline. This should not matter, because the callbacks usually complete fast.Instead of this (a bit over-specific)
--extend-serverset-timeout-for-vm-rebootflag, one could also introduce a--timeout-per-resourceflag mirroring--max-reconcile-rate-per-resource. That would make the timeout for any controller in this provider configurable, and then one just has to know to increase the timeout for SSet.If you'd prefer that solution, let me know.
Checklist
I have:
feat/fix/doc/test/refactor)make reviewableandmake crds.cleanto ensure the PR is ready for reviewmake docs.update(if applicable)docs/CHANGELOG.mdfile (label:upcoming release)