fix: remove cpu limit from resources in values.yaml and update schema accordingly

[macneib]

Description of changes:

Why remove cpu limits:

Setting CPU limits in Kubernetes can lead to unnecessary throttling due to how the Linux kernel enforces cgroup quotas. When a container hits its CPU limit, it is throttled even if the node has available CPU capacity. This can degrade application performance, especially for bursty or latency-sensitive workloads.

In our environment, we use Karpenter for dynamic provisioning. Karpenter works best when it can observe actual resource usage. Setting cpu requests without limits allows the scheduler and Karpenter to make better decisions about bin-packing and scaling, while avoiding performance degradation from artificial throttling.

We also enforce this practice with a Kyverno policy that disallows setting CPU limits, helping ensure consistent resource behavior cluster-wide.

Reference: Kubernetes docs on CPU limits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ack-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: macneib
Once this PR has been reviewed and has the lgtm label, please assign a-hilaly for approval by writing /assign @a-hilaly in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ack-prow]

Hi @macneib. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knottnt]

Instead of completely removing CPU limit from helm chart I think it would be better if it was made optional. Other users of the ACK controllers may still want to set this value.

[macneib]

Hey @knottnt

Thanks for the feedback! Just to clarify, this change removes the default CPU limit from values.yaml, but users can still explicitly set resources.limits.cpu in their own values.yaml or via --set when installing the chart.

We aim to avoid artificial CPU throttling in our environment, especially with Karpenter in use, and we enforce this pattern with a Kyverno policy. However, this change won't prevent other users from setting CPU limits if they want to.

That said, if we want to make it more explicit that CPU limits are optional, I’d be happy to add a note in values.yaml or the README to make this clear.

Would this work for you?

resources:
  requests:
    memory: "64Mi"
    cpu: "50m"
  limits:
    memory: "128Mi"
    # cpu: "100m"  # Optional: uncomment to set CPU limit

This way, it’s clear to all users that:

• The chart supports CPU limits.
• The chart does not intentionally set resources.limits.cpu by default.
• Users are free to opt-in.

[knottnt]

Hey @knottnt

Thanks for the feedback! Just to clarify, this change removes the default CPU limit from values.yaml, but users can still explicitly set resources.limits.cpu in their own values.yaml or via --set when installing the chart.

We aim to avoid artificial CPU throttling in our environment, especially with Karpenter in use, and we enforce this pattern with a Kyverno policy. However, this change won't prevent other users from setting CPU limits if they want to.

That said, if we want to make it more explicit that CPU limits are optional, I’d be happy to add a note in values.yaml or the README to make this clear.

Would this work for you?

resources:
  requests:
    memory: "64Mi"
    cpu: "50m"
  limits:
    memory: "128Mi"
    # cpu: "100m"  # Optional: uncomment to set CPU limit

This way, it’s clear to all users that:

• The chart supports CPU limits.
• The chart does not intentionally set resources.limits.cpu by default.
• Users are free to opt-in.

I have some reservations about changing the default install behavior from having a limit to having no limit at all. For users upgrading ACK controllers using the default CPU limit this would require them to add an explicit set just to keep the previous behavior.

For your use case would removing CPU limit from the set of required properties work? You could then explicitly set resources.limits.cpu=null.

[knottnt]

@macneib: Also any changes to the helm chart should be done in the code-generator repo's Helm templates. We use those files to keep our Helm charts consistent across all ACK controllers.

[StepanS-Enverus]

For your use case would removing CPU limit from the set of required properties work? You could then explicitly set resources.limits.cpu=null.

This fails on values.schema.json, cpu.limits is required

https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.schema.json

Error: values don't meet the specifications of the schema(s) in the following chart(s): ack-iam-controller: - resources.limits: cpu is required