Remove tablespaces stuff from snapshots stuff and add early exit if both features are enabled. Add string length validation to volume snapshot class name. If multiple snapshots with errors exist, delete all but the one with the latest error. Add/adjust tests.

Author: dsessler7

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

@@ -4330,6 +4330,7 @@ spec:
                       volumeSnapshotClassName:
                         description: Name of the VolumeSnapshotClass that should be
                           used by VolumeSnapshots
+                        minLength: 1
                         type: string
                     required:
                     - volumeSnapshotClassName

config/rbac/cluster/role.yaml

@@ -163,14 +163,6 @@ rules:
   - list
   - patch
   - watch
-- apiGroups:
-  - snapshot.storage.k8s.io
-  resources:
-  - volumesnapshotclasses
-  verbs:
-  - get
-  - list
-  - watch
 - apiGroups:
   - snapshot.storage.k8s.io
   resources:

config/rbac/namespace/role.yaml

@@ -163,14 +163,6 @@ rules:
   - list
   - patch
   - watch
-- apiGroups:
-  - snapshot.storage.k8s.io
-  resources:
-  - volumesnapshotclasses
-  verbs:
-  - get
-  - list
-  - watch
 - apiGroups:
   - snapshot.storage.k8s.io
   resources:

internal/controller/postgrescluster/controller.go

@@ -365,8 +365,7 @@ func (r *Reconciler) Reconcile(
 		}
 	}
 	if err == nil {
-		dedicatedSnapshotPVC, err = r.reconcileDedicatedSnapshotVolume(ctx, cluster, instances,
-			clusterVolumes)
+		dedicatedSnapshotPVC, err = r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)
 	}
 	if err == nil {
 		err = r.reconcileVolumeSnapshots(ctx, cluster, dedicatedSnapshotPVC)

internal/controller/postgrescluster/snapshots.go

@@ -28,7 +28,6 @@ import (
 )
 
 // +kubebuilder:rbac:groups="snapshot.storage.k8s.io",resources="volumesnapshots",verbs={get,list,create,patch,delete}
-// +kubebuilder:rbac:groups="snapshot.storage.k8s.io",resources="volumesnapshotclasses",verbs={get,list}
 
 // reconcileVolumeSnapshots creates and manages VolumeSnapshots if the proper VolumeSnapshot CRDs
 // are installed and VolumeSnapshots are enabled for the PostgresCluster. A VolumeSnapshot of the
@@ -52,10 +51,6 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
 	// Get feature gate state
 	volumeSnapshotsFeatureEnabled := feature.Enabled(ctx, feature.VolumeSnapshots)
 
-	// TODO(snapshots): If user has a snapshots section in their cluster spec, indicating
-	// that they would like to use snapshots, but the VolumeSnapshots feature gate is not
-	// turned on, should we emit an event or log something?
-
 	// Check if the Kube cluster has VolumeSnapshots installed. If VolumeSnapshots
 	// are not installed, we need to return early. If user is attempting to use
 	// VolumeSnapshots, return an error, otherwise return nil.
@@ -71,6 +66,16 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
 		}
 	}
 
+	// If user is attempting to use snapshots and has tablespaces enabled, we
+	// need to create a warning event indicating that the two features are not
+	// currently compatible and return early.
+	if postgrescluster.Spec.Backups.Snapshots != nil && volumeSnapshotsFeatureEnabled &&
+		clusterUsingTablespaces(ctx, postgrescluster) {
+		r.Recorder.Event(postgrescluster, corev1.EventTypeWarning, "IncompatibleFeatures",
+			"VolumeSnapshots not currently compatible with TablespaceVolumes; cannot create snapshot.")
+		return nil
+	}
+
 	// Get all snapshots for the cluster.
 	snapshots, err := r.getSnapshotsForCluster(ctx, postgrescluster)
 	if err != nil {
@@ -85,24 +90,22 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
 	// If we got here, then the snapshots are enabled (feature gate is enabled and the
 	// cluster has a Spec.Backups.Snapshots section defined).
 
-	// The specified VolumeSnapshotClass needs to exist.
-	volumeSnapshotClass := &volumesnapshotv1.VolumeSnapshotClass{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: postgrescluster.Spec.Backups.Snapshots.VolumeSnapshotClassName,
-		},
-	}
-	err = errors.WithStack(r.Client.Get(ctx, client.ObjectKeyFromObject(volumeSnapshotClass),
-		volumeSnapshotClass))
-	if err != nil {
-		return err
-	}
-
-	// Check snapshots for errors; if present, create an event. If there
-	// are multiple snapshots with errors, create event for the latest error.
-	latestSnapshotWithError := getLatestSnapshotWithError(snapshots)
-	if latestSnapshotWithError != nil {
+	// Check snapshots for errors; if present, create an event. If there are
+	// multiple snapshots with errors, create event for the latest error and
+	// delete any older snapshots with error.
+	snapshotWithLatestError := getSnapshotWithLatestError(snapshots)
+	if snapshotWithLatestError != nil {
 		r.Recorder.Event(postgrescluster, corev1.EventTypeWarning, "VolumeSnapshotError",
-			*latestSnapshotWithError.Status.Error.Message)
+			*snapshotWithLatestError.Status.Error.Message)
+		for _, snapshot := range snapshots.Items {
+			if snapshot.Status.Error != nil &&
+				snapshot.Status.Error.Time.Before(snapshotWithLatestError.Status.Error.Time) {
+				err = r.deleteControlled(ctx, postgrescluster, &snapshot)
+				if err != nil {
+					return err
+				}
+			}
+		}
 	}
 
 	// Get pvc backup job completion annotation. If it does not exist, there has not been
