Update Error Handling for Volume Functions

The getPVCName(), getRepoPVCNames() and getPGPVCNames() functions now return any errors encountered when obtaining PersistentVolumeClaim (PVC) names. This will ensure errors bubble up to the main reconcile functions as needed (for instance, to requeue in the event that an attempt to List PVCs fails due to a transient error), while also ensuring errors are handled differently that other non-error scenarios (e.g. currently an error when getting a PVC name produces the same result as a missing PVC). Issue: [ch11966]

Author: andrewlecuyer

internal/controller/postgrescluster/instance.go

@@ -1026,9 +1026,14 @@ func (r *Reconciler) reconcileInstance(
 spec, instanceCertificates, instanceConfigMap, &instance.Spec.Template)
 }
 
+// Get the pgBackRest repo PVC names
+var repoPVCNames map[string]string
+if err == nil {
+repoPVCNames, err = r.getRepoPVCNames(ctx, cluster)
+}
 // Add pgBackRest containers, volumes, etc. to the instance Pod spec
 if err == nil {
-err = addPGBackRestToInstancePodSpec(cluster, &instance.Spec.Template, instance, r.getRepoPVCNames(ctx, cluster))
+err = addPGBackRestToInstancePodSpec(cluster, &instance.Spec.Template, instance, repoPVCNames)
 }
 
 // Add pgMonitor resources to the instance Pod spec

internal/controller/postgrescluster/pgbackrest.go

@@ -581,8 +581,13 @@ func (r *Reconciler) generateRepoHostIntent(ctx context.Context,
 postgresCluster.Spec.Backups.PGBackRest.RepoHost.Dedicated.Resources); err != nil {
 return nil, errors.WithStack(err)
 }
+// Get the pgBackRest repo PVC names
+repoPVCNames, err := r.getRepoPVCNames(ctx, postgresCluster)
+if err != nil {
+return nil, errors.WithStack(err)
+}
 if err := pgbackrest.AddRepoVolumesToPod(postgresCluster, &repo.Spec.Template,
-r.getRepoPVCNames(ctx, postgresCluster), naming.PGBackRestRepoContainerName); err != nil {
+repoPVCNames, naming.PGBackRestRepoContainerName); err != nil {
 return nil, errors.WithStack(err)
 }
 // add configs to pod
