Skip to content

Allow user to set an annotation that will specify an existing PVC to … #4221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 30, 2025
+416 −32
Merged

Allow user to set an annotation that will specify an existing PVC to … #4221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions internal/controller/postgrescluster/pgbackrest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
"github.com/crunchydata/postgres-operator/internal/pki"
"github.com/crunchydata/postgres-operator/internal/postgres"
"github.com/crunchydata/postgres-operator/internal/util"
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
)

Expand Down Expand Up @@ -771,7 +772,7 @@ func (r *Reconciler) generateRepoVolumeIntent(postgresCluster *v1beta1.PostgresC
}

// generateBackupJobSpecIntent generates a JobSpec for a pgBackRest backup job
func generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.PostgresCluster,
func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.PostgresCluster,
repo v1beta1.PGBackRestRepo, serviceAccountName string,
labels, annotations map[string]string, opts ...string) *batchv1.JobSpec {

Expand Down Expand Up @@ -873,6 +874,27 @@ func generateBackupJobSpecIntent(ctx context.Context, postgresCluster *v1beta1.P
// to read certificate files
jobSpec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster)
pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)

// If the user has specified a PVC to use as a log volume via the PGBackRestCloudLogVolume
// annotation, check for the PVC. If we find it, mount it to the backup job.
// Otherwise, create a warning event.
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 add it to the backup job
util.AddVolumeAndMountsToPod(&jobSpec.Template.Spec, logVolume)
}
}
}

return jobSpec
Expand Down Expand Up @@ -2040,8 +2062,31 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context,
repoHostName, configHash, serviceName, serviceNamespace string,
instanceNames []string) error {

// 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.
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 If we check when generating the job spec, can we assume it exists here for generating the configmap?

Copy link
Contributor Author

@dsessler7 dsessler7 Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as of right now, if the annotation is in place but the PVC doesn't exist, we still create the backup job, we just don't mount the PVC (and we create a warning event). My thought behind this is that having backups is more important that having logs from backups, so cancelling the backup job just because it can't write the logs seems like overkill. Open to arguments to the contrary though. This does make me wonder though if an event is enough of an alarm to users that something is wrong. Maybe we should log an error too? 🤔 Either way, the backup will still be created. If we don't check for the PVC here where we reconcile the config, the backup job will attempt to write the logs to a directory that doesn't exist. I can't remember if it will fail or not (I'm guessing it won't fail for the same reason as earlier)... If it doesn't fail I guess we could remove the check for the PVC if we want... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like to skip an API call if I can -- like, I wish we called the func to generate the jobspec before we generated the configmap, because then we could just pass the jobspec in and if we had the volume on the spec that matched the annotation on the cluster, we could assume the PVC was found.

But because we use the configmap for non-job stuff, that doesn't work. I guess we cold log into the ether if the volume wasn't there and skip a step -- but maybe another event will get through to the user.

(I also don't know the best way to get through to the user for something that's a warning but not an error.)

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
}
}

backrestConfig, err := pgbackrest.CreatePGBackRestConfigMapIntent(ctx, postgresCluster, repoHostName,
configHash, serviceName, serviceNamespace, instanceNames)
configHash, serviceName, serviceNamespace, cloudLogPath, instanceNames)
if err != nil {
return err
}
Expand Down Expand Up @@ -2454,7 +2499,7 @@ func (r *Reconciler) reconcileManualBackup(ctx context.Context,
backupJob.Labels = labels
backupJob.Annotations = annotations

spec := generateBackupJobSpecIntent(ctx, postgresCluster, repo,
spec := r.generateBackupJobSpecIntent(ctx, postgresCluster, repo,
serviceAccount.GetName(), labels, annotations, backupOpts...)

backupJob.Spec = *spec
Expand Down Expand Up @@ -2631,7 +2676,7 @@ func (r *Reconciler) reconcileReplicaCreateBackup(ctx context.Context,
backupJob.Labels = labels
backupJob.Annotations = annotations

spec := generateBackupJobSpecIntent(ctx, postgresCluster, replicaCreateRepo,
spec := r.generateBackupJobSpecIntent(ctx, postgresCluster, replicaCreateRepo,
serviceAccount.GetName(), labels, annotations)

backupJob.Spec = *spec
Expand Down Expand Up @@ -3058,7 +3103,7 @@ func (r *Reconciler) reconcilePGBackRestCronJob(
// set backup type (i.e. "full", "diff", "incr")
backupOpts := []string{"--type=" + backupType}

jobSpec := generateBackupJobSpecIntent(ctx, cluster, repo,
jobSpec := r.generateBackupJobSpecIntent(ctx, cluster, repo,
serviceAccount.GetName(), labels, annotations, backupOpts...)

// Suspend cronjobs when shutdown or read-only. Any jobs that have already
Expand Down
Loading
Loading