Skip to content

Commit 36df1bc

Browse files
FxKuFxKu
authored
refactor GenerateResourceRequirements and provide unit tests (zalando#1822)
* refactor GenerateResourceRequirements and provide unit tests
1 parent 38db48c commit 36df1bc

File tree

13 files changed

+466
-217
lines changed

13 files changed

+466
-217
lines changed

‎charts/postgres-operator/crds/postgresqls.yaml

-18
Original file line numberDiff line numberDiff line change
@@ -150,15 +150,9 @@ spec:
150150
minimum: 1
151151
resources:
152152
type: object
153-
required:
154-
- requests
155-
- limits
156153
properties:
157154
limits:
158155
type: object
159-
required:
160-
- cpu
161-
- memory
162156
properties:
163157
cpu:
164158
type: string
@@ -168,9 +162,6 @@ spec:
168162
pattern: '^(\d+(e\d+)?|\d+(\.\d+)?(e\d+)?[EPTGMK]i?)$'
169163
requests:
170164
type: object
171-
required:
172-
- cpu
173-
- memory
174165
properties:
175166
cpu:
176167
type: string
@@ -406,15 +397,9 @@ spec:
406397
description: deprecated
407398
resources:
408399
type: object
409-
required:
410-
- requests
411-
- limits
412400
properties:
413401
limits:
414402
type: object
415-
required:
416-
- cpu
417-
- memory
418403
properties:
419404
cpu:
420405
type: string
@@ -443,9 +428,6 @@ spec:
443428
# than the corresponding limit.
444429
requests:
445430
type: object
446-
required:
447-
- cpu
448-
- memory
449431
properties:
450432
cpu:
451433
type: string

‎docs/reference/cluster_manifest.md

+1-3
Original file line numberDiff line numberDiff line change
@@ -322,9 +322,7 @@ explanation of `ttl` and `loop_wait` parameters.
322322

323323
Those parameters define [CPU and memory requests and limits](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/)
324324
for the Postgres container. They are grouped under the `resources` top-level
325-
key with subgroups `requests` and `limits`. The whole section is optional,
326-
however if you specify a request or limit you have to define everything
327-
(unless you are not modifying the default CRD schema validation).
325+
key with subgroups `requests` and `limits`.
328326

329327
### Requests
330328

‎docs/reference/operator_parameters.md

+3-3
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,9 @@ Those are top-level keys, containing both leaf keys and groups.
159159
at the cost of overprovisioning memory and potential scheduling problems for
160160
containers with high memory limits due to the lack of memory on Kubernetes
161161
cluster nodes. This affects all containers created by the operator (Postgres,
162-
Scalyr sidecar, and other sidecars except **sidecars** defined in the operator
163-
configuration); to set resources for the operator's own container, change the
164-
[operator deployment manually](https://github.com/zalando/postgres-operator/blob/master/manifests/postgres-operator.yaml#L20).
162+
connection pooler, logical backup, scalyr sidecar, and other sidecars except
163+
**sidecars** defined in the operator configuration); to set resources for the
164+
operator's own container, change the [operator deployment manually](https://github.com/zalando/postgres-operator/blob/master/manifests/postgres-operator.yaml#L20).
165165
The default is `false`.
166166

167167
## Postgres users

‎manifests/postgresql.crd.yaml

-18
Original file line numberDiff line numberDiff line change
@@ -148,15 +148,9 @@ spec:
148148
minimum: 1
149149
resources:
150150
type: object
151-
required:
152-
- requests
153-
- limits
154151
properties:
155152
limits:
156153
type: object
157-
required:
158-
- cpu
159-
- memory
160154
properties:
161155
cpu:
162156
type: string
@@ -166,9 +160,6 @@ spec:
166160
pattern: '^(\d+(e\d+)?|\d+(\.\d+)?(e\d+)?[EPTGMK]i?)$'
167161
requests:
168162
type: object
169-
required:
170-
- cpu
171-
- memory
172163
properties:
173164
cpu:
174165
type: string
@@ -404,15 +395,9 @@ spec:
404395
description: deprecated
405396
resources:
406397
type: object
407-
required:
408-
- requests
409-
- limits
410398
properties:
411399
limits:
412400
type: object
413-
required:
414-
- cpu
415-
- memory
416401
properties:
417402
cpu:
418403
type: string
@@ -441,9 +426,6 @@ spec:
441426
# than the corresponding limit.
442427
requests:
443428
type: object
444-
required:
445-
- cpu
446-
- memory
447429
properties:
448430
cpu:
449431
type: string

‎pkg/apis/acid.zalan.do/v1/crds.go

+6-12
Original file line numberDiff line numberDiff line change
@@ -238,12 +238,10 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
238238
Minimum: &min1,
239239
},
240240
"resources": {
241-
Type: "object",
242-
Required: []string{"requests", "limits"},
241+
Type: "object",
243242
Properties: map[string]apiextv1.JSONSchemaProps{
244243
"limits": {
245-
Type: "object",
246-
Required: []string{"cpu", "memory"},
244+
Type: "object",
247245
Properties: map[string]apiextv1.JSONSchemaProps{
248246
"cpu": {
249247
Type: "string",
@@ -256,8 +254,7 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
256254
},
257255
},
258256
"requests": {
259-
Type: "object",
260-
Required: []string{"cpu", "memory"},
257+
Type: "object",
261258
Properties: map[string]apiextv1.JSONSchemaProps{
262259
"cpu": {
263260
Type: "string",
@@ -648,12 +645,10 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
648645
Description: "deprecated",
649646
},
650647
"resources": {
651-
Type: "object",
652-
Required: []string{"requests", "limits"},
648+
Type: "object",
653649
Properties: map[string]apiextv1.JSONSchemaProps{
654650
"limits": {
655-
Type: "object",
656-
Required: []string{"cpu", "memory"},
651+
Type: "object",
657652
Properties: map[string]apiextv1.JSONSchemaProps{
658653
"cpu": {
659654
Type: "string",
@@ -666,8 +661,7 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
666661
},
667662
},
668663
"requests": {
669-
Type: "object",
670-
Required: []string{"cpu", "memory"},
664+
Type: "object",
671665
Properties: map[string]apiextv1.JSONSchemaProps{
672666
"cpu": {
673667
Type: "string",

‎pkg/apis/acid.zalan.do/v1/postgresql_type.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ type Patroni struct {
167167
Slots map[string]map[string]string `json:"slots,omitempty"`
168168
SynchronousMode bool `json:"synchronous_mode,omitempty"`
169169
SynchronousModeStrict bool `json:"synchronous_mode_strict,omitempty"`
170-
SynchronousNodeCount uint32 `json:"synchronous_node_count,omitempty" defaults:1`
170+
SynchronousNodeCount uint32 `json:"synchronous_node_count,omitempty" defaults:"1"`
171171
}
172172

173173
// StandbyDescription contains s3 wal path

‎pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go

+15-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/cluster/cluster.go

-57
Original file line numberDiff line numberDiff line change
@@ -256,10 +256,6 @@ func (c *Cluster) Create() error {
256256
c.KubeClient.SetPostgresCRDStatus(c.clusterName(), acidv1.ClusterStatusCreating)
257257
c.eventRecorder.Event(c.GetReference(), v1.EventTypeNormal, "Create", "Started creation of new cluster resources")
258258

259-
if err = c.enforceMinResourceLimits(&c.Spec); err != nil {
260-
return fmt.Errorf("could not enforce minimum resource limits: %v", err)
261-
}
262-
263259
for _, role := range []PostgresRole{Master, Replica} {
264260

265261
if c.Endpoints[role] != nil {
@@ -676,50 +672,6 @@ func comparePorts(a, b []v1.ContainerPort) bool {
676672
return true
677673
}
678674

679-
func (c *Cluster) enforceMinResourceLimits(spec *acidv1.PostgresSpec) error {
680-
681-
var (
682-
isSmaller bool
683-
err error
684-
)
685-
686-
if spec.Resources == nil {
687-
return nil
688-
}
689-
690-
// setting limits too low can cause unnecessary evictions / OOM kills
691-
minCPULimit := c.OpConfig.MinCPULimit
692-
minMemoryLimit := c.OpConfig.MinMemoryLimit
693-
694-
cpuLimit := spec.Resources.ResourceLimits.CPU
695-
if cpuLimit != "" {
696-
isSmaller, err = util.IsSmallerQuantity(cpuLimit, minCPULimit)
697-
if err != nil {
698-
return fmt.Errorf("could not compare defined CPU limit %s with configured minimum value %s: %v", cpuLimit, minCPULimit, err)
699-
}
700-
if isSmaller {
701-
c.logger.Warningf("defined CPU limit %s is below required minimum %s and will be increased", cpuLimit, minCPULimit)
702-
c.eventRecorder.Eventf(c.GetReference(), v1.EventTypeWarning, "ResourceLimits", "defined CPU limit %s is below required minimum %s and will be set to it", cpuLimit, minCPULimit)
703-
spec.Resources.ResourceLimits.CPU = minCPULimit
704-
}
705-
}
706-
707-
memoryLimit := spec.Resources.ResourceLimits.Memory
708-
if memoryLimit != "" {
709-
isSmaller, err = util.IsSmallerQuantity(memoryLimit, minMemoryLimit)
710-
if err != nil {
711-
return fmt.Errorf("could not compare defined memory limit %s with configured minimum value %s: %v", memoryLimit, minMemoryLimit, err)
712-
}
713-
if isSmaller {
714-
c.logger.Warningf("defined memory limit %s is below required minimum %s and will be increased", memoryLimit, minMemoryLimit)
715-
c.eventRecorder.Eventf(c.GetReference(), v1.EventTypeWarning, "ResourceLimits", "defined memory limit %s is below required minimum %s and will be set to it", memoryLimit, minMemoryLimit)
716-
spec.Resources.ResourceLimits.Memory = minMemoryLimit
717-
}
718-
}
719-
720-
return nil
721-
}
722-
723675
// Update changes Kubernetes objects according to the new specification. Unlike the sync case, the missing object
724676
// (i.e. service) is treated as an error
725677
// logical backup cron jobs are an exception: a user-initiated Update can enable a logical backup job
@@ -799,22 +751,13 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error {
799751

800752
// Statefulset
801753
func() {
802-
if err := c.enforceMinResourceLimits(&c.Spec); err != nil {
803-
c.logger.Errorf("could not sync resources: %v", err)
804-
updateFailed = true
805-
return
806-
}
807-
808754
oldSs, err := c.generateStatefulSet(&oldSpec.Spec)
809755
if err != nil {
810756
c.logger.Errorf("could not generate old statefulset spec: %v", err)
811757
updateFailed = true
812758
return
813759
}
814760

815-
// update newSpec to for latter comparison with oldSpec
816-
c.enforceMinResourceLimits(&newSpec.Spec)
817-
818761
newSs, err := c.generateStatefulSet(&newSpec.Spec)
819762
if err != nil {
820763
c.logger.Errorf("could not generate new statefulset spec: %v", err)

‎pkg/cluster/connection_pooler.go

+6-4
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,10 @@ func (c *Cluster) generateConnectionPoolerPodTemplate(role PostgresRole) (
210210
connectionPoolerSpec = &acidv1.ConnectionPooler{}
211211
}
212212
gracePeriod := int64(c.OpConfig.PodTerminateGracePeriod.Seconds())
213-
resources, err := generateResourceRequirements(
213+
resources, err := c.generateResourceRequirements(
214214
connectionPoolerSpec.Resources,
215-
makeDefaultConnectionPoolerResources(&c.OpConfig))
215+
makeDefaultConnectionPoolerResources(&c.OpConfig),
216+
connectionPoolerContainer)
216217

217218
effectiveDockerImage := util.Coalesce(
218219
connectionPoolerSpec.DockerImage,
@@ -627,8 +628,9 @@ func (c *Cluster) needSyncConnectionPoolerDefaults(Config *Config, spec *acidv1.
627628
reasons = append(reasons, msg)
628629
}
629630

630-
expectedResources, err := generateResourceRequirements(spec.Resources,
631-
makeDefaultConnectionPoolerResources(&Config.OpConfig))
631+
expectedResources, err := c.generateResourceRequirements(spec.Resources,
632+
makeDefaultConnectionPoolerResources(&Config.OpConfig),
633+
connectionPoolerContainer)
632634

633635
// An error to generate expected resources means something is not quite
634636
// right, but for the purpose of robustness do not panic here, just report

0 commit comments

Comments
 (0)