Snapshot handling improvements

api/v1beta1/questdbsnapshot_types.go

@@ -41,9 +41,9 @@ const (
 
 // QuestDBSnapshotSpec defines the desired state of QuestDBSnapshot
 type QuestDBSnapshotSpec struct {
-	QuestDBName             string `json:"questdbName"`
-	VolumeSnapshotClassName string `json:"volumeSnapshotClassName"`
-	JobBackoffLimit         int32  `json:"jobBackoffLimit,omitempty"`
+	QuestDBName             string  `json:"questdbName"`
+	VolumeSnapshotClassName *string `json:"volumeSnapshotClassName,omitempty"`
+	JobBackoffLimit         int32   `json:"jobBackoffLimit,omitempty"`
 }
 
 // QuestDBSnapshotStatus defines the observed state of QuestDBSnapshot

api/v1beta1/questdbsnapshot_webhook.go

@@ -18,6 +18,7 @@ package v1beta1
 
 import (
 	"errors"
+	"reflect"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -61,14 +62,14 @@ func validateSnapshotCreate(spec QuestDBSnapshotSpec) error {
 		return errors.New("QuestDBName is required")
 	}
 
-	if spec.VolumeSnapshotClassName == "" {
-		return errors.New("VolumeSnapshotClassName is required")
-	}
-
 	if spec.JobBackoffLimit <= 0 {
 		return errors.New("JobBackoffLimit must be greater than 0")
 	}
 
+	if spec.VolumeSnapshotClassName != nil && *spec.VolumeSnapshotClassName == "" {
+		return errors.New("VolumeSnapshotClassName must not be empty")
+	}
+
 	return nil
 }
 
@@ -77,7 +78,7 @@ func validateSnapshotUpdate(oldSpec, newSpec QuestDBSnapshotSpec) error {
 		return errors.New("QuestDBName is immutable")
 	}
 
-	if newSpec.VolumeSnapshotClassName != oldSpec.VolumeSnapshotClassName {
+	if !reflect.DeepEqual(newSpec.VolumeSnapshotClassName, oldSpec.VolumeSnapshotClassName) {
 		return errors.New("VolumeSnapshotClassName is immutable")
 	}
 

api/v1beta1/questdbsnapshot_webhook_test.go

@@ -7,6 +7,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/utils/pointer"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -39,7 +40,7 @@ var _ = Describe("QuestDBSnapshot Webhook", func() {
 			},
 			Spec: QuestDBSnapshotSpec{
 				QuestDBName:             q.Name,
-				VolumeSnapshotClassName: "csi-hostpath-snapclass",
+				VolumeSnapshotClassName: pointer.String("csi-hostpath-snapclass"),
 			},
 		}
 	})
@@ -56,10 +57,15 @@ var _ = Describe("QuestDBSnapshot Webhook", func() {
 		})
 
 		It("should reject empty volume snapshot class names", func() {
-			snap.Spec.VolumeSnapshotClassName = ""
+			snap.Spec.VolumeSnapshotClassName = pointer.String("")
 			Expect(k8sClient.Create(ctx, snap)).ToNot(Succeed())
 		})
 
