feat: Add a new field called minValues that introduces flexibility to the NodePool requirement. #1

nikmohan123 · 2024-02-09T18:21:48Z

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 corresponding changes in the karpenter aws-cloud-provider and is sent as a separate PR: aws/karpenter-provider-aws#5640

The main changes in the PR is related to changing the requirement struct to a new one that has flexibility of minValues in it and adding the new minValues field to the API and wire it to scheduler/consolidation layer. This PR tracks the already sent out PR: kubernetes-sigs#963 that has the core scheduler/consolidation layer changes.

How was this change tested?
Added functional test.
Manually tested by passing in the minValues through different requirements and checked the CreateFleet inputs to see if all the requirements were met for the NodeClaim.

TODO: Add more functional tests around consolidation and probably around more requirement keys.

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

…incorporate the API changes.

…uirement.

jonathan-innis · 2024-02-15T23:44:18Z

pkg/apis/v1beta1/nodeclaim.go

@@ -38,7 +38,7 @@ type NodeClaimSpec struct {
 	// +kubebuilder:validation:XValidation:message="requirements operator 'Gt' or 'Lt' must have a single positive integer value",rule="self.all(x, (x.operator == 'Gt' || x.operator == 'Lt') ? (x.values.size() == 1 && int(x.values[0]) >= 0) : true)"
 	// +kubebuilder:validation:MaxItems:=30
 	// +required
-	Requirements []v1.NodeSelectorRequirement `json:"requirements" hash:"ignore"`
+	Requirements []NodeSelectorRequirementWithFlexibility `json:"requirements" hash:"ignore"`


Suggested change

Requirements []NodeSelectorRequirementWithFlexibility `json:"requirements" hash:"ignore"`

Requirements []NodeSelectorRequirement`json:"requirements" hash:"ignore"`

Any reason to make this more verbose? Or can we just call this the Karpenter version of the NodeSelectorRequirement

Discussed offline that we would go with NodeSelectorRequirementWithMinValues since the K8 NodeSelectorRequirement is also used in the code which might cause confusion. Changed to this.

jonathan-innis · 2024-02-15T23:45:02Z

pkg/apis/v1beta1/nodeclaim.go

+// which represents the minimum number of unique values that needs to be considered for a particular requirement.
+type NodeSelectorRequirementWithFlexibility struct {
+	v1.NodeSelectorRequirement `json:",inline"`
+	// This is an alpha field.


Suggested change

// This is an alpha field.

// This field is ALPHA and can be dropped or replaced at any time

jonathan-innis · 2024-02-15T23:45:48Z

pkg/apis/v1beta1/nodeclaim.go

@@ -54,6 +54,18 @@ type NodeClaimSpec struct {
 	NodeClassRef *NodeClassReference `json:"nodeClassRef"`
 }

+// NodeSelectorRequirementWithFlexibility introduces flexibility to the NodeSelectorRequirement through minValues
+// which represents the minimum number of unique values that needs to be considered for a particular requirement.
+type NodeSelectorRequirementWithFlexibility struct {


We need a CEL expression here that does the same check that you have in the validation code around if we have less values listed in the requirement compared to the minValues that have been specified. If you need some help getting that CEL written, I can definitely help out with that

Added. Thanks.

jonathan-innis · 2024-02-15T23:50:13Z

pkg/apis/v1beta1/nodeclaim_validation_cel_test.go

+			}
+			Expect(env.Client.Create(ctx, nodeClaim)).ToNot(Succeed())
+		})
+		It("should error when minValues is more than 50", func() {


Should we have a check here that validates that we don't have fewer values in a NodeSelectorRequirement with the "In" operator than the minValues that are specified?

Yeah, seems like I added this in webhook test and missed it here. Thanks. Added it here as well.

jonathan-innis · 2024-02-16T00:00:17Z

pkg/apis/v1beta1/nodepool_validation_cel_test.go

+			}
+			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
+		})
+		It("should error when minValues is more than 50", func() {


Same question here: Probably a validation on the number of values relative to the minValues specified is warranted

Yeah, seems like I added this in webhook test and missed it here. Thanks. Added it here as well.

jonathan-innis · 2024-02-16T00:08:07Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+				Available:    true,
+			},
+		}
+		instanceTypes = append(instanceTypes, fake.NewInstanceType(opts1))


Is it worth creating 4 instance types here to ensure that Karpenter is actually fulfilling both of the requirements. This test could theoretically pass on the instance type requirement alone.

