From bc2273f3ef1c0f17344811c1e60a7f51af4f1dcd Mon Sep 17 00:00:00 2001 From: Ellis Tarn Date: Thu, 31 Mar 2022 14:32:00 -0700 Subject: [PATCH] Decoupled Constraints API object from internal representation (#1603) --- .../v1alpha5/provisioner_defaults.go | 7 +-- .../v1alpha5/provisioner_validation.go | 24 ++++---- pkg/apis/provisioning/v1alpha5/register.go | 4 +- .../provisioning/v1alpha5/requirements.go | 4 -- pkg/apis/provisioning/v1alpha5/suite_test.go | 7 --- pkg/cloudprovider/aws/amifamily/resolver.go | 12 ++-- .../aws/apis/v1alpha1/provider.go | 24 +++----- .../aws/apis/v1alpha1/provider_defaults.go | 57 ------------------- .../aws/apis/v1alpha1/register.go | 3 +- .../apis/v1alpha1/zz_generated.deepcopy.go | 26 --------- pkg/cloudprovider/aws/cloudprovider.go | 46 +++++++++------ pkg/cloudprovider/aws/instance.go | 36 ++++++------ pkg/cloudprovider/aws/launchtemplate.go | 22 +++---- pkg/cloudprovider/aws/securitygroups.go | 8 +-- pkg/cloudprovider/aws/subnets.go | 10 ++-- pkg/cloudprovider/aws/suite_test.go | 9 ++- pkg/cloudprovider/fake/cloudprovider.go | 6 +- pkg/cloudprovider/metrics/cloudprovider.go | 8 +-- pkg/cloudprovider/types.go | 14 ++++- pkg/controllers/provisioning/controller.go | 2 +- pkg/controllers/provisioning/provisioner.go | 15 +++-- 21 files changed, 134 insertions(+), 210 deletions(-) delete mode 100644 pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go diff --git a/pkg/apis/provisioning/v1alpha5/provisioner_defaults.go b/pkg/apis/provisioning/v1alpha5/provisioner_defaults.go index 6ebc90c717ae..e12262c72e8b 100644 --- a/pkg/apis/provisioning/v1alpha5/provisioner_defaults.go +++ b/pkg/apis/provisioning/v1alpha5/provisioner_defaults.go @@ -20,10 +20,5 @@ import ( // SetDefaults for the provisioner func (p *Provisioner) SetDefaults(ctx context.Context) { - p.Spec.Constraints.Default(ctx) -} - -// Default the constraints -func (c *Constraints) Default(ctx context.Context) { - DefaultHook(ctx, c) + DefaultHook(ctx, p) } diff --git a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go index 35486abc4692..38a6001a4ae8 100644 --- a/pkg/apis/provisioning/v1alpha5/provisioner_validation.go +++ b/pkg/apis/provisioning/v1alpha5/provisioner_validation.go @@ -35,6 +35,7 @@ func (p *Provisioner) Validate(ctx context.Context) (errs *apis.FieldError) { return errs.Also( apis.ValidateObjectMetadata(p).ViaField("metadata"), p.Spec.validate(ctx).ViaField("spec"), + ValidateHook(ctx, p), ) } @@ -61,17 +62,16 @@ func (s *ProvisionerSpec) validateTTLSecondsAfterEmpty() (errs *apis.FieldError) } // Validate the constraints -func (c *Constraints) Validate(ctx context.Context) (errs *apis.FieldError) { +func (s *ProvisionerSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs.Also( - c.validateLabels(), - c.validateTaints(), - c.validateRequirements(), - ValidateHook(ctx, c), + s.validateLabels(), + s.validateTaints(), + s.validateRequirements(), ) } -func (c *Constraints) validateLabels() (errs *apis.FieldError) { - for key, value := range c.Labels { +func (s *ProvisionerSpec) validateLabels() (errs *apis.FieldError) { + for key, value := range s.Labels { for _, err := range validation.IsQualifiedName(key) { errs = errs.Also(apis.ErrInvalidKeyName(key, "labels", err)) } @@ -85,8 +85,8 @@ func (c *Constraints) validateLabels() (errs *apis.FieldError) { return errs } -func (c *Constraints) validateTaints() (errs *apis.FieldError) { - for i, taint := range c.Taints { +func (s *ProvisionerSpec) validateTaints() (errs *apis.FieldError) { + for i, taint := range s.Taints { // Validate Key if len(taint.Key) == 0 { errs = errs.Also(apis.ErrInvalidArrayValue(errs, "taints", i)) @@ -113,9 +113,9 @@ func (c *Constraints) validateTaints() (errs *apis.FieldError) { // 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 requirements only support well known labels. -func (c *Constraints) validateRequirements() (errs *apis.FieldError) { +func (s *ProvisionerSpec) validateRequirements() (errs *apis.FieldError) { var err error - for _, requirement := range c.Requirements.Requirements { + for _, requirement := range s.Requirements.Requirements { // Ensure requirements operator is allowed if !SupportedProvisionerOps.Has(string(requirement.Operator)) { err = multierr.Append(err, fmt.Errorf("key %s has an unsupported operator %s, provisioner only supports %s", requirement.Key, requirement.Operator, SupportedProvisionerOps.UnsortedList())) @@ -124,7 +124,7 @@ func (c *Constraints) validateRequirements() (errs *apis.FieldError) { err = multierr.Append(err, e) } } - err = multierr.Append(err, c.Requirements.Validate()) + err = multierr.Append(err, s.Requirements.Validate()) if err != nil { errs = errs.Also(apis.ErrInvalidValue(err, "requirements")) } diff --git a/pkg/apis/provisioning/v1alpha5/register.go b/pkg/apis/provisioning/v1alpha5/register.go index 64cb1d3e4cc1..6281a4835a0f 100644 --- a/pkg/apis/provisioning/v1alpha5/register.go +++ b/pkg/apis/provisioning/v1alpha5/register.go @@ -24,8 +24,8 @@ import ( ) var ( - DefaultHook = func(ctx context.Context, constraints *Constraints) {} - ValidateHook = func(ctx context.Context, constraints *Constraints) *apis.FieldError { return nil } + DefaultHook = func(ctx context.Context, provisoner *Provisioner) {} + ValidateHook = func(ctx context.Context, provisoner *Provisioner) *apis.FieldError { return nil } ) var ( diff --git a/pkg/apis/provisioning/v1alpha5/requirements.go b/pkg/apis/provisioning/v1alpha5/requirements.go index 0f1b5d9b401b..eb4a7baf1045 100644 --- a/pkg/apis/provisioning/v1alpha5/requirements.go +++ b/pkg/apis/provisioning/v1alpha5/requirements.go @@ -163,10 +163,6 @@ func (r Requirements) Validate() (errs error) { if !SupportedNodeSelectorOps.Has(string(requirement.Operator)) { errs = multierr.Append(errs, fmt.Errorf("operator %s not in %s for key %s", requirement.Operator, SupportedNodeSelectorOps.UnsortedList(), requirement.Key)) } - // Combined requirements must have some possible value unless Operator=DoesNotExist. - if values := r.Get(requirement.Key); values.Len() == 0 && requirement.Operator != v1.NodeSelectorOpDoesNotExist { - errs = multierr.Append(errs, fmt.Errorf("no feasible value for key %s", requirement.Key)) - } } return errs } diff --git a/pkg/apis/provisioning/v1alpha5/suite_test.go b/pkg/apis/provisioning/v1alpha5/suite_test.go index 1de7e0787277..ad644281881b 100644 --- a/pkg/apis/provisioning/v1alpha5/suite_test.go +++ b/pkg/apis/provisioning/v1alpha5/suite_test.go @@ -185,13 +185,6 @@ var _ = Describe("Validation", func() { Expect(provisioner.Validate(ctx)).To(Succeed()) } }) - It("should fail because no feasible value", func() { - provisioner.Spec.Requirements = NewRequirements( - v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}, - v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"bar"}}, - ) - Expect(provisioner.Validate(ctx)).ToNot(Succeed()) - }) It("should allow non-empty set after removing overlapped value", func() { provisioner.Spec.Requirements = NewRequirements( v1.NodeSelectorRequirement{Key: v1.LabelTopologyZone, Operator: v1.NodeSelectorOpIn, Values: []string{"test", "foo"}}, diff --git a/pkg/cloudprovider/aws/amifamily/resolver.go b/pkg/cloudprovider/aws/amifamily/resolver.go index 89be78e63cf7..1a992e6cdc8f 100644 --- a/pkg/cloudprovider/aws/amifamily/resolver.go +++ b/pkg/cloudprovider/aws/amifamily/resolver.go @@ -85,10 +85,10 @@ func New(ssm ssmiface.SSMAPI, c *cache.Cache) *Resolver { // Resolve generates launch templates using the static options and dynamically generates launch template parameters. // Multiple ResolvedTemplates are returned based on the instanceTypes passed in to support special AMIs for certain instance types like GPUs. -func (r Resolver) Resolve(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType, options *Options) ([]*LaunchTemplate, error) { - amiFamily := r.getAMIFamily(constraints.AMIFamily, options) +func (r Resolver) Resolve(ctx context.Context, provider *v1alpha1.AWS, nodeRequest *cloudprovider.NodeRequest, options *Options) ([]*LaunchTemplate, error) { + amiFamily := r.getAMIFamily(provider.AMIFamily, options) amiIDs := map[string][]cloudprovider.InstanceType{} - for _, instanceType := range instanceTypes { + for _, instanceType := range nodeRequest.InstanceTypeOptions { amiID, err := r.amiProvider.Get(ctx, instanceType, amiFamily.SSMAlias(options.KubernetesVersion, instanceType)) if err != nil { return nil, err @@ -99,9 +99,9 @@ func (r Resolver) Resolve(ctx context.Context, constraints *v1alpha1.Constraints for amiID, instanceTypes := range amiIDs { resolved := &LaunchTemplate{ Options: options, - UserData: amiFamily.UserData(constraints.KubeletConfiguration, constraints.Taints, options.Labels, options.CABundle, instanceTypes), - BlockDeviceMappings: constraints.BlockDeviceMappings, - MetadataOptions: constraints.MetadataOptions, + UserData: amiFamily.UserData(nodeRequest.Template.KubeletConfiguration, nodeRequest.Template.Taints, options.Labels, options.CABundle, instanceTypes), + BlockDeviceMappings: provider.BlockDeviceMappings, + MetadataOptions: provider.MetadataOptions, AMIID: amiID, InstanceTypes: instanceTypes, } diff --git a/pkg/cloudprovider/aws/apis/v1alpha1/provider.go b/pkg/cloudprovider/aws/apis/v1alpha1/provider.go index 6b673032c15f..b25c9e764415 100644 --- a/pkg/cloudprovider/aws/apis/v1alpha1/provider.go +++ b/pkg/cloudprovider/aws/apis/v1alpha1/provider.go @@ -24,12 +24,6 @@ import ( "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" ) -// Constraints wraps generic constraints with AWS specific parameters -type Constraints struct { - *v1alpha5.Constraints - *AWS -} - // AWS contains parameters specific to this cloud provider // +kubebuilder:object:root=true type AWS struct { @@ -192,29 +186,29 @@ type BlockDevice struct { VolumeType *string `json:"volumeType,omitempty"` } -func Deserialize(constraints *v1alpha5.Constraints) (*Constraints, error) { - if constraints.Provider == nil { +func Deserialize(provider *v1alpha5.Provider) (*AWS, error) { + if provider == nil { return nil, fmt.Errorf("invariant violated: spec.provider is not defined. Is the defaulting webhook installed?") } - aws := &AWS{} - _, gvk, err := Codec.UniversalDeserializer().Decode(constraints.Provider.Raw, nil, aws) + a := &AWS{} + _, gvk, err := Codec.UniversalDeserializer().Decode(provider.Raw, nil, a) if err != nil { return nil, err } if gvk != nil { - aws.SetGroupVersionKind(*gvk) + a.SetGroupVersionKind(*gvk) } - return &Constraints{constraints, aws}, nil + return a, nil } -func (a *AWS) Serialize(constraints *v1alpha5.Constraints) error { - if constraints.Provider == nil { +func (a *AWS) Serialize(provider *v1alpha5.Provider) error { + if provider == nil { return fmt.Errorf("invariant violated: spec.provider is not defined. Is the defaulting webhook installed?") } bytes, err := json.Marshal(a) if err != nil { return err } - constraints.Provider.Raw = bytes + provider.Raw = bytes return nil } diff --git a/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go b/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go deleted file mode 100644 index 5e79996195d9..000000000000 --- a/pkg/cloudprovider/aws/apis/v1alpha1/provider_defaults.go +++ /dev/null @@ -1,57 +0,0 @@ -/* -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package v1alpha1 - -import ( - "context" - - v1 "k8s.io/api/core/v1" - - "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" -) - -// Default the constraints. -func (c *Constraints) Default(ctx context.Context) { - c.defaultArchitecture() - c.defaultCapacityTypes() -} - -func (c *Constraints) defaultCapacityTypes() { - if _, ok := c.Labels[v1alpha5.LabelCapacityType]; ok { - return - } - if c.Requirements.Keys().Has(v1alpha5.LabelCapacityType) { - return - } - c.Requirements = c.Requirements.Add(v1.NodeSelectorRequirement{ - Key: v1alpha5.LabelCapacityType, - Operator: v1.NodeSelectorOpIn, - Values: []string{CapacityTypeOnDemand}, - }) -} - -func (c *Constraints) defaultArchitecture() { - if _, ok := c.Labels[v1.LabelArchStable]; ok { - return - } - if c.Requirements.Keys().Has(v1.LabelArchStable) { - return - } - c.Requirements = c.Requirements.Add(v1.NodeSelectorRequirement{ - Key: v1.LabelArchStable, - Operator: v1.NodeSelectorOpIn, - Values: []string{v1alpha5.ArchitectureAmd64}, - }) -} diff --git a/pkg/cloudprovider/aws/apis/v1alpha1/register.go b/pkg/cloudprovider/aws/apis/v1alpha1/register.go index 3a0781ee8264..1757ad4652c0 100644 --- a/pkg/cloudprovider/aws/apis/v1alpha1/register.go +++ b/pkg/cloudprovider/aws/apis/v1alpha1/register.go @@ -15,12 +15,13 @@ limitations under the License. package v1alpha1 import ( - "github.com/aws/aws-sdk-go/service/ec2" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" ) diff --git a/pkg/cloudprovider/aws/apis/v1alpha1/zz_generated.deepcopy.go b/pkg/cloudprovider/aws/apis/v1alpha1/zz_generated.deepcopy.go index cea4f57c6232..f657e6abecb7 100644 --- a/pkg/cloudprovider/aws/apis/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/cloudprovider/aws/apis/v1alpha1/zz_generated.deepcopy.go @@ -20,7 +20,6 @@ limitations under the License. package v1alpha1 import ( - "github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5" "k8s.io/apimachinery/pkg/runtime" ) @@ -160,31 +159,6 @@ func (in *BlockDeviceMapping) DeepCopy() *BlockDeviceMapping { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Constraints) DeepCopyInto(out *Constraints) { - *out = *in - if in.Constraints != nil { - in, out := &in.Constraints, &out.Constraints - *out = new(v1alpha5.Constraints) - (*in).DeepCopyInto(*out) - } - if in.AWS != nil { - in, out := &in.AWS, &out.AWS - *out = new(AWS) - (*in).DeepCopyInto(*out) - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Constraints. -func (in *Constraints) DeepCopy() *Constraints { - if in == nil { - return nil - } - out := new(Constraints) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LaunchTemplate) DeepCopyInto(out *LaunchTemplate) { *out = *in diff --git a/pkg/cloudprovider/aws/cloudprovider.go b/pkg/cloudprovider/aws/cloudprovider.go index ceec7b299c89..954b99141326 100644 --- a/pkg/cloudprovider/aws/cloudprovider.go +++ b/pkg/cloudprovider/aws/cloudprovider.go @@ -101,20 +101,20 @@ func NewCloudProvider(ctx context.Context, options cloudprovider.Options) *Cloud // Create a node given the constraints. func (c *CloudProvider) Create(ctx context.Context, nodeRequest *cloudprovider.NodeRequest) (*v1.Node, error) { - vendorConstraints, err := v1alpha1.Deserialize(nodeRequest.Constraints) + vendorConstraints, err := v1alpha1.Deserialize(nodeRequest.Template.Provider) if err != nil { return nil, err } - return c.instanceProvider.Create(ctx, vendorConstraints, nodeRequest.InstanceTypeOptions) + return c.instanceProvider.Create(ctx, vendorConstraints, nodeRequest) } // GetInstanceTypes returns all available InstanceTypes despite accepting a Constraints struct (note that it does not utilize Requirements) func (c *CloudProvider) GetInstanceTypes(ctx context.Context, provider *v1alpha5.Provider) ([]cloudprovider.InstanceType, error) { - vendorConstraints, err := v1alpha1.Deserialize(&v1alpha5.Constraints{Provider: provider}) + aws, err := v1alpha1.Deserialize(provider) if err != nil { return nil, apis.ErrGeneric(err.Error()) } - return c.instanceTypeProvider.Get(ctx, vendorConstraints.AWS) + return c.instanceTypeProvider.Get(ctx, aws) } func (c *CloudProvider) Delete(ctx context.Context, node *v1.Node) error { @@ -122,24 +122,38 @@ func (c *CloudProvider) Delete(ctx context.Context, node *v1.Node) error { } // Validate the provisioner -func (c *CloudProvider) Validate(ctx context.Context, constraints *v1alpha5.Constraints) *apis.FieldError { - vendorConstraints, err := v1alpha1.Deserialize(constraints) +func (c *CloudProvider) Validate(ctx context.Context, provisioner *v1alpha5.Provisioner) *apis.FieldError { + provider, err := v1alpha1.Deserialize(provisioner.Spec.Provider) if err != nil { return apis.ErrGeneric(err.Error()) } - return vendorConstraints.AWS.Validate() + return provider.Validate() } // Default the provisioner -func (c *CloudProvider) Default(ctx context.Context, constraints *v1alpha5.Constraints) { - vendorConstraints, err := v1alpha1.Deserialize(constraints) - if err != nil { - logging.FromContext(ctx).Errorf("Failed to deserialize provider, %s", err) - return - } - vendorConstraints.Default(ctx) - if err := vendorConstraints.Serialize(constraints); err != nil { - logging.FromContext(ctx).Errorf("Failed to serialize provider, %s", err) +func (c *CloudProvider) Default(ctx context.Context, provisioner *v1alpha5.Provisioner) { + defaultLabels(provisioner) +} + +func defaultLabels(provisioner *v1alpha5.Provisioner) { + for key, value := range map[string]string{ + v1alpha5.LabelCapacityType: ec2.DefaultTargetCapacityTypeOnDemand, + v1.LabelArchStable: v1alpha5.ArchitectureAmd64, + } { + hasLabel := false + if _, ok := provisioner.Spec.Labels[key]; ok { + hasLabel = true + } + for _, requirement := range provisioner.Spec.Requirements.Requirements { + if requirement.Key == key { + hasLabel = true + } + } + if !hasLabel { + provisioner.Spec.Requirements.Requirements = append(provisioner.Spec.Requirements.Requirements, v1.NodeSelectorRequirement{ + Key: key, Operator: v1.NodeSelectorOpIn, Values: []string{value}, + }) + } } } diff --git a/pkg/cloudprovider/aws/instance.go b/pkg/cloudprovider/aws/instance.go index d59c69be1897..42ad9fb234af 100644 --- a/pkg/cloudprovider/aws/instance.go +++ b/pkg/cloudprovider/aws/instance.go @@ -69,13 +69,13 @@ func NewInstanceProvider(ec2api ec2iface.EC2API, instanceTypeProvider *InstanceT // instanceTypes should be sorted by priority for spot capacity type. // If spot is not used, the instanceTypes are not required to be sorted // because we are using ec2 fleet's lowest-price OD allocation strategy -func (p *InstanceProvider) Create(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType) (*v1.Node, error) { - instanceTypes = p.filterInstanceTypes(instanceTypes) - if len(instanceTypes) > MaxInstanceTypes { - instanceTypes = instanceTypes[0:MaxInstanceTypes] +func (p *InstanceProvider) Create(ctx context.Context, provider *v1alpha1.AWS, nodeRequest *cloudprovider.NodeRequest) (*v1.Node, error) { + nodeRequest.InstanceTypeOptions = p.filterInstanceTypes(nodeRequest.InstanceTypeOptions) + if len(nodeRequest.InstanceTypeOptions) > MaxInstanceTypes { + nodeRequest.InstanceTypeOptions = nodeRequest.InstanceTypeOptions[0:MaxInstanceTypes] } - id, err := p.launchInstance(ctx, constraints, instanceTypes) + id, err := p.launchInstance(ctx, provider, nodeRequest) if err != nil { return nil, err } @@ -98,7 +98,7 @@ func (p *InstanceProvider) Create(ctx context.Context, constraints *v1alpha1.Con getCapacityType(instance), ) // Convert Instance to Node - return p.instanceToNode(ctx, instance, instanceTypes), nil + return p.instanceToNode(ctx, instance, nodeRequest.InstanceTypeOptions), nil } func (p *InstanceProvider) Terminate(ctx context.Context, node *v1.Node) error { @@ -117,15 +117,15 @@ func (p *InstanceProvider) Terminate(ctx context.Context, node *v1.Node) error { return nil } -func (p *InstanceProvider) launchInstance(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType) (*string, error) { - capacityType := p.getCapacityType(constraints, instanceTypes) +func (p *InstanceProvider) launchInstance(ctx context.Context, provider *v1alpha1.AWS, nodeRequest *cloudprovider.NodeRequest) (*string, error) { + capacityType := p.getCapacityType(nodeRequest) // Get Launch Template Configs, which may differ due to GPU or Architecture requirements - launchTemplateConfigs, err := p.getLaunchTemplateConfigs(ctx, constraints, instanceTypes, capacityType) + launchTemplateConfigs, err := p.getLaunchTemplateConfigs(ctx, provider, nodeRequest, capacityType) if err != nil { return nil, fmt.Errorf("getting launch template configs, %w", err) } // Create fleet - tags := v1alpha1.MergeTags(ctx, constraints.Tags, map[string]string{fmt.Sprintf("kubernetes.io/cluster/%s", injection.GetOptions(ctx).ClusterName): "owned"}) + tags := v1alpha1.MergeTags(ctx, provider.Tags, map[string]string{fmt.Sprintf("kubernetes.io/cluster/%s", injection.GetOptions(ctx).ClusterName): "owned"}) createFleetInput := &ec2.CreateFleetInput{ Type: aws.String(ec2.FleetTypeInstant), LaunchTemplateConfigs: launchTemplateConfigs, @@ -154,20 +154,20 @@ func (p *InstanceProvider) launchInstance(ctx context.Context, constraints *v1al return createFleetOutput.Instances[0].InstanceIds[0], nil } -func (p *InstanceProvider) getLaunchTemplateConfigs(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType, capacityType string) ([]*ec2.FleetLaunchTemplateConfigRequest, error) { +func (p *InstanceProvider) getLaunchTemplateConfigs(ctx context.Context, provider *v1alpha1.AWS, nodeRequest *cloudprovider.NodeRequest, capacityType string) ([]*ec2.FleetLaunchTemplateConfigRequest, error) { // Get subnets given the constraints - subnets, err := p.subnetProvider.Get(ctx, constraints.AWS) + subnets, err := p.subnetProvider.Get(ctx, provider) if err != nil { return nil, fmt.Errorf("getting subnets, %w", err) } var launchTemplateConfigs []*ec2.FleetLaunchTemplateConfigRequest - launchTemplates, err := p.launchTemplateProvider.Get(ctx, constraints, instanceTypes, map[string]string{v1alpha5.LabelCapacityType: capacityType}) + launchTemplates, err := p.launchTemplateProvider.Get(ctx, provider, nodeRequest, map[string]string{v1alpha5.LabelCapacityType: capacityType}) if err != nil { return nil, fmt.Errorf("getting launch templates, %w", err) } for launchTemplateName, instanceTypes := range launchTemplates { launchTemplateConfig := &ec2.FleetLaunchTemplateConfigRequest{ - Overrides: p.getOverrides(instanceTypes, subnets, constraints.Requirements.Zones(), capacityType), + Overrides: p.getOverrides(instanceTypes, subnets, nodeRequest.Template.Requirements.Zones(), capacityType), LaunchTemplateSpecification: &ec2.FleetLaunchTemplateSpecificationRequest{ LaunchTemplateName: aws.String(launchTemplateName), Version: aws.String("$Latest"), @@ -308,11 +308,11 @@ func (p *InstanceProvider) updateUnavailableOfferingsCache(ctx context.Context, // getCapacityType selects spot if both constraints are flexible and there is an // available offering. The AWS Cloud Provider defaults to [ on-demand ], so spot // must be explicitly included in capacity type requirements. -func (p *InstanceProvider) getCapacityType(constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType) string { - if constraints.Requirements.CapacityTypes().Has(v1alpha1.CapacityTypeSpot) { - for _, instanceType := range instanceTypes { +func (p *InstanceProvider) getCapacityType(nodeRequest *cloudprovider.NodeRequest) string { + if nodeRequest.Template.Requirements.CapacityTypes().Has(v1alpha1.CapacityTypeSpot) { + for _, instanceType := range nodeRequest.InstanceTypeOptions { for _, offering := range instanceType.Offerings() { - if constraints.Requirements.Zones().Has(offering.Zone) && offering.CapacityType == v1alpha1.CapacityTypeSpot { + if nodeRequest.Template.Requirements.Zones().Has(offering.Zone) && offering.CapacityType == v1alpha1.CapacityTypeSpot { return v1alpha1.CapacityTypeSpot } } diff --git a/pkg/cloudprovider/aws/launchtemplate.go b/pkg/cloudprovider/aws/launchtemplate.go index 900120719abc..9781025f6500 100644 --- a/pkg/cloudprovider/aws/launchtemplate.go +++ b/pkg/cloudprovider/aws/launchtemplate.go @@ -79,19 +79,19 @@ func launchTemplateName(options *amifamily.LaunchTemplate) string { return fmt.Sprintf(launchTemplateNameFormat, options.ClusterName, fmt.Sprint(hash)) } -func (p *LaunchTemplateProvider) Get(ctx context.Context, constraints *v1alpha1.Constraints, instanceTypes []cloudprovider.InstanceType, additionalLabels map[string]string) (map[string][]cloudprovider.InstanceType, error) { +func (p *LaunchTemplateProvider) Get(ctx context.Context, provider *v1alpha1.AWS, nodeRequest *cloudprovider.NodeRequest, additionalLabels map[string]string) (map[string][]cloudprovider.InstanceType, error) { p.Lock() defer p.Unlock() // If Launch Template is directly specified then just use it - if constraints.LaunchTemplateName != nil { - return map[string][]cloudprovider.InstanceType{ptr.StringValue(constraints.LaunchTemplateName): instanceTypes}, nil + if provider.LaunchTemplateName != nil { + return map[string][]cloudprovider.InstanceType{ptr.StringValue(provider.LaunchTemplateName): nodeRequest.InstanceTypeOptions}, nil } - instanceProfile, err := p.getInstanceProfile(ctx, constraints) + instanceProfile, err := p.getInstanceProfile(ctx, provider) if err != nil { return nil, err } // Get constrained security groups - securityGroupsIDs, err := p.securityGroupProvider.Get(ctx, constraints) + securityGroupsIDs, err := p.securityGroupProvider.Get(ctx, provider) if err != nil { return nil, err } @@ -99,14 +99,14 @@ func (p *LaunchTemplateProvider) Get(ctx context.Context, constraints *v1alpha1. if err != nil { return nil, err } - resolvedLaunchTemplates, err := p.amiFamily.Resolve(ctx, constraints, instanceTypes, &amifamily.Options{ + resolvedLaunchTemplates, err := p.amiFamily.Resolve(ctx, provider, nodeRequest, &amifamily.Options{ ClusterName: injection.GetOptions(ctx).ClusterName, ClusterEndpoint: injection.GetOptions(ctx).ClusterEndpoint, AWSENILimitedPodDensity: injection.GetOptions(ctx).AWSENILimitedPodDensity, InstanceProfile: instanceProfile, SecurityGroupsIDs: securityGroupsIDs, - Tags: constraints.Tags, - Labels: functional.UnionStringMaps(constraints.Labels, additionalLabels), + Tags: provider.Tags, + Labels: functional.UnionStringMaps(nodeRequest.Template.Labels, additionalLabels), CABundle: p.caBundle, KubernetesVersion: kubeServerVersion, }) @@ -248,9 +248,9 @@ func (p *LaunchTemplateProvider) onCacheEvicted(key string, lt interface{}) { p.logger.Debugf("Deleted launch template %v", aws.StringValue(launchTemplate.LaunchTemplateId)) } -func (p *LaunchTemplateProvider) getInstanceProfile(ctx context.Context, constraints *v1alpha1.Constraints) (string, error) { - if constraints.InstanceProfile != nil { - return aws.StringValue(constraints.InstanceProfile), nil +func (p *LaunchTemplateProvider) getInstanceProfile(ctx context.Context, provider *v1alpha1.AWS) (string, error) { + if provider.InstanceProfile != nil { + return aws.StringValue(provider.InstanceProfile), nil } defaultProfile := injection.GetOptions(ctx).AWSDefaultInstanceProfile if defaultProfile == "" { diff --git a/pkg/cloudprovider/aws/securitygroups.go b/pkg/cloudprovider/aws/securitygroups.go index 8e8c6f9d119b..9c514446d0dc 100644 --- a/pkg/cloudprovider/aws/securitygroups.go +++ b/pkg/cloudprovider/aws/securitygroups.go @@ -42,11 +42,11 @@ func NewSecurityGroupProvider(ec2api ec2iface.EC2API) *SecurityGroupProvider { } } -func (p *SecurityGroupProvider) Get(ctx context.Context, constraints *v1alpha1.Constraints) ([]string, error) { +func (p *SecurityGroupProvider) Get(ctx context.Context, provider *v1alpha1.AWS) ([]string, error) { p.Lock() defer p.Unlock() // Get SecurityGroups - securityGroups, err := p.getSecurityGroups(ctx, p.getFilters(constraints)) + securityGroups, err := p.getSecurityGroups(ctx, p.getFilters(provider)) if err != nil { return nil, err } @@ -62,9 +62,9 @@ func (p *SecurityGroupProvider) Get(ctx context.Context, constraints *v1alpha1.C return securityGroupIds, nil } -func (p *SecurityGroupProvider) getFilters(constraints *v1alpha1.Constraints) []*ec2.Filter { +func (p *SecurityGroupProvider) getFilters(provider *v1alpha1.AWS) []*ec2.Filter { filters := []*ec2.Filter{} - for key, value := range constraints.SecurityGroupSelector { + for key, value := range provider.SecurityGroupSelector { filters = append(filters, &ec2.Filter{ Name: aws.String(fmt.Sprintf("tag:%s", key)), Values: []*string{aws.String(value)}, diff --git a/pkg/cloudprovider/aws/subnets.go b/pkg/cloudprovider/aws/subnets.go index 15a3d45e37b2..b70b5126ec52 100644 --- a/pkg/cloudprovider/aws/subnets.go +++ b/pkg/cloudprovider/aws/subnets.go @@ -43,10 +43,10 @@ func NewSubnetProvider(ec2api ec2iface.EC2API) *SubnetProvider { } } -func (p *SubnetProvider) Get(ctx context.Context, constraints *v1alpha1.AWS) ([]*ec2.Subnet, error) { +func (p *SubnetProvider) Get(ctx context.Context, provider *v1alpha1.AWS) ([]*ec2.Subnet, error) { p.Lock() defer p.Unlock() - filters := getFilters(constraints) + filters := getFilters(provider) hash, err := hashstructure.Hash(filters, hashstructure.FormatV2, nil) if err != nil { return nil, err @@ -59,17 +59,17 @@ func (p *SubnetProvider) Get(ctx context.Context, constraints *v1alpha1.AWS) ([] return nil, fmt.Errorf("describing subnets %s, %w", pretty.Concise(filters), err) } if len(output.Subnets) == 0 { - return nil, fmt.Errorf("no subnets matched selector %v", constraints.SubnetSelector) + return nil, fmt.Errorf("no subnets matched selector %v", provider.SubnetSelector) } p.cache.SetDefault(fmt.Sprint(hash), output.Subnets) logging.FromContext(ctx).Debugf("Discovered subnets: %s", prettySubnets(output.Subnets)) return output.Subnets, nil } -func getFilters(constraints *v1alpha1.AWS) []*ec2.Filter { +func getFilters(provider *v1alpha1.AWS) []*ec2.Filter { filters := []*ec2.Filter{} // Filter by subnet - for key, value := range constraints.SubnetSelector { + for key, value := range provider.SubnetSelector { if value == "*" { filters = append(filters, &ec2.Filter{ Name: aws.String("tag-key"), diff --git a/pkg/cloudprovider/aws/suite_test.go b/pkg/cloudprovider/aws/suite_test.go index 3d7ee4d4908e..9e4f73aecffa 100644 --- a/pkg/cloudprovider/aws/suite_test.go +++ b/pkg/cloudprovider/aws/suite_test.go @@ -774,15 +774,15 @@ var _ = Describe("Allocation", func() { // Intent here is that if updates occur on the controller, the Provisioner doesn't need to be recreated It("should not set the InstanceProfile with the default if none provided in Provisioner", func() { provisioner.SetDefaults(ctx) - constraints, err := v1alpha1.Deserialize(&provisioner.Spec.Constraints) + constraints, err := v1alpha1.Deserialize(provisioner.Spec.Provider) Expect(err).ToNot(HaveOccurred()) Expect(constraints.InstanceProfile).To(BeNil()) }) It("should default requirements", func() { provisioner.SetDefaults(ctx) - Expect(provisioner.Spec.Requirements.CapacityTypes().UnsortedList()).To(ConsistOf(v1alpha1.CapacityTypeOnDemand)) - Expect(provisioner.Spec.Requirements.Architectures().UnsortedList()).To(ConsistOf(v1alpha5.ArchitectureAmd64)) + Expect(v1alpha5.NewRequirements(provisioner.Spec.Requirements.Requirements...).CapacityTypes().UnsortedList()).To(ConsistOf(v1alpha1.CapacityTypeOnDemand)) + Expect(v1alpha5.NewRequirements(provisioner.Spec.Requirements.Requirements...).Architectures().UnsortedList()).To(ConsistOf(v1alpha5.ArchitectureAmd64)) }) }) Context("Validation", func() { @@ -1075,6 +1075,5 @@ func ProvisionerWithProvider(provisioner *v1alpha5.Provisioner, provider *v1alph } func ProviderFromProvisioner(provisioner *v1alpha5.Provisioner) (*v1alpha1.AWS, error) { - constraints, err := v1alpha1.Deserialize(&provisioner.Spec.Constraints) - return constraints.AWS, err + return v1alpha1.Deserialize(provisioner.Spec.Provider) } diff --git a/pkg/cloudprovider/fake/cloudprovider.go b/pkg/cloudprovider/fake/cloudprovider.go index d6cb021cbc6d..781ef6c85986 100644 --- a/pkg/cloudprovider/fake/cloudprovider.go +++ b/pkg/cloudprovider/fake/cloudprovider.go @@ -61,7 +61,7 @@ func (c *CloudProvider) Create(ctx context.Context, nodeRequest *cloudprovider.N instance := nodeRequest.InstanceTypeOptions[0] var zone, capacityType string for _, o := range instance.Offerings() { - if nodeRequest.Constraints.Requirements.CapacityTypes().Has(o.CapacityType) && nodeRequest.Constraints.Requirements.Zones().Has(o.Zone) { + if nodeRequest.Template.Requirements.CapacityTypes().Has(o.CapacityType) && nodeRequest.Template.Requirements.Zones().Has(o.Zone) { zone = o.Zone capacityType = o.CapacityType break @@ -147,10 +147,10 @@ func (c *CloudProvider) Delete(context.Context, *v1.Node) error { return nil } -func (c *CloudProvider) Default(context.Context, *v1alpha5.Constraints) { +func (c *CloudProvider) Default(context.Context, *v1alpha5.Provisioner) { } -func (c *CloudProvider) Validate(context.Context, *v1alpha5.Constraints) *apis.FieldError { +func (c *CloudProvider) Validate(context.Context, *v1alpha5.Provisioner) *apis.FieldError { return nil } diff --git a/pkg/cloudprovider/metrics/cloudprovider.go b/pkg/cloudprovider/metrics/cloudprovider.go index be733b57032b..6f964d255d94 100644 --- a/pkg/cloudprovider/metrics/cloudprovider.go +++ b/pkg/cloudprovider/metrics/cloudprovider.go @@ -82,12 +82,12 @@ func (d *decorator) GetInstanceTypes(ctx context.Context, provider *v1alpha5.Pro return d.CloudProvider.GetInstanceTypes(ctx, provider) } -func (d *decorator) Default(ctx context.Context, constraints *v1alpha5.Constraints) { +func (d *decorator) Default(ctx context.Context, provisioner *v1alpha5.Provisioner) { defer metrics.Measure(methodDurationHistogramVec.WithLabelValues(injection.GetControllerName(ctx), "Default", d.Name()))() - d.CloudProvider.Default(ctx, constraints) + d.CloudProvider.Default(ctx, provisioner) } -func (d *decorator) Validate(ctx context.Context, constraints *v1alpha5.Constraints) *apis.FieldError { +func (d *decorator) Validate(ctx context.Context, provisioner *v1alpha5.Provisioner) *apis.FieldError { defer metrics.Measure(methodDurationHistogramVec.WithLabelValues(injection.GetControllerName(ctx), "Validate", d.Name()))() - return d.CloudProvider.Validate(ctx, constraints) + return d.CloudProvider.Validate(ctx, provisioner) } diff --git a/pkg/cloudprovider/types.go b/pkg/cloudprovider/types.go index f8fbabdce121..56c1aea8311f 100644 --- a/pkg/cloudprovider/types.go +++ b/pkg/cloudprovider/types.go @@ -43,18 +43,26 @@ type CloudProvider interface { // Availability of types or zone may vary by provisioner or over time. GetInstanceTypes(context.Context, *v1alpha5.Provider) ([]InstanceType, error) // Default is a hook for additional defaulting logic at webhook time. - Default(context.Context, *v1alpha5.Constraints) + Default(context.Context, *v1alpha5.Provisioner) // Validate is a hook for additional validation logic at webhook time. - Validate(context.Context, *v1alpha5.Constraints) *apis.FieldError + Validate(context.Context, *v1alpha5.Provisioner) *apis.FieldError // Name returns the CloudProvider implementation name. Name() string } type NodeRequest struct { - Constraints *v1alpha5.Constraints + Template *NodeTemplate InstanceTypeOptions []InstanceType } +type NodeTemplate struct { + Provider *v1alpha5.Provider + Labels map[string]string + Taints []v1.Taint + Requirements v1alpha5.Requirements + KubeletConfiguration *v1alpha5.KubeletConfiguration +} + // InstanceType describes the properties of a potential node (either concrete attributes of an instance of this type // or supported options in the case of arrays) type InstanceType interface { diff --git a/pkg/controllers/provisioning/controller.go b/pkg/controllers/provisioning/controller.go index c598737c984f..3ab33a69cd75 100644 --- a/pkg/controllers/provisioning/controller.go +++ b/pkg/controllers/provisioning/controller.go @@ -101,7 +101,7 @@ func (c *Controller) Apply(ctx context.Context, provisioner *v1alpha5.Provisione return err } provisioner.Spec.Labels = functional.UnionStringMaps(provisioner.Spec.Labels, map[string]string{v1alpha5.ProvisionerNameLabelKey: provisioner.Name}) - provisioner.Spec.Requirements = provisioner.Spec.Requirements. + provisioner.Spec.Requirements = v1alpha5.NewRequirements(provisioner.Spec.Requirements.Requirements...). Add(cloudprovider.Requirements(instanceTypes).Requirements...). Add(v1alpha5.NewLabelRequirements(provisioner.Spec.Labels).Requirements...) if err := provisioner.Spec.Requirements.Validate(); err != nil { diff --git a/pkg/controllers/provisioning/provisioner.go b/pkg/controllers/provisioning/provisioner.go index 6c21427696d7..bd5e87b648bb 100644 --- a/pkg/controllers/provisioning/provisioner.go +++ b/pkg/controllers/provisioning/provisioner.go @@ -142,14 +142,21 @@ func (p *Provisioner) launch(ctx context.Context, node *scheduling.Node) error { if err := p.Spec.Limits.ExceededBy(latest.Status.Resources); err != nil { return err } - - nodeRequest := &cloudprovider.NodeRequest{Constraints: node.Constraints, InstanceTypeOptions: node.InstanceTypeOptions} - k8sNode, err := p.cloudProvider.Create(ctx, nodeRequest) + k8sNode, err := p.cloudProvider.Create(ctx, &cloudprovider.NodeRequest{ + InstanceTypeOptions: node.InstanceTypeOptions, + Template: &cloudprovider.NodeTemplate{ + Provider: p.Spec.Provider, + Labels: node.Constraints.Labels, + Taints: node.Constraints.Taints, + Requirements: node.Constraints.Requirements, + KubeletConfiguration: node.Constraints.KubeletConfiguration, + }, + }) if err != nil { return fmt.Errorf("creating cloud provider machine, %w", err) } - if err := mergo.Merge(k8sNode, nodeRequest.Constraints.ToNode()); err != nil { + if err := mergo.Merge(k8sNode, node.Constraints.ToNode()); err != nil { return fmt.Errorf("merging cloud provider node, %w", err) } // Idempotently create a node. In rare cases, nodes can come online and