Add hugepages 2Mi and 1Gi fields to ResourceDescription and pass them to the statefulset #2311
Conversation
silenium-dev
requested review from
sdudoladov,
Jan-M,
FxKu,
jopadi,
idanovinda and
hughcapet
as code owners
|
Is there anything blocking this from being accepted? This issue is currently preventing us from using postgres-operator. Would it be helpful to have a more minimal diff? |
@willstott101 I've cleaned up the diff as suggested, I wasn't aware that GoLand had formatted the entire docs file. |
|
I've just built this locally and it works as hoped, allowing hugepage allocations to be passed to the db pods which then don't crash when running on k8s nodes with hugepages enabled 👍 |
However, it would be even more useful to be able to completely disable hugepages in the postgres pods by being able to pass |
|
Any updates on this? |
|
@FxKu myself also curious about this. |
| @@ -688,6 +688,25 @@ manifest the operator will raise the limits to the configured minimum values. | |||
| If no resources are defined in the manifest they will be obtained from the | |||
| configured [default requests](reference/operator_parameters.md#kubernetes-resource-requests). | |||
|
|
|||
| ### HugePages support | |||
There was a problem hiding this comment.
Seems this comes with K8s 1.28
Can we also link https://kubernetes.io/docs/tasks/manage-hugepages/scheduling-hugepages/
|
Turning it off can work already via Patroni/Postgres config |
| @@ -2979,6 +2978,98 @@ func TestGenerateResourceRequirements(t *testing.T) { | |||
| ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "2Gi"}, | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Can we amend the other test to test for no huge pages set? Given its K8s 1.28 feature lets make sure its not set if not enabled.
There was a problem hiding this comment.
I've added a test to cover this
|
Tiny comments from my side |
This is true for a cluster that is already created, but when creating new clusters the initial bootstrap dies with a I think this might be the behavior #2311 (comment) was referring to It's also very possible there's an option I'm missing in the CRD that could accomplish this, I just have yet to find it. It seems like @silenium-dev has gotten the review changes completed. Any chance you could give this a re-review @Jan-M? |
|
We faced a huge issue with this too: I verified that huge pages was configured and i only updated the value from 1024 to 1536 to have enough for mayastore. After that the databases destroyed themselves. Due to me being sure that huge pages was available before, took ages to figure this out. postgresql segfaulted btw (will create a ticket at postgres for this segfault). |
|
Any chance this PR could be approved/merged soon? 😸 |
|
@silenium-dev looks good. Lets get this ready to get merged. Only minor stuff is missing now. Can you
Thanks in advance!! |
All done, please review the changes. |
|
The e2e tests use the full cluster manifest, but the test environment doesn't support hugepages. I could set the hugepages requests and limits in the manifest to 0, but this kind of defeats the purpose of being a full example. I don't have much experience with GitHub Actions, would it be possible to enable support for hugepages in the test environment? |
|
Any updates on this? It's currently impossible to run this operator alongside mayastor without manually adding the resources. |
|
👍 |
1 similar comment
|
👍 |
This is a simple approach to these issues and just allows hugepages requests and limits to be specified in the CR for postgres container and eventual sidecars.
At the moment, it is not possible to use the operator on clusters with enabled hugepages support, because postgres detects and tries to use hugepages, but fails to allocate as the limit on containers defaults to zero. This PR allows specifying a little amount of hugepages to prevent postgres from crashing.
Closes #1788 and closes #1549