fix(kubernetes): make MachineHealthCheck maxUnhealthy configurable - #2935
fix(kubernetes): make MachineHealthCheck maxUnhealthy configurable#2935myasnikovdaniil wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesmaxUnhealthy field for NodeGroup MachineHealthCheck
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a configurable Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new maxUnhealthy configuration option for Kubernetes node groups, allowing users to specify the maximum number or percentage of unhealthy nodes tolerated before auto-remediation stops. This option is integrated across API types, Helm templates, values schemas, and documentation. Feedback on the changes highlights a bug in the Helm template where using the default function overrides a valid 0 value with "100%", and suggests using hasKey to correctly preserve falsy values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| spec: | ||
| clusterName: {{ $.Release.Name }} | ||
| maxUnhealthy: 0 | ||
| maxUnhealthy: {{ $group.maxUnhealthy | default "100%" | quote }} |
There was a problem hiding this comment.
Using the default function in Helm/Go templates treats falsy values like 0 (integer) as empty, which causes them to be overridden by the default value ("100%"). Since 0 is a valid value for maxUnhealthy (used to explicitly disable auto-remediation), this will prevent users from setting maxUnhealthy: 0 as an integer in their values.
Using hasKey allows us to check if the key is explicitly defined in the node group map, preserving 0 or any other falsy values.
maxUnhealthy: {{ if hasKey $group "maxUnhealthy" }}{{ $group.maxUnhealthy | quote }}{{ else }}"100%"{{ end }}The MachineHealthCheck was hardcoded to maxUnhealthy: 0. In Cluster API semantics this blocks all remediation: as soon as a single machine is unhealthy the threshold is exceeded, so crashed worker nodes are never auto-replaced. Expose maxUnhealthy per node group (integer or percentage) with a default of "100%" so auto-remediation works out of the box while staying tunable for sensitive pools. Signed-off-by: mattia-eleuteri <mattia@hidora.io>
6a9047d to
dd1a6b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/apps/v1alpha1/kubernetes/types.go (1)
255-257: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding validation for the maxUnhealthy format.
The
MaxUnhealthyfield accepts either an integer or a percentage string, but there's no kubebuilder validation to enforce this format. Adding a pattern validation would provide better user experience by catching invalid values at admission time rather than later during reconciliation.📋 Proposed validation pattern
// Max unhealthy nodes (integer or percentage) tolerated before the MachineHealthCheck stops auto-remediation. // +kubebuilder:default:="100%" +// +kubebuilder:validation:Pattern=`^([0-9]+|[0-9]+%)$` MaxUnhealthy string `json:"maxUnhealthy,omitempty"`This pattern ensures the value is either:
- An integer:
0,1,10, etc.- A percentage:
0%,50%,100%, etc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/apps/v1alpha1/kubernetes/types.go` around lines 255 - 257, The MaxUnhealthy field currently lacks format validation, allowing invalid values to pass through. Add a kubebuilder validation pattern constraint to the MaxUnhealthy field definition to enforce that the value must be either a non-negative integer or a percentage string (ending with %). This will catch formatting errors at admission time by ensuring only valid formats like "100", "50%", or "100%" are accepted, preventing invalid values from reaching reconciliation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/apps/v1alpha1/kubernetes/types.go`:
- Line 26: The kubebuilder default annotation contains JSON syntax errors for
array-type fields. The `roles` field currently uses object notation
`{"ingress-nginx"}` but must use array notation `["ingress-nginx"]` to match the
`Roles` field definition as `[]string` (line 264). Similarly, the `gpus` field
currently uses empty object notation `{}` but must use empty array notation `[]`
to match the `Gpus` field definition as `[]GPU` (line 246). Update both field
notations in the default annotation to use proper JSON array syntax with square
brackets instead of curly braces.
---
Nitpick comments:
In `@api/apps/v1alpha1/kubernetes/types.go`:
- Around line 255-257: The MaxUnhealthy field currently lacks format validation,
allowing invalid values to pass through. Add a kubebuilder validation pattern
constraint to the MaxUnhealthy field definition to enforce that the value must
be either a non-negative integer or a percentage string (ending with %). This
will catch formatting errors at admission time by ensuring only valid formats
like "100", "50%", or "100%" are accepted, preventing invalid values from
reaching reconciliation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f3ca1b7-0d88-464c-8bba-c3407b2c9a6d
📒 Files selected for processing (6)
api/apps/v1alpha1/kubernetes/types.gopackages/apps/kubernetes/README.mdpackages/apps/kubernetes/templates/cluster.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/kubernetes/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/apps/kubernetes/values.yaml
- packages/apps/kubernetes/templates/cluster.yaml
- packages/system/kubernetes-rd/cozyrds/kubernetes.yaml
- packages/apps/kubernetes/values.schema.json
| StorageClass string `json:"storageClass"` | ||
| // Worker nodes configuration map. | ||
| // +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":{},"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"minReplicas":0,"resources":{},"roles":{"ingress-nginx"},"storageClass":""}} | ||
| // +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":{},"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":{"ingress-nginx"},"storageClass":""}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the kubebuilder default annotation contains valid JSON matching the NodeGroup type
# Extract the default JSON from line 26
default_json='{"md0":{"diskSize":"20Gi","gpus":{},"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":{"ingress-nginx"},"storageClass":""}}'
# Test JSON validity
echo "$default_json" | jq empty 2>&1
# Check if the roles field is an array
echo "$default_json" | jq '.md0.roles | type' 2>&1Repository: cozystack/cozystack
Length of output: 213
🏁 Script executed:
cat -n api/apps/v1alpha1/kubernetes/types.go | sed -n '20,35p'Repository: cozystack/cozystack
Length of output: 1070
🏁 Script executed:
cat -n api/apps/v1alpha1/kubernetes/types.go | sed -n '255,270p'Repository: cozystack/cozystack
Length of output: 1258
🏁 Script executed:
cat -n api/apps/v1alpha1/kubernetes/types.go | sed -n '245,255p'Repository: cozystack/cozystack
Length of output: 674
Fix JSON syntax in the kubebuilder default annotation — roles and gpus must use array notation.
The default JSON uses object syntax {"ingress-nginx"} for the roles field but Roles is defined as []string at line 264, and similarly gpus uses {} but Gpus is defined as []GPU at line 246. This causes JSON parsing to fail and breaks CRD generation.
🔧 Fix
-// +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":{},"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":{"ingress-nginx"},"storageClass":""}}
+// +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":[],"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":["ingress-nginx"],"storageClass":""}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":{},"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":{"ingress-nginx"},"storageClass":""}} | |
| // +kubebuilder:default:={"md0":{"diskSize":"20Gi","gpus":[],"instanceType":"u1.medium","kubelet":{},"maxReplicas":10,"maxUnhealthy":"100%","minReplicas":0,"resources":{},"roles":["ingress-nginx"],"storageClass":""}} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/apps/v1alpha1/kubernetes/types.go` at line 26, The kubebuilder default
annotation contains JSON syntax errors for array-type fields. The `roles` field
currently uses object notation `{"ingress-nginx"}` but must use array notation
`["ingress-nginx"]` to match the `Roles` field definition as `[]string` (line
264). Similarly, the `gpus` field currently uses empty object notation `{}` but
must use empty array notation `[]` to match the `Gpus` field definition as
`[]GPU` (line 246). Update both field notations in the default annotation to use
proper JSON array syntax with square brackets instead of curly braces.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM (REQUEST_CHANGES)
Business context: a hardcoded maxUnhealthy: 0 on the worker MachineHealthCheck disabled auto-remediation; this PR makes it a per-node-group setting defaulting to 100% to restore it.
The core fix is correct — defaulting to 100% restores auto-remediation and is safe for users who never set the field. My objection is that the new configuration surface does not match its own documentation.
Blocker
1. The field is documented as "integer or percentage", but only percentages actually work.
- It is typed
string(api/apps/v1alpha1/kubernetes/types.go:257,packages/apps/kubernetes/values.schema.json), so an integer value is rejected at schema validation before rendering. Evidence:helm template . --set nodeGroups.md0.maxUnhealthy=5→values don't meet the specifications of the schema(s) ... at '/nodeGroups/md0/maxUnhealthy': got number, want string. - The template force-quotes the value (
packages/apps/kubernetes/templates/cluster.yaml:591→{{ $group.maxUnhealthy | default "100%" | quote }}), so even a quoted integer like"3"renders as the string"3". Cluster API modelsmaxUnhealthyas an IntOrString and only interpretsN%strings as a threshold; a non-percentage string is not a valid node count. Evidence:packages/apps/kubernetes/templates/cluster.yaml:591. - The "Max unhealthy nodes (integer or percentage)" wording ships in
api/apps/v1alpha1/kubernetes/types.go:256,packages/apps/kubernetes/README.md,packages/apps/kubernetes/values.yaml:82, and the generatedpackages/system/kubernetes-rd/cozyrds/kubernetes.yamlschema, so a user who follows the docs and sets an integer hits a hard error. - Coupled defect:
| default "100%"treats an explicit integer0as empty (Sprigdefaultsemantics), somaxUnhealthy: 0(deliberately disabling remediation) is silently replaced by100%. Evidence:packages/apps/kubernetes/templates/cluster.yaml:591. AhasKey $group "maxUnhealthy"guard would preserve0. - Fix: model the field as int-or-string (
x-kubernetes-int-or-string, and drop the unconditional| quote), or restrict it to percentages in both docs and schema and add apattern.
Non-blocking
1. No format validation on the value. values.schema.json types maxUnhealthy as a bare string with no pattern, so values like "abc" pass admission and fail only at Cluster API apply time. Add a pattern that accepts an integer or N%. Evidence: packages/apps/kubernetes/values.schema.json maxUnhealthy property.
2. No test coverage for the new MachineHealthCheck value. The chart ships helm-unittest suites that render the MachineHealthCheck document, but none assert maxUnhealthy; chart changes here are expected to update fixtures in lockstep. Add an assertion for the default ("100%") and for an overridden value. Evidence: packages/apps/kubernetes/tests/cluster_test.yaml (all 129 tests pass, none reference maxUnhealthy).
|
Thanks for this mattia-eleuteri! Since it was opened, the Talos worker bootstrap work (#2931) introduced a cluster-wide Closing as superseded by #3053 — thank you for the original proposal. |
Adds per-node-group overrides for the worker MachineHealthCheck tuning introduced as a cluster-wide `nodeHealthCheck` by #2931. **What:** optional `nodeGroups[name].maxUnhealthy` and `nodeGroups[name].nodeStartupTimeout`; when unset a group inherits the cluster-wide `nodeHealthCheck.*` (no behaviour change). The per-group value reuses the existing int-or-string validation; an invalid override fails the render naming the offending group. **Why:** the MachineHealthCheck is rendered per node group, so groups can warrant different remediation tolerances (e.g. a stateful group at `0%`, stateless at `50%`). This is the per-group granularity of the standalone proposal (#2752 / #2935), rebuilt on top of #2931's `nodeHealthCheck`. **Notes:** stacked on `phase1-talos-migration` (#2931) — retarget to `main` once it merges. Supersedes #2935 (original author credited as co-author). helm-unittest covers per-group override (percent + bare int), `nodeStartupTimeout` override, fallback-when-unset, and invalid-override rejection. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added per-node-group overrides for health-check settings: `maxUnhealthy` and `nodeStartupTimeout`, with automatic fallback to cluster-wide `nodeHealthCheck` when unset. * **Bug Fixes** * Improved `maxUnhealthy` parsing/rendering to support both bare integers and percentage strings, including more precise validation errors for the exact override path. * **Documentation** * Updated kubernetes app docs and values schemas (including CRD chart-values schema) to document the new override fields. * **Tests** * Added MachineHealthCheck rendering tests for override, fallback, and invalid `maxUnhealthy` (e.g., `"abc"`) cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Same-repo mirror of #2752 by mattia-eleuteri, opened so the
BuildCI job can run. Fork PRs skip the OCIR registry login (if: !github.event.pull_request.head.repo.forkinpull-requests.yaml) and so can't push the per-PR images the build needs — meaning a fork PR can never passBuild, regardless of the change. The commits here are unchanged and authored by mattia-eleuteri; full description and discussion in #2752.Supersedes #2752.
Summary by CodeRabbit
New Features
nodeGroups[].maxUnhealthysetting (integer or percentage) to control how many unhealthy nodes are tolerated before automatic remediation stops; default is now"100%".Documentation
maxUnhealthyand its default.Chores
maxUnhealthyvalue.