Adjust function that provides the log path for cloud backup jobs and add tests.

Author: dsessler7

internal/controller/postgrescluster/pgbackrest.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"sort"
@@ -823,7 +824,7 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
 		}
 	} else {
 		mkdirCommand := ""
-		cloudLogPath := r.reconcileCloudLogPath(ctx, postgresCluster)
+		cloudLogPath := getCloudLogPath(postgresCluster)
 		if cloudLogPath != "" {
 			mkdirCommand += shell.MakeDirectories(cloudLogPath, cloudLogPath) + "; "
 		}
@@ -892,8 +893,8 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
 		pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)
 
 		// Mount the PVC named in the "pgbackrest-cloud-log-volume" annotation, if any.
-		if logVolumeName := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolumeName != "" {
-			util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolumeName)
+		if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
+			util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
 		}
 	}
 
@@ -2082,7 +2083,7 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context,
 	repoHostName, configHash, serviceName, serviceNamespace string,
 	instanceNames []string) error {
 
-	cloudLogPath := r.reconcileCloudLogPath(ctx, postgresCluster)
+	cloudLogPath := getCloudLogPath(postgresCluster)
 
 	backrestConfig, err := pgbackrest.CreatePGBackRestConfigMapIntent(ctx, postgresCluster, repoHostName,
 		configHash, serviceName, serviceNamespace, cloudLogPath, instanceNames)
@@ -3338,39 +3339,22 @@ func authorizeBackupRemovalAnnotationPresent(postgresCluster *v1beta1.PostgresCl
 	return false
 }
 
-// reconcileCloudLogPath is responsible for determining the appropriate log path
-// for pgbackrest in cloud backup jobs.
-func (r *Reconciler) reconcileCloudLogPath(ctx context.Context,
-	postgresCluster *v1beta1.PostgresCluster) string {
-	// If the user has specified a PVC to use as a log volume for cloud backups via the
-	// PGBackRestCloudLogVolume annotation, check for the PVC. If we find it, set the cloud
-	// log path. If the user has specified a PVC, but we can't find it, create a warning event.
-	// If the user has not set the PGBackRestCloudLogVolume annotation, but has set a log
-	// path via the spec, use that.
-	// TODO: Make sure this is what we want (i.e. annotation to take precedence over spec)
+// getCloudLogPath is responsible for determining the appropriate log path for pgbackrest
+// in cloud backup jobs. If the user has specified a PVC to use as a log volume for cloud
+// backups via the PGBackRestCloudLogVolume annotation, set the cloud log path accordingly.
+// If the user has not set the PGBackRestCloudLogVolume annotation, but has set a log path
+// via the spec, use that.
+// TODO: Make sure this is what we want (i.e. annotation to take precedence over spec)
+//
+// This function assumes that the backups/pgbackrest spec is present in postgresCluster.
+func getCloudLogPath(postgresCluster *v1beta1.PostgresCluster) string {
 	cloudLogPath := ""
-	if logVolumeName := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolumeName != "" {
-		logVolume := &corev1.PersistentVolumeClaim{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      logVolumeName,
-				Namespace: postgresCluster.GetNamespace(),
-			},
-		}
-		err := errors.WithStack(r.Client.Get(ctx,
-			client.ObjectKeyFromObject(logVolume), logVolume))
-		if err != nil {
-			// PVC not retrieved, create warning event
-			r.Recorder.Event(postgresCluster, corev1.EventTypeWarning,
-				"PGBackRestCloudLogVolumeNotFound", err.Error())
-		} else {
-			// We successfully found the specified PVC, so we will set the log path
-			cloudLogPath = "/volumes/" + logVolumeName
-		}
-		// TODO: Can we safely assume that backups are enabled?
+	if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
+		cloudLogPath = "/volumes/" + logVolume
 	} else if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
 		postgresCluster.Spec.Backups.PGBackRest.Jobs.Log != nil &&
 		postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path != "" {
-		cloudLogPath = postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path
+		cloudLogPath = filepath.Clean(postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path)
 	}
 	return cloudLogPath
 }

internal/controller/postgrescluster/pgbackrest_test.go

@@ -4559,3 +4559,51 @@ func TestGetRepoHostVolumeRequests(t *testing.T) {
 		})
 	}
 }
+
+func TestGetCloudLogPath(t *testing.T) {
+	t.Run("NoAnnotationNoSpecPath", func(t *testing.T) {
+		postgrescluster := &v1beta1.PostgresCluster{}
+		assert.Equal(t, getCloudLogPath(postgrescluster), "")
+	})
+
+	t.Run("AnnotationSetNoSpecPath", func(t *testing.T) {
+		postgrescluster := &v1beta1.PostgresCluster{}
+		postgrescluster.Annotations = map[string]string{}
+		postgrescluster.Annotations[naming.PGBackRestCloudLogVolume] = "another-pvc"
+		assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/another-pvc")
+	})
+
+	t.Run("NoAnnotationSpecPathSet", func(t *testing.T) {
+		postgrescluster := &v1beta1.PostgresCluster{
+			Spec: v1beta1.PostgresClusterSpec{
+				Backups: v1beta1.Backups{
+					PGBackRest: v1beta1.PGBackRestArchive{
+						Jobs: &v1beta1.BackupJobs{
+							Log: &v1beta1.LoggingConfiguration{
+								Path: "/volumes/test/log/",
+							},
+						},
+					},
+				},
+			},
+		}
+		assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/test/log")
+	})
+
+	t.Run("BothAnnotationAndSpecPathSet", func(t *testing.T) {
+		postgrescluster := &v1beta1.PostgresCluster{
+			Spec: v1beta1.PostgresClusterSpec{
+				Backups: v1beta1.Backups{
+					PGBackRest: v1beta1.PGBackRestArchive{
+						Log: &v1beta1.LoggingConfiguration{
+							Path: "/volumes/test/log",
+						},
+					},
+				},
+			},
+		}
+		postgrescluster.Annotations = map[string]string{}
+		postgrescluster.Annotations[naming.PGBackRestCloudLogVolume] = "another-pvc"
+		assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/another-pvc")
+	})
+}