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

Adds provisioner limits based on cpu and mem #817

Merged
merged 27 commits into from
Nov 24, 2021
Merged

Conversation

suket22
Copy link
Contributor

@suket22 suket22 commented Nov 19, 2021

1. Issue, if available:
N/A

2. Description of changes:
Adds limits to the Karpenter provisioner based on CPU and Memory. With the current implementation these are soft limits and aren't strictly enforced.

The way the limiting works is that we keep launching worker nodes until we have any amount of capacity yet. Only once we've actually hit / crossed our limits do we stop the provisioning. It's a pretty naive implementation, and can be problematic sometimes.

Scenario 1 -
You've got 2 CPU left, and a 10 new pods come in each asking for 1 CPU. In an ideal world, we should only provision a tiny instance with 2CPU that can host 2 pods and the other 8 remain stuck in pending. However, what this implementation will do is provision whatever instance type Karpenter would do anyway based on our bin packing logic and fit all 10 pods. The next batch of pending pods however, will be entirely stuck since we'd detect we've crossed our limits.

Scenario 2 -
If you set the CPU or Memory to 0 in the provisioner, Karpenter won't be able to launch any worker nodes at all. This is by design and can help when you're looking to block instance creation entirely. Will be more useful once I add GPUs in.

What is to come next?

  • Adding GPU support. That should come quick - I just need to play around with it a little more.
  • Adding documentation on how this limiting works.
  • Adding metrics for the resource consumption in the cluster per provisioner.

Testing done so far

  • It's backwards compatible so if you haven't used limits before, your provisioning will continue to remain unlimited.
  • Specifying just cpu or just memory and a combination etc.
  • Done small scale ups to verify that we allow provisioning until we detect that we've hit/crossed our limits and then it blocks all provisioning.

3. 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.

@netlify
Copy link

netlify bot commented Nov 19, 2021

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 2591f0a

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/619ebf3aefc2cb0008c10508

@suket22 suket22 changed the title [WIP] Adds provisioner limits based on cpu and mem Adds provisioner limits based on cpu and mem Nov 23, 2021
cmd/controller/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

Nice! I really like this approach. Some quick comments.

pkg/apis/provisioning/v1alpha5/limits.go Outdated Show resolved Hide resolved
pkg/apis/provisioning/v1alpha5/limits.go Outdated Show resolved Hide resolved
pkg/apis/provisioning/v1alpha5/limits.go Outdated Show resolved Hide resolved
@@ -29,6 +30,9 @@ type ProvisionerStatus struct {
// its target, and indicates whether or not those conditions are met.
// +optional
Conditions apis.Conditions `json:"conditions,omitempty"`

// Resources is the list of resources that have been provisioned.
Resources v1.ResourceList `json:"resources,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we had a mechanism to express the amount of resources reserved by pods as well as the total amount of capacity in the current nodes. Not that you need to implement them both, but we should make sure it makes sense on the API.

Provisioner.Status.Capacity
Provisioner.Status.Reserved

Maybe we want to be cautious of the Reserved bit, since it will mean updating on every pod event.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice call. I think there's definite value in having that in ProvisionerSpec. Might even help show how much better the instance utilization is with Karpenter. I think in the API though, it should could maybe be Provisioner.Status.Requests?

Agreed though, that it might mean we update the provisioner on each pod event so I'm a little concerned from a scaling front.

Copy link
Contributor

Choose a reason for hiding this comment

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

Controller-runtime will batch the reconciles and has a serial execution guarantee. I think this will roughly translate into a linear stream of writes to the API Server as it detects new resources. AFAICT, it shouldn't be enough to overwhelm anything.

pkg/controllers/counter/controller.go Outdated Show resolved Hide resolved
pkg/controllers/counter/controller.go Show resolved Hide resolved
pkg/controllers/provisioning/controller.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/controller.go Outdated Show resolved Hide resolved
pkg/apis/provisioning/v1alpha5/limits.go Outdated Show resolved Hide resolved
Comment on lines 53 to 56
provisioner, err := l.updateState(ctx, provisioner)
if err != nil {
return fmt.Errorf("unable to determine status of provisioner")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why I didn't need this in my earlier revisions. Maybe I was doing something else that was forcing the provisioner to get updated or I wasn't testing my scale ups fast enough, but without this the state in this provisioner object is really off. Since it's hitting a cache, I don't mind moving this into the for loop below either so we reduce the chance of staleness

Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

Nice! We just need a unit test now.

pkg/apis/provisioning/v1alpha5/provisioner_validation.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/launcher.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/suite_test.go Show resolved Hide resolved
ellistarn
ellistarn previously approved these changes Nov 24, 2021
@suket22 suket22 closed this Nov 24, 2021
@suket22 suket22 reopened this Nov 24, 2021
@suket22 suket22 merged commit 4bef8e9 into aws:main Nov 24, 2021
@suket22 suket22 deleted the limitsImpl branch January 5, 2022 20:38
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.

3 participants