✨ Implement AWSMachinePools #1860

mytunguyen · 2020-08-05T17:26:26Z

What this PR does / why we need it:

This PR implements machine pools for CAPA using AWS Auto Scaling Groups. In this PR, an /exp path is added similar to CAPI to be the home for the AWSMachinePool API and controller.

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 #1784

k8s-ci-robot · 2020-08-05T17:26:50Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

dthorsen · 2020-08-05T17:42:00Z

I signed it

dthorsen · 2020-08-05T18:03:46Z

/check-cla

ncdc · 2020-08-05T18:06:07Z

/retitle ✨ Implement AWSMachinePools

mytunguyen · 2020-08-05T18:11:39Z

/retest

benmoss · 2020-08-05T19:49:18Z

Should this be an experimental/feature-flag feature? I don't think we have any right now, but it is in CAPZ and CAPI.

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml

benmoss · 2020-08-06T14:13:30Z

For anyone looking to kick the tires on this, this is a decent starting manifest: https://gist.github.com/benmoss/db4ede41d2b767095aa6ab8b381bcb50

dennisme · 2020-08-06T21:58:53Z

hey 👋 , we have a tagging and subnet update PR ready to open once this lands. We didn't want to muddy the waters. Just a heads up you can see them here: newrelic-forks#55 newrelic-forks#56

pkg/cloud/services/ec2/launchtemplate.go

rudoi · 2020-08-11T17:22:15Z

-cracks knuckles- ok, linter, you wanna play?

randomvariable

Mostly lgtm

exp/controllers/awsmachinepool_controller.go

pkg/cloud/services/autoscaling/autoscalinggroup_test.go

pkg/cloud/services/ec2/launchtemplate.go

richardcase · 2020-09-10T08:38:18Z

Ah, linting failure. Does the MockEC2MachineInterface need updating with the change to the LaunchTemplateNeedsUpdate function?

randomvariable · 2020-09-11T14:44:31Z

Running make generate now should resolve the verify issue.

richardcase

Great work @mytunguyen 👍 . Some minor comments....will take another pass through later

richardcase · 2020-09-11T14:47:21Z

exp/api/v1alpha3/types.go

Be good to have the descriptions/validation comments here as well. On the types in this file.

sweet. i updated the descriptions and added validations where I thought made sense. let me know if i missed something!

Looks good!

exp/api/v1alpha3/types.go

pkg/cloud/services/autoscaling/service.go

exp/controllers/awsmachinepool_controller.go

richardcase · 2020-09-11T15:18:51Z

exp/controllers/awsmachinepool_controller.go

Do we need to filter the events if paused?

exp/api/v1alpha3/awsmachinepool_types.go

exp/controllers/awsmachinepool_controller.go

exp/api/v1alpha3/awsmachinepool_types.go

randomvariable · 2020-09-24T14:12:36Z

exp/api/v1alpha3/types.go

Suggested change

// +kubebuilder:validation:Enum=standard;io1;gp2;st1;sc1

// +kubebuilder:validation:Enum=standard;io1;io2;gp2;st1;sc1

Nit: io2 was added since the start of this PR. Can do as follow up

Honestly I'm not even using EBS with our machinepools/ASG's. I think this struct fell here on day 1 while I was listing out "what might we need in the future". I can definitely delete it for now if that helps?

Either add io2 now, or I can PR it next. Don't delete the validation though.

randomvariable · 2020-09-24T14:17:11Z

I think if we can resolve the choice of strings versus aliased string type in the API, the other issues we can deal with as follow ups.

randomvariable · 2020-09-25T10:28:14Z

CRDs need updating, as well as 3 linter issues need addressing.

randomvariable · 2020-09-28T20:42:22Z

If this is good to go, let's squash the "fix" commits, and can approve in the morning UK time.

Co-authored-by: Nicole Yson <[email protected]> Co-authored-by: Andrew Rudoi <[email protected]> Co-authored-by: mnguyen <[email protected]> Co-authored-by: Naadir Jeewa <[email protected]>

exp/controllers/awsmachinepool_controller.go

exp/api/v1alpha3/types.go

pkg/cloud/services/ec2/instances.go

richardcase · 2020-09-29T09:03:58Z

A few minor items that could be covered in the follow-up

/lgtm

randomvariable · 2020-09-29T09:36:36Z

Follow ups to do in #1968

/approve

k8s-ci-robot · 2020-09-29T09:36:46Z

