From 55b017895b7c98256160bd21fb238600440557ca Mon Sep 17 00:00:00 2001 From: Felix Zhe Huang Date: Sat, 22 Jan 2022 20:51:07 -0500 Subject: [PATCH] Clean up and refactor to be consistent to v1alpha5 API --- .../crds/karpenter.sh_provisioners.yaml | 79 +++---- pkg/apis/provisioning/v1alpha5/constraints.go | 6 +- .../v1alpha5/provisioner_validation.go | 16 +- .../provisioning/v1alpha5/requirements.go | 198 +++++++++--------- pkg/apis/provisioning/v1alpha5/suite_test.go | 19 +- .../v1alpha5/zz_generated.deepcopy.go | 9 +- .../aws/apis/v1alpha1/provider_defaults.go | 8 +- pkg/cloudprovider/aws/instance.go | 2 +- pkg/cloudprovider/aws/suite_test.go | 6 +- pkg/cloudprovider/fake/cloudprovider.go | 10 +- .../provisioning/binpacking/packable.go | 32 +-- pkg/controllers/provisioning/controller.go | 9 +- .../provisioning/scheduling/topology.go | 5 +- .../provisioning/scheduling/topologygroup.go | 2 +- pkg/controllers/selection/suite_test.go | 2 +- pkg/utils/sets/sets.go | 161 +++----------- pkg/utils/sets/suite_test.go | 97 +-------- 17 files changed, 225 insertions(+), 436 deletions(-) diff --git a/charts/karpenter/crds/karpenter.sh_provisioners.yaml b/charts/karpenter/crds/karpenter.sh_provisioners.yaml index ad36a504d832..ff6dac202d4d 100644 --- a/charts/karpenter/crds/karpenter.sh_provisioners.yaml +++ b/charts/karpenter/crds/karpenter.sh_provisioners.yaml @@ -78,62 +78,33 @@ spec: requirements: description: Requirements are layered with Labels and applied to every node. - properties: - allows: - additionalProperties: - description: Set is a logical set of string values for the requirements. - It supports representations using complement operator. e.g., - if C={"A", "B"}, setting isComplement = true means C' contains - every possible string values other than "A" and "B" - properties: - Members: - additionalProperties: - description: Empty is public since it is used by some - internal API objects for conversions between external - string arrays and internal sets, and conversion logic - requires public types today. - type: object - description: sets.String is a set of strings, implemented - via map[string]struct{} for minimal memory consumption. - type: object - allows: - type: boolean - type: object - type: object - requirements: - additionalProperties: + items: + description: A node selector requirement is a selector that contains + values, a key, and an operator that relates the key and values. + properties: + key: + description: The label key that the selector applies to. + type: string + operator: + description: Represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists, DoesNotExist. Gt, and + Lt. + type: string + values: + description: An array of string values. If the operator is In + or NotIn, the values array must be non-empty. If the operator + is Exists or DoesNotExist, the values array must be empty. + If the operator is Gt or Lt, the values array must have a + single element, which will be interpreted as an integer. This + array is replaced during a strategic merge patch. items: - description: A node selector requirement is a selector that - contains values, a key, and an operator that relates the - key and values. - properties: - key: - description: The label key that the selector applies to. - type: string - operator: - description: Represents a key's relationship to a set - of values. Valid operators are In, NotIn, Exists, DoesNotExist. - Gt, and Lt. - type: string - values: - description: An array of string values. If the operator - is In or NotIn, the values array must be non-empty. - If the operator is Exists or DoesNotExist, the values - array must be empty. If the operator is Gt or Lt, the - values array must have a single element, which will - be interpreted as an integer. This array is replaced - during a strategic merge patch. - items: - type: string - type: array - required: - - key - - operator - type: object + type: string type: array - description: Ground truth record of requirements - type: object - type: object + required: + - key + - operator + type: object + type: array taints: description: Taints will be applied to every node launched by the Provisioner. If specified, the provisioner will not provision nodes diff --git a/pkg/apis/provisioning/v1alpha5/constraints.go b/pkg/apis/provisioning/v1alpha5/constraints.go index 5d0acacb175c..87de41535af9 100644 --- a/pkg/apis/provisioning/v1alpha5/constraints.go +++ b/pkg/apis/provisioning/v1alpha5/constraints.go @@ -33,7 +33,7 @@ type Constraints struct { // +optional Taints Taints `json:"taints,omitempty"` // Requirements are layered with Labels and applied to every node. - Requirements *Requirements `json:"requirements,omitempty"` + Requirements *Requirements `json:"requirements,inline,omitempty"` // KubeletConfiguration are options passed to the kubelet when provisioning nodes //+optional KubeletConfiguration KubeletConfiguration `json:"kubeletConfiguration,omitempty"` @@ -49,7 +49,7 @@ func (c *Constraints) ValidatePod(pod *v1.Pod) error { return err } // Test if pod requirements are valid - podRequirements := PodRequirements(pod) + podRequirements := NewRequirements(PodRequirements(pod)...) if errs := podRequirements.Validate(); errs != nil { return fmt.Errorf("pod requirements not feasible, %v", errs) } @@ -63,7 +63,7 @@ func (c *Constraints) ValidatePod(pod *v1.Pod) error { func (c *Constraints) Tighten(pod *v1.Pod) *Constraints { return &Constraints{ Labels: c.Labels, - Requirements: c.Requirements.Merge(PodRequirements(pod)).WellKnown(), + Requirements: c.Requirements.Add(PodRequirements(pod)...).WellKnown(), Taints: c.Taints, Provider: c.Provider, KubeletConfiguration: c.KubeletConfiguration, diff --git a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go index f0598d31e6ac..b38821c44079 100644 --- a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go +++ b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go @@ -41,7 +41,7 @@ func (s *ProvisionerSpec) validate(ctx context.Context) (errs *apis.FieldError) return errs.Also( s.validateTTLSecondsUntilExpired(), s.validateTTLSecondsAfterEmpty(), - s.Constraints.Validate(ctx), + s.Validate(ctx), ) } @@ -132,6 +132,18 @@ func (c *Constraints) validateTaints() (errs *apis.FieldError) { return errs } +// This function is used by the provisioner validation webhook to verify the provisioner requirements. +// When this function is called, the provisioner's requirments do not include the requirements from labels. +// Provisioner requirement section only support wellknown labels for now. func (c *Constraints) validateRequirements() (errs *apis.FieldError) { - return c.Requirements.Validate() + // validate if requirement keys are well known labels + 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")) + } + } + if err := c.Requirements.Validate(); err != nil { + errs = errs.Also(err) + } + return errs } diff --git a/pkg/apis/provisioning/v1alpha5/requirements.go b/pkg/apis/provisioning/v1alpha5/requirements.go index 318d0ca5a217..c520eda932dd 100644 --- a/pkg/apis/provisioning/v1alpha5/requirements.go +++ b/pkg/apis/provisioning/v1alpha5/requirements.go @@ -15,6 +15,7 @@ limitations under the License. package v1alpha5 import ( + "encoding/json" "fmt" "sort" @@ -61,7 +62,6 @@ var ( v1.LabelArchStable, v1.LabelOSStable, LabelCapacityType, - ProvisionerNameLabelKey, v1.LabelHostname, // Used internally for hostname topology spread ) // NormalizedLabels translate aliased concepts into the controller's @@ -77,95 +77,79 @@ var ( ) type Requirements struct { - // Ground truth record of requirements - Requirements map[string][]v1.NodeSelectorRequirement `json:"requirements,omitempty"` - Allows map[string]*sets.Set `json:"allows,omitempty"` + // Requirements are layered with Labels and applied to every node. + Requirements []v1.NodeSelectorRequirement `json:"requirements,omitempty"` + // Ground truth for record keeping + Records map[string][]v1.NodeSelectorRequirement `json:"-"` + Allows map[string]sets.Set `json:"-"` } func NewRequirements(requirements ...v1.NodeSelectorRequirement) *Requirements { - r := Requirements{ - Requirements: map[string][]v1.NodeSelectorRequirement{}, - Allows: map[string]*sets.Set{}, + result := Requirements{ + Records: map[string][]v1.NodeSelectorRequirement{}, + Allows: map[string]sets.Set{}, } - r.insert(requirements...) - r.Normalize() - return &r + return result.Add(requirements...) } -// Allow returns the sets of values allowed by all included requirements -func (r *Requirements) Allow(key string) *sets.Set { - if _, ok := r.Allows[key]; !ok { - // init set to contains every possible values - r.Allows[key] = sets.NewSet(true) +func (r *Requirements) Add(requirements ...v1.NodeSelectorRequirement) *Requirements { + if r == nil { + return NewRequirements(requirements...) } - return r.Allows[key] -} - -// insert inserts v1.NodeSelectorRequirement type requirements -func (r *Requirements) insert(requirements ...v1.NodeSelectorRequirement) { + rd := r.DeepCopy() for _, requirement := range requirements { - r.Requirements[requirement.Key] = append(r.Requirements[requirement.Key], requirement) + // Normalizing the key + if newKey, ok := NormalizedLabels[requirement.Key]; ok { + requirement.Key = newKey + } + //rd.Requirements = append(rd.Requirements, requirement) + rd.Records[requirement.Key] = append(rd.Records[requirement.Key], requirement) switch requirement.Operator { case v1.NodeSelectorOpIn: - r.Allows[requirement.Key] = r.Allow(requirement.Key).Intersection(sets.NewSet(false, requirement.Values...)) + rd.Allows[requirement.Key] = rd.Allow(requirement.Key).Intersection(sets.NewSet(requirement.Values...)) case v1.NodeSelectorOpNotIn: - r.Allows[requirement.Key] = r.Allow(requirement.Key).Intersection(sets.NewSet(true, requirement.Values...)) + rd.Allows[requirement.Key] = rd.Allow(requirement.Key).Intersection(sets.NewComplementSet(requirement.Values...)) } } + return rd } -// Merge combines two requirement sets -func (r *Requirements) Merge(requirements *Requirements) *Requirements { - if r == nil { - r = NewRequirements() +func (r *Requirements) MarshalJSON() ([]byte, error) { + var result []v1.NodeSelectorRequirement + for _, requirements := range r.Records { + result = append(result, requirements...) } - combined := []v1.NodeSelectorRequirement{} - allKeys := stringsets.NewString(r.Keys()...).Union(stringsets.NewString(requirements.Keys()...)) - for key := range allKeys { - if req, ok := r.Requirements[key]; ok { - combined = append(combined, req...) - } - if req, ok := requirements.Requirements[key]; ok { - combined = append(combined, req...) - } - } - return NewRequirements(combined...) + return json.Marshal(result) } -// Normalize the requirements to use WellKnownLabels -func (r *Requirements) Normalize() { - allows := map[string]*sets.Set{} - requirements := map[string][]v1.NodeSelectorRequirement{} - for _, key := range r.Keys() { - if label, ok := NormalizedLabels[key]; ok { - if set, has := r.Allows[key]; has { - allows[label] = r.Allow(label).Intersection(set) - } - requirements[label] = r.Requirements[key] - } else { - if _, has := r.Allows[key]; has { - allows[key] = r.Allow(key) - } - requirements[key] = r.Requirements[key] - } +func (r *Requirements) UnmarshalJSON(b []byte) error { + var requirements []v1.NodeSelectorRequirement + json.Unmarshal(b, &requirements) + result := NewRequirements(requirements...) + r.Records = result.Records + r.Allows = result.Allows + return nil +} + +// Allow returns the sets of values allowed by all included requirements +// Allow follows a denylist method. Values are allowed except specified +func (r *Requirements) Allow(key string) sets.Set { + if r == nil { + return sets.NewComplementSet() + } + if _, ok := r.Allows[key]; !ok { + return sets.NewComplementSet() + //r.Allows[key] = sets.NewComplementSet() } - r.Allows = allows - r.Requirements = requirements + return r.Allows[key] } // Validate validates the feasibility of the requirements. func (r *Requirements) Validate() (errs *apis.FieldError) { if r == nil { - r = NewRequirements() + return errs } for _, key := range r.Keys() { - - // Disable checking for WellKnownLabels because label is represented as In opeartor requirement - /* - if !WellKnownLabels.Has(key) { - errs = errs.Also(apis.ErrInvalidKeyName(fmt.Sprintf("%s not in %v", key, WellKnownLabels.UnsortedList()), "key")) - } - */ for _, err := range validation.IsQualifiedName(key) { errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s, %s", key, err), "key")) } @@ -174,13 +158,13 @@ func (r *Requirements) Validate() (errs *apis.FieldError) { errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no feasible value for requirement, %s", key), "values")) } i := 0 - for value := range values.RawValues() { + for value := range values.Members { for _, err := range validation.IsValidLabelValue(value) { errs = errs.Also(apis.ErrInvalidArrayValue(fmt.Sprintf("%s, %s", value, err), "values", i)) } i++ } - for _, requirement := range r.Requirements[key] { + for _, requirement := range r.Records[key] { if !functional.ContainsString(SupportedNodeSelectorOps, string(requirement.Operator)) { errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", requirement.Operator, SupportedNodeSelectorOps), "operator")) } @@ -194,21 +178,21 @@ func (r *Requirements) Validate() (errs *apis.FieldError) { } // FromLabels constructs requriements from labels -func LabelRequirements(labels map[string]string) *Requirements { +func LabelRequirements(labels map[string]string) []v1.NodeSelectorRequirement { requirements := []v1.NodeSelectorRequirement{} for key, value := range labels { requirements = append(requirements, v1.NodeSelectorRequirement{Key: key, Operator: v1.NodeSelectorOpIn, Values: []string{value}}) } - return NewRequirements(requirements...) + return requirements } -func PodRequirements(pod *v1.Pod) *Requirements { +func PodRequirements(pod *v1.Pod) []v1.NodeSelectorRequirement { requirements := []v1.NodeSelectorRequirement{} for key, value := range pod.Spec.NodeSelector { requirements = append(requirements, v1.NodeSelectorRequirement{Key: key, Operator: v1.NodeSelectorOpIn, Values: []string{value}}) } if pod.Spec.Affinity == nil || pod.Spec.Affinity.NodeAffinity == nil { - return NewRequirements(requirements...) + return requirements } // The legal operators for pod affinity and anti-affinity are In, NotIn, Exists, DoesNotExist. // Select heaviest preference and treat as a requirement. An outer loop will iteratively unconstrain them if unsatisfiable. @@ -221,56 +205,68 @@ func PodRequirements(pod *v1.Pod) *Requirements { len(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms) > 0 { requirements = append(requirements, pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions...) } - return NewRequirements(requirements...) + return requirements } // Compatible returns errors with detailed messages when requirements are not compatible // Compatible is non-commutative (i.e., A.Compatible(B) != B.Compatible(A)) func (r *Requirements) Compatible(requirements *Requirements) (errs *apis.FieldError) { if r == nil { - r = NewRequirements() + errs = errs.Also(apis.ErrInvalidValue("nil requirements", "values")) + return errs } allKeys := stringsets.NewString(r.Keys()...).Union(stringsets.NewString(requirements.Keys()...)) for key := range allKeys { - - if r.Allow(key).Intersection(requirements.Allow(key)).Len() == 0 { - rValues, _ := r.Allow(key).Values() - sValues, _ := requirements.Allow(key).Values() - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no common values for key %s, %v not in %v", key, sValues, rValues), "values")) + if err := r.isKeyDefined(key, requirements); err != nil { + errs = errs.Also(err) } - if err := r.compatible(key, requirements); err != nil { + if err := r.hasCommons(key, requirements); err != nil { errs = errs.Also(err) } - // Directional condition: - // This is to capture pod specify a label selector but provisioner does not have the label - // Cases that are ignored: r has Allow values but requirements doesn't specify - // Cases that are caught: requirements has Allow values but r doesn't specify - if _, ok := requirements.Requirements[key]; ok { - _, exist := r.Requirements[key] - if !requirements.Allow(key).IsComplement && requirements.Allow(key).Len() != 0 && !exist { - values, _ := requirements.Allow(key).Values() - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("key %s, need %v but not defined", key, values), "values")) - } + if err := r.satisfyExistOperators(key, requirements); err != nil { + errs = errs.Also(err) + } + } + return errs +} + +// isKeyDefined returns errors when the given requirements requires a key that is not defined by this requirements +// This is a non-commutative function designed to catch the following cases: +// * Pod Spec has a label selector but provisioner doesn't have the label +// * One pod is not compatible with a schedule (group of pods) due to extra requirements +func (r *Requirements) isKeyDefined(key string, requirements *Requirements) (errs *apis.FieldError) { + if _, ok := requirements.Records[key]; ok { + _, exist := r.Records[key] + if !requirements.Allow(key).IsComplement && requirements.Allow(key).Len() != 0 && !exist { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("key %s, need %s but not defined", key, requirements.Allow(key)), "values")) } + } + return errs +} +// hasCommons returns errors when there is no overlap among the requirements' allowed values for a provided key. +func (r *Requirements) hasCommons(key string, requirements *Requirements) (errs *apis.FieldError) { + if r.Allow(key).Intersection(requirements.Allow(key)).Len() == 0 { + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("no common values for key %s, %s not in %s", key, r.Allow(key), requirements.Allow(key)), "values")) } return errs } -func (r *Requirements) compatible(key string, requirements *Requirements) (errs *apis.FieldError) { +// satisfyExistOperators returns errors with detailed messages when Exist or DoesNotExist operators requirements are violated. +func (r *Requirements) satisfyExistOperators(key string, requirements *Requirements) (errs *apis.FieldError) { for r1, r2 := range map[*Requirements]*Requirements{ r: requirements, requirements: r, } { - if records, ok := r1.Requirements[key]; ok { + if records, ok := r1.Records[key]; ok { for _, requirement := range records { switch requirement.Operator { case v1.NodeSelectorOpExists: - if _, exist := r2.Requirements[key]; !exist { + if _, exist := r2.Records[key]; !exist { errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("Exist operator violation, %s", key), "values")) } case v1.NodeSelectorOpDoesNotExist: - if _, exist := r2.Requirements[key]; exist { + if _, exist := r2.Records[key]; exist { errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("DoesNotExist operator violation, %s", key), "values")) } } @@ -280,23 +276,23 @@ func (r *Requirements) compatible(key string, requirements *Requirements) (errs return errs } -func (r *Requirements) Zones() *sets.Set { +func (r *Requirements) Zones() sets.Set { return r.Allow(v1.LabelTopologyZone) } -func (r *Requirements) InstanceTypes() *sets.Set { +func (r *Requirements) InstanceTypes() sets.Set { return r.Allow(v1.LabelInstanceTypeStable) } -func (r *Requirements) Architectures() *sets.Set { +func (r *Requirements) Architectures() sets.Set { return r.Allow(v1.LabelArchStable) } -func (r *Requirements) OperatingSystems() *sets.Set { +func (r *Requirements) OperatingSystems() sets.Set { return r.Allow(v1.LabelOSStable) } -func (r *Requirements) CapacityTypes() *sets.Set { +func (r *Requirements) CapacityTypes() sets.Set { return r.Allow(LabelCapacityType) } @@ -304,7 +300,7 @@ func (r *Requirements) WellKnown() *Requirements { requirements := []v1.NodeSelectorRequirement{} for _, key := range r.Keys() { if WellKnownLabels.Has(key) { - requirements = append(requirements, r.Requirements[key]...) + requirements = append(requirements, r.Records[key]...) } } return NewRequirements(requirements...) @@ -313,10 +309,10 @@ func (r *Requirements) WellKnown() *Requirements { // Keys returns unique set of the label keys from the requirements func (r *Requirements) Keys() []string { if r == nil { - r = NewRequirements() + return []string{} } - keys := make([]string, 0, len(r.Requirements)) - for k := range r.Requirements { + keys := make([]string, 0, len(r.Records)) + for k := range r.Records { keys = append(keys, k) } return keys diff --git a/pkg/apis/provisioning/v1alpha5/suite_test.go b/pkg/apis/provisioning/v1alpha5/suite_test.go index 2602e21a8813..dd91ba9eda52 100644 --- a/pkg/apis/provisioning/v1alpha5/suite_test.go +++ b/pkg/apis/provisioning/v1alpha5/suite_test.go @@ -28,6 +28,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" ) var ctx context.Context @@ -163,16 +164,14 @@ var _ = Describe("Validation", func() { Expect(provisioner.Validate(ctx)).To(Succeed()) } }) - /* - It("should fail for unknown labels", func() { - for label := range sets.NewString("unknown", "invalid", "rejected") { - provisioner.Spec.Requirements = NewRequirements( - v1.NodeSelectorRequirement{Key: label, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}, - ) - Expect(provisioner.Validate(ctx)).ToNot(Succeed()) - } - }) - */ + It("should fail for unknown labels", func() { + for label := range sets.NewString("unknown", "invalid", "rejected") { + provisioner.Spec.Requirements = NewRequirements( + v1.NodeSelectorRequirement{Key: label, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}, + ) + Expect(provisioner.Validate(ctx)).ToNot(Succeed()) + } + }) It("should fail because no feasible value", func() { provisioner.Spec.Requirements = NewRequirements( v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}, diff --git a/pkg/apis/provisioning/v1alpha5/zz_generated.deepcopy.go b/pkg/apis/provisioning/v1alpha5/zz_generated.deepcopy.go index 81539e4a3456..5efee01441e8 100644 --- a/pkg/apis/provisioning/v1alpha5/zz_generated.deepcopy.go +++ b/pkg/apis/provisioning/v1alpha5/zz_generated.deepcopy.go @@ -233,6 +233,13 @@ func (in *Requirements) DeepCopyInto(out *Requirements) { *out = *in if in.Requirements != nil { in, out := &in.Requirements, &out.Requirements + *out = make([]v1.NodeSelectorRequirement, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.Records != nil { + in, out := &in.Records, &out.Records *out = make(map[string][]v1.NodeSelectorRequirement, len(*in)) for key, val := range *in { var outVal []v1.NodeSelectorRequirement @@ -250,7 +257,7 @@ func (in *Requirements) DeepCopyInto(out *Requirements) { } if in.Allows != nil { in, out := &in.Allows, &out.Allows - *out = make(map[string]*sets.Set, len(*in)) + *out = make(map[string]sets.Set, len(*in)) for key, val := range *in { (*out)[key] = val.DeepCopy() } diff --git a/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go b/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go index a5874c74c5e3..588375bde09d 100644 --- a/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go +++ b/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go @@ -41,11 +41,11 @@ func (c *Constraints) defaultCapacityTypes() { if functional.ContainsString(c.Requirements.Keys(), v1alpha5.LabelCapacityType) { return } - c.Requirements = c.Requirements.Merge(v1alpha5.NewRequirements(v1.NodeSelectorRequirement{ + c.Requirements = c.Requirements.Add(v1.NodeSelectorRequirement{ Key: v1alpha5.LabelCapacityType, Operator: v1.NodeSelectorOpIn, Values: []string{CapacityTypeOnDemand}, - })) + }) } func (c *Constraints) defaultArchitecture() { @@ -55,11 +55,11 @@ func (c *Constraints) defaultArchitecture() { if functional.ContainsString(c.Requirements.Keys(), v1.LabelArchStable) { return } - c.Requirements = c.Requirements.Merge(v1alpha5.NewRequirements(v1.NodeSelectorRequirement{ + c.Requirements = c.Requirements.Add(v1.NodeSelectorRequirement{ Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{v1alpha5.ArchitectureAmd64}, - })) + }) } func (c *Constraints) defaultSubnets(clusterName string) { diff --git a/pkg/cloudprovider/aws/instance.go b/pkg/cloudprovider/aws/instance.go index fa2dc54772b5..8e50d841e0d6 100644 --- a/pkg/cloudprovider/aws/instance.go +++ b/pkg/cloudprovider/aws/instance.go @@ -180,7 +180,7 @@ func (p *InstanceProvider) getLaunchTemplateConfigs(ctx context.Context, constra // getOverrides creates and returns launch template overrides for the cross product of instanceTypeOptions and subnets (with subnets being constrained by // zones and the offerings in instanceTypeOptions) -func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, zones *sets.Set, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest { +func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, zones sets.Set, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest { var overrides []*ec2.FleetLaunchTemplateOverridesRequest for i, instanceType := range instanceTypeOptions { for _, offering := range instanceType.Offerings() { diff --git a/pkg/cloudprovider/aws/suite_test.go b/pkg/cloudprovider/aws/suite_test.go index 1dcd2eb9219e..607210ecc6c6 100644 --- a/pkg/cloudprovider/aws/suite_test.go +++ b/pkg/cloudprovider/aws/suite_test.go @@ -288,12 +288,12 @@ var _ = Describe("Allocation", func() { {CapacityType: v1alpha1.CapacityTypeOnDemand, InstanceType: "m5.xlarge", Zone: "test-zone-1a"}, } twoInstanceProvisioner := provisioner.DeepCopy() - twoInstanceProvisioner.Spec.Constraints.Requirements = twoInstanceProvisioner.Spec.Constraints.Requirements.Merge( - v1alpha5.NewRequirements(v1.NodeSelectorRequirement{ + twoInstanceProvisioner.Spec.Constraints.Requirements = twoInstanceProvisioner.Spec.Constraints.Requirements.Add( + v1.NodeSelectorRequirement{ Key: v1.LabelInstanceType, Operator: v1.NodeSelectorOpIn, Values: []string{"m5.large", "m5.xlarge"}, - })) + }) pods := []*v1.Pod{} for i := 0; i < 2; i++ { pods = append(pods, test.UnschedulablePod(test.PodOptions{ diff --git a/pkg/cloudprovider/fake/cloudprovider.go b/pkg/cloudprovider/fake/cloudprovider.go index 125c01cfb1ff..2a62448c6cc0 100644 --- a/pkg/cloudprovider/fake/cloudprovider.go +++ b/pkg/cloudprovider/fake/cloudprovider.go @@ -41,12 +41,10 @@ func (c *CloudProvider) Create(_ context.Context, constraints *v1alpha5.Constrai instance := instanceTypes[0] var zone, capacityType string for _, o := range instance.Offerings() { - if constraints.Requirements.CapacityTypes().Has(o.CapacityType) { - if constraints.Requirements.Zones().Has(o.Zone) { - zone = o.Zone - capacityType = o.CapacityType - break - } + if constraints.Requirements.CapacityTypes().Has(o.CapacityType) && constraints.Requirements.Zones().Has(o.Zone) { + zone = o.Zone + capacityType = o.CapacityType + break } } diff --git a/pkg/controllers/provisioning/binpacking/packable.go b/pkg/controllers/provisioning/binpacking/packable.go index 0a50d385fc77..74e59bf7f848 100644 --- a/pkg/controllers/provisioning/binpacking/packable.go +++ b/pkg/controllers/provisioning/binpacking/packable.go @@ -175,34 +175,22 @@ func (p *Packable) reservePod(pod *v1.Pod) bool { func (p *Packable) validateInstanceType(constraints *v1alpha5.Constraints) error { if !constraints.Requirements.InstanceTypes().Has(p.Name()) { - instanceTypes := constraints.Requirements.InstanceTypes() - if instanceTypes.IsComplement { - return fmt.Errorf("instance type %s not in complement set %v'", p.Name(), instanceTypes.RawValues()) - } - return fmt.Errorf("instance type %s not in %v", p.Name(), instanceTypes.RawValues()) + return fmt.Errorf("instance type %s not in %s", p.Name(), constraints.Requirements.InstanceTypes()) } return nil } func (p *Packable) validateArchitecture(constraints *v1alpha5.Constraints) error { if !constraints.Requirements.Architectures().Has(p.Architecture()) { - architectures := constraints.Requirements.Architectures() - if architectures.IsComplement { - return fmt.Errorf("architecture %s not in complement set %v'", p.Name(), architectures.RawValues()) - } - return fmt.Errorf("architecture %s not in %v", p.Name(), architectures.RawValues()) + return fmt.Errorf("architecture %s not in %s", p.Name(), constraints.Requirements.Architectures()) } return nil } func (p *Packable) validateOperatingSystems(constraints *v1alpha5.Constraints) error { - operatingSystem := sets.NewSet(false, p.OperatingSystems().UnsortedList()...) + operatingSystem := sets.NewSet(p.OperatingSystems().UnsortedList()...) if constraints.Requirements.OperatingSystems().Intersection(operatingSystem).Len() == 0 { - operatingSystems := constraints.Requirements.OperatingSystems() - if operatingSystems.IsComplement { - return fmt.Errorf("operating systems %s not in complement set %v'", p.Name(), operatingSystems.RawValues()) - } - return fmt.Errorf("operating systems %s not in %v", p.Name(), operatingSystems.RawValues()) + return fmt.Errorf("operating systems %s not in %s", p.Name(), constraints.Requirements.OperatingSystems()) } return nil } @@ -213,17 +201,7 @@ func (p *Packable) validateOfferings(constraints *v1alpha5.Constraints) error { return nil } } - capacityTypes := constraints.Requirements.CapacityTypes() - zones := constraints.Requirements.Zones() - var capacityModifier, zoneModifier string - if capacityTypes.IsComplement { - capacityModifier = "(complement set)" - } - if zones.IsComplement { - zoneModifier = "(complement set)" - } - - return fmt.Errorf("offerings %v are not available for capacity types %v%s and zones %v%s", p.Offerings(), capacityTypes.RawValues(), capacityModifier, zones.RawValues(), zoneModifier) + return fmt.Errorf("offerings %v are not available for capacity types %s and zones %s", p.Offerings(), constraints.Requirements.CapacityTypes(), constraints.Requirements.Zones()) } func (p *Packable) validateGPUs(pods []*v1.Pod) error { diff --git a/pkg/controllers/provisioning/controller.go b/pkg/controllers/provisioning/controller.go index 037f1d4cec49..754828e05de7 100644 --- a/pkg/controllers/provisioning/controller.go +++ b/pkg/controllers/provisioning/controller.go @@ -100,8 +100,8 @@ func (c *Controller) Apply(ctx context.Context, provisioner *v1alpha5.Provisione } provisioner.Spec.Labels = functional.UnionStringMaps(provisioner.Spec.Labels, map[string]string{v1alpha5.ProvisionerNameLabelKey: provisioner.Name}) provisioner.Spec.Requirements = provisioner.Spec.Requirements. - Merge(requirements(instanceTypes)). - Merge(v1alpha5.LabelRequirements(provisioner.Spec.Labels)) + Add(requirements(instanceTypes)...). + Add(v1alpha5.LabelRequirements(provisioner.Spec.Labels)...) if err := provisioner.Spec.Requirements.Validate(); err != nil { return fmt.Errorf("provisioner requirements validation failed, %v", err) } @@ -141,7 +141,7 @@ func (c *Controller) List(ctx context.Context) []*Provisioner { return provisioners } -func requirements(instanceTypes []cloudprovider.InstanceType) *v1alpha5.Requirements { +func requirements(instanceTypes []cloudprovider.InstanceType) []v1.NodeSelectorRequirement { supported := map[string]sets.String{ v1.LabelInstanceTypeStable: sets.NewString(), v1.LabelTopologyZone: sets.NewString(), @@ -162,7 +162,8 @@ func requirements(instanceTypes []cloudprovider.InstanceType) *v1alpha5.Requirem for key, values := range supported { requirements = append(requirements, v1.NodeSelectorRequirement{Key: key, Operator: v1.NodeSelectorOpIn, Values: values.UnsortedList()}) } - return v1alpha5.NewRequirements(requirements...) + 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 cc189bdea8cb..19f3cf9c4498 100644 --- a/pkg/controllers/provisioning/scheduling/topology.go +++ b/pkg/controllers/provisioning/scheduling/topology.go @@ -46,7 +46,7 @@ func (t *Topology) Inject(ctx context.Context, constraints *v1alpha5.Constraints return fmt.Errorf("computing topology, %w", err) } for _, pod := range topologyGroup.Pods { - domain := topologyGroup.NextDomain(constraints.Requirements.Merge(v1alpha5.PodRequirements(pod)).Allow(topologyGroup.Constraint.TopologyKey)) + domain := topologyGroup.NextDomain(constraints.Requirements.Add(v1alpha5.PodRequirements(pod)...).Allow(topologyGroup.Constraint.TopologyKey)) pod.Spec.NodeSelector = functional.UnionStringMaps(pod.Spec.NodeSelector, map[string]string{topologyGroup.Constraint.TopologyKey: domain}) } } @@ -99,8 +99,7 @@ func (t *Topology) computeHostnameTopology(topologyGroup *TopologyGroup, constra } topologyGroup.Register(domains...) // This is a bit of a hack that allows the constraints to recognize viable hostname topologies - constraints.Requirements = constraints.Requirements. - Merge(v1alpha5.NewRequirements(v1.NodeSelectorRequirement{Key: topologyGroup.Constraint.TopologyKey, Operator: v1.NodeSelectorOpIn, Values: domains})) + constraints.Requirements = constraints.Requirements.Add(v1.NodeSelectorRequirement{Key: topologyGroup.Constraint.TopologyKey, Operator: v1.NodeSelectorOpIn, Values: domains}) return nil } diff --git a/pkg/controllers/provisioning/scheduling/topologygroup.go b/pkg/controllers/provisioning/scheduling/topologygroup.go index 3a5608a1ac06..2aa6b4ee06b8 100644 --- a/pkg/controllers/provisioning/scheduling/topologygroup.go +++ b/pkg/controllers/provisioning/scheduling/topologygroup.go @@ -51,7 +51,7 @@ func (t *TopologyGroup) Increment(domain string) { } // NextDomain chooses a domain within the constraints that minimizes skew -func (t *TopologyGroup) NextDomain(requirement *sets.Set) string { +func (t *TopologyGroup) NextDomain(requirement sets.Set) string { minDomain := "" minCount := math.MaxInt32 for domain, count := range t.spread { diff --git a/pkg/controllers/selection/suite_test.go b/pkg/controllers/selection/suite_test.go index 30243bbcff12..ccfb6b6cc07a 100644 --- a/pkg/controllers/selection/suite_test.go +++ b/pkg/controllers/selection/suite_test.go @@ -47,7 +47,7 @@ var env *test.Environment func TestAPIs(t *testing.T) { ctx = TestContextWithLogger(t) RegisterFailHandler(Fail) - RunSpecs(t, "Controllers/Scheduling") + RunSpecs(t, "Controllers/Selection") } var _ = BeforeSuite(func() { diff --git a/pkg/utils/sets/sets.go b/pkg/utils/sets/sets.go index d1c0403b6c6c..739605926181 100644 --- a/pkg/utils/sets/sets.go +++ b/pkg/utils/sets/sets.go @@ -30,171 +30,76 @@ type Set struct { IsComplement bool `json:"allows,omitempty"` } -func (s *Set) DeepCopy() *Set { - return &Set{ - Members: sets.NewString(s.Members.UnsortedList()...), - IsComplement: s.IsComplement, +func NewSet(values ...string) Set { + return Set{ + Members: sets.NewString(values...), + IsComplement: false, } } -// Values returns the members of the set. -// If the set has an infinite size, returns an error -func (s *Set) Values() ([]string, error) { - if s.IsComplement { - return nil, fmt.Errorf("infinite set") +func NewComplementSet(values ...string) Set { + return Set{ + Members: sets.NewString(values...), + IsComplement: true, } - return s.Members.UnsortedList(), nil -} - -// RawValues returns the members of Members -// Do not use this to iterate the members of the set except for syntax validation -func (s *Set) RawValues() sets.String { - return s.Members } -func NewSet(isComplement bool, values ...string) *Set { - return &Set{ - Members: sets.NewString(values...), - IsComplement: isComplement, +// DeepCopy creates a deep copy of the set object +// It is required by the Kubernetes CRDs code generation +func (s Set) DeepCopy() Set { + members := s.Members.UnsortedList() + return Set{ + Members: sets.NewString(members...), + IsComplement: s.IsComplement, } } -// Insert inserts a value into the set. -func (s *Set) Insert(value string) *Set { +// Values returns the members of the set. +// If the set has an infinite size, returns an error +func (s Set) Values() ([]string, error) { if s.IsComplement { - s.Members.Delete(value) - } else { - s.Members.Insert(value) + return []string{}, fmt.Errorf("infinite set") } - return s + return s.Members.UnsortedList(), nil } -// Delete remove a value into the set. -func (s *Set) Delete(value string) *Set { +func (s Set) String() string { if s.IsComplement { - s.Members.Insert(value) - } else { - s.Members.Delete(value) + return fmt.Sprintf("%v' (complement set)", s.Members.UnsortedList()) } - return s + return fmt.Sprintf("%v", s.Members.UnsortedList()) + } // Has returns true if and only if item is contained in the set. -func (s *Set) Has(value string) bool { +func (s Set) Has(value string) bool { if s.IsComplement { return !s.Members.Has(value) } return s.Members.Has(value) } -// HasAll returns true if and only if all items are contained in the set. -func (s *Set) HasAll(values ...string) bool { - for _, value := range values { - if !s.Has(value) { - return false - } - } - return true -} - -// HasAny returns true if any items are contained in the set. -func (s *Set) HasAny(items ...string) bool { - for _, item := range items { - if s.Has(item) { - return true - } - } - return false -} - -// Difference returns a set of values that are not in provided set -func (s *Set) Difference(set *Set) *Set { - result := s.DeepCopy() - if s.IsComplement { - if set.IsComplement { - result.Members = set.Members.Difference(result.Members) - } else { - result.Members = result.Members.Union(set.Members) - } - } else { - if set.IsComplement { - result.Members = result.Members.Intersection(set.Members) - } else { - result.Members = result.Members.Difference(set.Members) - } - } - return result -} - -// Union returns a set of values that are in either sets -func (s *Set) Union(set *Set) *Set { - result := s.DeepCopy() - if s.IsComplement { - if set.IsComplement { - result.Members = result.Members.Intersection(set.Members) - } else { - result.Members = result.Members.Difference(set.Members) - } - } else { - if set.IsComplement { - result.Members = set.Members.Difference(result.Members) - result.IsComplement = true - } else { - result.Members = result.Members.Union(set.Members) - } - } - return result -} - // Intersection returns a new set containing the common values -func (s *Set) Intersection(set *Set) *Set { - result := s.DeepCopy() +func (s Set) Intersection(set Set) Set { if s.IsComplement { if set.IsComplement { - result.Members = result.Members.Union(set.Members) + s.Members = s.Members.Union(set.Members) } else { - result.Members = set.Members.Difference(result.Members) - result.IsComplement = false + s.Members = set.Members.Difference(s.Members) + s.IsComplement = false } } else { if set.IsComplement { - result.Members = result.Members.Difference(set.Members) + s.Members = s.Members.Difference(set.Members) } else { - result.Members = result.Members.Intersection(set.Members) - } - } - return result -} - -// Equal returns true if and only if two sets are equal (as a set). -// Two sets are equal if their membership is identical. -// (In practice, this means same elements, order doesn't matter) -func (s *Set) Equal(set *Set) bool { - // if isComplement do not agree, one set is finite and the other is infnite. - if len(s.Members) != len(set.Members) || (set.IsComplement && !s.IsComplement) || (!set.IsComplement && s.IsComplement) { - return false - } - for item := range set.Members { - if !s.Has(item) { - return false + s.Members = s.Members.Intersection(set.Members) } } - return true -} - -// Need this to perform schedule calcualtion later -// Intersection returns a new set containing the common values -/* -func (s *Set) isSuperset(set *Set) bool { - return s.Intersection(set).Equal(set) + return s } -*/ // Len returns the size of the set. -func (s *Set) Len() int { - if s == nil { - return 0 - } +func (s Set) Len() int { if s.IsComplement { return math.MaxInt64 - s.Members.Len() } diff --git a/pkg/utils/sets/suite_test.go b/pkg/utils/sets/suite_test.go index db190a9337a4..317830b41b6a 100644 --- a/pkg/utils/sets/suite_test.go +++ b/pkg/utils/sets/suite_test.go @@ -30,14 +30,14 @@ func TestSets(t *testing.T) { var _ = Describe("Set", func() { - var fullSet, emptySet, setA, setAComplement, setB, setAB *sets.Set + var fullSet, emptySet, setA, setAComplement, setB, setAB sets.Set BeforeEach(func() { - fullSet = sets.NewSet(true) - emptySet = sets.NewSet(false) - setA = sets.NewSet(false, "A") - setAComplement = sets.NewSet(true, "A") - setB = sets.NewSet(false, "B") - setAB = sets.NewSet(false, "A", "B") + fullSet = sets.NewComplementSet() + emptySet = sets.NewSet() + setA = sets.NewSet("A") + setAComplement = sets.NewComplementSet("A") + setB = sets.NewSet("B") + setAB = sets.NewSet("A", "B") }) Context("Intersection", func() { It("A should not intersect with B", func() { @@ -57,58 +57,8 @@ var _ = Describe("Set", func() { }) }) - Context("Different", func() { - It("A differece B should be A", func() { - Expect(setA.Difference(setB)).To(Equal(setA)) - }) - It("A difference with its own complement should be A", func() { - Expect(setA.Difference(setAComplement)).To(Equal(setA)) - }) - It("AB difference B should be A", func() { - Expect(setAB.Difference(setB)).To(Equal(setA)) - }) - It("A difference with full set should return empty", func() { - Expect(setA.Difference(fullSet)).To(Equal(emptySet)) - }) - It("A difference with empty set should return itself", func() { - Expect(setA.Difference(emptySet)).To(Equal(setA)) - }) - }) - - Context("Union", func() { - It("A union B should be AB", func() { - Expect(setA.Union(setB)).To(Equal(setAB)) - }) - It("A union with its own complement should return full set", func() { - Expect(setA.Union(setAComplement)).To(Equal(fullSet)) - }) - It("AB union with B should return AB", func() { - Expect(setAB.Union(setB)).To(Equal(setAB)) - }) - It("A union with full set should return fullSet", func() { - Expect(setA.Union(fullSet)).To(Equal(fullSet)) - }) - It("A union with empty set should return A", func() { - Expect(setA.Union(emptySet)).To(Equal(setA)) - }) - }) Context("Functional Correctness", func() { - It("A should equal A", func() { - Expect(setA.Equal(setA)).To(BeTrue()) - }) - It("A should not equal B", func() { - Expect(setA.Equal(setB)).To(BeFalse()) - }) - It("A should not equal AB", func() { - Expect(setA.Equal(setAB)).To(BeFalse()) - }) - It("A should not equal A complement", func() { - Expect(setA.Equal(setAComplement)).To(BeFalse()) - }) - It("size of A should be 1", func() { - Expect(setA.Len()).To(Equal(1)) - }) It("size of AB should be 2", func() { Expect(setAB.Len()).To(Equal(2)) }) @@ -118,46 +68,19 @@ var _ = Describe("Set", func() { It("size of full set should be MAX", func() { Expect(fullSet.Len()).To(Equal(math.MaxInt64)) }) - It("A should have A", func() { - Expect(setA.Has("A")).To(BeTrue()) - }) It("A complement should not have A", func() { Expect(setAComplement.Has("A")).To(BeFalse()) }) It("A should not have B", func() { Expect(setA.Has("B")).To(BeFalse()) }) - It("A should have either A or B", func() { - Expect(setA.HasAny("A", "B")).To(BeTrue()) - }) - It("A complement should have either A or B", func() { - Expect(setAComplement.HasAny("A", "B")).To(BeTrue()) - }) - It("A should not have either C or D", func() { - Expect(setA.HasAny("C", "D")).To(BeFalse()) - }) - It("A should not have both A and B", func() { - Expect(setA.HasAll("A", "B")).To(BeFalse()) - }) - It("A complement should not have both A and B", func() { - Expect(setAComplement.HasAll("A", "B")).To(BeFalse()) - }) - It("AB should have both A and B", func() { - Expect(setAB.HasAll("A", "B")).To(BeTrue()) - }) - It("A Insert B should return AB", func() { - Expect(setA.Insert("B")).To(Equal(setAB)) - }) - It("A complement Insert A should return full set", func() { - Expect(setAComplement.Insert("A")).To(Equal(fullSet)) - }) It("A should be able to call values", func() { _, err := setA.Values() Expect(err).To(Succeed()) }) - It("A should have values A", func() { - values, _ := setA.Values() - Expect(values).To(Equal([]string{"A"})) + It("A' should fail to call values", func() { + _, err := setAComplement.Values() + Expect(err).ToNot(Succeed()) }) }) })