From 8d8cd5c435a76007a0459cb6b8b1abca726967b1 Mon Sep 17 00:00:00 2001 From: TJ Moore Date: Wed, 10 Sep 2025 17:55:43 -0400 Subject: [PATCH 1/2] Update repo volume status handling This update ensures that the repo volume status is preserved in cases where there is an error when applying the repo volume. When there is an error, the status is now preserved until the underlying issue is corrected. Issue: PGO-2654 --- .../controller/postgrescluster/pgbackrest.go | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index d31350fd42..f4d43d827d 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -2743,7 +2743,7 @@ func (r *Reconciler) reconcileRepos(ctx context.Context, errors := []error{} errMsg := "reconciling repository volume" - repoVols := []*corev1.PersistentVolumeClaim{} + repoVols := make(map[string]*corev1.PersistentVolumeClaim) var replicaCreateRepo v1beta1.PGBackRestRepo if feature.Enabled(ctx, feature.AutoGrowVolumes) && pgbackrest.RepoHostVolumeDefined(postgresCluster) { @@ -2770,16 +2770,15 @@ func (r *Reconciler) reconcileRepos(ctx context.Context, // value to change later. spec.Resources.Limits = nil - repo, err := r.applyRepoVolumeIntent(ctx, postgresCluster, spec, + repoPVC, err := r.applyRepoVolumeIntent(ctx, postgresCluster, spec, repo.Name, repoResources) if err != nil { log.Error(err, errMsg) errors = append(errors, err) - continue - } - if repo != nil { - repoVols = append(repoVols, repo) } + // Store the repo volume after apply. If nil, that indicates a problem + // and the existing status should be preserved. + repoVols[repo.Name] = repoPVC } postgresCluster.Status.PGBackRest.Repos = @@ -2977,7 +2976,7 @@ func getRepoHostStatus(repoHost *appsv1.StatefulSet) *v1beta1.RepoHostStatus { // existing/current status for any repos in the cluster, the repository volumes // (i.e. PVCs) reconciled for the cluster, and the hashes calculated for the configuration for any // external repositories defined for the cluster. -func getRepoVolumeStatus(repoStatus []v1beta1.RepoStatus, repoVolumes []*corev1.PersistentVolumeClaim, +func getRepoVolumeStatus(repoStatus []v1beta1.RepoStatus, repoVolumes map[string]*corev1.PersistentVolumeClaim, configHashes map[string]string, replicaCreateRepoName string) []v1beta1.RepoStatus { // the new repository status that will be generated and returned @@ -2985,11 +2984,18 @@ func getRepoVolumeStatus(repoStatus []v1beta1.RepoStatus, repoVolumes []*corev1. // Update the repo status based on the repo volumes (PVCs) that were reconciled. This includes // updating the status for any existing repository volumes, and adding status for any new - // repository volumes. - for _, rv := range repoVolumes { + // repository volumes. If there was a problem with the volume when an apply was attempted, + // the existing status is preserved. + for repoName, rv := range repoVolumes { newRepoVolStatus := true - repoName := rv.Labels[naming.LabelPGBackRestRepo] for _, rs := range repoStatus { + // Preserve the previous status if it exists and the apply failed. + if rs.Name == repoName && rv == nil { + updatedRepoStatus = append(updatedRepoStatus, rs) + newRepoVolStatus = false + break + } + // treat as new status if contains properties of a cloud (s3, gcr or azure) repo if rs.Name == repoName && rs.RepoOptionsHash == "" { newRepoVolStatus = false From b54f123410e1a03e3b021a7de0ca47a0f240879f Mon Sep 17 00:00:00 2001 From: TJ Moore Date: Thu, 11 Sep 2025 11:53:37 -0400 Subject: [PATCH 2/2] Volume auto-grow naming updates Certain variables make the behavior of the volume auto-grow code unclear. This updates variables and adds a comment to better indicate the behavior in question. --- .../controller/postgrescluster/autogrow.go | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/internal/controller/postgrescluster/autogrow.go b/internal/controller/postgrescluster/autogrow.go index f06cd78ebe..e96d69f19c 100644 --- a/internal/controller/postgrescluster/autogrow.go +++ b/internal/controller/postgrescluster/autogrow.go @@ -156,12 +156,13 @@ func (r *Reconciler) setVolumeSize(ctx context.Context, cluster *v1beta1.Postgre // Otherwise, if the feature gate is not enabled, do not autogrow. } else if feature.Enabled(ctx, feature.AutoGrowVolumes) { - // determine the appropriate volume request based on what's set in the status - if dpv, err := getDesiredVolumeSize( + // Determine the appropriate volume request based on what's set in the status. + // Note: request size set by reference. + if badDesiredVolumeRequest, err := getDesiredVolumeSize( cluster, volumeType, host, volumeRequestSize, ); err != nil { log.Error(err, "For "+cluster.Name+"/"+host+ - ": Unable to parse "+volumeType+" volume request: "+dpv) + ": Unable to parse "+volumeType+" volume request: "+badDesiredVolumeRequest) } // If the volume request size is greater than or equal to the limit and the @@ -203,15 +204,15 @@ func getDesiredVolumeSize(cluster *v1beta1.PostgresCluster, case volumeType == "pgData": for i := range cluster.Status.InstanceSets { if instanceSpecName == cluster.Status.InstanceSets[i].Name { - for _, dpv := range cluster.Status.InstanceSets[i].DesiredPGDataVolume { - if dpv != "" { - desiredRequest, err := resource.ParseQuantity(dpv) + for _, desiredRequestString := range cluster.Status.InstanceSets[i].DesiredPGDataVolume { + if desiredRequestString != "" { + desiredRequest, err := resource.ParseQuantity(desiredRequestString) if err == nil { if desiredRequest.Value() > volumeRequestSize.Value() { *volumeRequestSize = desiredRequest } } else { - return dpv, err + return desiredRequestString, err } } } @@ -221,15 +222,15 @@ func getDesiredVolumeSize(cluster *v1beta1.PostgresCluster, case volumeType == "pgWAL": for i := range cluster.Status.InstanceSets { if instanceSpecName == cluster.Status.InstanceSets[i].Name { - for _, dpv := range cluster.Status.InstanceSets[i].DesiredPGWALVolume { - if dpv != "" { - desiredRequest, err := resource.ParseQuantity(dpv) + for _, desiredRequestString := range cluster.Status.InstanceSets[i].DesiredPGWALVolume { + if desiredRequestString != "" { + desiredRequest, err := resource.ParseQuantity(desiredRequestString) if err == nil { if desiredRequest.Value() > volumeRequestSize.Value() { *volumeRequestSize = desiredRequest } } else { - return dpv, err + return desiredRequestString, err } } } @@ -245,15 +246,15 @@ func getDesiredVolumeSize(cluster *v1beta1.PostgresCluster, } for i := range cluster.Status.PGBackRest.Repos { if volumeType == cluster.Status.PGBackRest.Repos[i].Name { - dpv := cluster.Status.PGBackRest.Repos[i].DesiredRepoVolume - if dpv != "" { - desiredRequest, err := resource.ParseQuantity(dpv) + desiredRequestString := cluster.Status.PGBackRest.Repos[i].DesiredRepoVolume + if desiredRequestString != "" { + desiredRequest, err := resource.ParseQuantity(desiredRequestString) if err == nil { if desiredRequest.Value() > volumeRequestSize.Value() { *volumeRequestSize = desiredRequest } } else { - return dpv, err + return desiredRequestString, err } } }