[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:

~~OWNERS~~ [randomvariable]

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 requested review from ncdc and vincepri August 5, 2020 17:26

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 5, 2020

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 5, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 5, 2020

k8s-ci-robot changed the title ~~⚠️ feat: Implement AWSMachinePools~~ ✨ Implement AWSMachinePools Aug 5, 2020

mytunguyen force-pushed the machinepools branch from ef921e7 to 671662d Compare August 5, 2020 18:10

benmoss reviewed Aug 5, 2020

View reviewed changes

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml Show resolved Hide resolved

rudoi reviewed Aug 11, 2020

View reviewed changes

pkg/cloud/services/ec2/launchtemplate.go Outdated Show resolved Hide resolved

randomvariable assigned rudoi Aug 11, 2020

rudoi force-pushed the machinepools branch from 9e2db78 to 0463b1e Compare August 11, 2020 17:31

randomvariable reviewed Aug 13, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2020

rudoi mentioned this pull request Aug 13, 2020

Enhanced AMI lookup logic for EKS #1869

Open

randomvariable added this to the v0.6.1 milestone Aug 14, 2020

michaelbeaumont mentioned this pull request Sep 2, 2020

✨ AWSManagedMachinePool (EKS managed nodegroup) support #1916

Merged

19 tasks

mytunguyen force-pushed the machinepools branch from a48015e to a22a546 Compare September 8, 2020 18:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2020

mytunguyen mentioned this pull request Sep 9, 2020

🐛 Add capi exp schema and fix manager args and rbac #1936

Merged

richardcase reviewed Sep 11, 2020

View reviewed changes

michaelbeaumont reviewed Sep 14, 2020

View reviewed changes

exp/api/v1alpha3/awsmachinepool_types.go Outdated Show resolved Hide resolved

richardcase reviewed Sep 18, 2020

View reviewed changes

mytunguyen force-pushed the machinepools branch from de75105 to 6461db7 Compare September 21, 2020 15:40

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020

mytunguyen force-pushed the machinepools branch from 631cec3 to 66229cb Compare September 23, 2020 21:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020

randomvariable mentioned this pull request Sep 24, 2020

Holding issue for followups on MachinePool #1968

Closed

randomvariable reviewed Sep 24, 2020

View reviewed changes

nicoleyson force-pushed the machinepools branch from e6efead to 04fc665 Compare September 24, 2020 21:58

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2020

mytunguyen force-pushed the machinepools branch from 059f772 to 7910325 Compare September 28, 2020 18:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2020

mytunguyen force-pushed the machinepools branch from 7910325 to 5b2e4d9 Compare September 29, 2020 04:30

added awsmachinepool controller and types

f05273a

Co-authored-by: Nicole Yson <[email protected]> Co-authored-by: Andrew Rudoi <[email protected]> Co-authored-by: mnguyen <[email protected]> Co-authored-by: Naadir Jeewa <[email protected]>

mytunguyen force-pushed the machinepools branch from 5b2e4d9 to f05273a Compare September 29, 2020 04:38

richardcase approved these changes Sep 29, 2020

View reviewed changes

exp/controllers/awsmachinepool_controller.go Outdated Show resolved Hide resolved

exp/api/v1alpha3/types.go Show resolved Hide resolved

pkg/cloud/services/ec2/instances.go Show resolved Hide resolved

k8s-ci-robot assigned richardcase Sep 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2020

k8s-ci-robot merged commit 8da0cb5 into kubernetes-sigs:master Sep 29, 2020

This was referenced Sep 29, 2020

Add e2e test for AWSMachinePools #1983

Closed

Implement AWSMachinePools using ASG #1722

Closed

	// +kubebuilder:validation:Enum=standard;io1;gp2;st1;sc1
	// +kubebuilder:validation:Enum=standard;io1;io2;gp2;st1;sc1

✨ Implement AWSMachinePools #1860

✨ Implement AWSMachinePools #1860

Uh oh!

Conversation

mytunguyen commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 5, 2020

Uh oh!

dthorsen commented Aug 5, 2020

Uh oh!

dthorsen commented Aug 5, 2020

Uh oh!

ncdc commented Aug 5, 2020

Uh oh!

mytunguyen commented Aug 5, 2020

Uh oh!

benmoss commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

benmoss commented Aug 6, 2020

Uh oh!

dennisme commented Aug 6, 2020

Uh oh!

Uh oh!

rudoi commented Aug 11, 2020

Uh oh!

randomvariable left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richardcase commented Sep 10, 2020

Uh oh!

randomvariable commented Sep 11, 2020

Uh oh!

richardcase left a comment

Choose a reason for hiding this comment

Uh oh!

richardcase Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mytunguyen Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

richardcase Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richardcase Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

randomvariable Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

mytunguyen Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

randomvariable Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

randomvariable commented Sep 24, 2020

Uh oh!

randomvariable commented Sep 25, 2020

mytunguyen commented Aug 5, 2020 •

edited

Loading

benmoss commented Aug 5, 2020 •

edited

Loading

richardcase Sep 11, 2020 •

edited

Loading