Skip to content

Multiple instance types for managed machine pools #4358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arjunrn wants to merge 1 commit into from
Closed

Multiple instance types for managed machine pools #4358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion config/crd/bases/infrastructure.cluster.x-k8s.io_awsmanagedmachinepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,8 +703,18 @@ spec:
and name of the managed machine pool.
type: string
instanceType:
description: InstanceType specifies the AWS instance type
description: InstanceType specifies the AWS instance type. This field
is deprecated. Use InstanceTypes instead.
type: string
instanceTypes:
description: InstanceTypes specifies the AWS instance types which
could be part of the machine pool. The order of instance types specified
determines priority of picking that instance type. This is also
influenced by the CapacityType. See AWS documentation https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types
for more information.
items:
type: string
type: array
labels:
additionalProperties:
type: string
Expand Down
32 changes: 32 additions & 0 deletions exp/api/v1beta1/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,18 @@ package v1beta1

import (
apiconversion "k8s.io/apimachinery/pkg/conversion"
"k8s.io/apimachinery/pkg/util/json"
infrav1beta1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1"
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
infrav1exp "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
"sigs.k8s.io/controller-runtime/pkg/conversion"
)

const (
instanceTypesAnnotationKey = "awsmanagedmachinepools.infrastructure.cluster.x-k8s.io/instance-types"
)

// ConvertTo converts the v1beta1 AWSMachinePool receiver to a v1beta2 AWSMachinePool.
func (src *AWSMachinePool) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*infrav1exp.AWSMachinePool)
Expand Down Expand Up @@ -79,6 +84,22 @@ func (src *AWSManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
return err
}

// set instance types from the annotation
if instanceTypes, ok := src.Annotations[instanceTypesAnnotationKey]; ok {
var deserializedInstanceTypes []string
err := json.Unmarshal([]byte(instanceTypes), &deserializedInstanceTypes)
if err != nil {
return err
}
dst.Spec.InstanceTypes = deserializedInstanceTypes
}

// drop instance types annotation from target
delete(dst.Annotations, instanceTypesAnnotationKey)
if len(dst.Annotations) == 0 {
dst.Annotations = nil
}

return nil
}

Expand All @@ -90,6 +111,17 @@ func (r *AWSManagedMachinePool) ConvertFrom(srcRaw conversion.Hub) error {
return err
}

if len(src.Spec.InstanceTypes) != 0 {
if r.Annotations == nil {
r.Annotations = make(map[string]string)
}
serializedInstances, err := json.Marshal(src.Spec.InstanceTypes)
if err != nil {
return err
}
r.Annotations[instanceTypesAnnotationKey] = string(serializedInstances)
}

return nil
}

Expand Down
1 change: 1 addition & 0 deletions exp/api/v1beta1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion exp/api/v1beta2/awsmanagedmachinepool_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,18 @@ type AWSManagedMachinePoolSpec struct {
// +optional
DiskSize *int32 `json:"diskSize,omitempty"`

// InstanceType specifies the AWS instance type
// InstanceType specifies the AWS instance type.
// This field is deprecated. Use InstanceTypes instead.
// +optional
InstanceType *string `json:"instanceType,omitempty"`

// InstanceTypes specifies the AWS instance types which could be part of the machine pool. The order of instance
// types specified determines priority of picking that instance type. This is also influenced by the CapacityType.
// See AWS documentation https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types
// for more information.
// +optional
Copy link
Member

@enxebre enxebre Jun 28, 2023

Choose a reason for hiding this comment

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

Can we elaborate in the comment what happens when you specify multiple instances types?
What's the expectation, allocation strategy...?

Copy link
Author

@arjunrn arjunrn Jun 29, 2023

Choose a reason for hiding this comment

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

The expectation is any of these instance types could be present in the node group. The allocation strategy is completely managed by AWS. Is there anything you think I should mention in the doc string?

Copy link
Member

@enxebre enxebre Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now by reading the API I'm not sure what intent the field is expressing. I'd add something like

The allocation strategy to provision On-Demand capacity is set to prioritized. Managed node groups use the order of instance types passed in the API to determine which instance type to use first when fulfilling On-Demand capacity.

From https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types

Or at minimum I'd clarify something like

Ordering of the passed values means priority. See https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types for more info

Just a suggestion feel free to proceed otherwise.

Copy link
Member

@richardcase richardcase Jun 30, 2023

Choose a reason for hiding this comment

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

I agree that would be a good idea to expand the comment. I quite like the suggestion fro @enxebre 👍

Ordering of the passed values means priority. See https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types for more info

Copy link
Author

@arjunrn arjunrn Jun 30, 2023

Choose a reason for hiding this comment

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

Updated the doc string. PTAL.

InstanceTypes []string `json:"instanceTypes,omitempty"`

// Scaling specifies scaling for the ASG behind this pool
// +optional
Scaling *ManagedMachinePoolScaling `json:"scaling,omitempty"`
Expand Down
22 changes: 22 additions & 0 deletions exp/api/v1beta2/awsmanagedmachinepool_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ func (r *AWSManagedMachinePool) validateLaunchTemplate() field.ErrorList {
return allErrs
}

if r.Spec.InstanceTypes != nil {
Copy link
Member

@richardcase richardcase Jun 28, 2023

Choose a reason for hiding this comment

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

We can also log in the Create about the deprecation of the InstanceType field

Copy link
Author

@arjunrn arjunrn Jun 29, 2023

Choose a reason for hiding this comment

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

Added log line.

allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "InstanceTypes"), r.Spec.InstanceTypes, "InstanceTypes cannot be specified when LaunchTemplate is specified"))
}

