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

Minimal and explicit resource requests for image-puller pods #1764

Conversation

consideRatio
Copy link
Member

If a k8s cluster has a LimitRange resource, it may end up for example
allocating 100m CPU for containers which is a waste of resources. This
would be a waste as the running container only does sh -c "Done!"
anyhow. While there is image pulling happening, that is done before the
actual container starts so its worth noting we don't need any resources
specified for that.

Another reason for specifying resources, and making them configurable,
is that sometimes there is a ResourceQuota resource that requires the
resources to be explicitly specified on pods.

Related:

Closes #1761

… pods

If a k8s cluster has a LimitRange resource, it may end up for example
allocating 100m CPU for containers which is a waste of resources. This
would be a waste as the running container only does `sh -c "Done!"`
anyhow. While there is image pulling happening, that is done before the
actual container starts so its worth noting we don't need any resources
specified for that.

Another reason for specifying resources, and making them configurable,
is that sometimes there is a ResourceQuota resource that requires the
resources to be explicitly specified on pods.

Related:
- Default CPU requests - https://kubernetes.io/docs/tasks/administer-cluster/manage-resources/cpu-default-namespace/
- LimitRange https://kubernetes.io/docs/concepts/policy/limit-range/
- ResourceQuota - https://kubernetes.io/docs/concepts/policy/resource-quotas/
@consideRatio consideRatio force-pushed the resource-for-image-puller-containers branch from f90c5ea to 1ddafd3 Compare September 1, 2020 09:56
@consideRatio consideRatio changed the title Set configurable minimal resource requests for image-puller daemonset pods Minimal and explicit resource requests for image-puller pods Sep 1, 2020
Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the limitrange has bit me more than once.

Does setting the request to '0' work well? I remember seeing that it would count it as unset and set request == limit, but not sure.

Small suggestion to reduce the limit even more!

cpu: 0
memory: 0
limits:
cpu: 10m
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cpu: 10m
cpu: 1m

memory: 0
limits:
cpu: 10m
memory: 10Mi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
memory: 10Mi
memory: 1Mi

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About 0 requests

These were the resources reported back from kubectl get pod -o yaml after trying to set the resources like described. I suspect that KubeSpawner may have a logic checking for a truthy value of the cpu request and leaving it out if not set or similar could be an issue though.

    resources:
      limits:
        cpu: 10m
        memory: 10Mi
      requests:
        cpu: "0"
        memory: "0"

Relevant to this is that I've now learned that LimitRange resources can specify a maxLimitRequestRatio which would enforce the resource type influenced to not have any field set to zero. Do we want to avoid this by having a request of 1?

Reducing limits further

We can do this, but are you sure that the containers will stay below 1Mi of memory usage? My main concern is that I'll have concern about this when debugging some other issue and spend time thinking about if this narrow limit could have caused problems.

I decided to try using a extremely narrow limit of 1m / 10Mi and found that in my k8s cluster with Calico etc the pod with 1m / 1Mi limits got stuck in ContainerCreating while the 10m / 10Mi limited pod got started successfully.

image

The issues were the following on the 1m / 10 Mi pod.

Events:
  Type     Reason                  Age                   From                                                Message
  ----     ------                  ----                  ----                                                -------
  Normal   Scheduled               3m18s                 default-scheduler                                   Successfully assigned default/tmp-6f699cc94f-62sm6 to gke-nh-2020-core-standard-2-b80a0890-kfvt
  Warning  FailedCreatePodSandBox  104s (x3 over 2m47s)  kubelet, gke-nh-2020-core-standard-2-b80a0890-kfvt  Failed create pod sandbox: rpc error: code = Unknown desc = failed to start sandbox container for pod "tmp-6f699cc94f-62sm6": Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:319: getting the final child's pid from pipe caused \"read init-p: connection reset by peer\"": unknown
  Warning  FailedCreatePodSandBox  33s (x2 over 68s)     kubelet, gke-nh-2020-core-standard-2-b80a0890-kfvt  Failed create pod sandbox: rpc error: code = Unknown desc = failed to start sandbox container for pod "tmp-6f699cc94f-62sm6": Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:449: container init caused \"read init-p: connection reset by peer\"": unknown
  Normal   SandboxChanged          32s (x5 over 2m46s)   kubelet, gke-nh-2020-core-standard-2-b80a0890-kfvt  Pod sandbox changed, it will be killed and re-created.

I think the newly introduced Pod Overhead could mitigate this and allow us to have extremely low limits, but since that is a lot of extra infrastructure, I suggest we stick with the current 10 m / 10 Mi to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuvipanda what do you think, merge as it is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should actually remove the limits. On my cluster (AKS), setting this to 10Mi causes the pods to constantly crash as well. Why don't we just remove the limits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuvipanda ah yes deal. Are we unlimited by leaving them out?

@yuvipanda yuvipanda merged commit dd4fd8b into jupyterhub:master Sep 4, 2020
@yuvipanda
Copy link
Collaborator

<3 for checking through this, @consideRatio. I always appreciate how thorough you are.

yuvipanda added a commit to yuvipanda/zero-to-jupyterhub-k8s that referenced this pull request Sep 16, 2020
This memory limit seems to include pod overhead, which
can be variable based on cluster config. In my cluster (AKS)
with Calico enabled, the memory limit was too low and kept
crashing these pods with very weird reasons - the usual
reasons aren't reported since it's not *our* process that
is causing OOM kills, but the cluster's overhead.

By removing the default resource limits, we prevent thi
from being a problem. Theoretically this would let the pods
have unbounded memory - but since we control the contents,
it wouldn't be the case.

We leave the explicit default requests be, for reasons
outlined in
jupyterhub#1764.
The capacity for admins to specify their own limits is
still in place, in case you want that for your cluster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to have resource requests/limits set on all our containers
2 participants