-
Notifications
You must be signed in to change notification settings - Fork 995
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
Add support for using topologyKey=karpenter.sh/capacity-type in TopologySpreadConstraint #1582
Add support for using topologyKey=karpenter.sh/capacity-type in TopologySpreadConstraint #1582
Conversation
✅ Deploy Preview for karpenter-docs-prod ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests to scheduling/suite_test.go
? There's a section there they'll fit in at: Describe("Topology",
I just noticed:
This change does impact docs. |
Thank you for the quick response and pointer for adding tests. Im working on adding some tests. I'll push an update in a few hours. |
Thank you for pointing that out. I'll update the docs. |
Will this change allow me to say I want an even spread between spot and on-demand? Am I able to specify something like 20% spot and 80% on demand with this? It doesn't look like it but I'm not sure the intended use case. |
I'd love to extend upstream topology to do percentage based domains. Right now, the spec implies an even split across domains (e.g. 50/50 here). |
e35a09a
to
b5f8861
Compare
@rajakshay Sorry, I was trying to add some docs as a pull-request to your PR but it ended up just adding them on as a new commit on this PR. Feel free to rebase and squash that into your commit if you would like. |
…ogySpreadConstraints
3f8d39a
to
52b9ea9
Compare
@tzneal Thank you for adding the docs. I updated an existing example as well, and squashed all commits together. This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I pulled this branch and verified that capacity-type spread is working on my cluster.
1. Issue, if available:
#1557
2. Description of changes:
3. How was this change tested?
Tested on EKS using the instructions here: https://karpenter.sh/v0.7.3/development-guide/
Karpenter was able to spin up the correct
capacity-type
based on the PodSpec to respect the distribution. Tested for distribution of pods across on-demand and spot EC2 instances.podSpec used for testing topologySpreadConstraint:
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.