Repository navigation
fix(kubernetes): widen and expose the worker node join timer budgets - #3898
Merged
Merged
Conversation
The cluster-autoscaler treats a machine as long-unregistered once its own max-node-provision-time elapses, and a worker that has not registered by then stops counting towards its node group's upcoming capacity. On a host slow enough that the Talos image import, the guest boot and the CNI rollout do not fit inside that window, the timer fires on machines that were still joining: the group reads as short of capacity, so the autoscaler asks for replacements, and the extra provisioning load makes the join it was waiting for slower still. Render the per-node-group override the autoscaler reads off the MachineDeployment, sourced from a new maxNodeProvisionTime value that defaults to 30m. The duration is checked at render time because the autoscaler discards one it cannot parse and falls back to its own default without failing, and because the values schema types the field as a string and so accepts every malformed duration. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…able The two unhealthyConditions timeouts were fixed in the template at 30s for `Ready=Unknown` and 300s for `Ready=False`, and both are too tight for a worker joining on a loaded host. `Ready=Unknown` is what the node lifecycle controller writes when a kubelet stops reporting, on a grace period of its own, so 30s on top of that deleted the VM of a guest that had merely stopped being scheduled for a stretch, before it had a chance to answer again. And `Ready=False` is the condition a registered Node holds until its CNI is up, so 300s was the entire budget a first boot got for the CNI rollout to reach it -- a worker deleted mid-rollout takes the rollout with it and its replacement starts over. Move both to values, defaulting to 5m and 10m. The remediation of a worker that is genuinely dead is correspondingly later, which costs less than it appears to: Pods leave an unreachable Node on the node lifecycle controller's taint-based eviction and the toleration that carries, not on either of these timers. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
nodeStartupTimeout is the window a Machine has to produce a Node at all, measured from the later of the Machine's creation and its InfrastructureReady transition. What has to fit inside it is the guest boot and the kubelet registering with the tenant apiserver, and on a host under load neither is quick. At 10m a worker that was still booting was deleted before it ever joined, and its replacement started the same race from the beginning. Also record what a percentage maxUnhealthy does at small group sizes, because the interaction is not what the field name suggests: the value is scaled against the machines the check covers and rounded down, and remediation proceeds only while the unhealthy count is at or below the result. At the default "50%" a group of one never remediates, and a group of two remediates one unhealthy machine but not two. The golden-parity gate pinned the child chart's MachineHealthCheck values to the parent's defaults as inputs, which made a default moved on both sides fail there for the wrong reason. Drop the pins so the comparison is between the two charts' defaults, which is the contract it exists to hold, and pin the defaults themselves in each chart's own unit tests instead. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
requested review from
IvanHunters,
Andrei Kvapil (kvaps),
Timofei Larkin (lllamnyp) and
myasnikovdaniil
as code owners
August 18, 2026 18:14
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds node provisioning and MachineHealthCheck timeout settings to both Kubernetes charts. Updates API types, chart values and schemas, templates, documentation, dashboard schemas, and Helm tests. ChangesNode timing configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant KubernetesTemplates
participant MachineDeployment
participant MachineHealthCheck
HelmValues->>KubernetesTemplates: Provide timing configuration
KubernetesTemplates->>KubernetesTemplates: Validate maxNodeProvisionTime
KubernetesTemplates->>MachineDeployment: Render autoscaler annotation
KubernetesTemplates->>MachineHealthCheck: Render readiness timeouts
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-18T18:16:07Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat api/apps/v1alpha1/.golangci.yml: no such file or directory: range error: stat api/apps/v1alpha1/.golangci.yml: no such file or directory Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 of 11 tasks
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Aug 18, 2026
…nder time (#3901) ## What this PR does Follow-up to #3898, closing two gaps in the render-time duration guard it introduced. Context: #3513. First, the guard now also covers the MachineHealthCheck condition timeouts (`readyUnknownTimeout`, `readyFalseTimeout`) in both the kubernetes and kubernetes-nodes charts. They need it more than the autoscaler annotation does: the CAPI webhook validates `nodeStartupTimeout` with a 30s floor but does not validate `unhealthyConditions` timeouts at all, so a zero or negative value used to apply silently and remediate a Machine the moment its Ready condition flipped, which for a KubeVirt worker means deleting the VM. Second, the guard accepts the canonical form a Go duration serializes back to ("20m0s", "1h0m0s"). `metav1.Duration` prints that shape, so a value copied from `kubectl get -o yaml` output previously failed the render. The first segment must still be positive and later segments may be zero, so every accepted string still parses to a positive duration and the overflow and fraction-to-zero rejections stay. Mechanically, the guard moves into a `kubernetes.positiveDuration` named template in each chart's `_helpers.tpl`, and the error message format is unchanged. Tests cover the canonical form on all three fields and the rejection of unitless, zero, and negative condition timeouts in both charts. The previous test case that passed unitless values through verbatim is reworked into rejections: a unitless value used to fail only at CAPI decode on apply, and a zero or negative one did not fail anywhere. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(kubernetes): reject zero and negative MachineHealthCheck condition timeouts at render time, and accept the canonical Go duration form ("20m0s", "1h0m0s") for the worker join timer values ```
github-actions
Bot
added
area/kubernetes
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
1 participant
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.
What this PR does
Worker node join on nested tenants regularly outruns the timer budgets the charts shipped, and the timers then destroy capacity that was about to arrive: the cluster-autoscaler treats a machine as long-unregistered after its built-in 15m default and orders a replacement, and the MachineHealthCheck deletes a Machine that is still legitimately booting. The timer audit in #3513 (#3513 (comment)) walked every timer on the join path against the cozyreport artifact of a red run; this PR sizes the budgets from those measurements and makes the hardcoded ones configurable. Context: #3513.
Three changes, one commit each:
cluster.x-k8s.io/autoscaling-options-maxnodeprovisiontimeannotation, rendered from a newmaxNodeProvisionTimevalue defaulting to "30m". The autoscaler starts this clock when the machine enters its node-instances cache with a real providerID, not at Machine creation, and on a value it cannot parse it silently falls back to its 15m default rather than failing, so the template validates the value at render time and rejects anything that does not parse to a positive duration.unhealthyConditionstimeouts become values (readyUnknownTimeoutdefault "5m",readyFalseTimeoutdefault "10m"; nested undernodeHealthCheckin the kubernetes chart, flat in kubernetes-nodes) instead of the hardcoded 30s and 300s. 30 seconds ofReady=Unknowndeletes a worker whose guest was merely starved of CPU, and 300 seconds ofReady=Falseis less than a CNI rollout takes on a loaded host.nodeStartupTimeoutdefault rises from "10m" to "20m". The MachineHealthCheck measures it from the later of the Machine's creation and its InfrastructureReady transition, so the clock restarts when the infrastructure reports ready: a worker gets up to 20m for the disk image import and the VM to come up, and then up to 20m again for the guest boot and kubelet registration. ThemaxUnhealthydocs now also spell out the round-down trap in small groups: at "50%" a group of one never remediates.The budgets are coherent with the measured phases: in the audited runs a healthy disk import took 2m22s and import plus VM start 2m53s, while the stuck runs spent over 17 minutes holding a running VM with no Node, so 20m per MachineHealthCheck leg and 30m on the autoscaler clock absorb that shape of slowness without letting a genuinely dead machine linger much longer.
New helm-unittest suites cover the annotation rendering, the render-time duration guard (including the values the guard exists for: unitless numbers, unknown units, zero, negatives, parser overflow), and the MachineHealthCheck timeout rendering in both charts. The render-parity test no longer pins the MachineHealthCheck values on either side and instead compares the two charts' defaults, so a future drift between parent and child defaults fails the suite.
Downstream repositories
Release note