Add go tests for latest iteration of volume snapshots feature. Other minor changes.

Author: dsessler7

internal/controller/postgrescluster/helpers_test.go

@@ -141,17 +141,19 @@ func testCluster() *v1beta1.PostgresCluster {
 	return cluster.DeepCopy()
 }
 
-func testJob(clusterName string) *batchv1.Job {
+func testBackupJob(cluster *v1beta1.PostgresCluster) *batchv1.Job {
 	job := batchv1.Job{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: batchv1.SchemeGroupVersion.String(),
 			Kind:       "Job",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "backup-job-1",
+			Name:      "backup-job-1",
+			Namespace: cluster.Namespace,
 			Labels: map[string]string{
-				naming.LabelCluster:          clusterName,
+				naming.LabelCluster:          cluster.Name,
 				naming.LabelPGBackRestBackup: "",
+				naming.LabelPGBackRestRepo:   "repo1",
 			},
 		},
 		Spec: batchv1.JobSpec{
@@ -167,6 +169,30 @@ func testJob(clusterName string) *batchv1.Job {
 	return job.DeepCopy()
 }
 
+func testRestoreJob(cluster *v1beta1.PostgresCluster) *batchv1.Job {
+	job := batchv1.Job{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: batchv1.SchemeGroupVersion.String(),
+			Kind:       "Job",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "restore-job-1",
+			Namespace: cluster.Namespace,
+			Labels:    naming.PGBackRestRestoreJobLabels(cluster.Name),
+		},
+		Spec: batchv1.JobSpec{
+			Template: corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers:    []corev1.Container{{Name: "test", Image: "test"}},
+					RestartPolicy: corev1.RestartPolicyNever,
+				},
+			},
+		},
+	}
+
+	return job.DeepCopy()
+}
+
 // setupManager creates the runtime manager used during controller testing
 func setupManager(t *testing.T, cfg *rest.Config,
 	controllerSetup func(mgr manager.Manager)) (context.Context, context.CancelFunc) {

internal/controller/postgrescluster/snapshots.go

@@ -182,11 +182,9 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 			Namespace: cluster.GetNamespace(),
 			Name:      existingPVCName,
 		}}
-		pvc.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("PersistentVolumeClaim"))
 	} else {
 		pvc = &corev1.PersistentVolumeClaim{ObjectMeta: naming.ClusterDedicatedSnapshotVolume(cluster)}
 	}
-
 	pvc.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("PersistentVolumeClaim"))
 
 	// If snapshots are disabled, delete the PVC if it exists and return early.
@@ -236,13 +234,13 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 	// job in existence at a time.
 	// TODO(dsessler7): Should this function throw an error or something if multiple
 	// DSV restores somehow exist?
-	restoreJobFound, restoreJob, err := r.getDedicatedSnapshotVolumeRestoreJob(ctx, cluster)
+	restoreJob, err := r.getDedicatedSnapshotVolumeRestoreJob(ctx, cluster)
 	if err != nil {
 		return pvc, err
 	}
 
 	// If we don't find a restore job, we run one.
-	if !restoreJobFound {
+	if restoreJob == nil {
 		err = r.dedicatedSnapshotVolumeRestore(ctx, cluster, pvc, instances, clusterVolumes, backupJob)
 		return pvc, err
 	}
@@ -281,7 +279,7 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
 }
 
 // createDedicatedSnapshotVolume creates/updates/gets the dedicated snapshot volume.
-// It expects that the volume name has already been set on the pvc that is passed in.
+// It expects that the volume name and GVK has already been set on the pvc that is passed in.
 // TODO(dsessler7): The above fact makes this method feel a little awkward to me.
 // Not sure this code really needs its own method. I'm tempted to just move it back
 // to reconcileDedicatedSnapshotVolume.
