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

cbandy · 2025-07-31T20:40:51Z

internal/controller/postgrescluster/util.go

+			additionalVolumeRequest.ClaimName)
+
+		additionalVolume := corev1.Volume{
+			Name: additionalVolumeMount.Name,


🤔 Ah, hmm. Volume names must be unique in a pod. What happens If someone uses postgres-data for their volume name? Maybe we should prefix with volume-?

🤔 🤔 What other (Pod) validation happens on this name in K8s? Maybe a limit on the length?

postgres-operator/internal/postgres/reconcile.go

Lines 26 to 28 in 6a30d72

func DataVolumeMount() corev1.VolumeMount {

return corev1.VolumeMount{Name: "postgres-data", MountPath: dataMountPath}

}

For the pod.spec.volumes, its says "Must be a DNS_LABEL and unique within the pod." (cite)

That's what the code comment says, at least.

+1 for a prefix that prevents other problems

K8s does a lot of validation in Go. I search these "validation" files for details: https://github.com/kubernetes/kubernetes/blob/790393ae92e97262827d4f1fba24e8ae65bbada0/pkg/apis/core/validation/validation.go#L411

I see only DNS1123Label (and unique) here ☝🏻 . Oh! 🔧 "subdomain" is a set of dot-separated "labels," so the two aren't interchangeable.

🔧 Please make a new shared type DNS1123Label = string with ~~these~~ length and regexp validations:

https://github.com/kubernetes/kubernetes/blob/790393ae92e97262827d4f1fba24e8ae65bbada0/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L188

... and if we're prepending something to the volume name, then the max length on our volume name field is less than upstream. (e.g. 63 minus 8 or so)

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 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 Show resolved Hide resolved

benjaminjb added 2 commits August 7, 2025 16:11

Add initcontainer case to tests

3e24547

Add assert to KUTTL

11e9c6c

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

	func DataVolumeMount() corev1.VolumeMount {
	return corev1.VolumeMount{Name: "postgres-data", MountPath: dataMountPath}
	}

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbandy Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbandy Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

cbandy Aug 12, 2025 •

edited

Loading

cbandy Aug 12, 2025 •

edited

Loading