From bb0356f86079704d37754deb79aefd77b876cf13 Mon Sep 17 00:00:00 2001 From: Felix Zhe Huang Date: Wed, 2 Feb 2022 21:41:44 -0500 Subject: [PATCH] Clean up and implement review comments --- .../crds/karpenter.sh_provisioners.yaml | 3 +- .../v1alpha5/provisioner_validation.go | 19 ++-- .../provisioning/v1alpha5/requirements.go | 88 +++++++++---------- pkg/apis/provisioning/v1alpha5/suite_test.go | 43 +++++---- pkg/controllers/provisioning/controller.go | 3 +- .../provisioning/scheduling/topology.go | 2 +- pkg/controllers/selection/controller.go | 4 +- pkg/utils/functional/functional.go | 32 ------- pkg/utils/functional/suite_test.go | 38 -------- pkg/utils/sets/sets.go | 2 +- pkg/utils/sets/suite_test.go | 1 - 11 files changed, 82 insertions(+), 153 deletions(-) diff --git a/charts/karpenter/crds/karpenter.sh_provisioners.yaml b/charts/karpenter/crds/karpenter.sh_provisioners.yaml index 0430c3b2a457..ff6dac202d4d 100644 --- a/charts/karpenter/crds/karpenter.sh_provisioners.yaml +++ b/charts/karpenter/crds/karpenter.sh_provisioners.yaml @@ -1,9 +1,10 @@ + --- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.8.0 + controller-gen.kubebuilder.io/version: v0.7.0 creationTimestamp: null name: provisioners.karpenter.sh spec: diff --git a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go index b8e5d17476f0..6b4f0c80323e 100644 --- a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go +++ b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go @@ -19,15 +19,16 @@ import ( "fmt" "strings" + "github.com/aws/karpenter/pkg/utils/ptr" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" - - "github.com/aws/karpenter/pkg/utils/ptr" ) var ( - SupportedNodeSelectorOps = []string{string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpNotIn), string(v1.NodeSelectorOpExists), string(v1.NodeSelectorOpDoesNotExist)} + SupportedNodeSelectorOps sets.String = sets.NewString(string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpNotIn), string(v1.NodeSelectorOpExists), string(v1.NodeSelectorOpDoesNotExist)) + SupportedProvisionerOps sets.String = sets.NewString(string(v1.NodeSelectorOpIn), string(v1.NodeSelectorOpExists)) ) func (p *Provisioner) Validate(ctx context.Context) (errs *apis.FieldError) { @@ -136,10 +137,14 @@ func (c *Constraints) validateTaints() (errs *apis.FieldError) { // When this function is called, the provisioner's requirments do not include the requirements from labels. // Provisioner requirements only support well known labels. func (c *Constraints) validateRequirements() (errs *apis.FieldError) { - // Ensure requirements are well known - for key := range c.Requirements.Keys() { - if !WellKnownLabels.Has(key) { - errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", key, WellKnownLabels.UnsortedList()), "key")) + for _, requirement := range c.Requirements.Requirements { + // Ensure requirements are well known + if !WellKnownLabels.Has(requirement.Key) { + errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", requirement.Key, WellKnownLabels.UnsortedList()), "key")) + } + // Ensure requirements operator is allowed + if !SupportedProvisionerOps.Has(string(requirement.Operator)) { + errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", requirement.Operator, SupportedProvisionerOps.UnsortedList()), "key")) } } if err := c.Requirements.Validate(); err != nil { diff --git a/pkg/apis/provisioning/v1alpha5/requirements.go b/pkg/apis/provisioning/v1alpha5/requirements.go index 2e41f488d7ec..f4691c50b8b1 100644 --- a/pkg/apis/provisioning/v1alpha5/requirements.go +++ b/pkg/apis/provisioning/v1alpha5/requirements.go @@ -19,7 +19,6 @@ import ( "fmt" "sort" - "github.com/aws/karpenter/pkg/utils/functional" "github.com/aws/karpenter/pkg/utils/sets" v1 "k8s.io/api/core/v1" stringsets "k8s.io/apimachinery/pkg/util/sets" @@ -74,11 +73,6 @@ var ( "beta.kubernetes.io/os": v1.LabelOSStable, v1.LabelInstanceType: v1.LabelInstanceTypeStable, } - // IgnoredLables are not considered in scheduling decisions - // and prevent validation errors when specified - IgnoredLabels = sets.NewString( - v1.LabelTopologyRegion, - ) ) // Requirements are an alias type that wrap []v1.NodeSelectorRequirement and @@ -151,9 +145,9 @@ func (r Requirements) Add(requirements ...v1.NodeSelectorRequirement) Requiremen r.Requirements = append(r.Requirements, requirement) switch requirement.Operator { case v1.NodeSelectorOpIn: - r.requirements[requirement.Key] = r.Values(requirement.Key).Intersection(sets.NewSet(requirement.Values...)) + r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewSet(requirement.Values...)) case v1.NodeSelectorOpNotIn: - r.requirements[requirement.Key] = r.Values(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...)) + r.requirements[requirement.Key] = r.Get(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...)) } } return r @@ -168,9 +162,9 @@ func (r Requirements) Keys() stringsets.String { return keys } -// Values returns the sets of values allowed by all included requirements +// Get returns the sets of values allowed by all included requirements // following a denylist method. Values are allowed except specified -func (r Requirements) Values(key string) sets.Set { +func (r Requirements) Get(key string) sets.Set { if _, ok := r.requirements[key]; !ok { return sets.NewComplementSet() } @@ -178,50 +172,47 @@ func (r Requirements) Values(key string) sets.Set { } func (r Requirements) Zones() stringsets.String { - return r.Values(v1.LabelTopologyZone).Values() + return r.Get(v1.LabelTopologyZone).Values() } func (r Requirements) InstanceTypes() stringsets.String { - return r.Values(v1.LabelInstanceTypeStable).Values() + return r.Get(v1.LabelInstanceTypeStable).Values() } func (r Requirements) Architectures() stringsets.String { - return r.Values(v1.LabelArchStable).Values() + return r.Get(v1.LabelArchStable).Values() } func (r Requirements) OperatingSystems() stringsets.String { - return r.Values(v1.LabelOSStable).Values() + return r.Get(v1.LabelOSStable).Values() } func (r Requirements) CapacityTypes() stringsets.String { - return r.Values(LabelCapacityType).Values() + return r.Get(LabelCapacityType).Values() } // Validate validates the feasibility of the requirements. func (r Requirements) Validate() (errs *apis.FieldError) { for i, requirement := range r.Requirements { for _, err := range validation.IsQualifiedName(requirement.Key) { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s, %s", requirement.Key, err), "key")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("key %s, %s", requirement.Key, err), "key")) } for _, value := range requirement.Values { for _, err := range validation.IsValidLabelValue(value) { - errs = errs.Also(apis.ErrInvalidArrayValue(fmt.Sprintf("%s, %s", value, err), "values", i)) + errs = errs.Also(apis.ErrInvalidArrayValue(fmt.Sprintf("key %s, value %s, %s", requirement.Key, value, err), "values", i)) } } - if !functional.ContainsString(SupportedNodeSelectorOps, string(requirement.Operator)) { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", requirement.Operator, SupportedNodeSelectorOps), "operator")) + if !SupportedNodeSelectorOps.Has(string(requirement.Operator)) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s for %s", requirement.Operator, SupportedNodeSelectorOps.UnsortedList(), requirement.Key), "operator")) } // Excludes cases when DoesNotExists appears together with In, NotIn, Exists if requirement.Operator == v1.NodeSelectorOpDoesNotExist && (r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpIn)) || r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpNotIn)) || r.hasRequirement(withKeyAndOperator(requirement.Key, v1.NodeSelectorOpExists))) { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("operator %s and %s conflict", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("operator %s and %s conflict for %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, requirement.Key), "operator")) } - } - for key := range r.Keys() { - values := r.Values(key) - if values.Len() == 0 { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no feasible value for requirement, %s", key), "values")) + if r.Get(requirement.Key).Len() == 0 { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no feasible value for %s", requirement.Key), "values")) } } return errs @@ -229,34 +220,41 @@ func (r Requirements) Validate() (errs *apis.FieldError) { // Compatible ensures the provided requirements can be met. It is // non-commutative (i.e., A.Compatible(B) != B.Compatible(A)) +//gocyclo:ignore func (r Requirements) Compatible(requirements Requirements) (errs *apis.FieldError) { for i, key := range r.Keys().Union(requirements.Keys()).UnsortedList() { // Key must be defined if required - if values := requirements.Values(key); values.Len() != 0 && !values.Complement && !r.hasRequirement(withKey(key)) { - errs = errs.Also(apis.ErrInvalidValue("is not defined", "key")).ViaFieldIndex("requirements", i) + if values := requirements.Get(key); values.Len() != 0 && !values.Complement && !r.hasRequirement(withKey(key)) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("require values for key %s but is not defined", key), "key")).ViaFieldIndex("requirements", i) } // Values must overlap - if values := r.Values(key); values.Intersection(requirements.Values(key)).Len() == 0 { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", values, requirements.Values(key)), "values")).ViaFieldIndex("requirements", i) + if values := r.Get(key); values.Intersection(requirements.Get(key)).Len() == 0 { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s, key %s", values, requirements.Get(key), key), "values")).ViaFieldIndex("requirements", i) } - - for r1, r2 := range map[*Requirements]*Requirements{ - &r: &requirements, - &requirements: &r, - } { - // Exists incompatible with DoesNotExist or undefined - if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) { - if r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key)) { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i) - } + // Exists incompatible with DoesNotExist or undefined + if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) { + if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key)) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i) } - // DoesNotExist requires DoesNotExist or undefined - if r1.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) { - if !(r2.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r2.hasRequirement(withKey(key))) { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist), "operator")).ViaFieldIndex("requirements", i) - } + } + // DoesNotExist requires DoesNotExist or undefined + if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) { + if !(r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !r.hasRequirement(withKey(key))) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i) + } + } + // Repeat for the other direction + // Exists incompatible with DoesNotExist or undefined + if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpExists)) { + if requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key)) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s prohibits %s, key %s", v1.NodeSelectorOpExists, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i) + } + } + // DoesNotExist requires DoesNotExist or undefined + if r.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) { + if !(requirements.hasRequirement(withKeyAndOperator(key, v1.NodeSelectorOpDoesNotExist)) || !requirements.hasRequirement(withKey(key))) { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s requires %s, key %s", v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpDoesNotExist, key), "operator")).ViaFieldIndex("requirements", i) } - } } return errs diff --git a/pkg/apis/provisioning/v1alpha5/suite_test.go b/pkg/apis/provisioning/v1alpha5/suite_test.go index 6037f6ce3ba6..f123dddac3a2 100644 --- a/pkg/apis/provisioning/v1alpha5/suite_test.go +++ b/pkg/apis/provisioning/v1alpha5/suite_test.go @@ -129,18 +129,16 @@ var _ = Describe("Validation", func() { Expect(provisioner.Validate(ctx)).ToNot(Succeed()) }) }) - Context("Requirements", func() { + Context("Validation", func() { It("should allow supported ops", func() { provisioner.Spec.Requirements = NewRequirements( v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}, - v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"bar"}}, v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}, - v1.NodeSelectorRequirement{Key: v1.LabelOSStable, Operator: v1.NodeSelectorOpDoesNotExist}, ) Expect(provisioner.Validate(ctx)).To(Succeed()) }) It("should fail for unsupported ops", func() { - for _, op := range []v1.NodeSelectorOperator{v1.NodeSelectorOpGt, v1.NodeSelectorOpLt} { + for _, op := range []v1.NodeSelectorOperator{v1.NodeSelectorOpNotIn, v1.NodeSelectorOpDoesNotExist, v1.NodeSelectorOpGt, v1.NodeSelectorOpLt} { provisioner.Spec.Requirements = NewRequirements( v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: op, Values: []string{"test"}}, ) @@ -196,6 +194,8 @@ var _ = Describe("Validation", func() { ) Expect(provisioner.Spec.Requirements.Keys().UnsortedList()).To(Equal([]string{v1.LabelTopologyZone})) }) + }) + Context("Compatibility", func() { It("A should be compatible to B, operator", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}}) B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}}) @@ -226,6 +226,11 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) Expect(A.Compatible(B)).ToNot(Succeed()) }) + It("A should be compatible to B, operator", func() { + A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}}) + B := NewRequirements() + Expect(A.Compatible(B)).To(Succeed()) + }) It("A should be compatible to B, operator", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}}) B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}}) @@ -251,7 +256,11 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) Expect(A.Compatible(B)).ToNot(Succeed()) }) - + It("A should be compatible to B, operator", func() { + A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}}) + B := NewRequirements() + Expect(A.Compatible(B)).To(Succeed()) + }) It("A should be compatible to B, operator", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}) B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}}) @@ -272,6 +281,11 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) Expect(A.Compatible(B)).ToNot(Succeed()) }) + It("A should fail to be compatible to B, operator, conflicting", func() { + A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}) + B := NewRequirements() + Expect(A.Compatible(B)).ToNot(Succeed()) + }) It("A should fail to be compatible to B, operator, conflicting", func() { A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}}) @@ -292,8 +306,8 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist}) Expect(A.Compatible(B)).To(Succeed()) }) - It("A should be compatible to B, operator", func() { - A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"foo"}}) + It("A should be compatible to B, operator", func() { + A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}}) B := NewRequirements() Expect(A.Compatible(B)).To(Succeed()) }) @@ -307,30 +321,15 @@ var _ = Describe("Validation", func() { B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}}) Expect(A.Compatible(B)).To(Succeed()) }) - It("A should be compatible to B, operator", func() { - A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpNotIn, Values: []string{"foo"}}) - B := NewRequirements() - Expect(A.Compatible(B)).To(Succeed()) - }) It("A should fail to be compatible to B,, operator, conflicting", func() { A := NewRequirements() B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}) Expect(A.Compatible(B)).ToNot(Succeed()) }) - It("A should fail to be compatible to B, operator, conflicting", func() { - A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpExists}) - B := NewRequirements() - Expect(A.Compatible(B)).ToNot(Succeed()) - }) It("A should be compatible to B, operator", func() { A := NewRequirements() B := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}}) Expect(A.Compatible(B)).To(Succeed()) }) - It("A should be compatible to B, operator", func() { - A := NewRequirements(v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpDoesNotExist, Values: []string{"foo"}}) - B := NewRequirements() - Expect(A.Compatible(B)).To(Succeed()) - }) }) }) diff --git a/pkg/controllers/provisioning/controller.go b/pkg/controllers/provisioning/controller.go index 954d0580dcd6..8b2af006e3a9 100644 --- a/pkg/controllers/provisioning/controller.go +++ b/pkg/controllers/provisioning/controller.go @@ -103,7 +103,7 @@ func (c *Controller) Apply(ctx context.Context, provisioner *v1alpha5.Provisione Add(requirements(instanceTypes)...). Add(v1alpha5.NewLabelRequirements(provisioner.Spec.Labels).Requirements...) if err := provisioner.Spec.Requirements.Validate(); err != nil { - return fmt.Errorf("provisioner requirements validation failed, %v", err) + return fmt.Errorf("validating requirements, %w", err) } // Update the provisioner if anything has changed if c.hasChanged(ctx, provisioner) { @@ -163,7 +163,6 @@ func requirements(instanceTypes []cloudprovider.InstanceType) []v1.NodeSelectorR requirements = append(requirements, v1.NodeSelectorRequirement{Key: key, Operator: v1.NodeSelectorOpIn, Values: values.UnsortedList()}) } return requirements - } // Register the controller to the manager diff --git a/pkg/controllers/provisioning/scheduling/topology.go b/pkg/controllers/provisioning/scheduling/topology.go index 5834fff7070a..c381620bcaf4 100644 --- a/pkg/controllers/provisioning/scheduling/topology.go +++ b/pkg/controllers/provisioning/scheduling/topology.go @@ -47,7 +47,7 @@ func (t *Topology) Inject(ctx context.Context, constraints *v1alpha5.Constraints } for _, pod := range topologyGroup.Pods { domain := topologyGroup.NextDomain(constraints.Requirements.Add(v1alpha5.NewPodRequirements(pod).Requirements...). - Values(topologyGroup.Constraint.TopologyKey). + Get(topologyGroup.Constraint.TopologyKey). Values()) pod.Spec.NodeSelector = functional.UnionStringMaps(pod.Spec.NodeSelector, map[string]string{topologyGroup.Constraint.TopologyKey: domain}) } diff --git a/pkg/controllers/selection/controller.go b/pkg/controllers/selection/controller.go index 4e6f7f5c59f6..79f27f454114 100644 --- a/pkg/controllers/selection/controller.go +++ b/pkg/controllers/selection/controller.go @@ -21,7 +21,6 @@ import ( "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" "github.com/aws/karpenter/pkg/controllers/provisioning" - "github.com/aws/karpenter/pkg/utils/functional" "github.com/aws/karpenter/pkg/utils/pod" "github.com/go-logr/zapr" "go.uber.org/multierr" @@ -97,7 +96,6 @@ func (c *Controller) selectProvisioner(ctx context.Context, pod *v1.Pod) (errs e return nil } for _, candidate := range c.provisioners.List(ctx) { - if err := candidate.Spec.DeepCopy().ValidatePod(pod); err != nil { errs = multierr.Append(errs, fmt.Errorf("tried provisioner/%s: %w", candidate.Name, err)) } else { @@ -168,7 +166,7 @@ func validateNodeSelectorTerm(term v1.NodeSelectorTerm) (errs error) { } if term.MatchExpressions != nil { for _, requirement := range term.MatchExpressions { - if !functional.ContainsString(v1alpha5.SupportedNodeSelectorOps, string(requirement.Operator)) { + if !v1alpha5.SupportedNodeSelectorOps.Has(string(requirement.Operator)) { errs = multierr.Append(errs, fmt.Errorf("node selector term has unsupported operator, %s", requirement.Operator)) } } diff --git a/pkg/utils/functional/functional.go b/pkg/utils/functional/functional.go index ee6666ac0d56..58eb91106205 100644 --- a/pkg/utils/functional/functional.go +++ b/pkg/utils/functional/functional.go @@ -43,38 +43,6 @@ func StringSliceWithout(vals []string, remove ...string) []string { return without } -// IntersectStringSlice takes the intersection of the slices. -// Semantically: -// 1. [],[a,b] -> []: Empty set will always result in [] -// 2. nil,[a,b] -> [a,b]: Nil is the universal set and does not constrain -// 3. ([a,b],[b]) -> [b]: Takes the intersection of the two sets -func IntersectStringSlice(slices ...[]string) []string { - if len(slices) == 0 { - return nil - } - if len(slices) == 1 { - return UniqueStrings(slices[0]) - } - if slices[0] == nil { - return IntersectStringSlice(slices[1:]...) - } - if slices[1] == nil { - sliced := append(slices[:1], slices[2:]...) - return IntersectStringSlice(sliced...) - } - counts := map[string]bool{} - for _, s := range slices[0] { - counts[s] = true - } - intersection := []string{} - for _, s := range slices[1] { - if _, ok := counts[s]; ok { - intersection = append(intersection, s) - } - } - return IntersectStringSlice(append(slices[2:], intersection)...) -} - func UniqueStrings(strings []string) []string { if strings == nil { return nil diff --git a/pkg/utils/functional/suite_test.go b/pkg/utils/functional/suite_test.go index da48b0911868..e821140f02e0 100644 --- a/pkg/utils/functional/suite_test.go +++ b/pkg/utils/functional/suite_test.go @@ -82,42 +82,4 @@ var _ = Describe("Functional", func() { Expect(UnionStringMaps(original, disjoiner, empty, uberwriter)).To(Equal(expected)) }) }) - Context("IntersectStringSlice", func() { - var nilset []string - empty := []string{} - universe := []string{"a", "b", "c"} - subset := []string{"a", "b"} - overlap := []string{"a", "b", "d"} - disjoint := []string{"d", "e"} - duplicates := []string{"a", "a"} - Specify("nil set", func() { - Expect(IntersectStringSlice()).To(BeNil()) - Expect(IntersectStringSlice(nilset)).To(BeNil()) - Expect(IntersectStringSlice(nilset, nilset)).To(BeNil()) - Expect(IntersectStringSlice(nilset, universe)).To(ConsistOf(universe)) - Expect(IntersectStringSlice(universe, nilset)).To(ConsistOf(universe)) - Expect(IntersectStringSlice(universe, nilset, nilset)).To(ConsistOf(universe)) - }) - Specify("empty set", func() { - Expect(IntersectStringSlice(empty, nilset)).To(And(BeEmpty(), Not(BeNil()))) - Expect(IntersectStringSlice(nilset, empty)).To(And(BeEmpty(), Not(BeNil()))) - Expect(IntersectStringSlice(universe, empty)).To(And(BeEmpty(), Not(BeNil()))) - Expect(IntersectStringSlice(universe, universe, empty)).To(And(BeEmpty(), Not(BeNil()))) - }) - Specify("intersect", func() { - Expect(IntersectStringSlice(universe, subset)).To(ConsistOf(subset)) - Expect(IntersectStringSlice(subset, universe)).To(ConsistOf(subset)) - Expect(IntersectStringSlice(universe, overlap)).To(ConsistOf(subset)) - Expect(IntersectStringSlice(overlap, universe)).To(ConsistOf(subset)) - Expect(IntersectStringSlice(universe, disjoint)).To(And(BeEmpty(), Not(BeNil()))) - Expect(IntersectStringSlice(disjoint, universe)).To(And(BeEmpty(), Not(BeNil()))) - Expect(IntersectStringSlice(overlap, disjoint, universe)).To(And(BeEmpty(), Not(BeNil()))) - }) - Specify("duplicates", func() { - Expect(IntersectStringSlice(duplicates)).To(ConsistOf("a")) - Expect(IntersectStringSlice(duplicates, nilset)).To(ConsistOf("a")) - Expect(IntersectStringSlice(duplicates, universe)).To(ConsistOf("a")) - Expect(IntersectStringSlice(duplicates, universe, subset)).To(ConsistOf("a")) - }) - }) }) diff --git a/pkg/utils/sets/sets.go b/pkg/utils/sets/sets.go index fbd16f5cfae9..9b408233626d 100644 --- a/pkg/utils/sets/sets.go +++ b/pkg/utils/sets/sets.go @@ -54,7 +54,7 @@ func (s Set) DeepCopy() Set { } // Values returns the values of the set. -// If the set has an infinite size, returns an error +// If the set has an infinite size, it will panic func (s Set) Values() sets.String { if s.Complement { panic("infinite set") diff --git a/pkg/utils/sets/suite_test.go b/pkg/utils/sets/suite_test.go index 186c5186fdf8..a2a66ebfb0f6 100644 --- a/pkg/utils/sets/suite_test.go +++ b/pkg/utils/sets/suite_test.go @@ -56,7 +56,6 @@ var _ = Describe("Set", func() { Expect(setA.Intersection(emptySet)).To(Equal(emptySet)) }) }) - Context("Functional Correctness", func() { It("size of AB should be 2", func() {