@@ -400,19 +398,6 @@ func (r *Reconciler) dedicatedSnapshotVolumeRestore(ctx context.Context,
 	restoreJob.Spec.BackoffLimit = initialize.Int32(0)
 	restoreJob.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyNever
 
-	jobs := &batchv1.JobList{}
-	selectJobs, err := naming.AsSelector(naming.ClusterBackupJobs(cluster.Name))
-	if err == nil {
-		err = errors.WithStack(
-			r.Client.List(ctx, jobs,
-				client.InNamespace(cluster.Namespace),
-				client.MatchingLabelsSelector{Selector: selectJobs},
-			))
-	}
-	if err != nil {
-		return err
-	}
-
 	// Add pgBackRest configs to template.
 	pgbackrest.AddConfigToRestorePod(cluster, cluster, &restoreJob.Spec.Template.Spec)
 
@@ -433,7 +418,8 @@ func (r *Reconciler) dedicatedSnapshotVolumeRestore(ctx context.Context,
 // provided backup job's UID.
 func (r *Reconciler) generateSnapshotOfDedicatedSnapshotVolume(
 	postgrescluster *v1beta1.PostgresCluster,
-	dedicatedSnapshotVolume *corev1.PersistentVolumeClaim) (*volumesnapshotv1.VolumeSnapshot, error) {
+	dedicatedSnapshotVolume *corev1.PersistentVolumeClaim,
+) (*volumesnapshotv1.VolumeSnapshot, error) {
 
 	snapshot, err := r.generateVolumeSnapshot(postgrescluster, *dedicatedSnapshotVolume,
 		postgrescluster.Spec.Backups.Snapshots.VolumeSnapshotClassName)
@@ -451,8 +437,8 @@ func (r *Reconciler) generateSnapshotOfDedicatedSnapshotVolume(
 // PersistentVolumeClaim and VolumeSnapshotClassName and will set the provided
 // PostgresCluster as the owner.
 func (r *Reconciler) generateVolumeSnapshot(postgrescluster *v1beta1.PostgresCluster,
-	pvc corev1.PersistentVolumeClaim,
-	volumeSnapshotClassName string) (*volumesnapshotv1.VolumeSnapshot, error) {
+	pvc corev1.PersistentVolumeClaim, volumeSnapshotClassName string,
+) (*volumesnapshotv1.VolumeSnapshot, error) {
 
 	snapshot := &volumesnapshotv1.VolumeSnapshot{
 		TypeMeta: metav1.TypeMeta{
@@ -478,12 +464,11 @@ func (r *Reconciler) generateVolumeSnapshot(postgrescluster *v1beta1.PostgresClu
 // getDedicatedSnapshotVolumeRestoreJob finds a dedicated snapshot volume (DSV)
 // restore job if one exists. Since we delete successful restore jobs and stop
 // creating new restore jobs when one fails, there should only ever be one DSV
-// restore job available at a time.
+// restore job present at a time. If a DSV restore cannot be found, we return nil.
 func (r *Reconciler) getDedicatedSnapshotVolumeRestoreJob(ctx context.Context,
-	postgrescluster *v1beta1.PostgresCluster) (bool, *batchv1.Job, error) {
+	postgrescluster *v1beta1.PostgresCluster) (*batchv1.Job, error) {
 
 	// Get all restore jobs for this cluster
-	var restoreJobFound bool
 	jobs := &batchv1.JobList{}
 	selectJobs, err := naming.AsSelector(naming.ClusterRestoreJobs(postgrescluster.Name))
 	if err == nil {
@@ -494,20 +479,18 @@ func (r *Reconciler) getDedicatedSnapshotVolumeRestoreJob(ctx context.Context,
 			))
 	}
 	if err != nil {
-		return restoreJobFound, nil, err
+		return nil, err
 	}
 
 	// Get restore job that has PGBackRestBackupJobCompletion annotation
-	var restoreJob batchv1.Job
 	for _, job := range jobs.Items {
 		_, annotationExists := job.GetAnnotations()[naming.PGBackRestBackupJobCompletion]
 		if annotationExists {
-			restoreJob = job
-			restoreJobFound = true
+			return &job, nil
 		}
 	}
 
-	return restoreJobFound, &restoreJob, nil
+	return nil, nil
 }
 
 // getLatestCompleteBackupJob finds the most recently completed
@@ -614,6 +597,8 @@ func getLatestReadySnapshot(snapshots *volumesnapshotv1.VolumeSnapshotList) *vol
 	return &latestReadySnapshot
 }
 
+// deleteSnapshots takes a postgrescluster and a snapshot list and deletes all snapshots
+// in the list that are controlled by the provided postgrescluster.
 func (r *Reconciler) deleteSnapshots(ctx context.Context,
 	postgrescluster *v1beta1.PostgresCluster, snapshots *volumesnapshotv1.VolumeSnapshotList) error {