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 minValues flexibility to the NodePool requirement. #5640

Merged
merged 27 commits into from
Feb 22, 2024

Conversation

nikmohan123
Copy link
Contributor

@nikmohan123 nikmohan123 commented Feb 9, 2024

Description
This PR adds minValues flexibility to the NodePool requirement. The new NodePool requirement will be represented as:

type NodeSelectorRequirementWithMinValues struct {
	v1.NodeSelectorRequirement `json:",inline"`
	// MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
	// +kubebuilder:validation:Minimum:=1
	// +kubebuilder:validation:Maximum:=100
	// +optional
	MinValues *int `json:"minValues,omitempty"`
}

minValues represents the minimum number of unique values that needs to be considered for a particular requirement. So, this value ensures that the scheduler creates NodeClaims meeting this new requirement.

This PR has a dependency on another PR (Karpenter-sigs) that I am going to send out shortly (Update: Here is the PR: https://github.com/nikmohan123/karpenter/pull/1/files). Most of the changes in this PR is about changing the requirement struct to a new one that has flexibility of minValues in it. The main changes are related to lifting the restrictions/truncation logic that we have for the list of InstanceTypes that we send today to CreateFleet and also bypassing the logic that filters the expensive instanceTypes. This is to ensure that the minValues flexibility of the requirement is not compromised.

More details in: kubernetes-sigs/karpenter#966

How was this change tested?
Functional tests added.
Manually tested as well.

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.

@nikmohan123 nikmohan123 requested a review from a team as a code owner February 9, 2024 01:56
@nikmohan123 nikmohan123 requested a review from engedaam February 9, 2024 01:56
Copy link

netlify bot commented Feb 9, 2024

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 5c062a4
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/65d687cb0709390008aac192
😎 Deploy Preview https://deploy-preview-5640--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.

pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/fake/utils.go Outdated Show resolved Hide resolved
pkg/providers/instance/instance.go Outdated Show resolved Hide resolved
test/suites/integration/validation_test.go Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
jonathan-innis
jonathan-innis previously approved these changes Feb 17, 2024
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Just some nits. LGTM 🚀

pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
jonathan-innis
jonathan-innis previously approved these changes Feb 21, 2024
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7997021303

Details

  • -2 of 66 (96.97%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 82.595%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/fake/utils.go 54 56 96.43%
Totals Coverage Status
Change from base Build 7996483757: 0.08%
Covered Lines: 5054
Relevant Lines: 6119

💛 - Coveralls

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

Snapshot successfully published to oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-5c062a4220db6714735b7364a39d0418853f7db0.
To install you must login to the ECR repo with an AWS account:

aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 021119463062.dkr.ecr.us-east-1.amazonaws.com

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis merged commit 2e3a0c5 into aws:main Feb 22, 2024
29 of 30 checks passed
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