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/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/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 diff --git a/internal/controller/memgraphcluster_controller.go b/internal/controller/memgraphcluster_controller.go index 5858da3..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.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 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..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" ) @@ -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) + } +}