Since minValues for InstanceTypes is 1 and we have an instance that definitely fits both the pods and is a cheaper option, it will always have 1 instance in the nodeclaim if not for the arch label which demands at least 2 in this case and hence the nodeclaim will have 2 instances because each one has different architecture.

Got it. Does it even make sense to have a minValues of 1 on the instance type? I don't really think that is really doing anything honestly since we already require that at least one value exist for us to be able to launch and bind.

I'm fine if we capture this in some of the other testing improvements and take care of this in a follow-on if that's eaisier.

jonathan-innis · 2024-02-16T00:09:07Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

@@ -603,4 +643,357 @@ var _ = Describe("Instance Type Selection", func() {
 		node := ExpectScheduled(ctx, env.Client, pod)
 		Expect(node.Labels[v1.LabelInstanceTypeStable]).To(Equal("test-instance1"))
 	})
+	It("should schedule respecting the minValues from instance-type requirements", func() {


Do we need to validate minValues with other types of operators here? We are testing In a bunch (which is good) but should we also test NotIn, Gt, Lt , Exists and NotExists?

Have added combinations and individual tests on NotIn, Gt, Lt and Exists operators. I think NotExists anyways wouldn't schedule the pods and we have tests for that. minValues is going to be a no-op there and we have tests in requirements_test file to get the intersections.

jonathan-innis · 2024-02-16T00:11:07Z

pkg/controllers/provisioning/scheduling/topology_test.go

+								NodeSelectorRequirement: v1.NodeSelectorRequirement{
+									Key:      v1beta1.CapacityTypeLabelKey,
+									Operator: v1.NodeSelectorOpExists,
+								},
 							},


Do we need a test with topology here to validate that topology won't work from an application with minValues. It's definitely a weird thing to ensure, but it validates our understanding of how the whole system fits together. Something like "should not succeed to meet minValues when using topologySpread on the same key"

jonathan-innis · 2024-02-16T00:15:23Z

pkg/scheduling/requirements.go

@@ -88,14 +100,14 @@ func newPodRequirements(pod *v1.Pod, typ podRequirementType) Requirements {
 		// Select heaviest preference and treat as a requirement. An outer loop will iteratively unconstrain them if unsatisfiable.
 		if preferred := pod.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution; len(preferred) > 0 {
 			sort.Slice(preferred, func(i int, j int) bool { return preferred[i].Weight > preferred[j].Weight })
-			requirements.Add(NewNodeSelectorRequirements(preferred[0].Preference.MatchExpressions...).Values()...)
+			requirements.Add(NewNodeSelectorRequirements(ConvertNodeSelectorRequirementToNodeSelectorRequirementWithFlexibility(preferred[0].Preference.MatchExpressions...)...).Values()...)


Rather than converting everything here, is it better to just create a different Add() function that can handle nodeSelectorRequirements without flexibility? I think that's going to result in less code churn and less memory utilization overall

Yeah, good idea. Thanks. Changed it.

jonathan-innis · 2024-02-16T00:16:08Z

pkg/scheduling/requirements_test.go

@@ -25,6 +25,8 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/samber/lo"
 	v1 "k8s.io/api/core/v1"
+


Should we have a test in here that validates that when we add multiple requirements that have the same key and are using minValues that we take the max of the two minValues?

I have it added here already. Are we talking about the same thing ?

Ah, got it. I think I got confused because we have both a requirement_test.go file and a requirements_test.go file

jonathan-innis · 2024-02-16T21:07:54Z

pkg/apis/v1beta1/nodeclaim.go

@@ -54,6 +55,18 @@ type NodeClaimSpec struct {
 	NodeClassRef *NodeClassReference `json:"nodeClassRef"`
 }

+// NodeSelectorRequirementWithMinValues introduces flexibility to the NodeSelectorRequirement through minValues


I'm wondering if we should try to match our description for this NodeSelectorRequirement to be close to the upstream one but with an addendum that says that you can specify minValues on it?

This is the upstream description

A node selector requirement is a selector that contains values, a key, and an operator that relates the key and values.

The only reason that I'm calling this out is because this gets put into the CRD description that gets auto-generated by the change

Got it. Changed it to closely mimic that.

// A node selector requirement with min values is a selector that contains values, a key, an operator that relates the key and values // and minValues that represent the requirement to have at least that many values.

Let me know if this sounds good.

jonathan-innis · 2024-02-16T21:08:57Z

pkg/apis/v1beta1/nodeclaim.go

@@ -36,9 +36,10 @@ type NodeClaimSpec struct {
 	// Requirements are layered with GetLabels and applied to every node.
 	// +kubebuilder:validation:XValidation:message="requirements with operator 'In' must have a value defined",rule="self.all(x, x.operator == 'In' ? x.values.size() != 0 : true)"
 	// +kubebuilder:validation:XValidation:message="requirements operator 'Gt' or 'Lt' must have a single positive integer value",rule="self.all(x, (x.operator == 'Gt' || x.operator == 'Lt') ? (x.values.size() == 1 && int(x.values[0]) >= 0) : true)"
+	// +kubebuilder:validation:XValidation:message="requirements with 'minValues' must have minimum values defined",rule="self.all(x, (x.operator == 'In' && has(x.minValues)) ? x.values.size() >= x.minValues : true)"


Suggested change

// +kubebuilder:validation:XValidation:message="requirements with 'minValues' must have minimum values defined",rule="self.all(x, (x.operator == 'In' && has(x.minValues)) ? x.values.size() >= x.minValues : true)"

// +kubebuilder:validation:XValidation:message="requirements with 'minValues' must have at least that many values specified in the "values" field",rule="self.all(x, (x.operator == 'In' && has(x.minValues)) ? x.values.size() >= x.minValues : true)"

jonathan-innis · 2024-02-16T21:09:31Z

pkg/apis/v1beta1/nodeclaim_validation.go

@@ -163,6 +163,11 @@ func ValidateRequirement(requirement v1.NodeSelectorRequirement) error { //nolin
 	if requirement.Operator == v1.NodeSelectorOpIn && len(requirement.Values) == 0 {
 		errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have a value defined", requirement.Key, requirement.Operator))
 	}
+
+	if requirement.Operator == v1.NodeSelectorOpIn && requirement.MinValues != nil && len(requirement.Values) < lo.FromPtr(requirement.MinValues) {
+		errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have at least minimum number of values defined", requirement.Key, requirement.Operator))


Suggested change

errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have at least minimum number of values defined", requirement.Key, requirement.Operator))

errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have at least minimum number of values defined in 'values' field", requirement.Key, requirement.Operator))

jonathan-innis · 2024-02-16T21:10:17Z

pkg/apis/v1beta1/nodeclaim_validation_cel_test.go

 				Expect(env.Client.Create(ctx, nodeClaim)).ToNot(Succeed())
 			}
 		})
+		It("should error when minValues is negative", func() {


It seems like we shouldn't just test that it's negative. Seems like 0 is also invalid here as well

Good point. Added.

jonathan-innis · 2024-02-16T21:11:41Z

pkg/apis/v1beta1/nodeclaim_validation_cel_test.go

+		})
+		It("should error when minValues is more than 50", func() {
+			nodeClaim.Spec.Requirements = []NodeSelectorRequirementWithMinValues{
+				{NodeSelectorRequirement: v1.NodeSelectorRequirement{Key: v1.LabelInstanceTypeStable, Operator: v1.NodeSelectorOpIn, Values: []string{"c4.large"}}, MinValues: lo.ToPtr(51)},


Which part of CEL does this fail on? The values check or the In requirement has enough values check? Seems like you should use an Exists here to ensure that we are only failing on the exact thing that we are testing

I see the ambiguity now. I was under the impression that this wouldn't even get to the point of trying to check how many values in the requirement to meet the minValues given that it should fail at the validation of minValues itself. I will change the operator.

jonathan-innis · 2024-02-16T21:18:15Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

@@ -603,4 +644,930 @@ var _ = Describe("Instance Type Selection", func() {
 		node := ExpectScheduled(ctx, env.Client, pod)
 		Expect(node.Labels[v1.LabelInstanceTypeStable]).To(Equal("test-instance1"))
 	})
+	It("should schedule respecting the minValues from instance-type requirements", func() {


Since these are all MinValues tests, do they deserve their own Context block in this suite_test.go file?

Yes, definitely. Changed it. Thanks.

jonathan-innis · 2024-02-16T21:21:21Z

pkg/controllers/provisioning/scheduling/topology_test.go

+			ExpectApplied(ctx, env.Client, nodePool)
+			pods := test.UnschedulablePods(test.PodOptions{ObjectMeta: metav1.ObjectMeta{Labels: labels}, TopologySpreadConstraints: topology}, 4)
+			ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pods...)
+			ExpectNotScheduled(ctx, env.Client, pods[0])


Can we explain why we expect the topology spread to fail here? It'll make it a little easier to parse out in the future. Essentially, we know that we have to select a value when we are using topologySpread, which directly constrains us down to 1 value, so asking for more than that 1 value on that key isn't possible for Karpenter to reason about today

Added this stmt. Thanks.

jonathan-innis · 2024-02-16T21:24:12Z

pkg/scheduling/requirements_test.go

@@ -25,6 +25,8 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/samber/lo"
 	v1 "k8s.io/api/core/v1"
+


Ah, got it. I think I got confused because we have both a requirement_test.go file and a requirements_test.go file

jonathan-innis · 2024-02-16T21:27:35Z

pkg/scheduling/requirement_test.go

@@ -25,9 +25,12 @@ import (
 	"github.com/samber/lo"


Something to think about (maybe for a different PR): Is there a way to categorize these tests better so they are more organized for someone who is trying to make changes and is coming at this file fresh?

I agree with you. This test file escalated quickly with a lot more changes than I expected with the combinations I had in my mind for intersections. I can take this as a follow-up item to see how I can get this to a better shape.

Sounds good. Worth tracking as a GH issue so that there's visibility generally? Could just call it "improve requirement testing organization" and mark it as a chore

Created one : kubernetes-sigs#1024

jonathan-innis · 2024-02-16T21:29:16Z

pkg/controllers/disruption/consolidation_test.go

@@ -1483,6 +1483,354 @@ var _ = Describe("Consolidation", func() {
 			// and delete the old one
 			ExpectNotFound(ctx, env.Client, spotNodeClaim, spotNode)
 		})
+		It("spot to spot consolidation should consider the max of default and minimum number of instanceTypeOptions from minValues in requirement for truncation.", func() {


Suggested change

It("spot to spot consolidation should consider the max of default and minimum number of instanceTypeOptions from minValues in requirement for truncation.", func() {

It("spot to spot consolidation should consider the max of default and minimum number of instanceTypeOptions from minValues in requirement for truncation if minimum number of instanceTypeOptions from minValues in requirement is greater than 15.", func() {

nit: This part would just help provide a little more clarity so that I can understand exactly what is being tested here and it also provides parity with the next test case

jonathan-innis · 2024-02-17T01:12:26Z

pkg/apis/v1beta1/nodepool_validation_cel_test.go

+		})
+		It("should error when minValues is more than 50", func() {
+			nodePool.Spec.Template.Spec.Requirements = []NodeSelectorRequirementWithMinValues{
+				{NodeSelectorRequirement: v1.NodeSelectorRequirement{Key: v1.LabelInstanceTypeStable, Operator: v1.NodeSelectorOpExists, Values: []string{"insance-type-1"}}, MinValues: lo.ToPtr(51)},


You can't specify values with an Exists requirement, or at least you shouldn't. Really, we shouldn't be allowing it, what's correct is to have no values with the Exists operator

Corrected it.

jonathan-innis · 2024-02-17T01:13:02Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

@@ -603,4 +644,932 @@ var _ = Describe("Instance Type Selection", func() {
 		node := ExpectScheduled(ctx, env.Client, pod)
 		Expect(node.Labels[v1.LabelInstanceTypeStable]).To(Equal("test-instance1"))
 	})
+	Context("Scheduler behavior for requirements with minValues", func() {


Suggested change

Context("Scheduler behavior for requirements with minValues", func() {

Context("MinValues", func() {

nit: I'd personally just shorten it

jonathan-innis

LGTM 🚀 Approved with some nits

jonathan-innis · 2024-02-17T08:01:26Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+		})
+		It("should schedule respecting the minValues in Gt operator.", func() {
+			// custom key that will help us with numerical values to be used for Gt operator
+			instanceGeneration := "karpenter/numerical-value"


Suggested change

instanceGeneration := "karpenter/numerical-value"

numericalRequirementKey := v1beta1.Group + "/numerical-value"

nit: Typically you prefix with the group name

It doesn't allow you do attach the label with karpenter.sh as it complains that it is a restricted label like I said here.

Error:

got an error: <*errors.StatusError | 0x1400069a1e0>: NodePool.karpenter.sh "speakerleather-1-znthvad9w8" is invalid: spec.template.spec.requirements[0].key: Invalid value: "string": label domain "karpenter.sh" is restricted {

So, not fixing this and for the rest.

jonathan-innis · 2024-02-17T08:01:57Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+		})
+		It("scheduler should fail if the minValues in Gt operator is not satisfied.", func() {
+			// custom key that will help us with numerical values to be used for Gt operator
+			instanceGeneration := "karpenter/numerical-value"


Suggested change

instanceGeneration := "karpenter/numerical-value"

numericalRequirementKey := v1beta1.Group + "/numerical-value"

Same comment here

jonathan-innis · 2024-02-17T08:02:12Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+		})
+		It("should schedule respecting the minValues in Lt operator.", func() {
+			// custom key that will help us with numerical values to be used for Lt operator
+			instanceGeneration := "karpenter/numerical-value"


Suggested change

instanceGeneration := "karpenter/numerical-value"

numericalRequirementKey := v1beta1.Group + "/numerical-value"

jonathan-innis · 2024-02-17T08:02:19Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+		})
+		It("scheduler should fail if the minValues in Lt operator is not satisfied.", func() {
+			// custom key that will help us with numerical values to be used for Lt operator
+			instanceGeneration := "karpenter/numerical-value"


Suggested change

instanceGeneration := "karpenter/numerical-value"

numericalRequirementKey := v1beta1.Group + "/numerical-value"

jonathan-innis · 2024-02-17T08:02:27Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+		})
+		It("should schedule considering the max of the minValues of Gt and Lt operators.", func() {
+			// custom key that will help us with numerical values to be used for Gt operator
+			instanceGeneration := "karpenter/numerical-value"


Suggested change

instanceGeneration := "karpenter/numerical-value"

numericalRequirementKey := v1beta1.Group + "/numerical-value"

jonathan-innis · 2024-02-17T08:04:01Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+			// Ensures that NodeClaims are created with 2 instanceTypes
+			Expect(len(supportedInstanceTypes(cloudProvider.CreateCalls[0]))).To(BeNumerically(">=", 2))
+		})
+		It("should schedule respecting the minValues in Gt operator.", func() {


Suggested change

It("should schedule respecting the minValues in Gt operator.", func() {

It("should schedule respecting the minValues in Gt operator", func() {

nit

jonathan-innis · 2024-02-17T08:04:24Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+			// Ensures that NodeClaims are created with 2 instanceTypes
+			Expect(len(supportedInstanceTypes(cloudProvider.CreateCalls[0]))).To(BeNumerically(">=", 2))
+		})
+		It("scheduler should fail if the minValues in Gt operator is not satisfied.", func() {


Suggested change

It("scheduler should fail if the minValues in Gt operator is not satisfied.", func() {

It("scheduler should fail if the minValues in Gt operator is not satisfied", func() {

nit

jonathan-innis · 2024-02-17T08:05:31Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+			ExpectNotScheduled(ctx, env.Client, pod1)
+			ExpectNotScheduled(ctx, env.Client, pod2)
+		})
+		It("should schedule respecting the minValues in Lt operator.", func() {


Suggested change

It("should schedule respecting the minValues in Lt operator.", func() {

It("should schedule respecting the minValues in Lt operator", func() {

jonathan-innis · 2024-02-17T08:05:41Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+			// Ensures that NodeClaims are created with 2 instanceTypes
+			Expect(len(supportedInstanceTypes(cloudProvider.CreateCalls[0]))).To(BeNumerically(">=", 2))
+		})
+		It("scheduler should fail if the minValues in Lt operator is not satisfied.", func() {


Suggested change

It("scheduler should fail if the minValues in Lt operator is not satisfied.", func() {

It("scheduler should fail if the minValues in Lt operator is not satisfied", func() {

jonathan-innis · 2024-02-17T08:06:22Z

pkg/controllers/provisioning/scheduling/instance_selection_test.go

+			// Ensures that NodeClaims are created with 2 instanceTypes
+			Expect(len(supportedInstanceTypes(cloudProvider.CreateCalls[0]))).To(BeNumerically(">=", 2))
+		})
+		It("should schedule and respect multiple requirement keys with minValues.", func() {


Suggested change

It("should schedule and respect multiple requirement keys with minValues.", func() {

It("should schedule and respect multiple requirement keys with minValues", func() {

jonathan-innis

LGTM 🚀

jonathan-innis

LGTM 🚀

nikmohan123 and others added 21 commits January 31, 2024 19:47

Initial commit for introducing the API changes and modified tests to …

5a575a4

…incorporate the API changes.

Wire the minValues from NodePool spec requirement into scheduling req…

1a9b1fe

…uirement.

Add functional tests and add validations.

a97be86

Add more documentation.

babd1a0

Merge branch 'kubernetes-sigs:main' into cps_api

5511c38

Take in latest changes from main.

4b3ae1f

Bump the version of CRD.

4436956

Merge branch 'cps_final' into cps_api

fddb8e1

Add more documentation.

87146ea

Merge branch 'cps_final' into cps_api

47533a0

Add additional logic to fail close.

8e9983f

Merge branch 'cps_final' into cps_api

0476b29

Merge branch 'cps_final' into cps_api

02bbc05

Add minValues to the error string of requirement.

43a66b6

Merge branch 'cps_final' into cps_api

bcce54f

Merge branch 'cps_final' into cps_api

45cb106

Merge branch 'cps_final' into cps_api

66e73c2

Merge branch 'cps_final' into cps_api

b927885

Merge branch 'cps_final' into cps_api

7f37a59

Restrict the minValues to 50.

25bb8c7

Add tests for consolidation related to minValues in requirement.

40abee1

jonathan-innis reviewed Feb 16, 2024

View reviewed changes

nikmohan123 added 4 commits February 15, 2024 18:27

Merge branch 'cps_final' into cps_api

c46f805

Addressed few comments.

bbbff6b

Added topology test.

4ed195c

Addressed rest of the comments.

9fa3442

nikmohan123 requested a review from jonathan-innis February 16, 2024 06:44

jonathan-innis reviewed Feb 16, 2024

View reviewed changes

Addressed more comments.

0ba1fde

nikmohan123 requested a review from jonathan-innis February 17, 2024 00:34

jonathan-innis reviewed Feb 17, 2024

View reviewed changes

Addressed few more comments.

50f249a

nikmohan123 requested a review from jonathan-innis February 17, 2024 01:52

jonathan-innis approved these changes Feb 17, 2024

View reviewed changes

Fix error message order.

2cbe86c

nikmohan123 requested a review from jonathan-innis February 20, 2024 21:14

jonathan-innis approved these changes Feb 21, 2024

View reviewed changes

Merge branch 'cps_final' into cps_api

3b61c23

jonathan-innis approved these changes Feb 21, 2024

View reviewed changes

nikmohan123 merged commit 8af4c61 into cps_final Feb 21, 2024
3 of 10 checks passed

	Requirements []NodeSelectorRequirementWithFlexibility `json:"requirements" hash:"ignore"`
	Requirements []NodeSelectorRequirement`json:"requirements" hash:"ignore"`

	// This is an alpha field.
	// This field is ALPHA and can be dropped or replaced at any time

	// +kubebuilder:validation:XValidation:message="requirements with 'minValues' must have minimum values defined",rule="self.all(x, (x.operator == 'In' && has(x.minValues)) ? x.values.size() >= x.minValues : true)"
	// +kubebuilder:validation:XValidation:message="requirements with 'minValues' must have at least that many values specified in the "values" field",rule="self.all(x, (x.operator == 'In' && has(x.minValues)) ? x.values.size() >= x.minValues : true)"

	errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have at least minimum number of values defined", requirement.Key, requirement.Operator))
	errs = multierr.Append(errs, fmt.Errorf("key %s with operator %s must have at least minimum number of values defined in 'values' field", requirement.Key, requirement.Operator))

	It("spot to spot consolidation should consider the max of default and minimum number of instanceTypeOptions from minValues in requirement for truncation.", func() {
	It("spot to spot consolidation should consider the max of default and minimum number of instanceTypeOptions from minValues in requirement for truncation if minimum number of instanceTypeOptions from minValues in requirement is greater than 15.", func() {

	Context("Scheduler behavior for requirements with minValues", func() {
	Context("MinValues", func() {

	instanceGeneration := "karpenter/numerical-value"
	numericalRequirementKey := v1beta1.Group + "/numerical-value"

	It("should schedule respecting the minValues in Gt operator.", func() {
	It("should schedule respecting the minValues in Gt operator", func() {

	It("scheduler should fail if the minValues in Gt operator is not satisfied.", func() {
	It("scheduler should fail if the minValues in Gt operator is not satisfied", func() {

	It("should schedule and respect multiple requirement keys with minValues.", func() {
	It("should schedule and respect multiple requirement keys with minValues", func() {

feat: Add a new field called minValues that introduces flexibility to the NodePool requirement. #1

feat: Add a new field called minValues that introduces flexibility to the NodePool requirement. #1

Conversation

nikmohan123 commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

nikmohan123 commented Feb 9, 2024 •

edited

Loading