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

feat: Add ability to select instance by performance mode #5779

Closed

Conversation

NetanelK
Copy link
Contributor

@NetanelK NetanelK commented Mar 4, 2024

Fixes #4367

Description
Added ability to select instance by its performance mode, either flex, burstable, or standard.
By using the label karpener.k8s.aws/instance-performance-mode

How was this change tested?
Ran unit tests

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

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

@NetanelK NetanelK requested a review from a team as a code owner March 4, 2024 15:54
@NetanelK NetanelK requested a review from jigisha620 March 4, 2024 15:54
Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 793fb4b
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/660086c1fcad270009659a3a
😎 Deploy Preview https://deploy-preview-5779--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 configuration.

@NetanelK NetanelK force-pushed the feature/performance-mode-selection branch 3 times, most recently from b3b4cad to 732742a Compare March 4, 2024 19:47
@tzneal tzneal requested a review from akestner March 5, 2024 00:33
@NetanelK NetanelK force-pushed the feature/performance-mode-selection branch from 732742a to bd4dbd2 Compare March 5, 2024 07:44
pkg/providers/instancetype/types.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/types.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/types.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/types.go Outdated Show resolved Hide resolved
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8152705293

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 82.682%

Totals Coverage Status
Change from base Build 8152324329: 0.05%
Covered Lines: 5271
Relevant Lines: 6375

💛 - Coveralls

@NetanelK NetanelK force-pushed the feature/performance-mode-selection branch from bd4dbd2 to eb96063 Compare March 11, 2024 07:11
@NetanelK NetanelK force-pushed the feature/performance-mode-selection branch from eb96063 to 793fb4b Compare March 24, 2024 20:02
@jonathan-innis jonathan-innis self-assigned this Apr 12, 2024
@@ -114,6 +118,7 @@ var (
LabelInstanceAcceleratorName = Group + "/instance-accelerator-name"
LabelInstanceAcceleratorManufacturer = Group + "/instance-accelerator-manufacturer"
LabelInstanceAcceleratorCount = Group + "/instance-accelerator-count"
LabelInstancePerformanceMode = Group + "/instance-performance-mode"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LabelInstancePerformanceMode = Group + "/instance-performance-mode"
LabelInstancePerformanceMode = Group + "/instance-capability-flex"

Suggesting this label name in the original issue because of the way that EC2 describes their own naming convention here: https://docs.aws.amazon.com/ec2/latest/instancetypes/instance-type-names.html. Given that page as well, I'm not sure that it makes sense to couple the burstable instance types with the flex ones. Can we treat flex as its own thing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I would prefer a value that can be fetched from the EC2 API, instead of relaying on the instance name structure.
Anyhow, it can be done fairly easy.

Copy link
Contributor Author

@NetanelK NetanelK May 19, 2024

Choose a reason for hiding this comment

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

@jonathan-innis

I tried to add a label per each instance capability (total 7) and encountered the following error:

"NodeClaim.karpenter.sh \"spiritslow-94-ywrmgisn0j-p88dm\" is invalid: [spec.requirements: Too many: 31: must have at most 30 items, <nil>: Invalid value: \"null\": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]"

The cause is this line from kubernetes-sigs/karpenter, can we increase the limit of requirements?

New code for reference main...NetanelK:karpenter-provider-aws:feature/instance-additional-capablities

Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

@NetanelK
Copy link
Contributor Author

Closing in favor of #6294

@NetanelK NetanelK closed this May 30, 2024
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.

How does Karpenter deal with the new m7i-flex instance type?
3 participants