Skip to content
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.

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

Fix resource constraints #2735

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dmotte
Copy link
Contributor

@dmotte dmotte commented Aug 20, 2024

The documentation about postgres_pod_resources says that you can disable the defaults by setting them to zero OR empty string:

Empty string or 0 disables the default.

Source:

## Kubernetes resource requests
This group allows you to configure resource requests for the Postgres pods.
Those parameters are grouped under the `postgres_pod_resources` key in a
CRD-based configuration.
* **default_cpu_request**
CPU request value for the Postgres containers, unless overridden by
cluster-specific settings. Empty string or `0` disables the default.
* **default_memory_request**
memory request value for the Postgres containers, unless overridden by
cluster-specific settings. Empty string or `0` disables the default.
* **default_cpu_limit**
CPU limits for the Postgres containers, unless overridden by cluster-specific
settings. Empty string or `0` disables the default.
* **default_memory_limit**
memory limits for the Postgres containers, unless overridden by cluster-specific
settings. Empty string or `0` disables the default.
* **max_cpu_request**
optional upper boundary for CPU request
* **max_memory_request**
optional upper boundary for memory request
* **min_cpu_limit**
hard CPU minimum what we consider to be required to properly run Postgres
clusters with Patroni on Kubernetes.
* **min_memory_limit**
hard memory minimum what we consider to be required to properly run Postgres
clusters with Patroni on Kubernetes.

But it's not currently possible to set them to empty string, due to the regular expressions in charts/postgres-operator/crds/operatorconfigurations.yaml.

Also, the enforceMinResourceLimits function currently supports only the empty string case, which is impossible to use due to the aforementioned regexes.

This PR aims to fix these two issues 🙂

@FxKu
Copy link
Member

FxKu commented Aug 21, 2024

@dmotte thanks! Could you provide also unit tests that cover this "new" behavior so I could also try them on the main branch and see what doesn't work? This would be a great improvement to this PR.

@FxKu FxKu added this to the 1.14.0 milestone Aug 21, 2024
@FxKu FxKu added the bugfix label Aug 21, 2024
@dmotte
Copy link
Contributor Author

dmotte commented Aug 22, 2024

@FxKu done 🙂 I hope it's ok

pkg/cluster/k8sres_test.go Outdated Show resolved Hide resolved
pkg/cluster/k8sres.go Outdated Show resolved Hide resolved
pkg/cluster/k8sres.go Outdated Show resolved Hide resolved
@FxKu
Copy link
Member

FxKu commented Aug 28, 2024

Seems to me that only the schema validation needs to change. It's still good though to add a "0" example to the unit test.

@dmotte
Copy link
Contributor Author

dmotte commented Aug 30, 2024

@FxKu yeah, I think I got your point and you are right :)

Just to double check, correct me if I'm wrong: util.IsSmallerQuantity(somePositiveQuantity, "0") returns false, so isSmaller ends up being false and the limit is not enforced.

Thank you! Fixed in 24df591

@FxKu
Copy link
Member

FxKu commented Sep 3, 2024

Thanks. Can you also update these files with the new validation pattern:

@dmotte
Copy link
Contributor Author

dmotte commented Sep 3, 2024

@FxKu I think you meant manifests/operatorconfiguration.crd.yaml instead of manifests/postgresql.crd.yaml, so I updated that one instead. Done in 107fe9b 😉 let me know if it's OK or you have other requests

@FxKu
Copy link
Member

FxKu commented Sep 4, 2024

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Good state for merge
Development

Successfully merging this pull request may close these issues.

2 participants