From 0f1cf63369edac3be547681431dd14640c7d2118 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 17 Jul 2025 15:53:42 -0500 Subject: [PATCH 01/10] Add additional volume for postgres pods Change the API to allow users to specify preexisting PVCs to attach to specified containers in the postgres instance pods. Issues: [PGO-2556] --- ...ator.crunchydata.com_postgresclusters.yaml | 76 ++++++++ .../controller/postgrescluster/instance.go | 5 + internal/controller/postgrescluster/util.go | 60 +++++++ .../controller/postgrescluster/util_test.go | 165 ++++++++++++++++++ .../v1/postgrescluster_types.go | 8 + .../v1/zz_generated.deepcopy.go | 7 + .../v1beta1/postgrescluster_types.go | 38 ++++ .../v1beta1/zz_generated.deepcopy.go | 27 +++ .../e2e/additional-volumes/00--cluster.yaml | 6 + .../e2e/additional-volumes/00-assert.yaml | 9 + .../e2e/additional-volumes/01-assert.yaml | 65 +++++++ .../files/00-cluster-created.yaml | 15 ++ .../files/00-create-cluster.yaml | 48 +++++ 13 files changed, 529 insertions(+) create mode 100644 testing/kuttl/e2e/additional-volumes/00--cluster.yaml create mode 100644 testing/kuttl/e2e/additional-volumes/00-assert.yaml create mode 100644 testing/kuttl/e2e/additional-volumes/01-assert.yaml create mode 100644 testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml create mode 100644 testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index 9fe1ccf439..a1e6a6a207 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -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/` + 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. @@ -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/` + 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. diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 0c91ca7157..9aff529318 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -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)) } diff --git a/internal/controller/postgrescluster/util.go b/internal/controller/postgrescluster/util.go index a1ba6ce087..ea4c187dd7 100644 --- a/internal/controller/postgrescluster/util.go +++ b/internal/controller/postgrescluster/util.go @@ -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") @@ -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-` +// and adds the directory to the path `/volumes/` +func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, + additionalVolumes []v1beta1.AdditionalVolume) { + + 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...) + + 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( + template.Spec.Volumes, + additionalVolume) + } +} diff --git a/internal/controller/postgrescluster/util_test.go b/internal/controller/postgrescluster/util_test.go index 8e7d5c434f..884f19e97c 100644 --- a/internal/controller/postgrescluster/util_test.go +++ b/internal/controller/postgrescluster/util_test.go @@ -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) { @@ -378,3 +379,167 @@ 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 container only", + additionalVolumes: []v1beta1.AdditionalVolume{{ + Containers: []string{"database"}, + ClaimName: "required", + Name: "required", + }}, + expectedContainers: `- name: database + resources: {} + volumeMounts: + - mountPath: /volumes/required + name: volumes-required +- name: other + resources: {}`, + expectedInitContainers: `- name: startup + resources: {} +- 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)) + }) + } +} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go index abd23670c3..1778fdefaf 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go @@ -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 { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go index 94a6ed3389..b296067c9b 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go @@ -656,6 +656,13 @@ func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { in, out := &in.Temp, &out.Temp *out = (*in).DeepCopy() } + if in.Additional != nil { + in, out := &in.Additional, &out.Additional + *out = make([]v1beta1.AdditionalVolume, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresVolumesSpec. diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index 07c6d4c805..e434816cb4 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -531,6 +531,44 @@ type PostgresVolumesSpec struct { // --- // +optional Temp *VolumeClaimSpec `json:"temp,omitempty"` + + // Additional pre-existing volumes to add to the pod. + // --- + // +optional + // +listType=map + // +listMapKey=name + // +kubebuilder:validation:MaxItems=10 + Additional []AdditionalVolume `json:"additional,omitempty"` +} + +type AdditionalVolume struct { + // The name of the volume used for mounting path. + // Volumes are mounted in the pods at `volumes/` + // Must be unique. + // --- + // The `Name` field is a `DNS1123Subdomain` type to enforce + // the max length and also allow us to more easily transition + // to CPK-provisioned volumes. + // +required + Name DNS1123Subdomain `json:"name"` + + // A reference to a preexisting PVC. + // --- + // +required + ClaimName string `json:"claimName"` + + // The containers to attach this volume to. + // A blank/unset `Containers` field matches all containers. + // --- + // +optional + // +listType=atomic + // +kubebuilder:validation:MaxItems=10 + Containers []string `json:"containers,omitempty"` + + // Sets the write/read mode of the volume + // --- + // +optional + ReadOnly bool `json:"readOnly,omitempty"` } type TablespaceVolume struct { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go index 747e363854..b6b32c63b0 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go @@ -33,6 +33,26 @@ func (in *APIResponses) DeepCopy() *APIResponses { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AdditionalVolume) DeepCopyInto(out *AdditionalVolume) { + *out = *in + if in.Containers != nil { + in, out := &in.Containers, &out.Containers + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AdditionalVolume. +func (in *AdditionalVolume) DeepCopy() *AdditionalVolume { + if in == nil { + return nil + } + out := new(AdditionalVolume) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BackupJobs) DeepCopyInto(out *BackupJobs) { *out = *in @@ -2514,6 +2534,13 @@ func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { in, out := &in.Temp, &out.Temp *out = (*in).DeepCopy() } + if in.Additional != nil { + in, out := &in.Additional, &out.Additional + *out = make([]AdditionalVolume, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresVolumesSpec. diff --git a/testing/kuttl/e2e/additional-volumes/00--cluster.yaml b/testing/kuttl/e2e/additional-volumes/00--cluster.yaml new file mode 100644 index 0000000000..801a22d460 --- /dev/null +++ b/testing/kuttl/e2e/additional-volumes/00--cluster.yaml @@ -0,0 +1,6 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +apply: +- files/00-create-cluster.yaml +assert: +- files/00-cluster-created.yaml diff --git a/testing/kuttl/e2e/additional-volumes/00-assert.yaml b/testing/kuttl/e2e/additional-volumes/00-assert.yaml new file mode 100644 index 0000000000..f54a5d7d4e --- /dev/null +++ b/testing/kuttl/e2e/additional-volumes/00-assert.yaml @@ -0,0 +1,9 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +collectors: +- type: command + command: kubectl -n $NAMESPACE describe pods --selector postgres-operator.crunchydata.com/cluster=additional-vols +- type: command + command: kubectl -n $NAMESPACE describe pvcs --selector created=before +- namespace: $NAMESPACE + selector: postgres-operator.crunchydata.com/cluster=additional-vols diff --git a/testing/kuttl/e2e/additional-volumes/01-assert.yaml b/testing/kuttl/e2e/additional-volumes/01-assert.yaml new file mode 100644 index 0000000000..e30e5897bf --- /dev/null +++ b/testing/kuttl/e2e/additional-volumes/01-assert.yaml @@ -0,0 +1,65 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + retry() { bash -ceu 'printf "$1\nSleeping...\n" && sleep 5' - "$@"; } + check_containers_ready() { bash -ceu 'echo "$1" | jq -e ".[] | select(.type==\"ContainersReady\") | .status==\"True\""' - "$@"; } + + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=additional-vols) + [ "$pod" = "" ] && retry "Pod not found" && exit 1 + + condition_json=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.conditions}") + [ "$condition_json" = "" ] && retry "conditions not found" && exit 1 + { check_containers_ready "$condition_json"; } || { + retry "containers not ready" + exit 1 + } + + extra_mounted_in_database=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='database')].volumeMounts[?(@.name=='volumes-extra')]}") + + [ "$extra_mounted_in_database" = "" ] && (echo "extra not found in database" && exit 1) + + ubiq_mounted_in_database=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='database')].volumeMounts[?(@.name=='volumes-ubiq')]}") + + [ "$ubiq_mounted_in_database" = "" ] && (echo "ubiq not found in database" && exit 1) + + extra_mounted_in_rcc=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='replication-cert-copy')].volumeMounts[?(@.name=='volumes-extra')]}") + + [ "$extra_mounted_in_rcc" = "" ] || (echo "extra found in rcc" && exit 1) + + ubiq_mounted_in_rcc=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='replication-cert-copy')].volumeMounts[?(@.name=='volumes-ubiq')]}") + + [ "$ubiq_mounted_in_rcc" = "" ] && (echo "ubiq not found in rcc" && exit 1) + + extra_mounted_in_nss=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='nss-wrapper-init')].volumeMounts[?(@.name=='volumes-extra')]}") + + [ "$extra_mounted_in_nss" = "" ] && (echo "extra not found in nss" && exit 1) + + ubiq_mounted_in_nss=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='nss-wrapper-init')].volumeMounts[?(@.name=='volumes-ubiq')]}") + + [ "$ubiq_mounted_in_nss" = "" ] && (echo "ubiq not found in nss" && exit 1) + + extra_mounted_in_startup=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='postgres-startup')].volumeMounts[?(@.name=='volumes-extra')]}") + + [ "$extra_mounted_in_startup" = "" ] || (echo "extra found in startup" && exit 1) + + ubiq_mounted_in_startup=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='postgres-startup')].volumeMounts[?(@.name=='volumes-ubiq')]}") + + [ "$ubiq_mounted_in_startup" = "" ] && (echo "ubiq not found in startup" && exit 1) + + extra_isnt_readonly=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.spec.volumes[?(@.name=='volumes-extra')].persistentVolumeClaim.readOnly}") + + [ "$extra_isnt_readonly" = "" ] || (echo "extra is readonly" && exit 1) + + ubiq_is_readonly=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.spec.volumes[?(@.name=='volumes-ubiq')].persistentVolumeClaim.readOnly}") + + [ "$ubiq_is_readonly" = "" ] && (echo "ubiq is not readonly" && exit 1) + + echo "clean bill of health" + +collectors: +- type: command + command: kubectl -n $NAMESPACE describe pods --selector postgres-operator.crunchydata.com/cluster=additional-vols +- namespace: $NAMESPACE + selector: postgres-operator.crunchydata.com/cluster=additional-vols diff --git a/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml b/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml new file mode 100644 index 0000000000..c683b8e445 --- /dev/null +++ b/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml @@ -0,0 +1,15 @@ +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: additional-vols +status: + instances: + - name: instance1 + readyReplicas: 1 + replicas: 1 + updatedReplicas: 1 +--- +apiVersion: v1 +kind: Service +metadata: + name: additional-vols-primary diff --git a/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml b/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml new file mode 100644 index 0000000000..bc807757b6 --- /dev/null +++ b/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml @@ -0,0 +1,48 @@ +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: additional-vol + label: + created: before +spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 1Gi +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: additional-vol-2 + label: + created: before +spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 1Gi +--- +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: additional-vols +spec: + postgresVersion: ${KUTTL_PG_VERSION} + instances: + - name: instance1 + volumes: + additional: + - name: extra + containers: [database, nss-wrapper-init] + claimName: additional-vol + - name: ubiq + claimName: additional-vol-2 + readOnly: true + dataVolumeClaimSpec: + accessModes: + - "ReadWriteOnce" + resources: + requests: + storage: 1Gi From 3e245472d20b5c16d32ad4ee3a01f9cb3b160044 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 7 Aug 2025 16:11:46 -0500 Subject: [PATCH 02/10] Add initcontainer case to tests --- internal/controller/postgrescluster/util_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/controller/postgrescluster/util_test.go b/internal/controller/postgrescluster/util_test.go index 884f19e97c..47193c7dba 100644 --- a/internal/controller/postgrescluster/util_test.go +++ b/internal/controller/postgrescluster/util_test.go @@ -475,9 +475,9 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { persistentVolumeClaim: claimName: also`, }, { - tcName: "database container only", + tcName: "database and startup containers only", additionalVolumes: []v1beta1.AdditionalVolume{{ - Containers: []string{"database"}, + Containers: []string{"database", "startup"}, ClaimName: "required", Name: "required", }}, @@ -490,6 +490,9 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { resources: {}`, expectedInitContainers: `- name: startup resources: {} + volumeMounts: + - mountPath: /volumes/required + name: volumes-required - name: config resources: {}`, expectedVolumes: `- name: volumes-required From 11e9c6cfc9e5b5e49a56bd8c45bc2ebbce825a41 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 7 Aug 2025 16:45:18 -0500 Subject: [PATCH 03/10] Add assert to KUTTL --- .../files/00-cluster-created.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml b/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml index c683b8e445..8209935728 100644 --- a/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml +++ b/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml @@ -13,3 +13,17 @@ apiVersion: v1 kind: Service metadata: name: additional-vols-primary +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: additional-vol +status: + phase: Bound +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: additional-vol-2 +status: + phase: Bound \ No newline at end of file From 91939e6227f9029047281b815914abf67925e48a Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 12 Aug 2025 16:13:48 -0500 Subject: [PATCH 04/10] Update name limit --- ...gres-operator.crunchydata.com_postgresclusters.yaml | 8 ++++---- .../v1beta1/postgrescluster_types.go | 7 +++---- .../v1beta1/shared_types.go | 10 ++++++++++ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index a1e6a6a207..61492ee806 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -11049,9 +11049,9 @@ spec: The name of the volume used for mounting path. Volumes are mounted in the pods at `volumes/` Must be unique. - maxLength: 253 + maxLength: 55 minLength: 1 - pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string readOnly: description: Sets the write/read mode of the volume @@ -29658,9 +29658,9 @@ spec: The name of the volume used for mounting path. Volumes are mounted in the pods at `volumes/` Must be unique. - maxLength: 253 + maxLength: 55 minLength: 1 - pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string readOnly: description: Sets the write/read mode of the volume diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index e434816cb4..f738e0e183 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -546,11 +546,10 @@ type AdditionalVolume struct { // Volumes are mounted in the pods at `volumes/` // Must be unique. // --- - // The `Name` field is a `DNS1123Subdomain` type to enforce - // the max length and also allow us to more easily transition - // to CPK-provisioned volumes. + // The `Name` field is a `DNS1123Label` type to enforce + // the max length. // +required - Name DNS1123Subdomain `json:"name"` + Name DNS1123Label `json:"name"` // A reference to a preexisting PVC. // --- diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go index c185cd4b24..fa57e4849b 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go +++ b/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 +// --- +// https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation#IsDNS1123Label +// https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#Format +// +// +kubebuilder:validation:MinLength=1 +// Max length is less than max 63 to allow prepending `volumes-` to name +// +kubebuilder:validation:MaxLength=55 +// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` +type DNS1123Label = string + // --- // Duration represents a string accepted by the Kubernetes API in the "duration" // [format]. This format extends the "duration" [defined by OpenAPI] by allowing From a7ea055e9012863a6bdb708321ee772669875c55 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 12 Aug 2025 16:25:22 -0500 Subject: [PATCH 05/10] Issue event for missing containers --- .../controller/postgrescluster/instance.go | 7 +++- internal/controller/postgrescluster/util.go | 22 ++++++++++-- .../controller/postgrescluster/util_test.go | 35 ++++++++++++++++++- 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 9aff529318..367c549dd5 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1255,7 +1255,12 @@ func (r *Reconciler) reconcileInstance( // 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) + 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) + } } if err == nil { diff --git a/internal/controller/postgrescluster/util.go b/internal/controller/postgrescluster/util.go index ea4c187dd7..1100db1bd5 100644 --- a/internal/controller/postgrescluster/util.go +++ b/internal/controller/postgrescluster/util.go @@ -303,8 +303,9 @@ func AdditionalVolumeMount(name string, readOnly bool) corev1.VolumeMount { // as `volumes-` // and adds the directory to the path `/volumes/` func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, - additionalVolumes []v1beta1.AdditionalVolume) { + additionalVolumes []v1beta1.AdditionalVolume) []string { + missingContainers := []string{} for _, additionalVolumeRequest := range additionalVolumes { additionalVolumeMount := AdditionalVolumeMount( @@ -323,25 +324,40 @@ func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, } names := sets.New(additionalVolumeRequest.Containers...) + allContainers := false + if names.Len() == 0 { + allContainers = true + } for i := range template.Spec.Containers { - if names.Len() == 0 || names.Has(template.Spec.Containers[i].Name) { + 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) + } } } for i := range template.Spec.InitContainers { - if names.Len() == 0 || names.Has(template.Spec.InitContainers[i].Name) { + 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()...) + template.Spec.Volumes = append( template.Spec.Volumes, additionalVolume) } + return missingContainers } diff --git a/internal/controller/postgrescluster/util_test.go b/internal/controller/postgrescluster/util_test.go index 47193c7dba..d9587869ae 100644 --- a/internal/controller/postgrescluster/util_test.go +++ b/internal/controller/postgrescluster/util_test.go @@ -399,6 +399,7 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { expectedContainers string expectedInitContainers string expectedVolumes string + expectedMissing []string }{{ tcName: "all", additionalVolumes: []v1beta1.AdditionalVolume{{ @@ -429,6 +430,7 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { expectedVolumes: `- name: volumes-required persistentVolumeClaim: claimName: required`, + expectedMissing: []string{}, }, { tcName: "multiple additional volumes", additionalVolumes: []v1beta1.AdditionalVolume{{ @@ -474,6 +476,7 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { - name: volumes-other persistentVolumeClaim: claimName: also`, + expectedMissing: []string{}, }, { tcName: "database and startup containers only", additionalVolumes: []v1beta1.AdditionalVolume{{ @@ -498,6 +501,32 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { expectedVolumes: `- name: volumes-required persistentVolumeClaim: claimName: required`, + expectedMissing: []string{}, + }, { + tcName: "container is missing", + additionalVolumes: []v1beta1.AdditionalVolume{{ + Containers: []string{"database", "startup", "missing", "container"}, + 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`, + expectedMissing: []string{"missing", "container"}, }, { tcName: "readonly", additionalVolumes: []v1beta1.AdditionalVolume{{ @@ -522,6 +551,7 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { persistentVolumeClaim: claimName: required readOnly: true`, + expectedMissing: []string{}, }} for _, tc := range testCases { @@ -529,7 +559,7 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { copyPodTemplate := podTemplate.DeepCopy() - addAdditionalVolumesToSpecifiedContainers( + missingContainers := addAdditionalVolumesToSpecifiedContainers( copyPodTemplate, tc.additionalVolumes, ) @@ -543,6 +573,9 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { assert.Assert(t, cmp.MarshalMatches( copyPodTemplate.Spec.Volumes, tc.expectedVolumes)) + assert.Assert(t, cmp.DeepEqual( + missingContainers, + tc.expectedMissing)) }) } } From 4901e187441bedde4b1c79dc46d1695934158e74 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 12 Aug 2025 16:41:52 -0500 Subject: [PATCH 06/10] Fix test --- internal/controller/postgrescluster/util_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/controller/postgrescluster/util_test.go b/internal/controller/postgrescluster/util_test.go index d9587869ae..a7bb061e5f 100644 --- a/internal/controller/postgrescluster/util_test.go +++ b/internal/controller/postgrescluster/util_test.go @@ -573,9 +573,17 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { assert.Assert(t, cmp.MarshalMatches( copyPodTemplate.Spec.Volumes, tc.expectedVolumes)) - assert.Assert(t, cmp.DeepEqual( - missingContainers, - tc.expectedMissing)) + if len(tc.expectedMissing) == 0 { + assert.Assert(t, cmp.DeepEqual( + missingContainers, + tc.expectedMissing)) + } else { + for _, mc := range tc.expectedMissing { + assert.Assert(t, cmp.Contains( + missingContainers, + mc)) + } + } }) } } From cedf6b419afcb2d3ce8a948198ffd841600205ce Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 13 Aug 2025 08:52:33 -0500 Subject: [PATCH 07/10] PR feedback --- ...perator.crunchydata.com_postgresclusters.yaml | 14 ++++++++++++-- internal/controller/postgrescluster/util.go | 15 +++++++++------ .../v1/postgrescluster_types.go | 12 ++++++------ .../v1beta1/postgrescluster_types.go | 16 +++++++++------- .../v1beta1/shared_types.go | 4 ++-- .../v1beta1/zz_generated.deepcopy.go | 8 ++++---- 6 files changed, 42 insertions(+), 27 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index 61492ee806..bb5640e19f 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -11034,6 +11034,9 @@ spec: properties: claimName: description: A reference to a preexisting PVC. + 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 containers: description: |- @@ -11045,11 +11048,13 @@ spec: type: array x-kubernetes-list-type: atomic name: + allOf: + - maxLength: 63 + - maxLength: 55 description: |- The name of the volume used for mounting path. Volumes are mounted in the pods at `volumes/` Must be unique. - maxLength: 55 minLength: 1 pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string @@ -29643,6 +29648,9 @@ spec: properties: claimName: description: A reference to a preexisting PVC. + 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 containers: description: |- @@ -29654,11 +29662,13 @@ spec: type: array x-kubernetes-list-type: atomic name: + allOf: + - maxLength: 63 + - maxLength: 55 description: |- The name of the volume used for mounting path. Volumes are mounted in the pods at `volumes/` Must be unique. - maxLength: 55 minLength: 1 pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string diff --git a/internal/controller/postgrescluster/util.go b/internal/controller/postgrescluster/util.go index 1100db1bd5..3efab08251 100644 --- a/internal/controller/postgrescluster/util.go +++ b/internal/controller/postgrescluster/util.go @@ -323,6 +323,12 @@ func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, }, } + // Create a set of all the requested containers, + // then in the loops below when we attach the volume to a container, + // we can safely remove that container name from the set. + // This gives us a way to track the containers that are requested but not found. + // This relies on `containers` and `initContainers` together being unique. + // - https://github.com/kubernetes/api/blob/b40c1cacbb902b21a7e0c7bf0967321860c1a632/core/v1/types.go#L3895C27-L3896C33 names := sets.New(additionalVolumeRequest.Containers...) allContainers := false if names.Len() == 0 { @@ -335,9 +341,7 @@ func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, template.Spec.Containers[i].VolumeMounts, additionalVolumeMount) - if names.Has(template.Spec.Containers[i].Name) { - names.Delete(template.Spec.Containers[i].Name) - } + names.Delete(template.Spec.Containers[i].Name) } } @@ -347,9 +351,8 @@ func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, template.Spec.InitContainers[i].VolumeMounts, additionalVolumeMount) - if names.Has(template.Spec.InitContainers[i].Name) { - names.Delete(template.Spec.InitContainers[i].Name) - } + names.Delete(template.Spec.InitContainers[i].Name) + } } diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go index 1778fdefaf..9463c2361e 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go @@ -529,12 +529,6 @@ type PostgresInstanceSetSpec struct { } type PostgresVolumesSpec struct { - // An ephemeral volume for temporary files. - // More info: https://kubernetes.io/docs/concepts/storage/ephemeral-volumes - // --- - // +optional - Temp *v1beta1.VolumeClaimSpec `json:"temp,omitempty"` - // Additional pre-existing volumes to add to the pod. // --- // +optional @@ -542,6 +536,12 @@ type PostgresVolumesSpec struct { // +listMapKey=name // +kubebuilder:validation:MaxItems=10 Additional []v1beta1.AdditionalVolume `json:"additional,omitempty"` + + // An ephemeral volume for temporary files. + // More info: https://kubernetes.io/docs/concepts/storage/ephemeral-volumes + // --- + // +optional + Temp *v1beta1.VolumeClaimSpec `json:"temp,omitempty"` } type TablespaceVolume struct { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index f738e0e183..e909e51441 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -526,12 +526,6 @@ type PostgresInstanceSetSpec struct { } type PostgresVolumesSpec struct { - // An ephemeral volume for temporary files. - // More info: https://kubernetes.io/docs/concepts/storage/ephemeral-volumes - // --- - // +optional - Temp *VolumeClaimSpec `json:"temp,omitempty"` - // Additional pre-existing volumes to add to the pod. // --- // +optional @@ -539,6 +533,12 @@ type PostgresVolumesSpec struct { // +listMapKey=name // +kubebuilder:validation:MaxItems=10 Additional []AdditionalVolume `json:"additional,omitempty"` + + // An ephemeral volume for temporary files. + // More info: https://kubernetes.io/docs/concepts/storage/ephemeral-volumes + // --- + // +optional + Temp *VolumeClaimSpec `json:"temp,omitempty"` } type AdditionalVolume struct { @@ -549,12 +549,14 @@ type AdditionalVolume struct { // The `Name` field is a `DNS1123Label` type to enforce // the max length. // +required + // Max length is less than max 63 to allow prepending `volumes-` to name + // +kubebuilder:validation:MaxLength=55 Name DNS1123Label `json:"name"` // A reference to a preexisting PVC. // --- // +required - ClaimName string `json:"claimName"` + ClaimName DNS1123Subdomain `json:"claimName"` // The containers to attach this volume to. // A blank/unset `Containers` field matches all containers. diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go index fa57e4849b..7a7d554273 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go @@ -33,12 +33,12 @@ type ConfigDataKey = string type DNS1123Subdomain = string // --- +// https://docs.k8s.io/concepts/overview/working-with-objects/names#dns-label-names // https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation#IsDNS1123Label // https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#Format // // +kubebuilder:validation:MinLength=1 -// Max length is less than max 63 to allow prepending `volumes-` to name -// +kubebuilder:validation:MaxLength=55 +// +kubebuilder:validation:MaxLength=63 // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` type DNS1123Label = string diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go index b6b32c63b0..02dd91b827 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go @@ -2530,10 +2530,6 @@ func (in *PostgresUserSpec) DeepCopy() *PostgresUserSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { *out = *in - if in.Temp != nil { - in, out := &in.Temp, &out.Temp - *out = (*in).DeepCopy() - } if in.Additional != nil { in, out := &in.Additional, &out.Additional *out = make([]AdditionalVolume, len(*in)) @@ -2541,6 +2537,10 @@ func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Temp != nil { + in, out := &in.Temp, &out.Temp + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresVolumesSpec. From 2c8a40ec8df664bff3c56a5e6e9973c5b27df613 Mon Sep 17 00:00:00 2001 From: Benjamin Blattberg Date: Wed, 13 Aug 2025 08:54:57 -0500 Subject: [PATCH 08/10] Update internal/controller/postgrescluster/instance.go Co-authored-by: Drew Sessler <36803518+dsessler7@users.noreply.github.com> --- internal/controller/postgrescluster/instance.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 367c549dd5..f9b8e12cd3 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1258,8 +1258,8 @@ func (r *Reconciler) reconcileInstance( 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) + r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound", + "The following containers were specified for additional volumes but cannot be found: %s.", missingContainers) } } From 0971139dbede6fd1b76ac943b63804b56cfc45cb Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 13 Aug 2025 09:19:48 -0500 Subject: [PATCH 09/10] Allow no containers --- ...ator.crunchydata.com_postgresclusters.yaml | 6 +- internal/controller/postgrescluster/util.go | 3 +- .../controller/postgrescluster/util_test.go | 72 +++++++++++++++++-- .../v1/zz_generated.deepcopy.go | 8 +-- .../v1beta1/postgrescluster_types.go | 3 +- .../e2e/additional-volumes/00--cluster.yaml | 6 -- .../e2e/additional-volumes/00-assert.yaml | 9 --- .../e2e/additional-volumes/01-assert.yaml | 65 ----------------- .../files/00-cluster-created.yaml | 29 -------- .../files/00-create-cluster.yaml | 48 ------------- 10 files changed, 78 insertions(+), 171 deletions(-) delete mode 100644 testing/kuttl/e2e/additional-volumes/00--cluster.yaml delete mode 100644 testing/kuttl/e2e/additional-volumes/00-assert.yaml delete mode 100644 testing/kuttl/e2e/additional-volumes/01-assert.yaml delete mode 100644 testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml delete mode 100644 testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index bb5640e19f..4db85ca848 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -11041,7 +11041,8 @@ spec: containers: description: |- The containers to attach this volume to. - A blank/unset `Containers` field matches all containers. + An omitted `Containers` field matches all containers. + An empty `Containers` field matches no containers. items: type: string maxItems: 10 @@ -29655,7 +29656,8 @@ spec: containers: description: |- The containers to attach this volume to. - A blank/unset `Containers` field matches all containers. + An omitted `Containers` field matches all containers. + An empty `Containers` field matches no containers. items: type: string maxItems: 10 diff --git a/internal/controller/postgrescluster/util.go b/internal/controller/postgrescluster/util.go index 3efab08251..2fc849956c 100644 --- a/internal/controller/postgrescluster/util.go +++ b/internal/controller/postgrescluster/util.go @@ -331,7 +331,8 @@ func addAdditionalVolumesToSpecifiedContainers(template *corev1.PodTemplateSpec, // - https://github.com/kubernetes/api/blob/b40c1cacbb902b21a7e0c7bf0967321860c1a632/core/v1/types.go#L3895C27-L3896C33 names := sets.New(additionalVolumeRequest.Containers...) allContainers := false - if names.Len() == 0 { + // If the containers list is omitted, we add the volume to all containers + if additionalVolumeRequest.Containers == nil { allContainers = true } diff --git a/internal/controller/postgrescluster/util_test.go b/internal/controller/postgrescluster/util_test.go index a7bb061e5f..0dde296aef 100644 --- a/internal/controller/postgrescluster/util_test.go +++ b/internal/controller/postgrescluster/util_test.go @@ -403,9 +403,8 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { }{{ tcName: "all", additionalVolumes: []v1beta1.AdditionalVolume{{ - Containers: []string{}, - ClaimName: "required", - Name: "required", + ClaimName: "required", + Name: "required", }}, expectedContainers: `- name: database resources: {} @@ -433,14 +432,75 @@ func TestAddAdditionalVolumesToSpecifiedContainers(t *testing.T) { expectedMissing: []string{}, }, { tcName: "multiple additional volumes", + additionalVolumes: []v1beta1.AdditionalVolume{{ + ClaimName: "required", + Name: "required", + }, { + 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`, + expectedMissing: []string{}, + }, { + tcName: "none", additionalVolumes: []v1beta1.AdditionalVolume{{ Containers: []string{}, ClaimName: "required", Name: "required", + }}, + expectedContainers: `- name: database + resources: {} +- name: other + resources: {}`, + expectedInitContainers: `- name: startup + resources: {} +- name: config + resources: {}`, + expectedVolumes: `- name: volumes-required + persistentVolumeClaim: + claimName: required`, + expectedMissing: []string{}, + }, { + tcName: "multiple additional volumes", + additionalVolumes: []v1beta1.AdditionalVolume{{ + ClaimName: "required", + Name: "required", }, { - Containers: []string{}, - ClaimName: "also", - Name: "other", + ClaimName: "also", + Name: "other", }}, expectedContainers: `- name: database resources: {} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go index b296067c9b..4c5826c021 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go @@ -652,10 +652,6 @@ func (in *PostgresUserInterfaceStatus) DeepCopy() *PostgresUserInterfaceStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { *out = *in - if in.Temp != nil { - in, out := &in.Temp, &out.Temp - *out = (*in).DeepCopy() - } if in.Additional != nil { in, out := &in.Additional, &out.Additional *out = make([]v1beta1.AdditionalVolume, len(*in)) @@ -663,6 +659,10 @@ func (in *PostgresVolumesSpec) DeepCopyInto(out *PostgresVolumesSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Temp != nil { + in, out := &in.Temp, &out.Temp + *out = (*in).DeepCopy() + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresVolumesSpec. diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index e909e51441..cd427d82a6 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -559,7 +559,8 @@ type AdditionalVolume struct { ClaimName DNS1123Subdomain `json:"claimName"` // The containers to attach this volume to. - // A blank/unset `Containers` field matches all containers. + // An omitted `Containers` field matches all containers. + // An empty `Containers` field matches no containers. // --- // +optional // +listType=atomic diff --git a/testing/kuttl/e2e/additional-volumes/00--cluster.yaml b/testing/kuttl/e2e/additional-volumes/00--cluster.yaml deleted file mode 100644 index 801a22d460..0000000000 --- a/testing/kuttl/e2e/additional-volumes/00--cluster.yaml +++ /dev/null @@ -1,6 +0,0 @@ -apiVersion: kuttl.dev/v1beta1 -kind: TestStep -apply: -- files/00-create-cluster.yaml -assert: -- files/00-cluster-created.yaml diff --git a/testing/kuttl/e2e/additional-volumes/00-assert.yaml b/testing/kuttl/e2e/additional-volumes/00-assert.yaml deleted file mode 100644 index f54a5d7d4e..0000000000 --- a/testing/kuttl/e2e/additional-volumes/00-assert.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: kuttl.dev/v1beta1 -kind: TestAssert -collectors: -- type: command - command: kubectl -n $NAMESPACE describe pods --selector postgres-operator.crunchydata.com/cluster=additional-vols -- type: command - command: kubectl -n $NAMESPACE describe pvcs --selector created=before -- namespace: $NAMESPACE - selector: postgres-operator.crunchydata.com/cluster=additional-vols diff --git a/testing/kuttl/e2e/additional-volumes/01-assert.yaml b/testing/kuttl/e2e/additional-volumes/01-assert.yaml deleted file mode 100644 index e30e5897bf..0000000000 --- a/testing/kuttl/e2e/additional-volumes/01-assert.yaml +++ /dev/null @@ -1,65 +0,0 @@ -apiVersion: kuttl.dev/v1beta1 -kind: TestAssert -commands: -- script: | - retry() { bash -ceu 'printf "$1\nSleeping...\n" && sleep 5' - "$@"; } - check_containers_ready() { bash -ceu 'echo "$1" | jq -e ".[] | select(.type==\"ContainersReady\") | .status==\"True\""' - "$@"; } - - pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ - -l postgres-operator.crunchydata.com/cluster=additional-vols) - [ "$pod" = "" ] && retry "Pod not found" && exit 1 - - condition_json=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.conditions}") - [ "$condition_json" = "" ] && retry "conditions not found" && exit 1 - { check_containers_ready "$condition_json"; } || { - retry "containers not ready" - exit 1 - } - - extra_mounted_in_database=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='database')].volumeMounts[?(@.name=='volumes-extra')]}") - - [ "$extra_mounted_in_database" = "" ] && (echo "extra not found in database" && exit 1) - - ubiq_mounted_in_database=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='database')].volumeMounts[?(@.name=='volumes-ubiq')]}") - - [ "$ubiq_mounted_in_database" = "" ] && (echo "ubiq not found in database" && exit 1) - - extra_mounted_in_rcc=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='replication-cert-copy')].volumeMounts[?(@.name=='volumes-extra')]}") - - [ "$extra_mounted_in_rcc" = "" ] || (echo "extra found in rcc" && exit 1) - - ubiq_mounted_in_rcc=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.containerStatuses[?(@.name=='replication-cert-copy')].volumeMounts[?(@.name=='volumes-ubiq')]}") - - [ "$ubiq_mounted_in_rcc" = "" ] && (echo "ubiq not found in rcc" && exit 1) - - extra_mounted_in_nss=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='nss-wrapper-init')].volumeMounts[?(@.name=='volumes-extra')]}") - - [ "$extra_mounted_in_nss" = "" ] && (echo "extra not found in nss" && exit 1) - - ubiq_mounted_in_nss=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='nss-wrapper-init')].volumeMounts[?(@.name=='volumes-ubiq')]}") - - [ "$ubiq_mounted_in_nss" = "" ] && (echo "ubiq not found in nss" && exit 1) - - extra_mounted_in_startup=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='postgres-startup')].volumeMounts[?(@.name=='volumes-extra')]}") - - [ "$extra_mounted_in_startup" = "" ] || (echo "extra found in startup" && exit 1) - - ubiq_mounted_in_startup=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.status.initContainerStatuses[?(@.name=='postgres-startup')].volumeMounts[?(@.name=='volumes-ubiq')]}") - - [ "$ubiq_mounted_in_startup" = "" ] && (echo "ubiq not found in startup" && exit 1) - - extra_isnt_readonly=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.spec.volumes[?(@.name=='volumes-extra')].persistentVolumeClaim.readOnly}") - - [ "$extra_isnt_readonly" = "" ] || (echo "extra is readonly" && exit 1) - - ubiq_is_readonly=$(kubectl get "${pod}" -n "${NAMESPACE}" -o jsonpath="{.spec.volumes[?(@.name=='volumes-ubiq')].persistentVolumeClaim.readOnly}") - - [ "$ubiq_is_readonly" = "" ] && (echo "ubiq is not readonly" && exit 1) - - echo "clean bill of health" - -collectors: -- type: command - command: kubectl -n $NAMESPACE describe pods --selector postgres-operator.crunchydata.com/cluster=additional-vols -- namespace: $NAMESPACE - selector: postgres-operator.crunchydata.com/cluster=additional-vols diff --git a/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml b/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml deleted file mode 100644 index 8209935728..0000000000 --- a/testing/kuttl/e2e/additional-volumes/files/00-cluster-created.yaml +++ /dev/null @@ -1,29 +0,0 @@ -apiVersion: postgres-operator.crunchydata.com/v1beta1 -kind: PostgresCluster -metadata: - name: additional-vols -status: - instances: - - name: instance1 - readyReplicas: 1 - replicas: 1 - updatedReplicas: 1 ---- -apiVersion: v1 -kind: Service -metadata: - name: additional-vols-primary ---- -apiVersion: v1 -kind: PersistentVolumeClaim -metadata: - name: additional-vol -status: - phase: Bound ---- -apiVersion: v1 -kind: PersistentVolumeClaim -metadata: - name: additional-vol-2 -status: - phase: Bound \ No newline at end of file diff --git a/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml b/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml deleted file mode 100644 index bc807757b6..0000000000 --- a/testing/kuttl/e2e/additional-volumes/files/00-create-cluster.yaml +++ /dev/null @@ -1,48 +0,0 @@ -apiVersion: v1 -kind: PersistentVolumeClaim -metadata: - name: additional-vol - label: - created: before -spec: - accessModes: - - ReadWriteOnce - resources: - requests: - storage: 1Gi ---- -apiVersion: v1 -kind: PersistentVolumeClaim -metadata: - name: additional-vol-2 - label: - created: before -spec: - accessModes: - - ReadWriteOnce - resources: - requests: - storage: 1Gi ---- -apiVersion: postgres-operator.crunchydata.com/v1beta1 -kind: PostgresCluster -metadata: - name: additional-vols -spec: - postgresVersion: ${KUTTL_PG_VERSION} - instances: - - name: instance1 - volumes: - additional: - - name: extra - containers: [database, nss-wrapper-init] - claimName: additional-vol - - name: ubiq - claimName: additional-vol-2 - readOnly: true - dataVolumeClaimSpec: - accessModes: - - "ReadWriteOnce" - resources: - requests: - storage: 1Gi From b4e667ae86feba7e5e743aad38d6fe61a47027a5 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 13 Aug 2025 09:22:28 -0500 Subject: [PATCH 10/10] Alphabetize --- .../v1beta1/postgrescluster_types.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index cd427d82a6..4e9af31b5b 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -542,17 +542,6 @@ type PostgresVolumesSpec struct { } type AdditionalVolume struct { - // The name of the volume used for mounting path. - // Volumes are mounted in the pods at `volumes/` - // Must be unique. - // --- - // The `Name` field is a `DNS1123Label` type to enforce - // the max length. - // +required - // Max length is less than max 63 to allow prepending `volumes-` to name - // +kubebuilder:validation:MaxLength=55 - Name DNS1123Label `json:"name"` - // A reference to a preexisting PVC. // --- // +required @@ -567,6 +556,17 @@ type AdditionalVolume struct { // +kubebuilder:validation:MaxItems=10 Containers []string `json:"containers,omitempty"` + // The name of the volume used for mounting path. + // Volumes are mounted in the pods at `volumes/` + // Must be unique. + // --- + // The `Name` field is a `DNS1123Label` type to enforce + // the max length. + // +required + // Max length is less than max 63 to allow prepending `volumes-` to name + // +kubebuilder:validation:MaxLength=55 + Name DNS1123Label `json:"name"` + // Sets the write/read mode of the volume // --- // +optional