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

Fix missing requests when only limits are supplied #1616

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

dewjam
Copy link
Contributor

@dewjam dewjam commented Apr 1, 2022

1. Issue, if available:
#1573

2. Description of changes: (Edit):
There are two primary changes included in this PR.

First, we will now merge Pod resource Limits into Requests when Requests are not defined. Normally when a pod is created, if only Limits are supplied for a specific resource (no Requests), then the PodSpec is updated so the Requests = Limits when the pod event is created. However, when Karpenter calculates node overhead, it looks at the limits/requests in the PodSpec template of applicable DaemonSets. If only Limits are supplied in the PodSpec template, then requests are never updated to equal limits for resources where requests are not defined. The result is pods can be packed and scheduled onto an instance type which doesn't have enough resources and pods stay in a "Pending" state.

For example, if a manifest contains the below:

resources:
  limits:
    cpu: 1

The pod will be created as such:

resources:
  limits:
    cpu: 1
  requests:
    cpu: 1

Second, we will now consider InitContainer resources when scheduling/packing pods onto nodes. In short, here is the formula to calculate the resource "Ceiling" for a pod:

  1. Get sum of all app container resources for the pod
  2. Get max of all init container resources for the pod (we get a Max because InitContainers are started sequentially)
  3. Get max of sum(appContainers) and max(initContainers)

3. How was this change tested?
Added a tests to validate the behavior after the change. Tested manually with a DaemonSet with only limits to ensure it was scheduled on a node which has enough resources. Tested manually to ensure init container resources are considered when scheduling.

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

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

@dewjam dewjam requested a review from a team as a code owner April 1, 2022 14:39
@netlify
Copy link

netlify bot commented Apr 1, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 3f00257
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/624ddd5213265700086d7082

@dewjam dewjam requested review from tzneal and ellistarn April 5, 2022 17:05
@dewjam dewjam requested review from bwagner5 and removed request for ellistarn April 6, 2022 18:39
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm

pod := ExpectProvisioned(ctx, env.Client, selectionController, provisioningController, provisioner, test.UnschedulablePod(test.PodOptions{}))[0]
ExpectNotScheduled(ctx, env.Client, pod)
})
It("should not schedule if initContainer resources are too large", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this test named wrong or is it supposed to be InitResourceRequirements rather than ResourceRequirements?

@bwagner5 bwagner5 merged commit 3f7a319 into aws:main Apr 7, 2022
@suket22 suket22 mentioned this pull request May 23, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants