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 6 commits into
base: main
Choose a base branch
from
+617 −0
Open

Attach additional volume for postgres, #4210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 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
91939e6
Update name limit
benjaminjb Aug 12, 2025
a7ea055
Issue event for missing containers
benjaminjb Aug 12, 2025
4901e18
Fix test
benjaminjb Aug 12, 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
Copy link
Member

Choose a reason for hiding this comment

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

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

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: 55
minLength: 1
pattern: ^[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: 55
minLength: 1
pattern: ^[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
10 changes: 10 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,16 @@ 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 {
missingContainers := addAdditionalVolumesToSpecifiedContainers(&instance.Spec.Template, spec.Volumes.Additional)

if len(missingContainers) > 0 {
r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MissingContainers",
"The following containers were specified for additional volumes but are missing: %s.", missingContainers)
Comment on lines +1261 to +1262
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

}
}

if err == nil {
err = errors.WithStack(r.apply(ctx, instance))
}
Expand Down
76 changes: 76 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,77 @@ 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,
additionalVolumes []v1beta1.AdditionalVolume) []string {

missingContainers := []string{}
for _, additionalVolumeRequest := range additionalVolumes {

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

additionalVolume := corev1.Volume{
Name: additionalVolumeMount.Name,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: additionalVolumeRequest.ClaimName,
ReadOnly: additionalVolumeMount.ReadOnly,
},
},
}

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

Choose a reason for hiding this comment

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

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 {

allContainers = true
}

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

if names.Has(template.Spec.Containers[i].Name) {
names.Delete(template.Spec.Containers[i].Name)
}
Comment on lines +338 to +340
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ 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)

}
}

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

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

missingContainers = append(missingContainers, names.UnsortedList()...)
Copy link
Member

Choose a reason for hiding this comment

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

💭 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


template.Spec.Volumes = append(
template.Spec.Volumes,
additionalVolume)
Comment on lines +358 to +360
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

}
return missingContainers
}
Loading
Loading