cdk8s-team/cdk8s-plus

Provide mechanism to allow containers to run without CPU limits

otterley opened this issue · 8 comments

otterley commented

Description of the feature or enhancement:

Provide a mechanism to allow containers to run without CPU or memory limits. Currently a default limit is applied to all containers in a Pod as a result of changes made in #1082 . You can specify higher limits, but you can't remove the limits altogether. I also recommend making "unlimited" the default for CPU.

Use Case:

Running containers with limits can, in certain causes, cause a nontrivial amount of time to be spent in the kernel maintaining that limit, due to bugs in older Linux kernels [1][2]. This typically happens with very large containers, e.g., 32 vCPU or higher, leading to decreased overall workload performance. The bugs have since been patched in newer kernel versions (in both 4.x and 5.x), but many customers may be unaware that they are running an impacted kernel.

Moreover, forcing a container to run with limits means it cannot take advantage of free CPU cycles to "burst" beyond its usual resource requirements. This leads to underutilized nodes in many cases, leading to economic waste and higher operational costs than otherwise might be required for a given workload.

A number of certain well-intended but misinformed "best practice" material and evaluation software state that CPU limits are the correct and proper way to prevent a container from becoming a "noisy neighbor" and impacting the performance of other containers running on a node [3][4]. However, this is is incorrect. Resource requests provide the same feature, but in reverse: If a "bursty" container is using more than its requested CPU, the Linux kernel will automatically throttle it back to its requested CPU allocation if another container needs access to the CPU resource (up to the latter's own requested value). That's why requests are also known as guarantees: they ensure the container can always get access to the resource, even if it's currently being used by something else. They set a floor, not a ceiling. And since every container is guaranteed the amount of CPU that it requests, limits aren't necessary to make that effective.

See also https://home.robusta.dev/blog/stop-using-cpu-limits for more background.

Proposed Solution:

Add a Cpu.UNLIMITED constant to cdk8s-plus that removes the CPU limit. Remove the default CPU limit so that container performance is not unnecessarily throttled.

Other:

[1] https://static.sched.com/hosted_files/kccncna19/dd/Kubecon_%20Throttling.pdf
[2] kubernetes/kubernetes#67577
[3] https://github.com/zegl/kube-score/blob/master/README_CHECKS.md
[4] https://polaris.docs.fairwinds.com/checks/efficiency/

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

iliapolo commented

@otterley Thanks for this. I definitely agree we should make it easier to remove CPU limits by providing a Cpu.UNLIMITED enum value. Regarding setting it as the default, your arguments make sense and we will dive deeper here and consider this.

In the meantime, you can remove the limit by applying the JsonPatch.remove escape hatch on your pods.

https://cdk8s.io/docs/latest/basics/escape-hatches/#patching-api-objects-behind-higher-level-apis

maybedino commented

We have been using containers with no CPU Limit like this since the default was added, without having to use JsonPatch:

new Deployment(this, "default", {
    containers: [ {
        resources: {
            cpu: {
                request: Cpu.units(1),
            },
            memory: {
                request: Size.gibibytes(1),
                limit: Size.gibibytes(1),
            },
        },
    }],
});

But having no CPU Limit as the default would be great! On a side note, maybe also consider setting memory limit = memory request by default, to avoid pods getting OOM killed when a node gets full (see https://home.robusta.dev/blog/kubernetes-memory-limit).

otterley commented

@maybedino Are you sure there's no limit? Can you please check your Pod specs in your cluster to ensure that a default limit is not being applied for you?

otterley commented

consider setting memory limit = memory request by default, to avoid pods getting OOM killed when a node gets full

This is a good idea.

maybedino commented

@maybedino Are you sure there's no limit? Can you please check your Pod specs in your cluster to ensure that a default limit is not being applied for you?

Sure, I checked. There are no CPU Limits, not in the generated YAML and not in the deployed pod. Using cdk8s-plus-25 2.4.23.

This is what I got:

> kubectl describe pod podname
(...)
Containers:
  (...)
    Limits:
      memory:  2Gi
    Requests:
      cpu:      1
      memory:   2Gi
otterley commented

Upon further review, I do not think a Cpu.UNLIMITED enum value is needed, since specifying only the CPU resource request has the effect of also removing any default CPU limit unless a limit is specified. So I would amend the request to remove the default CPU limit only.

mizuochik commented

Hi, any progress on this issue?

I am new to CDK8s but have several years of experience operating K8s.
I was a little confused by the cpu/mem default settings by cdk8s-plus. Although I usually don't specify resource limits to prevent noisy neighbors, cdk8s sets limits.
IMO, it doesn't seem intuitive and natural that the default values do not match the pure K8s default values (nulls).

I agree with removing the default limits.

iliapolo commented

This is tricky because it is a breaking change. Perhaps we can have an App level setting to disable this, sort of a feature flag.