fix(kubernetes): make the default md0 node group removable - #2936

Code Review

This pull request updates the Kubernetes application chart to change the default value of nodeGroups to an empty map ({}). The default md0 node group is now dynamically applied in the templates via a new helper (kubernetes.nodeGroups) only when no user-defined node groups are provided, preventing Helm merge issues. To support existing clusters and prevent their md0 node groups from being pruned on upgrade, a new platform migration script (Migration 44 -> 45) has been added to explicitly pin md0 on active clusters. The review feedback suggests minor shell script improvements in the migration script, specifically using the --patch flag for kubectl patch instead of piping to /dev/stdin, and using the more idiomatic -f - flag for kubectl apply.

Actionable comments posted: 3

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/core/platform/images/migrations/migrations/44`:
- Line 65: The current jq filter in the patch variable assignment generates a
merge patch containing the entire pre-read .spec.nodeGroups map, which can
overwrite concurrent edits to other node groups made by other actors between the
read and patch operations. Modify the jq filter to extract and patch only the
md0 node group from .spec.nodeGroups instead of the entire nodeGroups map. This
ensures the patch operation is minimal and only adds or updates md0 without
risking loss of concurrent modifications to other node groups.
- Around line 55-56: The current kubectl command silently suppresses all errors
with 2>/dev/null and || true, which means transient API or RBAC failures are
indistinguishable from a successfully deleted object. This allows the migration
to continue to version 45 without properly pinning md0 when the actual failure
cause was a transient issue. You need to separate the kubectl exit code from the
output check: capture the kubectl output in a variable while preserving its exit
code, then explicitly check whether the command succeeded before deciding to
skip the object. Only increment SKIPPED and return 0 if kubectl succeeded and
returned empty output (meaning the object was genuinely deleted), otherwise
propagate the error so the migration stops and alerts the operator to the
API/RBAC problem.
- Line 1: Change the shebang from `#!/bin/bash` to `#!/bin/sh` and rewrite the
migration script using POSIX-compatible syntax. Specifically, replace the
Bash-only syntax constructs: replace the tab character escape `$'\t'` at line 85
with a literal tab character (or printf equivalent), and replace the here-string
operator `<<<` at line 88 with a here-document or pipe approach. Additionally,
modify the error handling around lines 55-56 where `kubectl get` errors are
silently suppressed with `2>/dev/null` to distinguish between a missing CRD
(which is safe to skip) and actual API call failures (which should trigger a
retry or explicit error). Finally, at line 65, replace the merge patch operation
on the entire `.spec.nodeGroups` object with a strategic merge patch or
individual field patches to avoid overwriting concurrent edits to other node
groups made by users or controllers.

NOT LGTM (REQUEST_CHANGES)

Business context

The kubernetes app baked a default md0 node group into values.yaml, which Helm merged into every cluster and could not be removed; this PR moves the default into a kubernetes.nodeGroups helper applied only when nodeGroups is empty, and adds migration 44 to pin md0 explicitly on existing clusters so the chart change does not prune their live MachineDeployment.

Migration activation is correct: migrations.targetVersion is bumped 44 → 45, which activates /migrations/44 (not dead code). Evidence: packages/core/platform/values.yaml:9. The helper change is also complete — every template that iterated nodeGroups now routes through the helper, so no template renders an empty worker set. Evidence: packages/apps/kubernetes/templates/cluster.yaml:240, packages/apps/kubernetes/templates/dashboard-resourcemap.yaml:33.

Blockers

B1 — Version stamp drops the platform.cozystack.io/no-delete label

• File: packages/core/platform/images/migrations/migrations/44:102-104
• Evidence: the stamp is kubectl create configmap --namespace cozy-system cozystack-version --from-literal version=45 --dry-run=client --output yaml | kubectl apply --filename - — a label-less manifest. Migrations 42 and 43 both stamp via an inline manifest carrying labels: { platform.cozystack.io/no-delete: "true" }, and migrations/43 documents exactly this case: "A label-less apply by the same field manager ... would strip the platform.cozystack.io/no-delete label."
• Impact: applied by the same field manager, the label-less manifest strips no-delete from the cozystack-version ConfigMap, removing it from the deletion guardrail. Upgrade-path only, so E2E does not exercise it.
• Fix: stamp version 45 with the labeled inline manifest used by migrations/43.

B2 — Fail-open on patch/read failure lets the destructive chart change proceed

• File: packages/core/platform/images/migrations/migrations/44:55-56,97-100
• Evidence: the per-object read uses kubectl get ... 2>/dev/null || true, turning any non-NotFound error into a SKIP (lines 55-56); and on FAILURES > 0 the script logs a WARN and exit 0 (lines 97-100), so the pre-upgrade hook succeeds even when some clusters were not pinned.
• Impact: with the hook green, the platform upgrade completes and the new kubernetes chart rolls out to tenant clusters. An unpinned cluster (CR still without md0) then has its live md0 MachineDeployment pruned (→ CAPI Machine/node deletion) — the exact outcome this migration exists to prevent. Leaving the stamp at 44 for retry does not help, because the chart has already rolled out before the next upgrade.
• Fix: treat non-NotFound read errors as failures rather than skips, and make the failure path block the chart change (fail the hook) instead of completing the upgrade with clusters left unpinned.

Non-blocking

