StatefulSet does not apply resource limits

[tmirks]

Description

Elasticsearch cluster StatefulSets are created without resource limits set. Setting the limit values appear to be commented out in the code for whatever reason.

This is a problem if you've defined a LimitRange resource in your cluster to provide a default set of limits when otherwise unspecified.

Example

Our LimitRange:

...
  kind: LimitRange
  spec:
    limits:
    - default:
        cpu: "1"
        memory: 512Mi
...

The StatefulSet is created with the following resources (notice no limits):

...
          resources:
            requests:
              cpu: 500m
              memory: 1Gi
...

The result is an error on the StatefulSet because the request is higher than the LimitRange defaults:

Invalid value: "1Gi": must be less than or equal to memory limit

Type     Reason        Age                 From                    Message
----     ------        ----                ----                    -------
Warning  FailedCreate  16s (x15 over 55s)  statefulset-controller  create Pod xxxxx-0 in StatefulSet xxxxx failed error: Pod "xxxxx-0" is invalid: spec.containers[0].resources.requests: Invalid value: "1Gi": must be less than or equal to memory limit

[stevesloka]

We did have limits at one time, but ran into issues. There was a PR (#69) which addressed, but then some other issues came up. We should check upstream and see if any work has already been done that we could incorporate.

[stevesloka]

There was this PR as well with some good comments: #120

[tmirks]

@stevesloka what's the recommended course of action for resource limits then? Remove cluster defaults?

[stevesloka]

@tmirks the tricky part with limits (at least with memory) is when the pod exceeds those limits the pod is restarted. Initially, I had them set and then took them out since during some testing I found my data nodes restarting all the time. So I commented out that bit in the code.

I think the proper way might be to do some tuning of the limits in combination with some Java options to also talk to the JVM.

[nabadger]

Hi,

I do think this should be an option - our clusters have resource limits enforced so have to set them. We would like to be able to specify the exact limits/requests for cpu/ram (this seems the norm. for other many other operators I've used).

If pods are getting killed, it's more of a cluster-admins role to ensure it's resourced up sufficiently, configured and monitored right?

[stevesloka]

Clusters which enforce limits is a valid reason to add back. I'm not saying we shouldn't have them, just in my experience with using the operator, managing the JVM with limits can be tricky. I'll add a way to make them optional so you could still define and others could leave out.

[nabadger]

Awesome, many thanks 👍

[jimmyjones2]

Setting requests but not limit results in a Burstable QoS level. Given that Elasticsearch allocates pretty much all its memory on startup and it stays fixed, wouldn't it be better to set the limit and request to the same (ideally 2x heap), giving Guaranteed QoS so very unlikely to be killed (Guaranteed < Burstable < BestEfforts)?

The only situation I can see Burstable might be useful is setting the request to at least the Java heap size, allowing any unused memory on the node for the page cache - however I'm assuming when node memory pressure occurs the page cache would be reclaimed, bringing the container down to its request, rather than killing it.

[jimmyjones2]

Don't think my assumption about Burstable was correct - kubernetes/kubernetes#43916:

I'd like to share some observations, though I can't say I have a good solution to offer yet, other than to set a memory limit equal to the memory request for any pod that makes use of the file cache.

[kaarolch]

Anyway in most env people, teams have default request and limit set in case to limit all deployment without limit. In this type of env you have to recompile operator binary and create own container. Maybe a good solution would be to allow override default limits from CRD?

[alwinmarkcf]

Clusters which enforce limits is a valid reason to add back. I'm not saying we shouldn't have them, just in my experience with using the operator, managing the JVM with limits can be tricky. I'll add a way to make them optional so you could still define and others could leave out.

So is there something in there already? Do we need an additional value? I hit the same problem