Skip to content

Commit 7e38f44

Browse files
committed
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.
1 parent 743dc39 commit 7e38f44

File tree

3 files changed

+25
-18
lines changed

3 files changed

+25
-18
lines changed

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11683,6 +11683,7 @@ spec:
1168311683
required:
1168411684
- dataVolumeClaimSpec
1168511685
type: object
11686+
maxItems: 16
1168611687
minItems: 1
1168711688
type: array
1168811689
x-kubernetes-list-map-keys:
@@ -18412,7 +18413,9 @@ spec:
1841218413
- fieldPath: .config.parameters.log_directory
1841318414
message: all instances need an additional volume to log in "/volumes"
1841418415
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18415-
|| !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)
18416+
|| !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue()
18417+
&& i.volumes.additional.exists(volume, v.startsWith("/volumes/" +
18418+
volume.name)))).orValue(true)
1841618419
status:
1841718420
description: PostgresClusterStatus defines the observed state of PostgresCluster
1841818421
properties:

internal/testing/validation/postgrescluster/postgres_config_test.go

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -226,33 +226,39 @@ func TestPostgresConfigParametersV1(t *testing.T) {
226226
message: `"/pgwal/logs/postgres"`,
227227
},
228228

229-
// Directories inside /volumes are acceptable, but every instance set needs additional volumes.
230-
//
231-
// TODO(validation): This could be more precise and check the directory name of each additional
232-
// volume, but Kubernetes 1.33 incorrectly estimates the cost of volume.name:
233-
// https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184
229+
// Directories inside /volumes are acceptable, but every instance set needs the correct additional volume.
234230
{
235-
name: "two instance sets and two additional volumes",
236-
value: "/volumes/anything",
231+
name: "two instance sets and two correct additional volumes",
232+
value: "/volumes/yep",
237233
instances: `[
238-
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } },
239-
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: b }] } },
234+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } },
235+
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: b }] } },
240236
]`,
241237
valid: true,
242238
},
239+
{
240+
name: "two instance sets and one correct additional volume",
241+
value: "/volumes/yep",
242+
instances: `[
243+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } },
244+
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: diff, claimName: b }] } },
245+
]`,
246+
valid: false,
247+
message: `all instances need an additional volume`,
248+
},
243249
{
244250
name: "two instance sets and one additional volume",
245-
value: "/volumes/anything",
251+
value: "/volumes/yep",
246252
instances: `[
247-
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } },
253+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: yep, claimName: a }] } },
248254
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
249255
]`,
250256
valid: false,
251257
message: `all instances need an additional volume`,
252258
},
253259
{
254260
name: "two instance sets and no additional volumes",
255-
value: "/volumes/anything",
261+
value: "/volumes/yep",
256262
instances: `[
257263
{ name: one, dataVolumeClaimSpec: ` + volume + ` },
258264
{ name: two, dataVolumeClaimSpec: ` + volume + ` },

pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,7 @@ import (
2121
//
2222
// +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)`
2323
// +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)`
24-
//
25-
// +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)`
26-
//
27-
// TODO: Also check the above path against volume names: `i.?volumes.additional.hasValue() && i.volumes.additional.exists(directory.startsWith("/volumes/" + volume.name))`
28-
// https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184
24+
// +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)`
2925
type PostgresClusterSpec struct {
3026
// +optional
3127
Metadata *v1beta1.Metadata `json:"metadata,omitempty"`
@@ -110,9 +106,11 @@ type PostgresClusterSpec struct {
110106

111107
// Specifies one or more sets of PostgreSQL pods that replicate data for
112108
// this cluster.
109+
// ---
113110
// +listType=map
114111
// +listMapKey=name
115112
// +kubebuilder:validation:MinItems=1
113+
// +kubebuilder:validation:MaxItems=16
116114
// +operator-sdk:csv:customresourcedefinitions:type=spec,order=2
117115
InstanceSets []PostgresInstanceSetSpec `json:"instances"`
118116

0 commit comments

Comments
 (0)