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

Add support for using topologyKey=karpenter.sh/capacity-type in TopologySpreadConstraint #1582

Conversation

rajakshay
Copy link

@rajakshay rajakshay commented Mar 28, 2022

1. Issue, if available:
#1557

2. Description of changes:

  • Allows using karpenter.sh/capacity-type as topologyKey in TopologySpreadConstraint

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:

topologySpreadConstraints:
      - labelSelector:
          matchLabels:
            app: <app-name>
        maxSkew: 1
        topologyKey: karpenter.sh/capacity-type
        whenUnsatisfiable: DoNotSchedule
  • Karpenter provisioner specified:
  requirements:
  - key: karpenter.sh/capacity-type
    operator: In
    values:
    - spot
    - on-demand
  • Karpenter was successfully spinning up nodes across on-demand and spot as the workload instances was scaled up.

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.

@netlify
Copy link

netlify bot commented Mar 28, 2022

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 52b9ea9
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/6244c9e3e8b98700080d1421
😎 Deploy Preview https://deploy-preview-1582--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@tzneal tzneal left a 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",

@ellistarn
Copy link
Contributor

ellistarn commented Mar 28, 2022

I just noticed:

  1. Does this change impact docs?
    Yes, PR includes docs updates
    Yes, issue opened: link to issue
    [ x] No

This change does impact docs.

@rajakshay
Copy link
Author

Can you add some tests to scheduling/suite_test.go? There's a section there they'll fit in at: Describe("Topology",

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.

@rajakshay
Copy link
Author

I just noticed:

  1. Does this change impact docs?
    Yes, PR includes docs updates
    Yes, issue opened: link to issue
    [ x] No

This change does impact docs.

Thank you for pointing that out. I'll update the docs.

@rothgar
Copy link
Contributor

rothgar commented Mar 29, 2022

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.

@ellistarn
Copy link
Contributor

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

@rajakshay rajakshay force-pushed the support-capacity-type-as-topology-key-in-TopologySpreadConstraint branch from e35a09a to b5f8861 Compare March 30, 2022 09:37
@rajakshay rajakshay requested a review from a team as a code owner March 30, 2022 09:37
@tzneal
Copy link
Contributor

tzneal commented Mar 30, 2022

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

@rajakshay rajakshay force-pushed the support-capacity-type-as-topology-key-in-TopologySpreadConstraint branch from 3f8d39a to 52b9ea9 Compare March 30, 2022 21:21
@rajakshay
Copy link
Author

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

@tzneal Thank you for adding the docs. I updated an existing example as well, and squashed all commits together. This is ready for review.

@rajakshay rajakshay requested a review from tzneal March 30, 2022 21:26
Copy link
Contributor

@tzneal tzneal left a 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.

@tzneal tzneal merged commit 5158b6f into aws:main Mar 31, 2022
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