Skip to content

Attach additional volume for postgres, #4210

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Attach additional volume for postgres, #4210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f1cf63
Add additional volume for postgres pods
benjaminjb Jul 17, 2025
3e24547
Add initcontainer case to tests
benjaminjb Aug 7, 2025
11e9c6c
Add assert to KUTTL
benjaminjb Aug 7, 2025
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
76 changes: 76 additions & 0 deletions config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11027,6 +11027,44 @@ spec:
type: array
volumes:
properties:
additional:
description: Additional pre-existing volumes to add to the
pod.
items:
properties:
claimName:
description: A reference to a preexisting PVC.
type: string
containers:
description: |-
The containers to attach this volume to.
A blank/unset `Containers` field matches all containers.
items:
type: string
maxItems: 10
type: array
x-kubernetes-list-type: atomic
name:
description: |-
The name of the volume used for mounting path.
Volumes are mounted in the pods at `volumes/<NAME>`
Must be unique.
maxLength: 253
minLength: 1
pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
readOnly:
description: Sets the write/read mode of the volume
type: boolean
required:
- claimName
- name
type: object
maxItems: 10
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
temp:
description: |-
An ephemeral volume for temporary files.
Expand Down Expand Up @@ -29598,6 +29636,44 @@ spec:
type: array
volumes:
properties:
additional:
description: Additional pre-existing volumes to add to the
pod.
items:
properties:
claimName:
description: A reference to a preexisting PVC.
type: string
containers:
description: |-
The containers to attach this volume to.
A blank/unset `Containers` field matches all containers.
items:
type: string
maxItems: 10
type: array
x-kubernetes-list-type: atomic
name:
description: |-
The name of the volume used for mounting path.
Volumes are mounted in the pods at `volumes/<NAME>`
Must be unique.
maxLength: 253
minLength: 1
pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
readOnly:
description: Sets the write/read mode of the volume
type: boolean
required:
- claimName
- name
type: object
maxItems: 10
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
temp:
description: |-
An ephemeral volume for temporary files.
Expand Down
5 changes: 5 additions & 0 deletions internal/controller/postgrescluster/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,6 +1253,11 @@ func (r *Reconciler) reconcileInstance(
addDevSHM(&instance.Spec.Template)
}

// mount additional volumes to the Postgres instance containers
if err == nil && spec.Volumes != nil && len(spec.Volumes.Additional) > 0 {
addAdditionalVolumesToSpecifiedContainers(&instance.Spec.Template, spec.Volumes.Additional)
}

if err == nil {
err = errors.WithStack(r.apply(ctx, instance))
}
Expand Down
60 changes: 60 additions & 0 deletions internal/controller/postgrescluster/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/sets"

"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
)

var tmpDirSizeLimit = resource.MustParse("16Mi")
Expand Down Expand Up @@ -285,3 +287,61 @@ func safeHash32(content func(w io.Writer) error) (string, error) {
}
return rand.SafeEncodeString(fmt.Sprint(hash.Sum32())), nil
}

// AdditionalVolumeMount returns the name and mount path of the additional volume.
func AdditionalVolumeMount(name string, readOnly bool) corev1.VolumeMount {
return corev1.VolumeMount{
Name: fmt.Sprintf("volumes-%s", name),
MountPath: "/volumes/" + name,
ReadOnly: readOnly,
}
}

// addAdditionalVolumesToSpecifiedContainers adds additional volumes to the specified
// containers in the specified pod
// addAdditionalVolumesToSpecifiedContainers adds the volumes to the pod
// as `volumes-<additionalVolumeRequest.Name>`
// and adds the directory to the path `/volumes/<additionalVolumeRequest.Name>`
func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec,
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 I like that this takes a template.

additionalVolumes []v1beta1.AdditionalVolume) {

for _, additionalVolumeRequest := range additionalVolumes {

additionalVolumeMount := AdditionalVolumeMount(
additionalVolumeRequest.Name,
additionalVolumeRequest.ReadOnly,
)

additionalVolume := corev1.Volume{
Name: additionalVolumeMount.Name,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: additionalVolumeRequest.ClaimName,
ReadOnly: additionalVolumeMount.ReadOnly,
},
},
}

names := sets.New(additionalVolumeRequest.Containers...)

for i := range template.Spec.Containers {
if names.Len() == 0 || names.Has(template.Spec.Containers[i].Name) {
template.Spec.Containers[i].VolumeMounts = append(
template.Spec.Containers[i].VolumeMounts,
additionalVolumeMount)
}
}

for i := range template.Spec.InitContainers {
if names.Len() == 0 || names.Has(template.Spec.InitContainers[i].Name) {
template.Spec.InitContainers[i].VolumeMounts = append(
template.Spec.InitContainers[i].VolumeMounts,
additionalVolumeMount)
}
}

