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

VERSION

@@ -1 +1 @@
-0.2.2
+0.3.0

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

api/v1alpha1/memgraphcluster_types_test.go

@@ -3,13 +3,17 @@
 package v1alpha1
 
 import (
+	"encoding/json"
+	"strings"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	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)
+	}
+}

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

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.Enabled {
-		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

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)
 	}
 

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"
 )
@@ -29,7 +36,7 @@ func TestBuildSnapshotCronJob(t *testing.T) {
 				},
 			},
 			Snapshot: memgraphv1alpha1.SnapshotSpec{
-				Enabled:  true,
+				Enabled:  ptr(true),
 				Schedule: "0 */6 * * *", // Every 6 hours
 			},
 		},
@@ -99,7 +106,7 @@ func TestBuildSnapshotCronJobPropagatesScheduling(t *testing.T) {
 					},
 				},
 			},
-			Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: true},
+			Snapshot: memgraphv1alpha1.SnapshotSpec{Enabled: ptr(true)},
 		},
 	}
 
@@ -130,7 +137,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 +237,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 +265,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 +294,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 +319,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 +393,7 @@ func TestBuildSnapshotInitContainers(t *testing.T) {
 		},
 		Spec: memgraphv1alpha1.MemgraphClusterSpec{
 			Snapshot: memgraphv1alpha1.SnapshotSpec{
-				Enabled: true,
+				Enabled: ptr(true),
 			},
 		},
 	}
@@ -422,7 +429,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 +613,7 @@ func TestBuildSnapshotVolumes(t *testing.T) {
 			cluster: &memgraphv1alpha1.MemgraphCluster{
 				Spec: memgraphv1alpha1.MemgraphClusterSpec{
 					Snapshot: memgraphv1alpha1.SnapshotSpec{
-						Enabled: true,
+						Enabled: ptr(true),
 					},
 				},
 			},
@@ -617,7 +624,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 +666,7 @@ func TestBuildSnapshotMainContainers(t *testing.T) {
 			cluster: &memgraphv1alpha1.MemgraphCluster{
 				Spec: memgraphv1alpha1.MemgraphClusterSpec{
 					Snapshot: memgraphv1alpha1.SnapshotSpec{
-						Enabled: true,
+						Enabled: ptr(true),
 					},
 				},
 			},
@@ -670,7 +677,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 +735,7 @@ func TestBuildS3EnvWithNilS3(t *testing.T) {
 	cluster := &memgraphv1alpha1.MemgraphCluster{
 		Spec: memgraphv1alpha1.MemgraphClusterSpec{
 			Snapshot: memgraphv1alpha1.SnapshotSpec{
-				Enabled: true,
+				Enabled: ptr(true),
 			},
 		},
 	}
@@ -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)
+	}
+}