+		It("should accept nil volume snapshot class names", func() {
+			snap.Spec.VolumeSnapshotClassName = nil
+			Expect(k8sClient.Create(ctx, snap)).To(Succeed())
+		})
+
 	})
 
 	Context("When validating QuestDBSnapshot Updates", func() {
@@ -72,7 +78,7 @@ var _ = Describe("QuestDBSnapshot Webhook", func() {
 
 		It("should reject updates to volume snapshot class names", func() {
 			Expect(k8sClient.Create(ctx, snap)).To(Succeed())
-			snap.Spec.VolumeSnapshotClassName = "foo"
+			snap.Spec.VolumeSnapshotClassName = pointer.String("foo")
 			Expect(k8sClient.Update(ctx, snap)).ToNot(Succeed())
 		})
 

api/v1beta1/questdbsnapshotschedule_webhook_test.go

@@ -7,6 +7,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/utils/pointer"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -40,7 +41,7 @@ var _ = Describe("QuestDBSnapshotSchedule Webhook", func() {
 			Spec: QuestDBSnapshotScheduleSpec{
 				Snapshot: QuestDBSnapshotSpec{
 					QuestDBName:             q.Name,
-					VolumeSnapshotClassName: "csi-hostpath-snapclass",
+					VolumeSnapshotClassName: pointer.String("csi-hostpath-snapclass"),
 				},
 				Schedule: "*/1 * * * *",
 			},
@@ -65,7 +66,7 @@ var _ = Describe("QuestDBSnapshotSchedule Webhook", func() {
 		})
 
 		It("should reject empty volume snapshot class names", func() {
-			sched.Spec.Snapshot.VolumeSnapshotClassName = ""
+			sched.Spec.Snapshot.VolumeSnapshotClassName = pointer.String("")
 			Expect(k8sClient.Create(ctx, sched)).ToNot(Succeed())
 		})
 
@@ -87,7 +88,7 @@ var _ = Describe("QuestDBSnapshotSchedule Webhook", func() {
 
 		It("should reject updates to volume snapshot class names", func() {
 			Expect(k8sClient.Create(ctx, sched)).To(Succeed())
-			sched.Spec.Snapshot.VolumeSnapshotClassName = "foo"
+			sched.Spec.Snapshot.VolumeSnapshotClassName = pointer.String("foo")
 			Expect(k8sClient.Update(ctx, sched)).ToNot(Succeed())
 		})
 

config/crd/bases/crd.questdb.io_questdbsnapshots.yaml

@@ -54,7 +54,6 @@ spec:
                 type: string
             required:
             - questdbName
-            - volumeSnapshotClassName
             type: object
           status:
             description: QuestDBSnapshotStatus defines the observed state of QuestDBSnapshot

config/crd/bases/crd.questdb.io_questdbsnapshotschedules.yaml

@@ -64,7 +64,6 @@ spec:
                     type: string
                 required:
                 - questdbName
-                - volumeSnapshotClassName
                 type: object
             required:
             - retention

config/manager/kustomization.yaml

@@ -5,4 +5,4 @@ kind: Kustomization
 images:
 - name: controller
   newName: questdb/questdb-operator
-  newTag: 0.0.1
+  newTag: 0.1.0

internal/controller/questdbsnapshot_controller.go

@@ -69,15 +69,8 @@ func (r *QuestDBSnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// Get status of any related secrets. These will be used later to add pgwire credentials
-	// to the pre- and post-snapshot jobs.
-	s, err := secrets.GetSecrets(ctx, r.Client, client.ObjectKeyFromObject(snap))
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	// Handle finalizer.  This will ensure that the "SNAPSHOT COMPLETE;" is run before the snapshot is deleted
-	if finalizerResult, err := r.handleFinalizer(ctx, snap, s); err != nil {
+	if finalizerResult, err := r.handleFinalizer(ctx, snap); err != nil {
 		return finalizerResult, err
 	}
 
@@ -99,11 +92,11 @@ func (r *QuestDBSnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	case "":
 		return r.handlePhaseEmpty(ctx, snap)
 	case crdv1beta1.SnapshotPending:
-		return r.handlePhasePending(ctx, snap, s)
+		return r.handlePhasePending(ctx, snap)
 	case crdv1beta1.SnapshotRunning:
 		return r.handlePhaseRunning(ctx, snap)
 	case crdv1beta1.SnapshotFinalizing:
-		return r.handlePhaseFinalizing(ctx, snap, s)
+		return r.handlePhaseFinalizing(ctx, snap)
 	case crdv1beta1.SnapshotFailed:
 		return r.handlePhaseFailed(ctx, snap)
 	case crdv1beta1.SnapshotSucceeded:
@@ -122,21 +115,28 @@ func (r *QuestDBSnapshotReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func (r *QuestDBSnapshotReconciler) buildPreSnapshotJob(snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets) (batchv1.Job, error) {
-	return r.buildGenericSnapshotJob(snap, s, "pre-snapshot", "SNAPSHOT PREPARE;")
+func (r *QuestDBSnapshotReconciler) buildPreSnapshotJob(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (batchv1.Job, error) {
+	return r.buildGenericSnapshotJob(ctx, snap, "pre-snapshot", "SNAPSHOT PREPARE;")
 }
 
-func (r *QuestDBSnapshotReconciler) buildPostSnapshotJob(snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets) (batchv1.Job, error) {
-	return r.buildGenericSnapshotJob(snap, s, "post-snapshot", "SNAPSHOT COMPLETE;")
+func (r *QuestDBSnapshotReconciler) buildPostSnapshotJob(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (batchv1.Job, error) {
+	return r.buildGenericSnapshotJob(ctx, snap, "post-snapshot", "SNAPSHOT COMPLETE;")
 }
 
-func (r *QuestDBSnapshotReconciler) buildGenericSnapshotJob(snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets, nameSuffix, command string) (batchv1.Job, error) {
+func (r *QuestDBSnapshotReconciler) buildGenericSnapshotJob(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot, nameSuffix, command string) (batchv1.Job, error) {
 	var (
 		err      error
 		user     = "admin"
 		password = "quest"
 	)
 
+	// Get status of any related secrets. These will be used later to add pgwire credentials
+	// to the pre- and post-snapshot jobs.
+	s, err := secrets.GetSecrets(ctx, r.Client, client.ObjectKeyFromObject(snap))
+	if err != nil {
+		return batchv1.Job{}, err
+	}
+
 	if s.PsqlSecret != nil {
 		if val, found := s.PsqlSecret.Data["QDB_PSQL_USER"]; found {
 			user = string(val)
@@ -218,20 +218,17 @@ func (r *QuestDBSnapshotReconciler) buildVolumeSnapshot(snap *crdv1beta1.QuestDB
 			Source: volumesnapshotv1.VolumeSnapshotSource{
 				PersistentVolumeClaimName: pointer.String(snap.Spec.QuestDBName),
 			},
+			VolumeSnapshotClassName: snap.Spec.VolumeSnapshotClassName,
 		},
 	}
 
-	if snap.Spec.VolumeSnapshotClassName != "" {
-		volSnap.Spec.VolumeSnapshotClassName = pointer.String(snap.Spec.VolumeSnapshotClassName)
-	}
-
 	err = ctrl.SetControllerReference(snap, &volSnap, r.Scheme)
 	return volSnap, err
 
 }
 
 // handleFinalizer is guaranteed to run before any other reconciliation logic
-func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets) (ctrl.Result, error) {
+func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (ctrl.Result, error) {
 	var err error
 
 	if snap.DeletionTimestamp.IsZero() {
@@ -269,7 +266,8 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
 						return ctrl.Result{}, err
 					}
 					r.Recorder.Eventf(snap, v1.EventTypeNormal, "SnapshotFinalizing", "Running 'SNAPSHOT COMPLETE;' for snapshot %s", snap.Name)
-					return r.handlePhaseFinalizing(ctx, snap, s)
+
+					return r.handlePhaseFinalizing(ctx, snap)
 				}
 
 				// If the job is active, we need to wait until it is not
@@ -359,14 +357,20 @@ func (r *QuestDBSnapshotReconciler) handlePhaseEmpty(ctx context.Context, snap *
 	return ctrl.Result{}, nil
 }
 
-func (r *QuestDBSnapshotReconciler) handlePhasePending(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets) (ctrl.Result, error) {
+func (r *QuestDBSnapshotReconciler) handlePhasePending(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (ctrl.Result, error) {
 
 	// Add the snapshot protection finalizer to the questdb
 	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 
 		questdb := &crdv1beta1.QuestDB{}
 		err := r.Get(ctx, client.ObjectKey{Name: snap.Spec.QuestDBName, Namespace: snap.Namespace}, questdb)
 		if err != nil {
+			// Fail the snapshot if the questdb is not found
+			if apierrors.IsNotFound(err) {
+				snap.Status.Phase = crdv1beta1.SnapshotFailed
+				err = r.Status().Update(ctx, snap)
+				r.Recorder.Eventf(snap, v1.EventTypeWarning, "SnapshotFailed", "QuestDB %s not found", snap.Spec.QuestDBName)
+			}
 			return err
 		}
 		if !controllerutil.ContainsFinalizer(questdb, crdv1beta1.QuestDBSnapshotProtectionFinalizer) {
@@ -380,7 +384,7 @@ func (r *QuestDBSnapshotReconciler) handlePhasePending(ctx context.Context, snap
 	}
 
 	// Create the pre-snapshot job
-	job, err := r.buildPreSnapshotJob(snap, s)
+	job, err := r.buildPreSnapshotJob(ctx, snap)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -414,6 +418,21 @@ func (r *QuestDBSnapshotReconciler) handlePhasePending(ctx context.Context, snap
 }
 
 func (r *QuestDBSnapshotReconciler) handlePhaseRunning(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (ctrl.Result, error) {
+	// Check that the volume snapshot class exists
+	if snap.Spec.VolumeSnapshotClassName != nil {
+		volSnapClass := &volumesnapshotv1.VolumeSnapshotClass{}
+		err := r.Get(ctx, client.ObjectKey{Name: *snap.Spec.VolumeSnapshotClassName}, volSnapClass)
+		if err != nil {
+			// If the volume snapshot class does not exist, fail the snapshot
+			if apierrors.IsNotFound(err) {
+				snap.Status.Phase = crdv1beta1.SnapshotFailed
+				err = r.Status().Update(ctx, snap)
+				r.Recorder.Eventf(snap, v1.EventTypeWarning, "SnapshotFailed", "VolumeSnapshotClass %s not found", *snap.Spec.VolumeSnapshotClassName)
+			}
+			return ctrl.Result{}, err
+		}
+	}
+
 	// Create the volume snapshot
 	volumeSnap, err := r.buildVolumeSnapshot(snap)
 	if err != nil {
@@ -443,10 +462,10 @@ func (r *QuestDBSnapshotReconciler) handlePhaseRunning(ctx context.Context, snap
 
 }
 
-func (r *QuestDBSnapshotReconciler) handlePhaseFinalizing(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot, s secrets.QuestDBSecrets) (ctrl.Result, error) {
+func (r *QuestDBSnapshotReconciler) handlePhaseFinalizing(ctx context.Context, snap *crdv1beta1.QuestDBSnapshot) (ctrl.Result, error) {
 
 	// Create the pre-snapshot job
-	job, err := r.buildPostSnapshotJob(snap, s)
+	job, err := r.buildPostSnapshotJob(ctx, snap)
 	if err != nil {
 		return ctrl.Result{}, err
 	}