if r.Spec.InstanceType != nil {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "InstanceType"), r.Spec.InstanceType, "InstanceType cannot be specified when LaunchTemplate is specified"))
}
Expand Down Expand Up @@ -158,13 +162,20 @@ func (r *AWSManagedMachinePool) ValidateCreate() error {
if errs := r.validateLaunchTemplate(); len(errs) > 0 {
allErrs = append(allErrs, errs...)
}
if errs := r.validateInstanceConfig(); len(errs) > 0 {
allErrs = append(allErrs, errs...)
}

allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)

if len(allErrs) == 0 {
return nil
}

if r.Spec.InstanceType != nil {
mmpLog.Info("AWSManagedMachine pool spec.instanceType is deprecated", "managed-machine-pool", klog.KObj(r))
}

return apierrors.NewInvalid(
r.GroupVersionKind().GroupKind(),
r.Name,
Expand Down Expand Up @@ -195,6 +206,9 @@ func (r *AWSManagedMachinePool) ValidateUpdate(old runtime.Object) error {
if errs := r.validateLaunchTemplate(); len(errs) > 0 {
allErrs = append(allErrs, errs...)
}
if errs := r.validateInstanceConfig(); len(errs) > 0 {
allErrs = append(allErrs, errs...)
}

if len(allErrs) == 0 {
return nil
Expand All @@ -214,6 +228,14 @@ func (r *AWSManagedMachinePool) ValidateDelete() error {
return nil
}

func (r *AWSManagedMachinePool) validateInstanceConfig() field.ErrorList {
var allErrs field.ErrorList
if r.Spec.InstanceType != nil && r.Spec.InstanceTypes != nil {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "InstanceTypes"), "InstanceTypes cannot be specified when InstanceType is set"))
}
return allErrs
}

func (r *AWSManagedMachinePool) validateImmutable(old *AWSManagedMachinePool) field.ErrorList {
var allErrs field.ErrorList

Expand Down
28 changes: 28 additions & 0 deletions exp/api/v1beta2/awsmanagedmachinepool_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,17 @@ func TestAWSManagedMachinePoolValidateCreate(t *testing.T) {
},
wantErr: false,
},
{
name: "both instanceType and instanceTypes are specified",
pool: &AWSManagedMachinePool{
Spec: AWSManagedMachinePoolSpec{
EKSNodegroupName: "eks-node-group-3",
InstanceTypes: []string{"m5.xlarge", "m5.2xlarge"},
InstanceType: pointer.String("m5.xlarge"),
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -253,6 +264,23 @@ func TestAWSManagedMachinePoolValidateUpdate(t *testing.T) {
},
wantErr: false,
},
{
name: "adding both instanceType and instanceTypes is rejected",
old: &AWSManagedMachinePool{
Spec: AWSManagedMachinePoolSpec{
EKSNodegroupName: "eks-node-group-1",
InstanceType: pointer.String("m5.xlarge"),
},
},
new: &AWSManagedMachinePool{
Spec: AWSManagedMachinePoolSpec{
EKSNodegroupName: "eks-node-group-1",
InstanceType: pointer.String("m5.xlarge"),
InstanceTypes: []string{"m5.xlarge", "m6.xlarge"},
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions exp/api/v1beta2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions pkg/cloud/services/eks/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,10 @@ func (s *NodegroupService) createNodegroup() (*eks.Nodegroup, error) {
if managedPool.InstanceType != nil {
input.InstanceTypes = []*string{managedPool.InstanceType}
}
for _, instanceType := range managedPool.InstanceTypes {
Copy link
Member

@enxebre enxebre Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

may be clarify in the API docs as well that setting both InstanceType and InstanceTypes will append the latter to the former? That UX might come us a surprise. Or maybe event prevent both from being set in validation

Copy link
Author

@arjunrn arjunrn Jul 5, 2023

Choose a reason for hiding this comment

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

That's already the case through validation. The code looks this way but the webhooks would prevent both of these from being set.

Copy link
Author

@arjunrn arjunrn Jul 5, 2023

Choose a reason for hiding this comment

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

https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/a16dd53553cfc688f2ce29e7ab08181572181d03/exp/api/v1beta2/awsmanagedmachinepool_webhook.go#L231-L237

input.InstanceTypes = append(input.InstanceTypes, aws.String(instanceType))
}

if len(managedPool.Taints) > 0 {
s.Info("adding taints to nodegroup", "nodegroup", nodegroupName)
taints, err := converters.TaintsToSDK(managedPool.Taints)
Expand Down