docs: proposal for using launch templates with machine pools #3365
Conversation
This proposal is for adding support for laucnh templates to `AWSManagedMachinePool`. Signed-off-by: Richard Case <richard@weave.works>
k8s-ci-robot
added
kind/design
|
@richardcase: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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
added
the
size/L
richardcase
changed the title
|
This is ready to go. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
| 5. [Refactor the code](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/ec057ad6e613a6578f67bf68a6c77fbe772af933/exp/controllers/awsmachinepool_controller.go#L383) from the `AWSMachinePool` controller that reconciles `AWSLaunchTemplate` into a common location so that it can be shared. | ||
| 6. Update the controller for `AWSManagedMachinePool` to use the `AWSLaunchTemplate` reconciliation logic. | ||
| 7. Add checks in the `AWSManagedMachinePool` create/update validation webhooks that stops users specifying `AWSLaunchTemplate` if fields `AMIType,AMIVersion,InstanceType,DiskSize,InstanceProfile` are set | ||
| 8. Add warning logs to the `AWSManagedMachinePool` create/update validation webhooks if fields `AMIType,AMIVersion,InstanceType,DiskSize,InstanceProfile` stating that these fields will be deprecated in the future and that `AWSLaunchTemplate` should be used instead |
There was a problem hiding this comment.
Adding discussion from #3094 (comment).
Let's decide whether we want to deprecate these fields or not.
There was a problem hiding this comment.
I've been thinking about this and think that its confusing to have 2 ways to specify the configuration for a machine pool.
@pydctw @richardchen331 @sedefsavas - i would say that we deprecate these fields still. wdyt?
There was a problem hiding this comment.
After thinking some more, +1 to deprecate these fields.
Clarifying questions
- My understanding from the comment was that
AMITypeis still needed when using LaunchTemplate. Is this correct? IamInstanceProfileis a field in LaunchTemplate and I don't think it was in AWSManagedMachinePool spec, meaning it is not a field to deprecate. If I read the doc correctly, IAM InstanceProfile is prohibited in LaunchTemplate for EKS, the check is already implemented in the webhook
There was a problem hiding this comment.
I'm ok with deprecating the fields.
We still need to pass a AMIType of CUSTOM to when making calls to EKS using the AWS SDK. We could hardcode the value though. Please let me know what you think. Thanks!
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind design
What this PR does / why we need it:
This proposal is for adding support for laucnh templates to
AWSManagedMachinePool.Note: this is the successor to #3024
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist: