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

cbandy · 2025-07-23T02:29:02Z

internal/naming/names.go

+// volume for instance.
+func AdditionalVolume(cluster *v1beta1.PostgresCluster,
+	volume *v1beta1.AdditionalVolume) metav1.ObjectMeta {
+	// TODO: What's the name for the PVC if not given?


🔧 The name field, I imagine. That field should have validation that rejects weird filepath stuff (e.g. slash) and is also valid for K8s API name (subdomain?). We have a few predefined types for names.

postgres-operator/internal/naming/limitations.md

Line 80 in 899c4a0

[PersistentVolumeClaim (PVC) names are DNS subdomains](https://releases.k8s.io/v1.23.0/pkg/apis/core/validation/validation.go#L2066).

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

Lines 32 to 33 in 899c4a0

// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`

type DNS1123Subdomain = string

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]

dsessler7 · 2025-08-06T17:58:48Z

internal/controller/postgrescluster/util.go

+		template.Spec.Volumes = append(
+			template.Spec.Volumes,
+			additionalVolume)


This is probably fine, but just to clarify, it seems we will always add the volume even if we don't mount it to any containers (which would only happen if the user specifies a container(s) but none of the containers specified actually exist in the pod)...

Do we want to warn the user in any way if any of the containers they've specified don't exist? 🤔 Seems unlikely the user would do this on purpose, but if they misspell/typo their container name and don't realize it...

Yeah, it's funny, I was just talking with Andrew earlier in the week about how some people might want to not add the volume to any containers through our system, but mount them manually through, say, the sidecar container spec.

But we don't actually let people mount to no containers: we have some (list the containers) and all (a blank list). Should we add a "none" option or special keyword?

But yeah, right now, we have (a) no way to skip mounting and (b) no validation on the containers being present.

Now if I mispell "database" when trying to mount to the pod, what should our behavior be? I guess we should error out because, likely, if a user put in the wrong container name, and we just warned and created the pod, then they'd find out later, and fix the spec, causing the pod to regenerate. What do you think?

What would be the point of the user adding their PVC to volumes.additional if they don't want us to mount it to any containers?

Yeah I think doing something (error/warning event/log) makes sense if the user specifies a container that doesn't exist.

Yeah, the only potential that I see -- though there might be others -- is that the user has a container definition that already has that volume mounted and it's easier for them (for some reason, potentially CI related) to use that as a custom sidecar (with the volume mounted) rather than define the postgrescluster spec with that volume mounted to that container.

@andrewlecuyer This question isn't a blocker (as we can refine the solution as we move forwards) but I want to raise it:

If the user supplies a container that doesn't exist, what should our behavior be?

Options:

Ignore that issue, let them figure it out. (Con: not very user friendly to not provide feedback)

Issue a warning event, but continue to reconcile the pod (Con: if they see the warning, the user will have to fix the spec, which will cause a pod regeneration of a mis-spec'ed pod)

Error, stop reconciling pod, issue event

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?

cbandy · 2025-08-12T21:34:04Z

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

@@ -32,6 +32,16 @@ type ConfigDataKey = string
 // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
 type DNS1123Subdomain = string

+// ---


⛏️ When possible, I like linking to the user-doc-y stuff, too.

Suggested change

// ---

// ---

// https://docs.k8s.io/concepts/overview/working-with-objects/names#dns-label-names

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"

cbandy · 2025-08-12T21:44:22Z

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

+	// A reference to a preexisting PVC.
+	// ---
+	// +required
+	ClaimName string `json:"claimName"`


🔧 PVC names are 1123 subdomains, so we can use that type here.

https://github.com/kubernetes/kubernetes/blob/v1.33.0/pkg/apis/core/validation/validation.go#L2253 → ValidatePersistentVolumeName → NameIsDNSSubdomain → IsDNS1123Subdomain

cbandy · 2025-08-12T21:52:05Z

internal/controller/postgrescluster/util.go

+				if names.Has(template.Spec.Containers[i].Name) {
+					names.Delete(template.Spec.Containers[i].Name)
+				}


⛏️ The guard is unnecessary.

Suggested change

if names.Has(template.Spec.Containers[i].Name) {

names.Delete(template.Spec.Containers[i].Name)

}

names.Delete(template.Spec.Containers[i].Name)

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.”

cbandy · 2025-08-12T22:11:33Z

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

🤩 This diff is so small and tidy without the template!

cbandy · 2025-08-12T22:12:43Z

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

+	// +listType=map
+	// +listMapKey=name
+	// +kubebuilder:validation:MaxItems=10
+	Additional []v1beta1.AdditionalVolume `json:"additional,omitempty"`


⛏️ I like to alphabetize things. Can Additional go before/above Temp?

cbandy · 2025-08-12T22:16:16Z

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

🤔 This doesn't seem like the kind of change that needs E2E. Is this here because we have more to add in the future? Are there are tests in the postgrescluster package more like cluster-spec-in → pod-spec-out ?

dsessler7 · 2025-08-12T23:50:32Z

internal/controller/postgrescluster/instance.go

+			r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MissingContainers",
+				"The following containers were specified for additional volumes but are missing: %s.", missingContainers)


I think what you have is fine, but you expressed dissatisfaction with what you have, so maybe something like this?

Suggested change

r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MissingContainers",

"The following containers were specified for additional volumes but are missing: %s.", missingContainers)

r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",

"The following containers were specified for additional volumes but cannot be found: %s.", missingContainers)

dsessler7 · 2025-08-13T01:19:50Z

internal/controller/postgrescluster/util.go

+
+		names := sets.New(additionalVolumeRequest.Containers...)
+		allContainers := false
+		if names.Len() == 0 {


IF we want omission of containers to mean "All" and an empty slice for containers to mean "None":

Suggested change

if names.Len() == 0 {

if additionalVolumeRequest.Containers == nil {

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_test.go Show resolved Hide resolved

dsessler7 reviewed Aug 6, 2025

View reviewed changes

testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml 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

dsessler7 reviewed Aug 13, 2025

View reviewed changes

	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9][a-z0-9])?([.][a-z0-9]([-a-z0-9][a-z0-9])?)*$`
	type DNS1123Subdomain = string

	// ---
	// ---
	// https://docs.k8s.io/concepts/overview/working-with-objects/names#dns-label-names

		r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MissingContainers",
		"The following containers were specified for additional volumes but are missing: %s.", missingContainers)

	if names.Len() == 0 {
	if additionalVolumeRequest.Containers == nil {

Attach additional volume for postgres, #4210

Are you sure you want to change the base?

Attach additional volume for postgres, #4210

Conversation

benjaminjb commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!