From d9a75caa3a3918eff4d24d6b96ae1d8207fd1da5 Mon Sep 17 00:00:00 2001 From: thilak Date: Mon, 17 Aug 2026 07:35:16 +0530 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20fix(api):=20make=20snapshot.?= =?UTF-8?q?enabled=20a=20pointer=20so=20false=20is=20representable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enabled was a non-pointer bool with omitempty and +kubebuilder:default=true. false is Go's zero value, so it was dropped on marshal and the API server re-applied the default, meaning an explicit false could not survive the operator's own full-object write when adding the finalizer. Verified before the fix: enabled=false marshals to: {"schedule":"0 * * * *","retentionCount":5} enabled=true marshals to: {"enabled":true,"schedule":"0 * * * *","retentionCount":5} A CR that declares snapshot.enabled: false therefore ends up running a snapshot CronJob anyway. Behaviour-neutral on its own: the call site still gates on Enabled, so no CronJob is deleted until that gate is removed. The CRD schema is unchanged — *bool and bool both render as type: boolean with default: true. S3BackupSpec.Enabled is deliberately left a plain bool: it has no kubebuilder default, so absent and explicit-false are already equivalent. Co-Authored-By: Claude Opus 5 (1M context) --- api/v1alpha1/memgraphcluster_types.go | 15 ++++++- api/v1alpha1/memgraphcluster_types_test.go | 45 +++++++++++++++++-- api/v1alpha1/zz_generated.deepcopy.go | 5 +++ .../controller/memgraphcluster_controller.go | 2 +- internal/controller/snapshot.go | 2 +- internal/controller/snapshot_test.go | 28 ++++++------ 6 files changed, 75 insertions(+), 22 deletions(-) diff --git a/api/v1alpha1/memgraphcluster_types.go b/api/v1alpha1/memgraphcluster_types.go index 5de8011..6a09c36 100644 --- a/api/v1alpha1/memgraphcluster_types.go +++ b/api/v1alpha1/memgraphcluster_types.go @@ -201,10 +201,11 @@ const ( // SnapshotSpec defines snapshot and backup configuration type SnapshotSpec struct { - // Enabled enables periodic snapshots + // Enabled enables periodic snapshots. Set to false to disable them and remove + // the snapshot CronJob. // +kubebuilder:default=true // +optional - Enabled bool `json:"enabled,omitempty"` + Enabled *bool `json:"enabled,omitempty"` // Schedule is a cron expression for snapshot frequency // +kubebuilder:default="*/15 * * * *" @@ -244,6 +245,16 @@ type SnapshotSpec struct { S3 *S3BackupSpec `json:"s3,omitempty"` } +// IsEnabled reports whether periodic snapshots are enabled. A nil Enabled means +// the field was never set, which matches the CRD default of true. +// +// Enabled is a *bool rather than a bool because with `omitempty` a false bool is +// dropped on marshal, so the API server re-applies +kubebuilder:default=true and +// the operator's own full-object writes silently re-enable snapshots. +func (s *SnapshotSpec) IsEnabled() bool { + return s.Enabled == nil || *s.Enabled +} + // S3BackupSpec defines S3 backup configuration type S3BackupSpec struct { // Enabled enables S3 backups diff --git a/api/v1alpha1/memgraphcluster_types_test.go b/api/v1alpha1/memgraphcluster_types_test.go index c8f8374..cb16853 100644 --- a/api/v1alpha1/memgraphcluster_types_test.go +++ b/api/v1alpha1/memgraphcluster_types_test.go @@ -3,6 +3,8 @@ package v1alpha1 import ( + "encoding/json" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -10,6 +12,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func boolPtr(v bool) *bool { return &v } + func TestMemgraphCluster_DeepCopy(t *testing.T) { original := &MemgraphCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -36,7 +40,7 @@ func TestMemgraphCluster_DeepCopy(t *testing.T) { PreferredMain: "test-cluster-0", }, Snapshot: SnapshotSpec{ - Enabled: true, + Enabled: boolPtr(true), Schedule: "*/15 * * * *", RetentionCount: 5, S3: &S3BackupSpec{ @@ -170,7 +174,7 @@ func TestMemgraphClusterSpec_DeepCopy(t *testing.T) { ReadSuffix: "-secondary", }, Snapshot: SnapshotSpec{ - Enabled: true, + Enabled: boolPtr(true), Schedule: "0 * * * *", RetentionCount: 10, S3: &S3BackupSpec{ @@ -291,7 +295,7 @@ func TestStorageSpec_DeepCopy(t *testing.T) { func TestSnapshotSpec_DeepCopy(t *testing.T) { original := SnapshotSpec{ - Enabled: true, + Enabled: boolPtr(true), Schedule: "0 0 * * *", RetentionCount: 7, S3: &S3BackupSpec{ @@ -733,7 +737,7 @@ func TestAllDeepCopyFunctions(t *testing.T) { } // SnapshotSpec - snapshotSpec := SnapshotSpec{Enabled: true, Schedule: "0 * * * *"} + snapshotSpec := SnapshotSpec{Enabled: boolPtr(true), Schedule: "0 * * * *"} if snapCopy := snapshotSpec.DeepCopy(); snapCopy == nil { t.Error("SnapshotSpec.DeepCopy returned nil") } @@ -758,3 +762,36 @@ func TestAllDeepCopyFunctions(t *testing.T) { t.Error("ValidationStatus.DeepCopy returned nil") } } + +func TestSnapshotSpecIsEnabled(t *testing.T) { + tests := []struct { + name string + spec SnapshotSpec + want bool + }{ + {"nil means enabled, matching the CRD default", SnapshotSpec{}, true}, + {"explicit false", SnapshotSpec{Enabled: boolPtr(false)}, false}, + {"explicit true", SnapshotSpec{Enabled: boolPtr(true)}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.spec.IsEnabled(); got != tt.want { + t.Errorf("IsEnabled() = %v, want %v", got, tt.want) + } + }) + } +} + +// Regression test: a non-pointer bool with omitempty is dropped on +// marshal, so the API server re-applies +kubebuilder:default=true and the +// operator's own full-object write silently re-enables snapshots. +func TestSnapshotSpecEnabledFalseSurvivesMarshal(t *testing.T) { + disabled := false + out, err := json.Marshal(SnapshotSpec{Enabled: &disabled, Schedule: "0 * * * *"}) + if err != nil { + t.Fatalf("marshal failed: %v", err) + } + if !strings.Contains(string(out), `"enabled":false`) { + t.Errorf("enabled:false was dropped on marshal: %s", out) + } +} diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 11741bc..539b2f6 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -297,6 +297,11 @@ func (in *ServiceNamesSpec) DeepCopy() *ServiceNamesSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SnapshotSpec) DeepCopyInto(out *SnapshotSpec) { *out = *in + if in.Enabled != nil { + in, out := &in.Enabled, &out.Enabled + *out = new(bool) + **out = **in + } if in.ActiveDeadlineSeconds != nil { in, out := &in.ActiveDeadlineSeconds, &out.ActiveDeadlineSeconds *out = new(int64) diff --git a/internal/controller/memgraphcluster_controller.go b/internal/controller/memgraphcluster_controller.go index 5858da3..0ab38e0 100644 --- a/internal/controller/memgraphcluster_controller.go +++ b/internal/controller/memgraphcluster_controller.go @@ -242,7 +242,7 @@ func (r *MemgraphClusterReconciler) reconcileResources(ctx context.Context, clus } // 8. Reconcile snapshot CronJob if enabled - if cluster.Spec.Snapshot.Enabled { + if cluster.Spec.Snapshot.IsEnabled() { if err := r.reconcileSnapshotCronJob(ctx, cluster, log); err != nil { log.Error("failed to reconcile snapshot CronJob", zap.Error(err)) } diff --git a/internal/controller/snapshot.go b/internal/controller/snapshot.go index 1c6f50e..c63f68b 100644 --- a/internal/controller/snapshot.go +++ b/internal/controller/snapshot.go @@ -424,7 +424,7 @@ func snapshotContainersEqual(existing, desired []corev1.Container) bool { // reconcileSnapshotCronJob ensures the snapshot CronJob exists and is configured correctly func (r *MemgraphClusterReconciler) reconcileSnapshotCronJob(ctx context.Context, cluster *memgraphv1alpha1.MemgraphCluster, log *zap.Logger) error { // If snapshots are not enabled, ensure CronJob doesn't exist - if !cluster.Spec.Snapshot.Enabled { + if !cluster.Spec.Snapshot.IsEnabled() { return r.deleteSnapshotCronJob(ctx, cluster, log) } diff --git a/internal/controller/snapshot_test.go b/internal/controller/snapshot_test.go index 2bd0d01..6a7d04e 100644 --- a/internal/controller/snapshot_test.go +++ b/internal/controller/snapshot_test.go @@ -29,7 +29,7 @@ func TestBuildSnapshotCronJob(t *testing.T) { }, }, Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), Schedule: "0 */6 * * *", // Every 6 hours }, }, @@ -99,7 +99,7 @@ func TestBuildSnapshotCronJobPropagatesScheduling(t *testing.T) { }, }, }, - Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: true}, + Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: ptr(true)}, }, } @@ -130,7 +130,7 @@ func TestBuildSnapshotCronJobWithS3(t *testing.T) { Replicas: 3, Image: "memgraph/memgraph:2.21.0", Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), Schedule: "*/15 * * * *", S3: &memgraphv1alpha1.S3BackupSpec{ Enabled: true, @@ -230,7 +230,7 @@ func TestBuildSnapshotCronJobDefaults(t *testing.T) { }, Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), // No schedule specified - should use default }, }, @@ -258,7 +258,7 @@ func TestBuildSnapshotCronJobWedgePreventionDefaults(t *testing.T) { cluster := &memgraphv1alpha1.MemgraphCluster{ ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, Spec: memgraphv1alpha1.MemgraphClusterSpec{ - Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: true}, + Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: ptr(true)}, }, } @@ -287,7 +287,7 @@ func TestBuildSnapshotCronJobWedgePreventionOverrides(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), ConcurrencyPolicy: memgraphv1alpha1.SnapshotConcurrencyReplace, ActiveDeadlineSeconds: ptr(int64(1800)), StartingDeadlineSeconds: ptr(int64(120)), @@ -312,7 +312,7 @@ func snapshotTestCluster(tolerated bool) *memgraphv1alpha1.MemgraphCluster { c := &memgraphv1alpha1.MemgraphCluster{ ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, Spec: memgraphv1alpha1.MemgraphClusterSpec{ - Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: true, Schedule: "0 * * * *"}, + Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: ptr(true), Schedule: "0 * * * *"}, }, } if tolerated { @@ -386,7 +386,7 @@ func TestBuildSnapshotInitContainers(t *testing.T) { }, Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), }, }, } @@ -422,7 +422,7 @@ func TestBuildSnapshotInitContainersWithS3(t *testing.T) { }, Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), S3: &memgraphv1alpha1.S3BackupSpec{ Enabled: true, Bucket: "backup-bucket", @@ -606,7 +606,7 @@ func TestBuildSnapshotVolumes(t *testing.T) { cluster: &memgraphv1alpha1.MemgraphCluster{ Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), }, }, }, @@ -617,7 +617,7 @@ func TestBuildSnapshotVolumes(t *testing.T) { cluster: &memgraphv1alpha1.MemgraphCluster{ Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), S3: &memgraphv1alpha1.S3BackupSpec{ Enabled: true, Bucket: "test-bucket", @@ -659,7 +659,7 @@ func TestBuildSnapshotMainContainers(t *testing.T) { cluster: &memgraphv1alpha1.MemgraphCluster{ Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), }, }, }, @@ -670,7 +670,7 @@ func TestBuildSnapshotMainContainers(t *testing.T) { cluster: &memgraphv1alpha1.MemgraphCluster{ Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), S3: &memgraphv1alpha1.S3BackupSpec{ Enabled: true, Bucket: "test-bucket", @@ -728,7 +728,7 @@ func TestBuildS3EnvWithNilS3(t *testing.T) { cluster := &memgraphv1alpha1.MemgraphCluster{ Spec: memgraphv1alpha1.MemgraphClusterSpec{ Snapshot: memgraphv1alpha1.SnapshotSpec{ - Enabled: true, + Enabled: ptr(true), }, }, } From a95bd01fd8c4473ad605b0da20ad978244bcccd4 Mon Sep 17 00:00:00 2001 From: thilak Date: Mon, 17 Aug 2026 07:35:16 +0530 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20fix(controller):=20reconcile?= =?UTF-8?q?=20snapshot=20CronJob=20unconditionally=20so=20disable=20remove?= =?UTF-8?q?s=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The call site gated on Spec.Snapshot.Enabled, but the delete branch lives inside the function being gated. The function could therefore 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. Combined with the enabled:false fix, "disable snapshots" now works end to end and cleans up CronJobs left behind on clusters that no longer want them. Co-Authored-By: Claude Opus 5 (1M context) --- .../controller/memgraphcluster_controller.go | 10 ++--- internal/controller/snapshot_test.go | 41 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/internal/controller/memgraphcluster_controller.go b/internal/controller/memgraphcluster_controller.go index 0ab38e0..f7465da 100644 --- a/internal/controller/memgraphcluster_controller.go +++ b/internal/controller/memgraphcluster_controller.go @@ -241,11 +241,11 @@ func (r *MemgraphClusterReconciler) reconcileResources(ctx context.Context, clus } } - // 8. Reconcile snapshot CronJob if enabled - if cluster.Spec.Snapshot.IsEnabled() { - if err := r.reconcileSnapshotCronJob(ctx, cluster, log); err != nil { - log.Error("failed to reconcile snapshot CronJob", zap.Error(err)) - } + // 8. Reconcile snapshot CronJob. Called unconditionally: the function itself + // handles the disabled case by deleting any existing CronJob, which is + // unreachable if the call is gated on Enabled. + if err := r.reconcileSnapshotCronJob(ctx, cluster, log); err != nil { + log.Error("failed to reconcile snapshot CronJob", zap.Error(err)) } // 9. Update snapshot status diff --git a/internal/controller/snapshot_test.go b/internal/controller/snapshot_test.go index 6a7d04e..5d26eaf 100644 --- a/internal/controller/snapshot_test.go +++ b/internal/controller/snapshot_test.go @@ -3,13 +3,20 @@ package controller import ( + "context" "strings" "testing" + "go.uber.org/zap" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client/fake" memgraphv1alpha1 "github.com/base14/memgraph-operator/api/v1alpha1" ) @@ -794,3 +801,37 @@ func TestNewSnapshotManager(t *testing.T) { t.Error("NewSnapshotManager returned nil") } } + +func TestReconcileSnapshotCronJobDeletesWhenDisabled(t *testing.T) { + scheme := runtime.NewScheme() + if err := memgraphv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("add memgraph scheme: %v", err) + } + if err := batchv1.AddToScheme(scheme); err != nil { + t.Fatalf("add batch scheme: %v", err) + } + + cluster := &memgraphv1alpha1.MemgraphCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, + Spec: memgraphv1alpha1.MemgraphClusterSpec{ + Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: ptr(false)}, + }, + } + orphan := &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster-snapshot", Namespace: "default"}, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, orphan).Build() + r := &MemgraphClusterReconciler{Client: c, Scheme: scheme, Recorder: record.NewFakeRecorder(10)} + + if err := r.reconcileSnapshotCronJob(context.Background(), cluster, zap.NewNop()); err != nil { + t.Fatalf("reconcile failed: %v", err) + } + + err := c.Get(context.Background(), + types.NamespacedName{Name: "test-cluster-snapshot", Namespace: "default"}, + &batchv1.CronJob{}) + if !apierrors.IsNotFound(err) { + t.Errorf("expected CronJob to be deleted, got err=%v", err) + } +} From f35225ce4818aa4411a9d0549fedff53d8f5207a Mon Sep 17 00:00:00 2001 From: thilak Date: Mon, 17 Aug 2026 07:35:16 +0530 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=94=96=20chore:=20bump=20version=20to?= =?UTF-8?q?=200.3.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Minor, not patch: disabling snapshots now actually disables them, which changes behaviour on every existing cluster. Co-Authored-By: Claude Opus 5 (1M context) --- VERSION | 2 +- config/crd/bases/memgraph.base14.io_memgraphclusters.yaml | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index ee1372d..0d91a54 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.2.2 +0.3.0 diff --git a/config/crd/bases/memgraph.base14.io_memgraphclusters.yaml b/config/crd/bases/memgraph.base14.io_memgraphclusters.yaml index 8703c05..6366077 100644 --- a/config/crd/bases/memgraph.base14.io_memgraphclusters.yaml +++ b/config/crd/bases/memgraph.base14.io_memgraphclusters.yaml @@ -1179,7 +1179,9 @@ spec: type: string enabled: default: true - description: Enabled enables periodic snapshots + description: |- + Enabled enables periodic snapshots. Set to false to disable them and remove + the snapshot CronJob. type: boolean retentionCount: default: 5