eks: allow specifying multiple instance types for AWSManagedMachinePool

[jashandeep-sohi]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds a new field to AWSManagedmachinePool called spec.instanceTypeList that let's you specify a list of instance types that the underlying node group should use. This is primarily useful for spec.capacityType: spot, where specifying a set of instances increases your odds of finding an adequate instance, but there's nothing stopping you from using it with spec.capacityType: onDemand. More here https://docs.aws.amazon.com/eks/latest/userguide/managed-node-groups.html#managed-node-group-capacity-types

For backwards compatibility (and to avoid confusion), spec.instanceType is left as-is, but it's made mutually exclusive with spec.instanceTypeList so only one can be set.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
#2701 had this as a request, but it was never added.

Special notes for your reviewer: I've named it instanceTypeList instead of instanceTypes to make it visually different from instanceType to avoid any confusion, but I'm not too strongly attached to that if you want to change it.

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jashandeep-sohi / name: Jashandeep Sohi (231d4d97be7b587c9380b12583bad71e6ec8e760)

[k8s-ci-robot]

@jashandeep-sohi: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[k8s-ci-robot]

Welcome @jashandeep-sohi!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @jashandeep-sohi. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign enxebre after the PR has been reviewed.
You can assign the PR to them by writing /assign @enxebre in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pydctw]

Hi @jashandeep-sohi, thanks for the PR. Agree that it will be good to support specifying multiple instance types for AWSMMP.

My initial thoughts

• How about changing the API in the next revision, v1beta2, making InstanceType to be an array with a conversion webhook implemented? Having both InstanceType and InstanceTypeList can be confusing to a user and I think in the long run, we want to have only one field for specifying an instance type instead of having two fields and making them mutually exclusive with a webhook.
• There is a proposal to deprecate InstanceType field in favor of using a LaunchTemplate. @richardcase, thoughts on how the launch template implementation will impact the field?

[jashandeep-sohi]

• How about changing the API in the next revision, v1beta2, making InstanceType to be an array with a conversion webhook implemented? Having both InstanceType and InstanceTypeList can be confusing to a user and I think in the long run, we want to have only one field for specifying an instance type instead of having two fields and making them mutually exclusive with a webhook.

Sounds good to me.

• There is a proposal to deprecate InstanceType field in favor of using a LaunchTemplate. @richardcase, thoughts on how the launch template implementation will impact the field?

I see. This is definitely a better direction to go, however as far as I can tell it doesn't cover multiple instance types. Looks like in AWSMachinePool that's done via spec.mixedInstancesPolicy.overrides[].instanceType. Perhaps something similar could be added to AWSManagedMachinePool as well to keep the APIs similar.

I just needed a quick and easy way to showcase that this controller could handle managed EKS nodes w/ multiple spot instance types in a prototype, so I was looking for the smallest diff.

[richardcase]

This would be good to discuss at the office hours on Monday, wdyt?

[pydctw]

This would be good to discuss at the office hours on Monday, wdyt?

Sounds great. @jashandeep-sohi, there is a CAPA office hr on Monday 9am Pacific (7/11/22). We will discuss this issue and capture the notes (in case you cannot make it).

[pydctw]

I see. This is definitely a better direction to go, however as far as I can tell it doesn't cover multiple instance types. Looks like in AWSMachinePool that's done via spec.mixedInstancesPolicy.overrides[].instanceType. Perhaps something similar could be added to AWSManagedMachinePool as well to keep the APIs similar.

I just needed a quick and easy way to showcase that this controller could handle managed EKS nodes w/ multiple spot instance types in a prototype, so I was looking for the smallest diff.

@jashandeep-sohi, we had discussion during today's office hr. There are two options depending on timeline.

1. Wait for AWSManagedMachinePool - Launch Template support #3094 to be merged. Add sth similar as spec.mixedInstancesPolicy.overrides[].instanceType as you suggested.
2. Merge this PR as is but deprecate the field when Launch Template PR is merged.

The first option is preferred as adding a field and deprecating soon after is not ideal but it may take time to merge launch template PR.

Is the capability of supporting multiple instance types a blocker/urgent for your team? Based on your previous comment that it can wait for v1beta2 API, I didn't think so but want to confirm.

[jashandeep-sohi]

I see. This is definitely a better direction to go, however as far as I can tell it doesn't cover multiple instance types. Looks like in AWSMachinePool that's done via spec.mixedInstancesPolicy.overrides[].instanceType. Perhaps something similar could be added to AWSManagedMachinePool as well to keep the APIs similar.
I just needed a quick and easy way to showcase that this controller could handle managed EKS nodes w/ multiple spot instance types in a prototype, so I was looking for the smallest diff.

@jashandeep-sohi, we had discussion during today's office hr. There are two options depending on timeline.

1. Wait for [AWSManagedMachinePool - Launch Template support #3094](https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3094) to be merged. Add sth similar as `spec.mixedInstancesPolicy.overrides[].instanceType` as you suggested.

2. Merge this PR as is but deprecate the field when Launch Template PR is merged.

The first option is preferred as adding a field and deprecating soon after is not ideal but it may take time to merge launch template PR.

Is the capability of supporting multiple instance types a blocker/urgent for your team? Based on your previous comment that it can wait for v1beta2 API, I didn't think so but want to confirm.

Thanks for the update! Yes, that's right, this is not exactly a blocker for us atm since we can just carry on using this patch internally. But I wouldn't want to do that for too long either. So it is a priority, but I agree that incorporating it via option 1 would be better in the long run.

/close

[k8s-ci-robot]

@jashandeep-sohi: Closed this PR.

In response to this:

I see. This is definitely a better direction to go, however as far as I can tell it doesn't cover multiple instance types. Looks like in AWSMachinePool that's done via spec.mixedInstancesPolicy.overrides[].instanceType. Perhaps something similar could be added to AWSManagedMachinePool as well to keep the APIs similar.
I just needed a quick and easy way to showcase that this controller could handle managed EKS nodes w/ multiple spot instance types in a prototype, so I was looking for the smallest diff.

@jashandeep-sohi, we had discussion during today's office hr. There are two options depending on timeline.

1. Wait for [AWSManagedMachinePool - Launch Template support #3094](https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3094) to be merged. Add sth similar as `spec.mixedInstancesPolicy.overrides[].instanceType` as you suggested.

2. Merge this PR as is but deprecate the field when Launch Template PR is merged.

The first option is preferred as adding a field and deprecating soon after is not ideal but it may take time to merge launch template PR.

Is the capability of supporting multiple instance types a blocker/urgent for your team? Based on your previous comment that it can wait for v1beta2 API, I didn't think so but want to confirm.

Thanks for the update! Yes, that's right, this is not exactly a blocker for us atm since we can just carry on using this patch internally. But I wouldn't want to do that for too long either. So it is a priority, but I agree that incorporating it via option 1 would be better in the long run.

/close

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.