Attach additional volume for postgres #4210

benjaminjb · 2025-07-17T20:55:31Z

Checklist:

Have you added an explanation of what your changes do and why you'd like them to be included?
Have you updated or added documentation for the change, as applicable?
Have you tested your changes on all related environments with successful results, as applicable?
- Have you added automated tests?

Type of Changes:

What is the current behavior (link to any open issues here)?

Only can add additional volumes to database container through abuse of the tablespace volumes.

What is the new behavior (if this is a feature change)?

Breaking change (fix or feature that would cause existing functionality to change)

Ability to add PVC (either BYO or create from template) to multiple containers in the Postgres pod

Other Information:
Issues: [PGO-2556]

internal/controller/postgrescluster/volumes.go

pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

internal/naming/names.go

pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go

internal/controller/postgrescluster/util.go

internal/controller/postgrescluster/instance.go

internal/controller/postgrescluster/util.go

Change the API to allow users to specify preexisting PVCs to attach to specified containers in the postgres instance pods. Issues: [PGO-2556]

internal/controller/postgrescluster/util.go

internal/controller/postgrescluster/util_test.go

testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml

internal/controller/postgrescluster/util.go

cbandy · 2025-08-12T21:30:46Z

pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go

+//
+// +kubebuilder:validation:MinLength=1
+// Max length is less than max 63 to allow prepending `volumes-` to name
+// +kubebuilder:validation:MaxLength=55


❓ Can this be 63 here and AdditionalVolume.Name override it with 55 there?

In the spec that results in name having the reqs:

allOf: - maxLength: 63 - maxLength: 55

Interesting!

pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go

cbandy · 2025-08-12T21:39:39Z

pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

+	ClaimName string `json:"claimName"`
+
+	// The containers to attach this volume to.
+	// A blank/unset `Containers` field matches all containers.


❓ Where did we land on empty-list? Does it mean all or none?

📝 I've used the phrase "when omitted" or "when this is omitted" to explain what happens when a field is missing from the spec.

I still don't quite understand the use-case for adding a volume that is not mounted into any containers... But I could see how a zero value (empty list) could mean "none" while leaving the field off entirely would mean "all"

I think the issue is still up for debate, to wit:

what is the use care for attaching to none? (My squinting suggestion is: you have the volume defined in your sidecar definition and for whatever reason, it's easier to leave it there than to put it into our spec)

what is the best way to signal none vs. all? I agree that an explicit "[]" looks more like none than leaving the field off, but that's a real thin line.

Ah, what the heck, let's do it.

pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

internal/controller/postgrescluster/util.go

cbandy · 2025-08-12T22:09:34Z

internal/controller/postgrescluster/util.go

+			}
+		}
+
+		missingContainers = append(missingContainers, names.UnsortedList()...)


💭 Using Delete to subtract from names relies on init and regular containers having unique names together. That's correct and fine, but I wonder if it is worth calling out.

🤔 Should the resulting slice be a Set?

I was about to raise this concern myself... Is it correct though, when you take sidecars into consideration? Couldn't the user add a sidecar to Containers that has the same name as one in InitContainers?

Answering my own question: No, the user could not add a sidecar with the same name as an init container. Ben tried it out and the Pod fails to roll out. He also found this in k8s documentation: “The name of each app and init container in a Pod must be unique; a validation error is thrown for any container sharing a name with another.”

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go

testing/kuttl/e2e/additional-volumes/01-assert.yaml

internal/controller/postgrescluster/instance.go

internal/controller/postgrescluster/util.go

Co-authored-by: Drew Sessler <36803518+dsessler7@users.noreply.github.com>

cbandy reviewed Jul 28, 2025

View reviewed changes

benjaminjb force-pushed the benjb/additional-volumes-postgres branch from d9d4932 to f906775 Compare July 30, 2025 20:03

cbandy reviewed Jul 31, 2025

View reviewed changes

benjaminjb force-pushed the benjb/additional-volumes-postgres branch from 88d226d to 955087a Compare August 4, 2025 20:32

Add additional volume for postgres pods

0f1cf63

Change the API to allow users to specify preexisting PVCs to attach to specified containers in the postgres instance pods. Issues: [PGO-2556]

benjaminjb force-pushed the benjb/additional-volumes-postgres branch from 955087a to 0f1cf63 Compare August 4, 2025 20:33

dsessler7 reviewed Aug 6, 2025

View reviewed changes

internal/controller/postgrescluster/util.go Show resolved Hide resolved

dsessler7 reviewed Aug 6, 2025

View reviewed changes

internal/controller/postgrescluster/util_test.go Show resolved Hide resolved

dsessler7 reviewed Aug 6, 2025

View reviewed changes

testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml Outdated Show resolved Hide resolved

dsessler7 reviewed Aug 6, 2025

View reviewed changes

internal/controller/postgrescluster/util.go Outdated Show resolved Hide resolved

benjaminjb added 4 commits August 7, 2025 16:11

Add initcontainer case to tests

3e24547

Add assert to KUTTL

11e9c6c

Update name limit

91939e6

Issue event for missing containers

a7ea055

benjaminjb force-pushed the benjb/additional-volumes-postgres branch from 22ab593 to a7ea055 Compare August 12, 2025 21:32

Fix test

4901e18

cbandy approved these changes Aug 12, 2025

View reviewed changes

dsessler7 reviewed Aug 12, 2025

View reviewed changes

internal/controller/postgrescluster/instance.go Outdated Show resolved Hide resolved

dsessler7 reviewed Aug 13, 2025

View reviewed changes

internal/controller/postgrescluster/util.go Outdated Show resolved Hide resolved

benjaminjb and others added 4 commits August 13, 2025 08:52

PR feedback

cedf6b4

Update internal/controller/postgrescluster/instance.go

2c8a40e

Co-authored-by: Drew Sessler <36803518+dsessler7@users.noreply.github.com>

Allow no containers

0971139

Alphabetize

b4e667a

benjaminjb merged commit 7844fe2 into CrunchyData:main Aug 13, 2025
17 checks passed

benjaminjb changed the title ~~Attach additional volume for postgres,~~ Attach additional volume for postgres Aug 13, 2025

Attach additional volume for postgres #4210

Attach additional volume for postgres #4210

Conversation

benjaminjb commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!