@@ -623,7 +628,10 @@ func (r *Reconciler) generateRepoVolumeIntent(ctx context.Context,
 meta := naming.PGBackRestRepoVolume(postgresCluster, repoName)
 
 // but if there is an existing volume for this PVC, use it
-repoPVCNames := r.getRepoPVCNames(ctx, postgresCluster)
+repoPVCNames, err := r.getRepoPVCNames(ctx, postgresCluster)
+if err != nil {
+return nil, errors.WithStack(err)
+}
 if repoPVCNames[repoName] != "" {
 meta = metav1.ObjectMeta{
 Name: repoPVCNames[repoName],

internal/controller/postgrescluster/postgres.go

@@ -463,7 +463,11 @@ func (r *Reconciler) reconcilePostgresDataVolume(
 }
 
 var pvc *v1.PersistentVolumeClaim
-if existingPVCName := r.getPGPVCNames(ctx, cluster, labelMap); existingPVCName != "" {
+existingPVCName, err := r.getPGPVCNames(ctx, cluster, labelMap)
+if err != nil {
+return nil, errors.WithStack(err)
+}
+if existingPVCName != "" {
 pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
 Namespace: cluster.GetNamespace(),
 Name: existingPVCName,
@@ -474,7 +478,7 @@ func (r *Reconciler) reconcilePostgresDataVolume(
 
 pvc.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("PersistentVolumeClaim"))
 
-err := errors.WithStack(r.setControllerReference(cluster, pvc))
+err = errors.WithStack(r.setControllerReference(cluster, pvc))
 
 pvc.Annotations = naming.Merge(
 cluster.Spec.Metadata.GetAnnotationsOrNil(),
@@ -515,7 +519,11 @@ func (r *Reconciler) reconcilePostgresWALVolume(
 }
 
 var pvc *v1.PersistentVolumeClaim
-if existingPVCName := r.getPGPVCNames(ctx, cluster, labelMap); existingPVCName != "" {
+existingPVCName, err := r.getPGPVCNames(ctx, cluster, labelMap)
+if err != nil {
+return nil, errors.WithStack(err)
+}
+if existingPVCName != "" {
 pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
 Namespace: cluster.GetNamespace(),
 Name: existingPVCName,
@@ -572,7 +580,7 @@ func (r *Reconciler) reconcilePostgresWALVolume(
 return pvc, err
 }
 
-err := errors.WithStack(r.setControllerReference(cluster, pvc))
+err = errors.WithStack(r.setControllerReference(cluster, pvc))
 
 pvc.Annotations = naming.Merge(
 cluster.Spec.Metadata.GetAnnotationsOrNil(),

internal/controller/postgrescluster/volumes.go

@@ -210,15 +210,15 @@ func (r *Reconciler) getPVCName(
 ctx context.Context,
 cluster *v1beta1.PostgresCluster,
 selector labels.Selector,
-) string {
+) (string, error) {
 
 pvcs := &v1.PersistentVolumeClaimList{}
 err := r.Client.List(ctx, pvcs,
 client.InNamespace(cluster.Namespace),
 client.MatchingLabelsSelector{Selector: selector},
 )
 if err != nil {
-return ""
+return "", errors.WithStack(err)
 }
 
 if len(pvcs.Items) > 0 {
@@ -229,18 +229,18 @@ func (r *Reconciler) getPVCName(
 &pvcs.Items[j].CreationTimestamp)
 })
 
-return pvcs.Items[0].Name
+return pvcs.Items[0].Name, nil
 }
 
-return ""
+return "", nil
 }
 
 // getRepoPVCNames returns a map containing the names of repo PVCs that have
 // the appropriate labels for each defined pgBackRest repo, if found.
 func (r *Reconciler) getRepoPVCNames(
 ctx context.Context,
 cluster *v1beta1.PostgresCluster,
-) map[string]string {
+) (map[string]string, error) {
 
 repoPVCs := make(map[string]string)
 for _, repo := range cluster.Spec.Backups.PGBackRest.Repos {
@@ -260,29 +260,35 @@ func (r *Reconciler) getRepoPVCNames(
 MatchLabels: labelMap,
 })
 if err != nil {
-return nil
+return nil, errors.WithStack(err)
 }
 
-repoPVCs[repo.Name] = r.getPVCName(ctx, cluster, selector)
+repoPVCs[repo.Name], err = r.getPVCName(ctx, cluster, selector)
+if err != nil {
+return nil, errors.WithStack(err)
+}
 }
 
-return repoPVCs
+return repoPVCs, nil
 }
 
 // getPGPVCName returns the name of a PVC that has the provided labels, if found.
 func (r *Reconciler) getPGPVCNames(
 ctx context.Context,
 cluster *v1beta1.PostgresCluster, labelMap map[string]string,
-) string {
+) (string, error) {
 
 selector, err := naming.AsSelector(metav1.LabelSelector{
 MatchLabels: labelMap,
 })
 if err != nil {
-return ""
+return "", errors.WithStack(err)
 }
 
-name := r.getPVCName(ctx, cluster, selector)
+name, err := r.getPVCName(ctx, cluster, selector)
+if err != nil {
+return "", errors.WithStack(err)
+}
 
-return name
+return name, nil
 }

internal/controller/postgrescluster/volumes_test.go

@@ -574,35 +574,49 @@ func TestGetPVCNameMethods(t *testing.T) {
 })
 
 assert.NilError(t, err)
-assert.Assert(t, reconciler.getPVCName(ctx, cluster, selector) == "testpgdatavol")
+
+pvcName, err := reconciler.getPVCName(ctx, cluster, selector)
+assert.NilError(t, err)
+
+assert.Assert(t, pvcName == "testpgdatavol")
 
 })
 
 t.Run("get pgdata PVC", func(t *testing.T) {
 
-assert.Assert(t, reconciler.getPGPVCNames(ctx, cluster, map[string]string{
+pvcNames, err := reconciler.getPGPVCNames(ctx, cluster, map[string]string{
 naming.LabelCluster: cluster.Name,
 naming.LabelInstanceSet: "testinstance1",
 naming.LabelInstance: "testinstance1-abcd",
 naming.LabelRole: naming.RolePostgresData,
-}) == "testpgdatavol")
+})
+assert.NilError(t, err)
+
+assert.Assert(t, pvcNames == "testpgdatavol")
 })
 
 t.Run("get wal PVC", func(t *testing.T) {
 
-assert.Assert(t, reconciler.getPGPVCNames(ctx, cluster, map[string]string{
+pvcNames, err := reconciler.getPGPVCNames(ctx, cluster, map[string]string{
 naming.LabelCluster: cluster.Name,
 naming.LabelInstanceSet: "testinstance1",
 naming.LabelInstance: "testinstance1-abcd",
 naming.LabelRole: naming.RolePostgresWAL,
-}) == "testwalvol")
+})
+assert.NilError(t, err)
+
+assert.Assert(t, pvcNames == "testwalvol")
 })
 
 t.Run("get one repo PVC", func(t *testing.T) {
 expectedMap := map[string]string{
 "testrepo1": "testrepovol1",
 }
-assert.DeepEqual(t, reconciler.getRepoPVCNames(ctx, cluster), expectedMap)
+
+repoPVCNames, err := reconciler.getRepoPVCNames(ctx, cluster)
+assert.NilError(t, err)
+
+assert.DeepEqual(t, repoPVCNames, expectedMap)
 })
 
 t.Run("get two repo PVCs", func(t *testing.T) {
@@ -620,6 +634,10 @@ func TestGetPVCNameMethods(t *testing.T) {
 "testrepo1": "testrepovol1",
 "testrepo2": "testrepovol2",
 }
-assert.DeepEqual(t, reconciler.getRepoPVCNames(ctx, cluster), expectedMap)
+
+repoPVCNames, err := reconciler.getRepoPVCNames(ctx, cluster)
+assert.NilError(t, err)
+
+assert.DeepEqual(t, repoPVCNames, expectedMap)
 })
 }