Skip to content
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

docs: proposal for using launch templates with machine pools #3024

Closed
wants to merge 2 commits into from
Closed

docs: proposal for using launch templates with machine pools #3024

wants to merge 2 commits into from

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented Dec 10, 2021

What type of PR is this?

/kind design

What this PR does / why we need it:

This is a work-in-progress PR for a proposal on how we handle launch templates with managed machine pools. Comments can be here on the PR or within the discussion

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:

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

Release note:

Proposal for using launch templates with `AWSManagedMachinePools`

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/design Categorizes issue or PR as related to design. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Dec 10, 2021
@k8s-ci-robot
Copy link
Contributor

@richardcase: 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 k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from richardcase after the PR has been reviewed.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2021
@sedefsavas
Copy link
Contributor

@richardcase
Is running prebootsrap commands useful in any other place than AWSManagedMachinePool for EKS, maybe EKS control plane nodes?

If it is only MachinePools as pointed in the issue #2771, we may add its design here.

@richardcase
Copy link
Member Author

I think it's useful in every machine type for EKS, so machines, machine pools and managed machine pools.....basically everywhere the EKS bootstrap provider is used via EKSConfig.

The launch template support in managed machine pools  proposal has been updated
based on further discussions and investigation.

Signed-off-by: Richard Case <[email protected]>
@richardcase richardcase changed the title WIP docs: proposal for using launch templates with machine pools docs: proposal for using launch templates with machine pools Feb 5, 2022
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 5, 2022
@richardcase richardcase marked this pull request as ready for review February 5, 2022 17:00
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2022
@richardcase richardcase added the kind/proposal Issues or PRs related to proposals. label Feb 7, 2022
@sedefsavas
Copy link
Contributor

sedefsavas commented Mar 1, 2022

One major clarification needed here is how we will support existing AWSManagedMachinePools as pointed above.

Managed node groups that were not initially created with a launch template cannot use one the way we will support in this proposal: here

Existing node groups that don't use a custom launch template can't be updated directly. Rather, you must create a new node group with a custom launch template to do so.

Also this:

If you don't use a custom launch template when first creating a managed node group, there is an auto-generated launch template. Don't manually modify this auto-generated template or errors occur.

An option would be to keep existing support as is and move the fields and support in the next major release. Given that MachinePools are experimental, we do not need to provide a migration path as there might not be a feasible one.


## Motivation

We are increasingly hearing requests from users of CAPA that a particular feature / configuration option isn't exposed by CAPAs implementation of managed machine pools (i.e. `AWSManagedMachinePool`) and on investigation the feature is available via a launch template (nitro enclaves or placement as an example). It some instances, users of CAPA have had to use unmanaged machine pools (i.e. `AWSMachinePool`) instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We are increasingly hearing requests from users of CAPA that a particular feature / configuration option isn't exposed by CAPAs implementation of managed machine pools (i.e. `AWSManagedMachinePool`) and on investigation the feature is available via a launch template (nitro enclaves or placement as an example). It some instances, users of CAPA have had to use unmanaged machine pools (i.e. `AWSMachinePool`) instead.
We are increasingly hearing requests from users of CAPA that a particular feature / configuration option isn't exposed by CAPAs implementation of managed machine pools (i.e. `AWSManagedMachinePool`) and on investigation the feature is available via a launch template (nitro enclaves or placement as an example). In some instances, users of CAPA have had to use unmanaged machine pools (i.e. `AWSMachinePool`) instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed


We are increasingly hearing requests from users of CAPA that a particular feature / configuration option isn't exposed by CAPAs implementation of managed machine pools (i.e. `AWSManagedMachinePool`) and on investigation the feature is available via a launch template (nitro enclaves or placement as an example). It some instances, users of CAPA have had to use unmanaged machine pools (i.e. `AWSMachinePool`) instead.

The motivation is to improve consistency between the2 varieties of machine pools and expose to the user features of launch templates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The motivation is to improve consistency between the2 varieties of machine pools and expose to the user features of launch templates.
The motivation is to improve consistency between the 2 varieties of machine pools and expose to the user features of launch templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed


### Goals

* Consistent API to declare a use launch templates for `AWSMachinePool` and `AWSManagedMachinePool`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Consistent API to declare a use launch templates for `AWSMachinePool` and `AWSManagedMachinePool`
* Consistent API to use launch templates for `AWSMachinePool` and `AWSManagedMachinePool`

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

12. Update the EKS e2e tests to add an additional test step where we create an additional managed machine pool using `AWSLaunchTemplate`.
13. Update any relevant documentation
14. Release note must mention that "action is required" in the future, as fields are being deprecated.
15. As part of the implementation, we should investigate how in the f
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing half of the sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, what happened there 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

#### Story 3

As a CAPA user
I want to ensure launch templates are versioned
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do anything specific here? Every time we update a template, new version is created.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reconciliation will need to create a new version of the launch template or new launch template depending on if its a create or update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed so it hopefully clarifies it a bit better

#### Story 4

As a CAPA user
I want to be able to set the maximum number of versions to keep for launch templates
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a limit to this:

You are limited to creating 5,000 launch templates per Region and 10,000 versions per launch template.

Should we delete older ones as new ones are added? Auto cleanup?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should cleanup older ones.

#### Story 5

As a CAPA user
I want to be able to use the output of a bootstrap provider in my launch template
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Do we need to use bootstrap for managed node pools or could they join the EKS control plane by themselves?
I see that the nodes in managed node pools already come up with Kubernetes.
Is this to provide flexibility to use custom AMIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

They can use the bootstrapper. I have created #3363 to add this support.


#### Functional Requirements

**FR1:** CAPA MUST support using launch templates with non-managed ASG based machine pools (i.e. `AWSMachinePool`).
Copy link
Contributor

Choose a reason for hiding this comment

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

This proposal is only for managed machine pools, might drop this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to ensure it "continues" to support it


### Graduation Criteria

With this proposal, we are planning to deprecate a number of fields on `AWSManagedMachinePool` and as this is a **beta level** API this means:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we follow beta API criteria for experimental features?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a good question. I would say that we don't so i have updated the proposal.


The machine pools feature is also marked as experimental in Cluster API and as such this has to be explicitly enabled via a feature flag. Even though it is experimental, we should adhere to the Kubernetes rules on API deprecation.

So based on the change, we will need to support the deprecated fields until December 2022 or CAPA v1.5...whichever is longer. So probably December 2022
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no upgrade path for the existing AWSManagedMachinePools considering we cannot add LaunchTemplates to already created ones, we might want to drop the support in a major release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to state this

@richardcase
Copy link
Member Author

@sedefsavas @shivi28 - i am closing this PR as i deleted my repo and re-forked a while back. I have created #3365 with the updated version of the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. kind/proposal Issues or PRs related to proposals. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants