feat(kubernetes): per-node-group nodeHealthCheck overrides - #3053

Code Review

This pull request introduces per-group overrides for maxUnhealthy and nodeStartupTimeout in the Kubernetes application package, allowing individual node groups to override cluster-wide health check settings. The changes span API types, Helm templates, schemas, documentation, and tests. The review feedback identifies potential template rendering failures in cluster.yaml when overrides are explicitly set to empty strings or nil, and suggests more robust checks. Additionally, the reviewer noted that the PR description needs to include a release-note block to comply with the repository guidelines.

LGTM — the per-node-group nodeHealthCheck override flow is correct and well-tested.

Verified:

• The MachineHealthCheck renders inside range $groupName, $group := .Values.nodeGroups, and its selector targets cluster.x-k8s.io/deployment-name: <release>-<groupName>, so each override applies only to its own group with no cross-group leakage.
• When a group omits the field, hasKey falls back cleanly to the cluster-wide nodeHealthCheck.* — no nil-deref, no behaviour change for existing clusters.
• maxUnhealthy reuses the existing IntOrString handling: percentages quoted, bare integers unquoted, invalid input fails the render with a message naming the offending group.
• Schema is fully in sync across types.go, values.yaml, values.schema.json, README, and the embedded openAPISchema; the codegen-drift check is green.
• helm-unittest covers percent override, bare-int override, nodeStartupTimeout override, fallback-when-unset, and invalid-input rejection.

One non-blocking robustness point before merge: hasKey alone treats an explicitly-empty value as "set", so a group with maxUnhealthy: "" (or null) would hard-fail the render and nodeStartupTimeout: "" would render a null timeout. The sibling optional-string field storageClass already guards this with {{- with .group.storageClass }} (and values.yaml ships storageClass: ""), so the empty value is a shape the chart elsewhere expects. Suggest matching that precedent — and (hasKey $group "maxUnhealthy") (ne (toString $group.maxUnhealthy) "") (same for nodeStartupTimeout) — plus a small maxUnhealthy: "" → falls back test.

The failing E2E run is unrelated — it dies on LINSTOR controller connectivity during management-cluster bootstrap, before any tenant MachineHealthCheck is rendered; a rerun should clear it.

Actionable comments posted: 1

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 `@packages/apps/kubernetes/templates/cluster.yaml`:
- Around line 680-698: Empty node-group overrides are being treated as real
values, which breaks fallback to nodeHealthCheck defaults. Update the cluster
template logic around maxUnhealthy and nodeStartupTimeout so the per-group value
is used only when it is non-empty, not just when hasKey is true; keep the
existing validation/rendering paths in the same template block and preserve
fallback to $.Values.nodeHealthCheck.* for empty strings. Add a regression test
covering nodeGroups.<name>.maxUnhealthy and nodeGroups.<name>.nodeStartupTimeout
set to empty strings to confirm they inherit the global defaults.

Approve — strictly opt-in and additive.

make-generate artifacts (values.yaml, values.schema.json, README, types.go, cozyrds openAPISchema) are all in sync, the per-field fallback is correct (setting only maxUnhealthy keeps the inherited nodeStartupTimeout, no accidental null-out), and upgrade / fresh-install are a no-op.

Non-blocking, but worth resolving since all three prior reviews raised it and it's still open on the current tip:

• cluster.yaml:682,697 use a bare hasKey, so an explicit maxUnhealthy: "" / nodeStartupTimeout: "" is treated as "set" — the former hard-fails the render, the latter emits an empty field instead of inheriting. Match the sibling storageClass guard: and (hasKey $group "x") (ne (toString $group.x) ""), plus a "" falls back test.
• nodeStartupTimeout renders unquoted (cluster.yaml:698); a unitless numeric override becomes a YAML int and the CAPI MachineHealthCheck webhook rejects it (metav1.Duration is a string). Suggest | quote.
• PR body is missing the required release-note fenced block.

Verdict

LGTM with non-blocking notes

Purely additive, omitempty per-node-group overrides that fall back to the existing cluster-wide nodeHealthCheck via hasKey gating; no migration, no CRD tightening, no default flip, schema/README/openAPISchema regenerated consistently, and helm unittest passes 149/149. Two small consistency/hygiene notes below, none blocking.

Findings

[MINOR] packages/apps/kubernetes/templates/cluster.yaml:728-730 — per-group nodeStartupTimeout override has no render-time validation, unlike maxUnhealthy

The maxUnhealthy path validates the value (bare int or N%) and fails at render time with the offending source named. The sibling nodeStartupTimeout override is interpolated raw (nodeStartupTimeout: {{ $nodeStartupTimeout }}) with no format check, so a typo like nodeGroups.md0.nodeStartupTimeout: "10 minutes" is only rejected later by the CAPI MHC admission webhook (a worse failure surface than a chart-render error). This is not a regression — the cluster-wide nodeHealthCheck.nodeStartupTimeout already renders unvalidated the same way — so it is consistent with existing behaviour. Optional improvement: add a metav1.Duration-style regex check (^[0-9]+(s|m|h)$ or similar) mirroring the maxUnhealthy guard so both overrides fail fast at render with the group named.

Claim mismatches

[MISSING] release-note fenced block — CONTRIBUTING.md requires a release-note block in the PR body; the current body has What/Why/Notes prose but no fenced release-note block. Add one (e.g. feat(kubernetes): support per-node-group nodeHealthCheck maxUnhealthy / nodeStartupTimeout overrides).

Caveats

• Existing-customer upgrade (scenario A): verified safe. Both fields are new optional string members of NodeGroup (api/apps/v1alpha1/kubernetes/types.go:277,282). The schema required list on nodeGroups.additionalProperties is unchanged (["diskSize","instanceType","maxReplicas","minReplicas"]), so existing Kubernetes CRs in customer clusters re-admit unchanged. The template gates on hasKey $group "...", so clusters with no per-group values render identically to today — no migration script needed, migrations.targetVersion correctly untouched. No default flip, no RBAC/cozyrds surface change, no image bump.
• Fresh install (scenario B): verified safe. No new PackageSource/bundle wiring, no new .Values._cluster/_namespace keys, no cert-manager/CRD dependency, no new image references. make generate artifacts are all present and mutually consistent: types.go, values.yaml (@field annotations), values.schema.json, README.md, and the kubernetes-rd/cozyrds/kubernetes.yaml openAPISchema all carry the two new properties as optional strings.
• Edge case (non-blocking): setting a per-group override to an empty string (maxUnhealthy: "") makes hasKey true and then fails render (empty matches no branch). To inherit the cluster-wide value the operator must omit the key entirely, which the field docs ("When unset ... applies") describe correctly. Acceptable, but worth a docs mention if this bites operators.
• PR body says "stacked on phase1-talos-migration (#2931), retarget to main once it merges" — this is stale: the branch is already based on main and the commit sits on top of current main. No action needed, just note the body is out of date.