Conversation
Replace the piraeus classic HelmRepository chart (snapshot-controller 5.2.0) with oci://ghcr.io/home-operations/charts/snapshot-controller 0.1.1 (appVersion v8.6.0, unchanged), via an OCIRepository + chartRef following the tuppr/volsync pattern. Values ported: controller.serviceMonitor.create -> monitoring.serviceMonitor.enabled, controller.volumeGroupSnapshots set explicitly to keep the CSIVolumeGroupSnapshot feature gate on, and resources set per repo convention. Drops the now-unused piraeus HelmRepository. Refs #3828
Contributor
|
Warning
Note +5 added · 5 changed · −11 removed: 21 resources across 3 apps · 6 CRDs Blast radius
konflate 0.6.4 · rendered |
CreateReplace force-replaces the live CRDs before the Helm upgrade runs, which strips the helm.sh/resource-policy=keep annotation that stops Helm deleting them a moment later. Skip leaves it intact for the migration.
Owner
Author
|
Since I am not sure when I'll get chance to take a look at this and this is a low priority here is handoff document |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3 of #3828. Draft on purpose. Do not merge until the pre-merge sequence below has been run and verified.
This upgrade would delete the six volume-snapshot CRDs unless it is sequenced correctly. The hazard is real, the fix is small, and nothing in CI can see either — details and evidence below.
What changed
kubernetes/apps/volsync-system/snapshot-controller/app/repository.yaml—OCIRepositoryforoci://ghcr.io/home-operations/charts/snapshot-controller, using the samelayerSelectormediaType/operation: copypattern astupprandvolsync.helmrelease.yaml—chart.spec→chartRef.upgrade.crds: CreateReplace→Skip— transitional, for this migration only. This is the safety fix; see "Why CreateReplace does not save you".install.crdsstaysCreateReplace(correct for a fresh install).controller.serviceMonitor.create: true→monitoring.serviceMonitor.enabled: true(key moved).controller.volumeGroupSnapshots: true— set explicitly; the chart default is alreadytrue, so this is a no-op that documents intent and keeps the feature gate identical to the piraeus release.resourcesadded (chart default is{}):cpu: 10m/memory: 64Mirequests,memory: 128Milimit.resourcesis top-level in this chart, not undercontroller.kubernetes/flux/meta/repos/piraeus.yamldeleted and dropped from the repos kustomization.grep -rn piraeus kubernetes/returns nothing, and this was the only HelmRelease using that source.Chart version is
0.1.1, not the0.1.0the issue names.0.1.1was published 2026-09-09; the only change is a vendir lockfile maintenance bump. appVersion isv8.6.0in both.The hazard
The two charts ship the same CRDs by different mechanisms:
5.2.0templates/crds.yaml, gated oninstallCRDs(defaulttrue)0.1.1crds/directorycrds/Confirmed against the live release: the stored manifest for
sh.helm.release.v1.snapshot-controller.v9contains 6kind: CustomResourceDefinition, all sourced fromsnapshot-controller/templates/crds.yaml. Flux's.status.inventorylists all six. None of the live CRDs carrieshelm.sh/resource-policy: keep; all carrymeta.helm.sh/release-name: snapshot-controller.helm upgradedeletes everything present in the old release manifest and absent from the new one. So the delete set for this upgrade is the six CRDs plus the four renamed RBAC objects (harmless).Deleting those CRDs cascades to every VolumeSnapshot and VolumeSnapshotContent in the cluster. Current inventory: 12 VolumeSnapshots and 12 VolumeSnapshotContents, all
deletionPolicy: Delete, ~68 GiB of Ceph RBD snapshots onrook-ceph.rbd.csi.ceph.com. They are thestatus.latestImageof the 12 ReplicationDestinations. Because the controller is still running during its own upgrade, it would process each deletion and instruct the Ceph CSI driver to delete the underlying RBD snapshots.It also takes both VolumeSnapshotClasses (
csi-ceph-blockpool,csi-ceph-filesystem) with it. Their ownhelm.sh/resource-policy: keepdoes not help — deleting a CRD destroys its instances regardless. They belong to therook-ceph-clusterrelease under apost-install,pre-upgradehook, so they would only return on the next rook-ceph HelmRelease upgrade, not on a plain reconcile.Scope of the damage, stated honestly: the kopia repositories are untouched, so no backup history is lost. What is lost is the 12 pre-staged restore images, rebuildable only by re-running all 12 ReplicationDestinations. Nightly backups for the 12 volsync apps would also stop until the CRDs are restored —
VolSyncVolumeOutOfSync(critical, 5m) would fire.Why
CreateReplacedoes not save youThis is the part that is easy to get wrong, and it is why this PR sets
Skip.Helm skips deleting a resource whose live object carries
helm.sh/resource-policy: keep, so annotating the CRDs beforehand looks sufficient. On this cluster it is not, because Flux's own CRD step runs first and removes the annotation:applyCRDs()before the Helm upgrade —internal/action/upgrade.go:85, then:91.serverSideApplyresolves from the last release:serverSideApply = lastReleaseTyped.ApplyMethod == "ssa"(upgrade.go:74). Release v9 recordsapply_method: csa, so this is false.CreateReplaceaddsClientUpdateOptionForceReplace(true)(internal/action/crds.go:225-227).pkg/kube/client.go:839→:1143). The chart'scrds/*.yamlcarry noresource-policyannotation, so the PUT strips thekeepannotation.client.go:654), reads the live object (:678), finds nokeep, and deletes.Annotation applied → stripped by Flux → checked by Helm. The CRDs go anyway.
Versions this was verified against: helm-controller v1.6.4, Helm SDK v4.2.4, Flux v2.9.5. Under server-side apply the annotation would survive, which is why
apply_method: csais the decisive fact — do not assume this generalises to a release that was applied server-side.upgrade.crds: Skipcloses it:applyCRDsreturns before touching anything (crds.go:56), so the annotation is still there when Helm checks. This costs nothing — see the CRD equivalence check below.The Service is an accidental safety net — do not remove it early
The old chart made the metrics Service headless; the new one renders a normal
ClusterIPService with noclusterIPfield. Live object and the stored v9 manifest both pinclusterIP: None.spec.clusterIPis immutable, so Helm's patch is rejected with a "field is immutable" error.Helm aborts
Update()on the first patch error (client.go:651) before reaching the delete loop (:654). So on an unprepared cluster this failure would accidentally spare the CRDs.That is ordering luck, not a safeguard, and it matters for sequencing: deleting the Service is what arms the CRD deletion. Doing the Service fix without the CRD protection actually in place is worse than doing neither. Hence the order below.
Only the ServiceMonitor consumes this Service and it selects by label, so deleting it is harmless; Helm recreates it non-headless.
Pre-merge sequence — run in this order
1. Protect the CRDs.
2. Verify all six — do not skip this.
Every row must read
keep. If any says<none>, stop.3. Back up the CRD manifests. Cheap insurance, and with the specs identical this makes even the worst case a one-command recovery.
kubectl get crd -o yaml \ volumesnapshots.snapshot.storage.k8s.io \ volumesnapshotcontents.snapshot.storage.k8s.io \ volumesnapshotclasses.snapshot.storage.k8s.io \ volumegroupsnapshots.groupsnapshot.storage.k8s.io \ volumegroupsnapshotcontents.groupsnapshot.storage.k8s.io \ volumegroupsnapshotclasses.groupsnapshot.storage.k8s.io \ > snapshot-crds-backup.yaml4. Delete the headless Service — only now that step 2 has passed.
5. Merge, then watch the release:
Optional, if you want proof before committing to it: merge with the Service still in place. The upgrade fails on the immutable
clusterIPand the CRDs stay untouched, which demonstrates the protection is holding. It will churn throughremediation.retries: 3with rollbacks, so it is noisy — then do step 4 andflux reconcile hr snapshot-controller -n volsync-system.Backups last ran cleanly at 02:00 today, so the kopia repositories are current; a manual
ReplicationSourcetrigger first is optional rather than necessary.Post-merge checks
Then trigger one
ReplicationSourcemanually to confirm end to end, and check the next nightly run at 02:00.Follow-up
Restore
upgrade.crds: CreateReplacein a separate PR once this has landed. At that point the CRDs are in neither the old nor the new release manifest, so Helm has nothing to delete and the force-replace is harmless. LeavingSkipin place permanently would mean future CRD updates from the chart never apply.Why CI cannot catch any of this
flaterenders zero CRDs on both sides — verified by renderingorigin/mainand this branch. The piraeus CRDs come fromtemplates/, yet they do not appear in flate's output either.Konflate's summary (
+5 added · 5 changed · −5 removed) accounts exactly for the four RBAC renames plus the repo swap, the five added counterparts, and the five changed objects. No CRD appears anywhere in the diff.A renderer compares rendered output. The deletion here comes from Helm's manifest diff against the stored release, which no offline renderer models. The whole validation chain is structurally blind to this class of change — worth remembering for future
templates/→crds/migrations.What was verified
templates/crds.yamlwith this repo's values and diffed all six.specblocks against the new chart'scrds/*.yaml: byte-identical. Samecontroller-gen v0.15.0. Both leaveconversionunset (webhook.enabled: false), and the live CRDs reportconversion.strategy: None. Storage versions match:v1for the three snapshot CRDs,v1beta2for the three group CRDs. Nothing is lost by not applying them, which is what makesSkipfree.values.schema.json; rendered args are unchanged in substance:--v=2 --leader-election=true --leader-election-namespace=$(NAMESPACE) --http-endpoint=:8080 --feature-gates=CSIVolumeGroupSnapshot=true.media/overseerrhas a destination but no source; pre-existing, unrelated.)app.kubernetes.io/created-by: volsyncwith a single ReplicationDestination owner; no PVC anywhere has adataSourceof kindVolumeSnapshot.Rendered diff
Names stay put — Deployment, Service, ServiceAccount and ServiceMonitor remain
snapshot-controller, and the Deployment'sselector.matchLabelsis unchanged, so this is an in-place upgrade with nothing orphaned.RBAC is renamed (Helm creates the new and removes the old within the release; rules are byte-identical):
snapshot-controllersnapshot-controller-runnersnapshot-controllersnapshot-controller-leaderelectionThe RoleBinding subject also gains an explicit
namespace: volsync-systeminstead of relying on defaulting.Deployment changes:
http→metrics(same port 8080).initialDelaySeconds/periodSeconds; args gain--v=2and are reordered.resourcesnow set (was{}). Peak working set over the last 7d is 28 MiB.runAsUser1000 → 65532, plus a pod-level securityContext (fsGroup/runAsGroup65532,seccompProfile: RuntimeDefault),allowPrivilegeEscalation: falseandreadOnlyRootFilesystem: true. The controller writes nothing to disk.priorityClassName: system-cluster-critical,enableServiceLinks: false,minReadySeconds: 35, and amaxSurge: 0/maxUnavailable: 1strategy — so the single replica is down for roughly 40–60s during rollout. Harmless outside the 02:00 window.interval: 30s/scrapeTimeout: 10s.Validation
just configure— cleanjust validate(yayamlls) — cleanjust flate-test— 169 passed, includingOCIRepository volsync-system/snapshot-controller; the one warning (external-dns-cloudflare: values not used by the chart) is pre-existing and unrelatedpython3 scripts/find_mistakes.py— exits 0, only the two known clean-tree warningspre-commit run --all-files— all hooks passed