@@ -125,7 +128,8 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
 
 	// If a snapshot exists for the latest backup that has been restored into the dedicated pvc
 	// and the snapshot is Ready, delete all other snapshots.
-	if snapshotFoundForPvcUpdate && *snapshots.Items[snapshotForPvcUpdateIdx].Status.ReadyToUse {
+	if snapshotFoundForPvcUpdate && snapshots.Items[snapshotForPvcUpdateIdx].Status.ReadyToUse != nil &&
+		*snapshots.Items[snapshotForPvcUpdateIdx].Status.ReadyToUse {
 		for idx, snapshot := range snapshots.Items {
 			if idx != snapshotForPvcUpdateIdx {
 				err = r.deleteControlled(ctx, postgrescluster, &snapshot)
@@ -157,7 +161,6 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
 // after a successful backup.
 func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
-	instances *observedInstances,
 	clusterVolumes []corev1.PersistentVolumeClaim,
 ) (*corev1.PersistentVolumeClaim, error) {
 
@@ -219,19 +222,11 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 		return pvc, nil
 	}
 
-	// If the pvc has not been annotated, it has not been restored and we want to proceed
-	// with a restore.
-	pvcAnnotationTimestampString, pvcAnnotationExists := pvc.GetAnnotations()[naming.PGBackRestBackupJobCompletion]
-	if pvcAnnotationExists {
-		// If pvc has been annotated, we want to compare its timestamp to the latest backup job's
-		// timestamp. If the pvc's timestamp is older than the latest backup job's
-		// completion time, then we want to do a restore.
-		backupJobCompletionTime := backupJob.Status.CompletionTime.Time
-		pvcAnnotationTime, err := time.Parse(time.RFC3339, pvcAnnotationTimestampString)
-		if err != nil {
-			return pvc, err
-		}
-		if backupJobCompletionTime.Compare(pvcAnnotationTime) <= 0 {
+	// Return early if the pvc is annotated with a timestamp newer or equal to the latest backup job.
+	// If the annotation value cannot be parsed, we want to proceed with a restore.
+	pvcAnnotationTimestampString := pvc.GetAnnotations()[naming.PGBackRestBackupJobCompletion]
+	if pvcAnnotationTime, err := time.Parse(time.RFC3339, pvcAnnotationTimestampString); err == nil {
+		if backupJob.Status.CompletionTime.Compare(pvcAnnotationTime) <= 0 {
 			return pvc, nil
 		}
 	}
@@ -250,7 +245,7 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 
 	// If we don't find a restore job, we run one.
 	if restoreJob == nil {
-		err = r.dedicatedSnapshotVolumeRestore(ctx, cluster, pvc, instances, clusterVolumes, backupJob)
+		err = r.dedicatedSnapshotVolumeRestore(ctx, cluster, pvc, backupJob)
 		return pvc, err
 	}
 
@@ -339,7 +334,6 @@ func (r *Reconciler) createDedicatedSnapshotVolume(ctx context.Context,
 // dedicated snapshot volume.
 func (r *Reconciler) dedicatedSnapshotVolumeRestore(ctx context.Context,
 	cluster *v1beta1.PostgresCluster, dedicatedSnapshotVolume *corev1.PersistentVolumeClaim,
-	instances *observedInstances, clusterVolumes []corev1.PersistentVolumeClaim,
 	backupJob *batchv1.Job,
 ) error {
 
@@ -351,25 +345,9 @@ func (r *Reconciler) dedicatedSnapshotVolumeRestore(ctx context.Context,
 		"--pg1-path=" + pgdata,
 		"--repo=" + regexRepoIndex.FindString(repoName),
 		"--delta",
-		// "--link-all",
 	}
 
-	// NOTE: It seems like our current architecture is forcing us to care about the instanceSet/instance
-	// and consistently use the same one as the user *could* have multiple instanceSets with different
-	// storage settings. Do we want to simply always choose the first instanceSet and instance in their
-	// respective arrays? Or do we want to add something to the API for the user to specify an instanceSet?
-	// For now, I am choosing the first instanceSet/instance in their arrays.
-	instanceSet := &cluster.Spec.InstanceSets[0]
-	instance := instances.bySet[instanceSet.Name][0].Runner
-
-	// How are we going to handle tablespace volumes? Feels like if they exist we need to provide them to
-	// the restore command otherwise they will break in any clones made from a snapshot.
-	pgtablespaces, err := r.reconcileTablespaceVolumes(ctx, cluster, instanceSet, instance, clusterVolumes)
-	if err != nil {
-		return errors.WithStack(err)
-	}
-
-	cmd := pgbackrest.DedicatedSnapshotVolumeRestoreCommand(pgdata, pgtablespaces, strings.Join(opts, " "))
+	cmd := pgbackrest.DedicatedSnapshotVolumeRestoreCommand(pgdata, strings.Join(opts, " "))
 
 	// Create the volume resources required for the Postgres data directory.
 	dataVolumeMount := postgres.DataVolumeMount()
@@ -539,28 +517,30 @@ func (r *Reconciler) getLatestCompleteBackupJob(ctx context.Context,
 	return &latestCompleteBackupJob, nil
 }
 
-// getLatestSnapshotWithError takes a VolumeSnapshotList and returns a pointer to the
-// most recently created snapshot that has an error. If no snapshot errors exist
+// getSnapshotWithLatestError takes a VolumeSnapshotList and returns a pointer to the
+// snapshot that has most recently had an error. If no snapshot errors exist
 // then it returns nil.
-func getLatestSnapshotWithError(snapshots *volumesnapshotv1.VolumeSnapshotList) *volumesnapshotv1.VolumeSnapshot {
+func getSnapshotWithLatestError(snapshots *volumesnapshotv1.VolumeSnapshotList) *volumesnapshotv1.VolumeSnapshot {
 	zeroTime := metav1.NewTime(time.Time{})
-	latestSnapshotWithError := volumesnapshotv1.VolumeSnapshot{
+	snapshotWithLatestError := volumesnapshotv1.VolumeSnapshot{
 		Status: &volumesnapshotv1.VolumeSnapshotStatus{
-			CreationTime: &zeroTime,
+			Error: &volumesnapshotv1.VolumeSnapshotError{
+				Time: &zeroTime,
+			},
 		},
 	}
 	for _, snapshot := range snapshots.Items {
 		if snapshot.Status.Error != nil &&
-			latestSnapshotWithError.Status.CreationTime.Before(snapshot.Status.CreationTime) {
-			latestSnapshotWithError = snapshot
+			snapshotWithLatestError.Status.Error.Time.Before(snapshot.Status.Error.Time) {
+			snapshotWithLatestError = snapshot
 		}
 	}
 
-	if latestSnapshotWithError.Status.CreationTime.Equal(&zeroTime) {
+	if snapshotWithLatestError.Status.Error.Time.Equal(&zeroTime) {
 		return nil
 	}
 
-	return &latestSnapshotWithError
+	return &snapshotWithLatestError
 }
 
 // getSnapshotsForCluster gets all the VolumeSnapshots for a given postgrescluster.
@@ -590,7 +570,7 @@ func getLatestReadySnapshot(snapshots *volumesnapshotv1.VolumeSnapshotList) *vol
 		},
 	}
 	for _, snapshot := range snapshots.Items {
-		if *snapshot.Status.ReadyToUse &&
+		if snapshot.Status.ReadyToUse != nil && *snapshot.Status.ReadyToUse &&
 			latestReadySnapshot.Status.CreationTime.Before(snapshot.Status.CreationTime) {
 			latestReadySnapshot = snapshot
 		}
@@ -617,3 +597,14 @@ func (r *Reconciler) deleteSnapshots(ctx context.Context,
 	}
 	return nil
 }
+
+// tablespaceVolumesInUse determines if the TablespaceVolumes feature is enabled and the given
+// cluster has tablespace volumes in place.
+func clusterUsingTablespaces(ctx context.Context, postgrescluster *v1beta1.PostgresCluster) bool {
+	for _, instanceSet := range postgrescluster.Spec.InstanceSets {
+		if instanceSet.TablespaceVolumes != nil && len(instanceSet.TablespaceVolumes) > 0 {
+			return feature.Enabled(ctx, feature.TablespaceVolumes)
+		}
+	}
+	return false
+}