Skip to content

Adjust cloud backup volume behavior such that additional volumes take… #4294

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
Sep 18, 2025
+342 −13
Merged

Adjust cloud backup volume behavior such that additional volumes take… #4294

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
41 changes: 31 additions & 10 deletions internal/controller/postgrescluster/pgbackrest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,9 +892,29 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
jobSpec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster)
pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)

// Mount the PVC named in the "pgbackrest-cloud-log-volume" annotation, if any.
// If the "pgbackrest-cloud-log-volume" annotation has a value, check if it is the
// same as any of the additional volume names. If there is a collision of names,
// create a warning event. If there is no name collision, mount the volume referenced
// by the annotation.
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
var collisionFound bool
if jobs != nil && jobs.Volumes != nil {
for _, volume := range jobs.Volumes.Additional {
if volume.Name == logVolume {
collisionFound = true
r.Recorder.Event(postgresCluster, corev1.EventTypeWarning,
"DuplicateCloudBackupVolume", "The volume name specified in the "+
"pgbackrest-cloud-log-volume annotation is the same as one "+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. "+
"Cannot mount duplicate volume names. Defaulting to the "+
"additional volume.")
break
}
}
}
if !collisionFound {
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
}
}
}

Expand Down Expand Up @@ -3340,21 +3360,22 @@ func authorizeBackupRemovalAnnotationPresent(postgresCluster *v1beta1.PostgresCl
}

// 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)
// in cloud backup jobs. If the user specified a log path via the spec, use it. Otherwise,
// if the user specified a log volume for cloud backups via the PGBackRestCloudLogVolume
// annotation, we will use that. If neither scenario is true, return an empty string.
//
// This function assumes that the backups/pgbackrest spec is present in postgresCluster.
func getCloudLogPath(postgresCluster *v1beta1.PostgresCluster) string {
cloudLogPath := ""
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
cloudLogPath = "/volumes/" + logVolume
} else if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log != nil &&
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path != "" {
// TODO: I know it should be caught by CEL validation, but is it worthwhile to also
// check that Log.Path ~= "/volumes/" + existingAdditionalVolume.name here??
Comment on lines +3373 to +3374
Copy link
Contributor

@benjaminjb benjaminjb Sep 17, 2025

Choose a reason for hiding this comment

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

That's a good question. I'm usually very defensive ("before we pass the info, let's make sure it's correct -- ok, now that we've passed the info, let's check it again"), but here I feel like it really could go either way, and I'm leaning towards not running the check here:

  • if we change the CEL rules ("you can now attach a tmp and log there"), then we change the admission validation rules and don't need to remember all the places where the value is being used.
  • if someone gets around the CEL validation, that's good information for us on our validation. (Although one way they can do that is always: just remove the CEL lines before installing the CRD.)

Copy link
Contributor Author

@dsessler7 dsessler7 Sep 18, 2025

Choose a reason for hiding this comment

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

Agreed. Maybe I'll just leave the TODO for now


cloudLogPath = filepath.Clean(postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path)
} else if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
cloudLogPath = "/volumes/" + logVolume
}
return cloudLogPath
}
314 changes: 311 additions & 3 deletions internal/controller/postgrescluster/pgbackrest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3130,6 +3130,312 @@ volumes:
// No events created
assert.Equal(t, len(recorder.Events), 0)
})

