🐛 fix(api): snapshot.enabled: false is unsettable and never cleans up

[thilak009]

Stacked on #5 — base is fix/snapshot-cronjob-scheduling. Review that one first; this diff shows only the changes on top of it. Retarget to main once #5 merges.

Problem

A CR that declares snapshot.enabled: false runs a snapshot CronJob anyway. Two defects, and each one alone is enough to cause it.

1. false is not representable. SnapshotSpec.Enabled was a non-pointer bool with omitempty and +kubebuilder:default=true. false is Go's zero value, so it is dropped on marshal and the API server re-applies the default:

enabled=false marshals to: {"schedule":"0 * * * *","retentionCount":5}
enabled=true  marshals to: {"enabled":true,"schedule":"0 * * * *","retentionCount":5}

false is indistinguishable from "unset" on the wire, so it cannot survive the operator's own full-object write when adding the finalizer (memgraphcluster_controller.go:100). A client that applies literal JSON gets false stored correctly on create; the operator's write then drops just the zero-valued bool, and the API server defaults it back to true.

The signature is distinctive: every other field in spec.snapshot round-trips exactly, and only enabled flips.

Worth noting for anyone reproducing this: it is not "every reconcile". The only full-object CR writes are the finalizer add — which fires once, on the first reconcile of a new CR — and the finalizer removal on delete. The other r.Update calls target the StatefulSet and Service. So the flip happens once at CR creation and then sticks, because nothing writes the CR again to correct it.

2. The cleanup path was unreachable. The call site gated on Spec.Snapshot.Enabled, but the delete branch lives inside the function being gated. The function could only ever run when snapshots were enabled, and its first act was to check whether they were disabled. Dead code — disabling snapshots never removed an existing CronJob.

Changes

1. memgraphcluster_types.go — Enabled becomes *bool, plus an IsEnabled() accessor treating nil as true to match the CRD default. omitempty skips only nil pointers, so an explicit false now marshals correctly.

   The CRD schema is unchanged — *bool and bool both render as type: boolean with default: true. The only yaml diff is the field description. Existing CRs are unaffected.

   S3BackupSpec.Enabled is deliberately left a plain bool: it has no kubebuilder default, so absent and explicit-false are already equivalent there.

2. memgraphcluster_controller.go — call reconcileSnapshotCronJob unconditionally and let it decide, making the existing delete path reachable. Owns(&batchv1.CronJob{}) is already wired, so deletions re-trigger reconcile correctly.

Behaviour change

⚠️ Any cluster whose CR declares snapshot.enabled: false will have its snapshot CronJob deleted on the first reconcile after upgrade.

That is the intended fix — those clusters asked for no snapshots and have been running one regardless — but it is a real behaviour change on upgrade rather than a pure bugfix, which is why it is versioned as a minor and split from #5.

Clusters that want snapshots (enabled: true, or the field unset) are unaffected; nil still means enabled.

Testing

make ci passes — lint 0 issues, all packages green, build succeeds. api/v1alpha1 coverage 89.9% → 90.2%.

New coverage:

• TestSnapshotSpecEnabledFalseSurvivesMarshal — direct regression test asserting "enabled":false is present after marshal.
• TestSnapshotSpecIsEnabled — nil/true/false accessor behaviour.
• TestReconcileSnapshotCronJobDeletesWhenDisabled — a disabled cluster's CronJob is deleted.

Note on that last test: it passes without the call-site change, because the delete branch itself was always correct — the bug was purely that nothing called it. The guard for the actual fix is that exactly one Snapshot.IsEnabled() read site now remains, inside snapshot.go where the delete branch lives.

Verification on a cluster

kubectl patch memgraphcluster <name> --type=merge -p '{"spec":{"snapshot":{"enabled":false}}}'
kubectl get memgraphcluster <name> -o jsonpath='{.spec.snapshot.enabled}'   # must stay false
kubectl get cronjob <name>-snapshot                                          # must be NotFound

Not addressed

spec.snapshot.retentionCount and spec.snapshot.s3.retentionDays are accepted by the CRD and read by no controller code. Local snapshots are pruned solely by Memgraph's own --storage-snapshot-retention-count; S3 backups are pruned by nothing. Tracked separately.

Note the near-miss for whoever picks that up: spec.config.snapshotRetentionCount is a different field and is wired up, at statefulset.go:205.

🤖 Generated with Claude Code