fix(kubernetes): make MachineHealthCheck maxUnhealthy configurable#2935
fix(kubernetes): make MachineHealthCheck maxUnhealthy configurable#2935myasnikovdaniil wants to merge 1 commit into
Conversation
github-actions
Bot
added
area/kubernetes
📝 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 |
github-actions
Bot
added
the
size/S
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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>
myasnikovdaniil
force-pushed
the
fix/kubernetes-mhc-maxunhealthy
branch
from
6a9047d to
dd1a6b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
2 participants
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.