1. No test covers the empty-nodeGroups → md0 default. Evidence: packages/apps/kubernetes/tests/values-ci.yaml changed from nodeGroups: null to an explicit md0, and no tests/*_test.yaml asserts that an empty nodeGroups renders the default md0 MachineDeployment. The new helper's core behavior is therefore unverified by helm-unittest.

NOT LGTM — both prior blockers are genuinely fixed and well-tested; one gap remains: the fail-closed safety branch (which is what prevents data loss) has no automated test, and it is testable with a pattern this repo already uses.

Credit where due — the fixes are solid and verified:

• B1 (version stamp dropped the no-delete label): fixed. Migration 45 stamps via the shared stamp_cozystack_version helper (:131), which emits the ConfigMap carrying platform.cozystack.io/no-delete, and the cozystack-version-stamp.bats suite structurally enforces that every migration stamps through that helper, so it can't regress to a label-less apply.
• B2 (fail-open let the destructive chart change proceed): fixed. The migration is now fail-closed — set -euo pipefail (:23), per-object failures counted instead of skipped (:73, :103), list errors abort (:108), and FAILURES > 0 exits 1 before the version stamp (:121-123), so the upgrade halts and Helm never rolls the new chart against clusters whose md0 isn't pinned.
• The empty-nodeGroups -> md0 default is now pinned by tests/nodegroups_default_test.yaml, run under make unit-tests.

Blocker

B1: the fail-closed abort path has no regression test

The FAILURES > 0 -> exit 1 before the stamp branch is the entire safety mechanism of this migration — if it ever regresses to fail-open, the destructive chart change proceeds against un-pinned clusters and their live md0 MachineDeployment is pruned (the exact data loss this PR exists to prevent). That branch is currently verified by reading only. It is testable with the kubectl-mock bats pattern this repo already uses in cozystack-version-stamp.bats.

Fix: add a bats case that mocks a kubectl get/patch failure and asserts the migration exits non-zero before the version stamp is written (i.e. CURRENT_VERSION is not advanced). A negative helm-unittest (a custom nodeGroups without md0 renders no *-md0 MachineDeployment) would also be worth adding to pin the removability branch directly, but the fail-closed test is the one that guards against data loss.

Actionable comments posted: 3

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 `@hack/e2e-apps/run-kubernetes.sh`:
- Around line 154-170: The new kubectl wait calls in run-kubernetes.sh can fail
early with NotFound if the target resources are not created yet, so add the same
existence backstop pattern used earlier in the script before each wait. Update
the sections around kubernetes-${test_name},
kubernetes-${test_name}-cluster-autoscaler/kccm/kcsi-controller, and
kubernetes-${test_name}-md0 to first wait for object existence, then run the
existing condition waits. Keep the fix aligned with the existing event-driven
check style already present in the script.
- Around line 730-761: The server-side dry-run fallback in run-kubernetes.sh
writes stderr to a predictable /tmp path, which can be tampered with by other
processes. Replace the hardcoded /tmp/sc-fallback-render.err usage in the helm
install block with a unique tempfile created via mktemp, store it in a variable,
and use that variable consistently for both the redirected stderr and the later
cat/readback. Make sure the cleanup path removes the tempfile after use and keep
the existing rc/raw handling intact.

In `@packages/core/platform/images/migrations/migrations/47`:
- Around line 116-123: The dry-run path in the migration script is returning
success before checking whether any reads or patches failed, so non-NotFound
errors can be hidden. Update the logic around the DRY_RUN and FAILURES checks in
the migration script for version 47 so dry-run still reports failures from
patch_md0() and only exits 0 when FAILURES is zero; keep the existing
version-stamp gating behavior intact.

---

Nitpick comments:
In `@hack/kubernetes-md0-migration.bats`:
- Around line 85-154: Add a regression test for the dry-run path in the
kubernetes migration suite, since `MIGRATION_DRY_RUN=1` is currently untested.
Extend the `migration 47` cases in `hack/kubernetes-md0-migration.bats` with a
`MOCK_FAIL=read` dry-run scenario using `write_fake_kubectl` and the `MIGRATION`
runner, and assert the script still exits appropriately without advancing the
version stamp. Make sure the new test specifically covers the
success-on-read-failure branch so the dry-run behavior stays locked down.

LGTM — the remaining blocker is resolved and the migration's correctness re-verified after the rebase to /47.

The fail-closed abort path now has a regression test: hack/kubernetes-md0-migration.bats puts a fake kubectl on PATH and asserts that a read failure and a patch failure each abort non-zero WITHOUT the version stamp ever running, with a clean-run positive control so the "no stamp" assertions aren't vacuous. It is a unit bats (not e2e-prefixed), so BATS_UNIT_FILES picks it up under make unit-tests, and it respects the cozytest.sh constraints (no run/$status, no column-0 }, inline cleanup).

Re-verified after the renumber to /47:

• Activation is correct: targetVersion 47→48 activates migration /47, and main is at 46/47 so there is no /47 collision.
• Fail-closed survived the rebase: set -euo pipefail, per-object failures counted rather than skipped, list errors abort, and FAILURES>0 -> exit 1 before the stamp; the stamp goes through stamp_cozystack_version 48, so the cozystack-version ConfigMap keeps the platform.cozystack.io/no-delete label.
• patch_md0 is data-safe: it distinguishes NotFound (skip) from other read errors (failure), and patches only {spec:{nodeGroups:{md0}}}, so a concurrent edit to sibling node groups is not clobbered.
• No live drift: the injected MD0_JSON is byte-identical to both the new kubernetes.nodeGroups helper default and the previous baked values.yaml md0, so pinning an existing cluster leaves its md0 MachineDeployment unchanged.