fix(platform): migrate ephemeralStorage to diskSize via pre-upgrade hook - #2688
Conversation
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 improves the upgrade experience for Cozystack by relaxing constraints on deprecated configuration fields. By removing the strict validation that caused template rendering to fail when 'ephemeralStorage' was present, the operator can now successfully reconcile existing Kubernetes application objects while transitioning to the newer 'diskSize' field. 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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds platform migration 41 to automatically rename ChangesephemeralStorage → diskSize migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
There was a problem hiding this comment.
Code Review
This pull request removes the migration guard for the deprecated ephemeralStorage field and updates tests to allow its presence without failing. However, the current implementation silently ignores the field, defaulting to 20Gi instead. Feedback indicates this is a regression and suggests using ephemeralStorage as a fallback for diskSize in the template and updating the tests to verify this behavior.
| {{- if hasKey .group "ephemeralStorage" }} | ||
| {{- fail (printf "nodeGroup %q: ephemeralStorage is no longer supported. Rename it to diskSize. See README.md for migration instructions." .groupName) }} | ||
| {{- end }} | ||
| source: |
There was a problem hiding this comment.
While removing the fail guard prevents upgrade blocks, the current implementation silently ignores ephemeralStorage. If a user has not yet migrated their configuration to use diskSize, their nodes will be provisioned with the default 20Gi disk (as defined in values.yaml), which could lead to data loss or service failure if the previous size was larger.
To provide true backward compatibility as suggested by the PR title, consider using ephemeralStorage as a fallback for diskSize in the template (line 46). For example:
storage: {{ .group.diskSize | default .group.ephemeralStorage | default "20Gi" | quote }}| - notFailedTemplate: {} | ||
| - equal: | ||
| path: spec.template.spec.virtualMachineTemplate.spec.dataVolumeTemplates[0].spec.storage.resources.requests.storage | ||
| value: "20Gi" |
There was a problem hiding this comment.
This test case asserts that the custom ephemeralStorage value (50Gi) is ignored and the default 20Gi is used instead. This confirms a regression for users who haven't migrated their configuration. If the template is updated to support ephemeralStorage as a fallback, this test should be updated to expect the provided value.
value: 50Gi
Arsolitt (Arsolitt)
left a comment
There was a problem hiding this comment.
NOT LGTM — the user-facing migration guide still says Helm rendering will fail on ephemeralStorage, but this PR removes that behavior and instead lets the value be silently dropped in favor of the 20Gi schema default. Shipping the new behavior without updating the docs will mislead anyone following the migration guide.
Business context: After upgrade, the operator pre-fills diskSize: "20Gi" from the schema's leaf default into existing Kubernetes applications. The old chart's {{- fail }} guard then blocked reconciliation indefinitely. This PR removes the guard so upgrades unblock. The accepted tradeoff is that nodeGroups carrying only ephemeralStorage silently fall back to the schema default — users must migrate manually to preserve their disk size.
Blockers
B1: Migration guide contradicts the new behavior
File: packages/apps/kubernetes/README.md:88
The "Breaking Changes" section still states:
There is no backward-compatibility fallback; users MUST update their configurations to use
diskSizeinstead ofephemeralStorage. IfephemeralStorageis still present in values, Helm template rendering will fail with an error directing you to usediskSize.
After this PR, both claims are wrong:
- Rendering no longer fails — the
{{- fail }}guard attemplates/cluster.yaml:41-43is removed. - There is a (dead-code) fallback expression in the chart, but the practical outcome is silent shrinkage to 20Gi for unmigrated configs.
A user reading the migration guide after this PR ships will:
- See no build error during upgrade → assume the migration is complete or non-urgent.
- Not realize their
ephemeralStorage: 50Giis silently coerced to20Gion next reconciliation. - Discover the regression only when CAPI replaces VMs with fresh 20Gi PVCs and nodes hit disk pressure or
ImagePullBackOff.
Evidence: README.md:88 contradicts cluster.yaml:46. Reproduced: helm template with {nodeGroups.md0: {diskSize: "20Gi", ephemeralStorage: "100Gi"}} renders storage: "20Gi". The operator path that makes the chart's coalesce fallback unreachable: pkg/registry/apps/application/rest_defaulting.go:101-105 (fills absent property from ps.Default.Object) → values.schema.json:40 ("default": "20Gi" for diskSize).
Fix: Update README.md:88 to honestly describe the new behavior. Suggested wording (adapt as appropriate):
ephemeralStorageis deprecated and silently ignored: SettingephemeralStorageon anodeGroupno longer blocks upgrades, but it does NOT preserve the disk size — the field is dropped anddiskSizedefaults to20Giunless set explicitly. Any cluster that previously relied onephemeralStorageto set a non-default size MUST migrate todiskSizebefore upgrading; otherwise the next reconciliation will provision smallerdisk-kubeletPVCs (via CAPI rolling update) and existing nodes may hit disk pressure orImagePullBackOff.
The PR description ("ephemeralStorage is the fallback when diskSize is absent") should be corrected in the same spirit — that statement is false because the operator's schema defaulting fills diskSize first.
Non-blocking follow-ups
-
Simplify the chart expression.
coalesce .group.diskSize .group.ephemeralStorage "20Gi"attemplates/cluster.yaml:46reads as a working fallback but never reaches theephemeralStoragebranch in practice — the operator's schema defaulting (pkg/registry/apps/application/rest_defaulting.go:101-105) fillsdiskSizefromvalues.schema.json:40's"default": "20Gi"before the chart sees the values. Reducing to{{ .group.diskSize | default "20Gi" | quote }}produces identical behavior without implying a fallback that doesn't exist. -
Make the silent-drop test self-documenting.
tests/cluster_test.yaml:349-360is named "uses diskSize default (20Gi) when ephemeralStorage is set alongside default diskSize", which reads like a benign default — but the actual behavior is silent loss of the user's50Gi. A more explicit name and a comment would help future readers see this is documented design, not an oversight. Suggested:"ephemeralStorage is silently dropped — users with non-default sizes must migrate to diskSize to preserve them". -
Complete the empty-string test.
tests/cluster_test.yaml:362-370only assertsnotFailedTemplate: {}. Adding anequalassertion on the final storage value would make the empty-string semantics explicit.
Note: Gemini Code Assist flagged the silent-ignore concern against the first commit before coalesce was added. This review adds new evidence — the operator-side schema defaulting in rest_defaulting.go:101-105 makes the coalesce fallback structurally unreachable, which the bot did not examine.
| resources: | ||
| requests: | ||
| storage: {{ .group.diskSize | default "20Gi" | quote }} | ||
| storage: {{ coalesce .group.diskSize .group.ephemeralStorage "20Gi" | quote }} |
There was a problem hiding this comment.
Non-blocking nit: the ephemeralStorage branch of this coalesce is unreachable — the operator's schema defaulting (pkg/registry/apps/application/rest_defaulting.go:101-105) fills diskSize from values.schema.json:40's "default": "20Gi" before the chart sees the values. Consider simplifying to {{ .group.diskSize | default "20Gi" | quote }} to avoid suggesting a fallback that never triggers. The silent-drop-to-20Gi behavior is the documented tradeoff of this PR; this comment is about clarity of the expression, not the design choice.
| - notFailedTemplate: {} | ||
| - equal: | ||
| path: spec.template.spec.virtualMachineTemplate.spec.dataVolumeTemplates[0].spec.storage.resources.requests.storage | ||
| value: "20Gi" |
There was a problem hiding this comment.
Non-blocking nit: this assertion documents a deliberate behavior — the user set ephemeralStorage: 50Gi and gets back 20Gi, i.e. the value is silently lost — but the test name reads as if it's testing a benign default. A clearer name like "ephemeralStorage is silently dropped — users must migrate to diskSize to preserve size" plus a YAML comment would make the intent obvious to future readers.
Arsolitt (Arsolitt)
left a comment
There was a problem hiding this comment.
LGTM — all blockers and follow-ups from the previous review are addressed in 4f6b3c6.
- B1 (README contradicts new behavior):
README.md:88now honestly describes the silent-drop behavior, names the operator-side cause, and includes the explicit "MUST set diskSize before upgrading" warning with the disk-pressure / ImagePullBackOff failure mode. - Chart simplification:
coalescereverted to{{ .group.diskSize | default "20Gi" | quote }}— same behavior, no false fallback in the source. - Silent-drop test naming and comments: explicit and self-documenting (
"ephemeralStorage is silently dropped — diskSize defaults to 20Gi regardless"+ clarifying comment block). - Empty-string test: now asserts the final storage value, not just
notFailedTemplate.
Verified locally: helm unittest tests/cluster_test.yaml — 21 passed; helm template with ephemeralStorage: 50Gi renders storage: "20Gi", matching the documented behavior.
4f6b3c6 to
016af88
Compare
Arsolitt (Arsolitt)
left a comment
There was a problem hiding this comment.
LGTM — solid migration design that follows the established #39 pattern. Three coordination concerns worth surfacing before merge; none of them are correctness bugs in the code itself.
Business context: PR #2454 (v1.4.0) renamed nodeGroups[*].ephemeralStorage → nodeGroups[*].diskSize with a hard {{ fail }} guard. On clusters with existing tenant Kubernetes apps still carrying the legacy field, the chart fails to render at all — so the tenant kubernetes HelmRelease cannot reconcile, blocking unrelated control-plane updates and MachineHealthCheck remediations. This PR adds platform migration 40 as a pre-upgrade hook that walks every kuberneteses.apps.cozystack.io CR cluster-wide and renames the field in-place, while keeping the chart-side guard as a safety net (with an updated message that points operators at the migration job logs).
Non-blocking follow-ups
-
Migration slot 40 is contested with PR #2650. PR #2650 (
feat(platform): add deletion-protection guardrail via ValidatingAdmissionPolicy) is also OPEN and adds a different migration at the exact same path: commitcc107fe54writespackages/core/platform/images/migrations/migrations/40(backfillsplatform.cozystack.io/no-delete=trueon the cozystack-version ConfigMap) and bumpsmigrations.targetVersion40 → 41 in the same line ofvalues.yaml. Hard conflict at merge time. Whichever PR lands first owns slot 40; the second must renumber to 41. For this PR specifically the renumber would rename the script, bumpvalues.yaml:9(targetVersion → 42), and update the literal"platform migration 40"strings baked intotemplates/cluster.yaml:42andtests/cluster_test.yaml:354,364,375. The two authors should coordinate the merge order. -
Image rebuild is required before the migration actually runs.
values.yaml:8still pinsplatform-migrations:v1.4.0-rc.2@sha256:17390197...— that's the v1.4.0 image, baked before this PR added the script. The runner (run-migrations.sh:27-39) will iterate overi=40, find no/migrations/40in that image, log "Migration 40 not found, skipping", exit 0, and the version stamp won't advance. So the rename never happens until the platform-migrations image is rebuilt with this script andvalues.yaml:8is bumped. The release-tag CI handles this automatically (make image-migrationsinpackages/core/platform/Makefile:19-28bumps values.yaml on tag push), and migration #39 followed exactly the same pattern (added inf18ce1974with the v1.3.0 image still pinned, image bumped at v1.4.0-rc.2 release prep). So this is conventional — just worth confirming with maintainers that the next release tag is close enough that main-following users won't sit with the broken{{ fail }}for too long. -
v1.4.0 changelog misattributes the rename to migration 39.
docs/changelogs/v1.4.0.md:25and:77claim "Migration 39 rewrites legacyephemeralStoragevalues todiskSizeon upgrade". Migration 39 (commitf18ce1974) is the resourcesPreset legacy-aliases → instance-type-names mapping and does NOT touchephemeralStorage— that's the gap this PR fills. Users who upgraded to v1.4.0 expecting the automatic migration are exactly the population this PR is meant to rescue. Either v1.4.0 docs should be corrected in a separate PR, or the next changelog should make the actual mechanism (migration 40, this PR) explicit and call out the v1.4.0 mistake.
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Approach is sound and a clear improvement over the previous (now-dismissed) silent-drop design — this preserves the user's storage size across what is actually more than a rename: PR #2454 also flipped ephemeral (in-VM emptyDisk) → persistent (DataVolume / PVC). The migration's .diskSize = .ephemeralStorage branch is what carries the user's intent across that semantic shift.
What I verified
Static
- Helm unit tests pass on the kubernetes package (123/123).
- The
kuberneteses.apps.cozystack.iokind is served by the aggregated API server (cozystack-api), backed byHelmReleaseobjects in etcd. Patching the virtual resource flows throughconvertApplicationToHelmRelease(pkg/registry/apps/application/rest.go:1494-1554) —Values: app.Specwrites the spec straight ontoHelmRelease.spec.values, the same field Flux uses to render the chart. - JQ transform behaves correctly on the standard cases (move-only, both-set, already-migrated).
- Schema defaulting (
rest_defaulting.go:101-114) only fills missing properties, so it won't overwrite the migrateddiskSizeand (in v1.4) won't re-introduceephemeralStorage. release-1.3still usesephemeralStoragenatively, so no backport needed.
Dynamic (one v1.3.4 dev cluster, one synthetic Kubernetes app with nodeGroups.md0.ephemeralStorage: 33Gi, 1 worker)
- Dry-run preview correctly identified the target CR and showed the right transform.
- Real run: PATCHED=1, FAILURES=0. After the patch:
HelmRelease.spec.values.nodeGroups.md0keys =[diskSize, instanceType, maxReplicas, minReplicas, roles]—ephemeralStoragecleanly dropped (JSON merge-patch null semantics work as expected against the aggregated API server).diskSize: "33Gi"— the user's value preserved.
- Two-run idempotency: the actual stored HR data is identical across runs (diff empty). One caveat below.
Caveat worth knowing about
When the migration runs against a v1.3 cozystack-api (which still has ephemeralStorage as a schema property with default: "20Gi"), every read re-injects ephemeralStorage: "20Gi" on top of the stored HR data. The migration's idempotency check (current == new) therefore reports PATCHED=1 on the second run even though the patch is functionally a no-op (HR.spec.values unchanged).
In a real v1.3→v1.4 upgrade this is self-healing: the migration runs as the platform's pre-upgrade hook, then cozystack-api itself is upgraded to v1.4 (whose schema no longer knows ephemeralStorage), after which the idempotency check works correctly. Worth keeping in mind for anyone re-running the migration manually post-upgrade — it'll look like it patched something when it didn't.
What I did NOT verify
- A full v1.3.4-with-broken-chart → v1.4-with-this-PR cluster upgrade cycle (helm upgrade of cozystack core, migration hook firing as a real Job, Flux picking up the new chart). The dev cluster I had access to is still on v1.3, so I tested the migration's data path in isolation. The chart-side guard's behavior is already covered by the existing helm unit tests; the only remaining gap is "does the pre-upgrade hook actually fire and run migration 40 as expected during a real platform upgrade." That's the same machinery as migrations 1–39 and isn't changed by this PR.
Non-blocking nits
- The migration only patches via the aggregated API server. If
cozystack-apiis down at migration time, the migration exits non-zero and the Job retries (perbackoffLimit: 3). Acceptable. - Like migration 39, no automated test for the script. Manual end-to-end is the only safety net. Matches existing convention.
- Minor JQ edge case for
ephemeralStorage: null— inline comment on line 33.
LGTM modulo the JQ edge case, which is non-blocking.
| if type == "object" and has("ephemeralStorage") then | ||
| (if has("diskSize") and (.diskSize // "") != "" then . | ||
| else .diskSize = .ephemeralStorage end) | ||
| | .ephemeralStorage = null |
There was a problem hiding this comment.
Minor edge case: when ephemeralStorage is set but its value is null or empty string, the else .diskSize = .ephemeralStorage branch copies the null/empty into diskSize. After JSON merge-patch:
diskSize: null→ key dropped from the stored object → schema default20Gire-injected on next read. User who hadephemeralStorage: null(probably means "I cleared the field, give me default") gets 20Gi, which matches intent.diskSize: ""→ empty string stored →default "20Gi"in the chart template kicks in. Also matches intent.
Both outcomes are survivable, but the explicit diskSize: null patch is a little noisy in audit logs and could cause confusion if the operator stares at the diff. Consider an extra arm:
if has("diskSize") and (.diskSize // "") != "" then .
elif (.ephemeralStorage // "") != "" then .diskSize = .ephemeralStorage
else . # neither has a meaningful value, let the schema default fill diskSize
end
| .ephemeralStorage = nullThis keeps the patch minimal in the no-meaningful-value case. Non-blocking — the existing logic is functionally correct, just slightly noisier.
| items=$(list_objects kuberneteses.apps.cozystack.io) || exit 1 | ||
| while IFS=$'\t' read -r ns name; do | ||
| [ -z "$ns" ] && continue | ||
| patch_object kuberneteses.apps.cozystack.io "$ns" "$name" .spec |
There was a problem hiding this comment.
Worth noting for the next reader: this only walks kuberneteses.apps.cozystack.io via the aggregated API server. Migration 39 also walks raw helmreleases.helm.toolkit.fluxcd.io because the legacy resourcesPreset field can appear in HelmReleases that aren't backed by an Application CR. For ephemeralStorage, the field only exists in the kubernetes app chart values, and the only path that creates those HelmReleases is via cozystack-api (pkg/registry/apps/application/rest.go:1494-1554 writes Values: app.Spec into the HelmRelease and the CR is the source of truth). So patching just the kuberneteses CR is sufficient here. No change requested — calling out the difference.
| spec: | ||
| {{- if hasKey .group "ephemeralStorage" }} | ||
| {{- fail (printf "nodeGroup %q: ephemeralStorage is no longer supported. Rename it to diskSize. See README.md for migration instructions." .groupName) }} | ||
| {{- fail (printf "nodeGroup %q: ephemeralStorage is no longer supported and should have been automatically migrated to diskSize by platform migration 40. If you see this error after upgrading, the migration did not run — check the cozystack-migration-hook Job logs in cozy-system." .groupName) }} |
There was a problem hiding this comment.
The updated error message is actionable — names migration 40 and points to the right Job log. Good.
Add pre-upgrade migration 41 that walks all kuberneteses.apps.cozystack.io Application CRs and renames nodeGroups[*].ephemeralStorage to diskSize, preserving the user's value. Without this migration, clusters upgraded after PR #2454 either fail to reconcile (hard fail blocks all Flux operations) or silently lose the user's disk size setting (default 20Gi replaces whatever was configured). The migration is idempotent and best-effort: a failed patch is logged and leaves the version stamp at 41 for retry on next upgrade. Update the chart-side guard error message to direct operators to the migration job logs when the field appears post-upgrade (regression detector). Bump migrations.targetVersion 41 -> 42. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
e97e39a to
ed1bb53
Compare
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.4
git worktree add -d .worktree/backport-2688-to-release-1.4 origin/release-1.4
cd .worktree/backport-2688-to-release-1.4
git switch --create backport-2688-to-release-1.4
git cherry-pick -x ed1bb53d2c33588b018b2642e3ac1fd4709fa844 |
Problem
PR #2454 renamed
nodeGroups[*].ephemeralStorage→nodeGroups[*].diskSizewitha hard
{{ fail }}guard. Any cluster whose HelmRelease still carries the legacyfield cannot be reconciled by Flux at all — unrelated control-plane changes and
MachineHealthCheck remediations are also blocked.
Solution
Add platform migration 41 that runs as a pre-upgrade hook before any chart
resources are applied. The migration walks every
kuberneteses.apps.cozystack.ioApplication CR cluster-wide and renames
nodeGroups[*].ephemeralStoragetonodeGroups[*].diskSize, preserving the user's value.migration retries on the next platform upgrade
migrations.targetVersion41 → 42The chart-side guard remains in place with an updated error message that directs
operators to the migration job logs if the field somehow reappears post-upgrade.
Testing
Verified on dev cluster: migration correctly renames
ephemeralStoragevaluesin existing Application CRs. All 123 helm unit tests pass.
Summary by CodeRabbit
Release Notes
Documentation
ephemeralStoragetodiskSizefield rename for Kubernetes node groupsTests