t.Run("AdditionalVolumesMissingContainers", func(t *testing.T) {
recorder := events.NewRecorder(t, runtime.Scheme)
r.Recorder = recorder

cluster := cluster.DeepCopy()
cluster.Namespace = ns.Name
cluster.Annotations = map[string]string{}
cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
Log: &v1beta1.LoggingConfiguration{
Path: "/volumes/stuff/log",
},
Volumes: &v1beta1.PGBackRestVolumesSpec{
Additional: []v1beta1.AdditionalVolume{
{
ClaimName: "additional-pvc",
Containers: []v1beta1.DNS1123Label{
"pgbackrest",
"non-existent-container",
},
Name: "stuff",
},
},
},
}

spec := r.generateBackupJobSpecIntent(ctx,
cluster, v1beta1.PGBackRestRepo{},
"",
nil, nil,
)

assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
containers:
- command:
- sh
- -c
- --
- mkdir -p '/volumes/stuff/log' && { chmod 0775 '/volumes/stuff/log' || :; }; exec
"$@"
- --
- /bin/pgbackrest
- backup
- --stanza=db
- --repo=
name: pgbackrest
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
privileged: false
readOnlyRootFilesystem: true
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
volumeMounts:
- mountPath: /etc/pgbackrest/conf.d
name: pgbackrest-config
readOnly: true
- mountPath: /tmp
name: tmp
- mountPath: /volumes/stuff
name: volumes-stuff
enableServiceLinks: false
restartPolicy: Never
securityContext:
fsGroup: 26
fsGroupChangePolicy: OnRootMismatch
volumes:
- name: pgbackrest-config
projected:
sources:
- configMap:
items:
- key: pgbackrest_cloud.conf
path: pgbackrest_cloud.conf
name: hippo-test-pgbackrest-config
- secret:
items:
- key: pgbackrest.ca-roots
path: ~postgres-operator/tls-ca.crt
- key: pgbackrest-client.crt
path: ~postgres-operator/client-tls.crt
- key: pgbackrest-client.key
mode: 384
path: ~postgres-operator/client-tls.key
name: hippo-test-pgbackrest
- emptyDir:
sizeLimit: 16Mi
name: tmp
- name: volumes-stuff
persistentVolumeClaim:
claimName: additional-pvc`))

// Missing containers warning event created
assert.Equal(t, len(recorder.Events), 1)
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
assert.Equal(t, recorder.Events[0].Reason, "SpecifiedContainerNotFound")
assert.Equal(t, recorder.Events[0].Note, "The following Backup Job Pod "+
"containers were specified for additional volumes but cannot be "+
"found: [non-existent-container].")
})

t.Run("AnnotationAndAdditionalVolumeWithPath", func(t *testing.T) {
recorder := events.NewRecorder(t, runtime.Scheme)
r.Recorder = recorder

cluster := cluster.DeepCopy()
cluster.Namespace = ns.Name
cluster.Annotations = map[string]string{}
cluster.Annotations[naming.PGBackRestCloudLogVolume] = "stuff"

cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
Log: &v1beta1.LoggingConfiguration{
Path: "/volumes/stuff/log",
},
Volumes: &v1beta1.PGBackRestVolumesSpec{
Additional: []v1beta1.AdditionalVolume{
{
ClaimName: "additional-pvc",
Name: "stuff",
},
},
},
}

spec := r.generateBackupJobSpecIntent(ctx,
cluster, v1beta1.PGBackRestRepo{},
"",
nil, nil,
)

assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
containers:
- command:
- sh
- -c
- --
- mkdir -p '/volumes/stuff/log' && { chmod 0775 '/volumes/stuff/log' || :; }; exec
"$@"
- --
- /bin/pgbackrest
- backup
- --stanza=db
- --repo=
name: pgbackrest
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
privileged: false
readOnlyRootFilesystem: true
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
volumeMounts:
- mountPath: /etc/pgbackrest/conf.d
name: pgbackrest-config
readOnly: true
- mountPath: /tmp
name: tmp
- mountPath: /volumes/stuff
name: volumes-stuff
enableServiceLinks: false
restartPolicy: Never
securityContext:
fsGroup: 26
fsGroupChangePolicy: OnRootMismatch
volumes:
- name: pgbackrest-config
projected:
sources:
- configMap:
items:
- key: pgbackrest_cloud.conf
path: pgbackrest_cloud.conf
name: hippo-test-pgbackrest-config
- secret:
items:
- key: pgbackrest.ca-roots
path: ~postgres-operator/tls-ca.crt
- key: pgbackrest-client.crt
path: ~postgres-operator/client-tls.crt
- key: pgbackrest-client.key
mode: 384
path: ~postgres-operator/client-tls.key
name: hippo-test-pgbackrest
- emptyDir:
sizeLimit: 16Mi
name: tmp
- name: volumes-stuff
persistentVolumeClaim:
claimName: additional-pvc`))

