-
Notifications
You must be signed in to change notification settings - Fork 560
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.
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
Multiple instance types for managed machine pools #4358
Multiple instance types for managed machine pools #4358
Conversation
Welcome @arjunrn! |
Hi @arjunrn. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I have added InstanceType
to the issue where we are tracking changes for a future v1beta3 API version. so we can drop the field all together in the future.
@@ -120,6 +120,10 @@ type AWSManagedMachinePoolSpec struct { | |||
// +optional | |||
InstanceType *string `json:"instanceType,omitempty"` | |||
|
|||
// InstanceTypes specifies the AWS instance types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add a note to InstanceType
to say the filed is deprecated and that InstanceTypes
should be used instead.
When we bump the API version we can then remove the old field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the note.
@@ -123,6 +123,10 @@ func (r *AWSManagedMachinePool) validateLaunchTemplate() field.ErrorList { | |||
return allErrs | |||
} | |||
|
|||
if r.Spec.InstanceTypes != nil { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added log line.
@@ -120,6 +120,10 @@ type AWSManagedMachinePoolSpec struct { | |||
// +optional | |||
InstanceType *string `json:"instanceType,omitempty"` | |||
|
|||
// InstanceTypes specifies the AWS instance types | |||
// +optional |
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
c5bdab4
to
e7f8f41
Compare
@@ -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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more comment #4358 (comment). |
a16dd53
to
c151661
Compare
nit: commit is missing imperative verb i.e. Add multiple instances...
This comes in more handy for spot but is also valid for on demand, correct? |
EKS node groups support [multiple instance types ](https://docs.aws.amazon.com/eks/latest/APIReference/API_CreateNodegroup.html#API_CreateNodegroup_RequestSyntax). The order of the instance types and capacity type together determine which instances are created by AWS when the node group is scaled up. Also added validation so that both fields `instanceType` and `instanceTypes` cannot be specified together.
c151661
to
a5a5187
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@enxebre @richardcase Please take another look and let me know if you need anything else. |
@arjunrn - did you mean to close this? |
@richardcase it looks like there was a bit of miscommunication here. |
@idanshaby Please feel free to use the changes in my branch to create a new PR. We've discontinued the use of CAPI/CAPA and I'm not working on this feature any more. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
EKS node groups support multiple instance types. The order of the instance types and capacity type together determine which instances are created by AWS when the node group is scaled up. Also added validation so that both fields
instanceType
andinstanceTypes
cannot be specified together. . Added this as a new field to theAWSManagedMachinePool
in in versionv1beta2
.Fixes #3585
Special notes for your reviewer:
This new field does not exist in the older
v1beta1
type ofAWSManagedMachinePool
. I'm not sure if it is even needed because the older single instance type field is still present. However code-gen complains that this field is not properly converted. Note thatv1beta2
is the stored version so there's no chance for a loss of data.Checklist:
Release note: