From 423c4af661ac5b503c49b9df5f0105d7f9f3c243 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Tue, 16 Sep 2025 09:38:46 -0500 Subject: [PATCH] Limit the number of allowed instances sets in v1 This allows CEL rules inside instance sets to fit within the cost budget. Without it, Kubernetes estimates that rules may execute over the maximum size of a request. The cost of the affected rule changes from ~700k to ~9k. --- ...ator.crunchydata.com_postgresclusters.yaml | 5 +++- .../postgrescluster/postgres_config_test.go | 30 +++++++++++-------- .../v1/postgrescluster_types.go | 8 ++--- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index 5fe0db4d0..c863abe73 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -11683,6 +11683,7 @@ spec: required: - dataVolumeClaimSpec type: object + maxItems: 16 minItems: 1 type: array x-kubernetes-list-map-keys: @@ -18412,7 +18413,9 @@ spec: - fieldPath: .config.parameters.log_directory message: all instances need an additional volume to log in "/volumes" rule: self.?config.parameters.log_directory.optMap(v, type(v) != string - || !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true) + || !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue() + && i.volumes.additional.exists(volume, v.startsWith("/volumes/" + + volume.name)))).orValue(true) status: description: PostgresClusterStatus defines the observed state of PostgresCluster properties: diff --git a/internal/testing/validation/postgrescluster/postgres_config_test.go b/internal/testing/validation/postgrescluster/postgres_config_test.go index d9529a8d0..5a636ac43 100644 --- a/internal/testing/validation/postgrescluster/postgres_config_test.go +++ b/internal/testing/validation/postgrescluster/postgres_config_test.go @@ -226,25 +226,31 @@ func TestPostgresConfigParametersV1(t *testing.T) { message: `"/pgwal/logs/postgres"`, }, - // Directories inside /volumes are acceptable, but every instance set needs additional volumes. - // - // TODO(validation): This could be more precise and check the directory name of each additional - // volume, but Kubernetes 1.33 incorrectly estimates the cost of volume.name: - // https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184 + // Directories inside /volumes are acceptable, but every instance set needs the correct additional volume. { - name: "two instance sets and two additional volumes", - value: "/volumes/anything", + name: "two instance sets and two correct additional volumes", + value: "/volumes/yep", instances: `[ - { name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } }, - { name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: b }] } }, + { name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } }, + { name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: b }] } }, ]`, valid: true, }, + { + name: "two instance sets and one correct additional volume", + value: "/volumes/yep", + instances: `[ + { name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } }, + { name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: diff, claimName: b }] } }, + ]`, + valid: false, + message: `all instances need an additional volume`, + }, { name: "two instance sets and one additional volume", - value: "/volumes/anything", + value: "/volumes/yep", instances: `[ - { name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } }, + { name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } }, { name: two, dataVolumeClaimSpec: ` + volume + ` }, ]`, valid: false, @@ -252,7 +258,7 @@ func TestPostgresConfigParametersV1(t *testing.T) { }, { name: "two instance sets and no additional volumes", - value: "/volumes/anything", + value: "/volumes/yep", instances: `[ { name: one, dataVolumeClaimSpec: ` + volume + ` }, { name: two, dataVolumeClaimSpec: ` + volume + ` }, 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 2c3b9c411..1a642e13c 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go @@ -21,11 +21,7 @@ import ( // // +kubebuilder:validation:XValidation:fieldPath=`.config.parameters.log_directory`,message=`all instances need "volumes.temp" to log in "/pgtmp"`,rule=`self.?config.parameters.log_directory.optMap(v, type(v) != string || !v.startsWith("/pgtmp/logs/postgres") || self.instances.all(i, i.?volumes.temp.hasValue())).orValue(true)` // +kubebuilder:validation:XValidation:fieldPath=`.config.parameters.log_directory`,message=`all instances need "walVolumeClaimSpec" to log in "/pgwal"`,rule=`self.?config.parameters.log_directory.optMap(v, type(v) != string || !v.startsWith("/pgwal/logs/postgres") || self.instances.all(i, i.?walVolumeClaimSpec.hasValue())).orValue(true)` -// -// +kubebuilder:validation:XValidation:fieldPath=`.config.parameters.log_directory`,message=`all instances need an additional volume to log in "/volumes"`,rule=`self.?config.parameters.log_directory.optMap(v, type(v) != string || !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)` -// -// TODO: Also check the above path against volume names: `i.?volumes.additional.hasValue() && i.volumes.additional.exists(directory.startsWith("/volumes/" + volume.name))` -// https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184 +// +kubebuilder:validation:XValidation:fieldPath=`.config.parameters.log_directory`,message=`all instances need an additional volume to log in "/volumes"`,rule=`self.?config.parameters.log_directory.optMap(v, type(v) != string || !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue() && i.volumes.additional.exists(volume, v.startsWith("/volumes/" + volume.name)))).orValue(true)` type PostgresClusterSpec struct { // +optional Metadata *v1beta1.Metadata `json:"metadata,omitempty"` @@ -110,9 +106,11 @@ type PostgresClusterSpec struct { // Specifies one or more sets of PostgreSQL pods that replicate data for // this cluster. + // --- // +listType=map // +listMapKey=name // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:MaxItems=16 // +operator-sdk:csv:customresourcedefinitions:type=spec,order=2 InstanceSets []PostgresInstanceSetSpec `json:"instances"`