// Annotation/additional volume collision warning event created
assert.Equal(t, len(recorder.Events), 1)
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
assert.Equal(t, recorder.Events[0].Reason, "DuplicateCloudBackupVolume")
assert.Equal(t, recorder.Events[0].Note, "The volume name specified in "+
"the pgbackrest-cloud-log-volume annotation is the same as one "+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. Cannot "+
"mount duplicate volume names. Defaulting to the additional volume.")
})

t.Run("AnnotationAndAdditionalVolumeNoPath", func(t *testing.T) {
recorder := events.NewRecorder(t, runtime.Scheme)
r.Recorder = recorder

cluster := cluster.DeepCopy()
cluster.Namespace = ns.Name
cluster.Annotations = map[string]string{}
cluster.Annotations[naming.PGBackRestCloudLogVolume] = "stuff"

cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
Volumes: &v1beta1.PGBackRestVolumesSpec{
Additional: []v1beta1.AdditionalVolume{
{
ClaimName: "additional-pvc",
Name: "stuff",
},
},
},
}

spec := r.generateBackupJobSpecIntent(ctx,
cluster, v1beta1.PGBackRestRepo{},
"",
nil, nil,
)

assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
containers:
- command:
- sh
- -c
- --
- mkdir -p '/volumes/stuff' && { chmod 0775 '/volumes/stuff' || :; }; exec "$@"
- --
- /bin/pgbackrest
- backup
- --stanza=db
- --repo=
name: pgbackrest
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
privileged: false
readOnlyRootFilesystem: true
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
volumeMounts:
- mountPath: /etc/pgbackrest/conf.d
name: pgbackrest-config
readOnly: true
- mountPath: /tmp
name: tmp
- mountPath: /volumes/stuff
name: volumes-stuff
enableServiceLinks: false
restartPolicy: Never
securityContext:
fsGroup: 26
fsGroupChangePolicy: OnRootMismatch
volumes:
- name: pgbackrest-config
projected:
sources:
- configMap:
items:
- key: pgbackrest_cloud.conf
path: pgbackrest_cloud.conf
name: hippo-test-pgbackrest-config
- secret:
items:
- key: pgbackrest.ca-roots
path: ~postgres-operator/tls-ca.crt
- key: pgbackrest-client.crt
path: ~postgres-operator/client-tls.crt
- key: pgbackrest-client.key
mode: 384
path: ~postgres-operator/client-tls.key
name: hippo-test-pgbackrest
- emptyDir:
sizeLimit: 16Mi
name: tmp
- name: volumes-stuff
persistentVolumeClaim:
claimName: additional-pvc`))

// Annotation/additional volume collision warning event created
assert.Equal(t, len(recorder.Events), 1)
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
assert.Equal(t, recorder.Events[0].Reason, "DuplicateCloudBackupVolume")
assert.Equal(t, recorder.Events[0].Note, "The volume name specified in "+
"the pgbackrest-cloud-log-volume annotation is the same as one "+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. Cannot "+
"mount duplicate volume names. Defaulting to the additional volume.")
})
}

func TestGenerateRepoHostIntent(t *testing.T) {
Expand Down Expand Up @@ -4599,15 +4905,17 @@ func TestGetCloudLogPath(t *testing.T) {
Spec: v1beta1.PostgresClusterSpec{
Backups: v1beta1.Backups{
PGBackRest: v1beta1.PGBackRestArchive{
Log: &v1beta1.LoggingConfiguration{
Path: "/volumes/test/log",
Jobs: &v1beta1.BackupJobs{
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")
assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/test/log")
})
}
Loading