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.

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

Jump to bottom

✨ AWSManagedMachinePool (EKS managed nodegroup) support #1916

Merged
merged 8 commits into from
Oct 2, 2020
Merged

✨ AWSManagedMachinePool (EKS managed nodegroup) support #1916

merged 8 commits into from
Oct 2, 2020

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Sep 2, 2020 •
edited
Loading

What this PR does / why we need it:
Add EKS managed node groups support

Which issue(s) this PR fixes:
Closes #1785

Status:

  • Create
  • Delete
  • Update
  • IAM Roles

Points of interest:

  • Roles:
    • similar to managed control plane, create a role only if necessary
    • respect enable IAM option
  • Subnet selection: default to all (like in AWS console) if none specified
  • K8s version handling
  • AMI version handling
  • Launch template support - Will wait for ✨ Implement AWSMachinePools #1860 since AWSLaunchTemplate is introduced there
  • Optimize watches/predicates
    • Watch AWSManagedControlPlane
  • # Replicas
  • MachinePool picks up Spec.ProviderIDList & Status.Replicas and creates NodeRefs
  • Conditions
  • Other options (remote access, disk size)
  • tag reconciliation
  • Tests

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @michaelbeaumont. 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 k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 2, 2020
@michaelbeaumont michaelbeaumont force-pushed the eks_nodegroups branch 2 times, most recently from 318b512 to 012665b Compare September 3, 2020 20:41
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@michaelbeaumont michaelbeaumont force-pushed the eks_nodegroups branch 4 times, most recently from fbbdee4 to ea02810 Compare September 17, 2020 16:19
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2020
@randomvariable
Copy link
Member

/ok-to-test

Let me know when to review

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2020
@michaelbeaumont michaelbeaumont marked this pull request as ready for review September 24, 2020 17:16
@michaelbeaumont michaelbeaumont changed the title [WIP] ✨ EKS managed nodegroup support ✨ EKS managed nodegroup support Sep 24, 2020
@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 Sep 24, 2020
@michaelbeaumont
Copy link
Contributor Author

@randomvariable I think it's a good time for a review, thanks!

@randomvariable
Copy link
Member

Cool, will take a look tomorrow

@michaelbeaumont
Copy link
Contributor Author

I've just pushed support for scaling parameters

@randomvariable randomvariable added this to the v0.6.1 milestone Sep 30, 2020
randomvariable
randomvariable reviewed Oct 1, 2020
View reviewed changes
exp/api/v1alpha3/awsmanagedmachinepool_webhook.go
}

var allErrs field.ErrorList
allErrs = append(allErrs, r.validateImmutable(oldPool)...)
Copy link
Member

Choose a reason for hiding this comment

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

Going to add a follow up for 0.6.2 to replace all this in our webhooks code with the use of kerrors.NewAggregate, so we don't run into #1996 again

@randomvariable
Copy link
Member

@michaelbeaumont can you rebase this?

This looks fine to go in.

@richardcase if you want to lgtm after rebase, i'll approve.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2020
@richardcase
Copy link
Member

if you want to lgtm after rebase, i'll approve.

Will do 👍

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2020
richardcase
richardcase reviewed Oct 2, 2020
View reviewed changes
main.go
@@ -159,6 +161,24 @@ func main() {
if feature.Gates.Enabled(feature.EKS) {
setupLog.Info("enabling EKS controllers")

enableIAM := feature.Gates.Enabled(feature.EKSEnableIAM)
allowAddRoles := feature.Gates.Enabled(feature.EKSAllowAddRoles)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is used in the manager.....i'll create a new issue to remove it

Copy link
Member

Choose a reason for hiding this comment

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

Issue created #2001

@richardcase
Copy link
Member

Maybe squash commits but

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2020
@richardcase richardcase mentioned this pull request Oct 2, 2020
Closed
@randomvariable
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit ec01c50 into kubernetes-sigs:master Oct 2, 2020
@michaelbeaumont michaelbeaumont deleted the eks_nodegroups branch October 2, 2020 10:11
This was referenced Oct 20, 2020
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randomvariable randomvariable randomvariable left review comments

@richardcase richardcase richardcase requested changes

@justinsb justinsb Awaiting requested review from justinsb

@rudoi rudoi Awaiting requested review from rudoi

Assignees

@richardcase richardcase

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.6.1
Development

Successfully merging this pull request may close these issues.

EKS managed node groups support
4 participants
@michaelbeaumont @k8s-ci-robot @randomvariable @richardcase