template.Spec.Volumes = append(
Comment on lines +327 to +343
Copy link
Contributor

Choose a reason for hiding this comment

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

If the PVC specified by the user doesn't actually exist, what happens? I'm pretty sure that the Pod will break in some fashion... Do we want to provide any protection from this footgun? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the PVC doesn't exist, the pod will not be scheduled and we get a warning event from K8s:

5s          Warning   FailedScheduling        pod/hippo-instance1-f8kj-0                          0/1 nodes are available: persistentvolumeclaim "task-pv-claim" not found.

It might be nice to check if the PVCs exist, but we don't really do anything for those areas in our spec where we allow a user to specify a preexisting volume (or configmap, secret, etc.).

(I do kind of like the idea of checking the env before accepting a postgrescluster spec, but that's not really what we do.)

template.Spec.Volumes,
additionalVolume)
Comment on lines +343 to +345
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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:

  1. Ignore that issue, let them figure it out. (Con: not very user friendly to not provide feedback)
  2. 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)
  3. Error, stop reconciling pod, issue event

}
}
168 changes: 168 additions & 0 deletions internal/controller/postgrescluster/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
)

func TestSafeHash32(t *testing.T) {
Expand Down Expand Up @@ -378,3 +379,170 @@ func TestJobFailed(t *testing.T) {
})
}
}

func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) {

podTemplate := &corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{Name: "startup"},
{Name: "config"},
},
Containers: []corev1.Container{
{Name: "database"},
{Name: "other"},
}}}

testCases := []struct {
tcName string
additionalVolumes []v1beta1.AdditionalVolume
expectedContainers string
expectedInitContainers string
expectedVolumes string
}{{
tcName: "all",
additionalVolumes: []v1beta1.AdditionalVolume{{
Containers: []string{},
ClaimName: "required",
Name: "required",
}},
expectedContainers: `- name: database
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- name: other
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required`,
expectedInitContainers: `- name: startup
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- name: config
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required`,
expectedVolumes: `- name: volumes-required
persistentVolumeClaim:
claimName: required`,
}, {
tcName: "multiple additional volumes",
additionalVolumes: []v1beta1.AdditionalVolume{{
Containers: []string{},
ClaimName: "required",
Name: "required",
}, {
Containers: []string{},
ClaimName: "also",
Name: "other",
}},
expectedContainers: `- name: database
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- mountPath: /volumes/other
name: volumes-other
- name: other
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- mountPath: /volumes/other
name: volumes-other`,
expectedInitContainers: `- name: startup
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- mountPath: /volumes/other
name: volumes-other
- name: config
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- mountPath: /volumes/other
name: volumes-other`,
expectedVolumes: `- name: volumes-required
persistentVolumeClaim:
claimName: required
- name: volumes-other
persistentVolumeClaim:
claimName: also`,
}, {
tcName: "database and startup containers only",
additionalVolumes: []v1beta1.AdditionalVolume{{
Containers: []string{"database", "startup"},
ClaimName: "required",
Name: "required",
}},
expectedContainers: `- name: database
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- name: other
resources: {}`,
expectedInitContainers: `- name: startup
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
- name: config
resources: {}`,
expectedVolumes: `- name: volumes-required
persistentVolumeClaim:
claimName: required`,
}, {
tcName: "readonly",
additionalVolumes: []v1beta1.AdditionalVolume{{
Containers: []string{"database"},
ClaimName: "required",
Name: "required",
ReadOnly: true,
}},
expectedContainers: `- name: database
resources: {}
volumeMounts:
- mountPath: /volumes/required
name: volumes-required
readOnly: true
- name: other
resources: {}`,
expectedInitContainers: `- name: startup
resources: {}
- name: config
resources: {}`,
expectedVolumes: `- name: volumes-required
persistentVolumeClaim:
claimName: required
readOnly: true`,
}}

for _, tc := range testCases {
t.Run(tc.tcName, func(t *testing.T) {

copyPodTemplate := podTemplate.DeepCopy()

addAdditionalVolumesToSpecifiedContainers(
copyPodTemplate,
tc.additionalVolumes,
)

assert.Assert(t, cmp.MarshalMatches(
copyPodTemplate.Spec.Containers,
tc.expectedContainers))
assert.Assert(t, cmp.MarshalMatches(
copyPodTemplate.Spec.InitContainers,
tc.expectedInitContainers))
assert.Assert(t, cmp.MarshalMatches(
copyPodTemplate.Spec.Volumes,
tc.expectedVolumes))
})
}
}
8 changes: 8 additions & 0 deletions pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,14 @@ type PostgresVolumesSpec struct {
// ---
// +optional
Temp *v1beta1.VolumeClaimSpec `json:"temp,omitempty"`

// Additional pre-existing volumes to add to the pod.
// ---
// +optional
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems=10
Additional []v1beta1.AdditionalVolume `json:"additional,omitempty"`
}

type TablespaceVolume struct {
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading