⚠️ MachinePool API Controller Implementation #1952

juan-lee · 2019-12-20T20:35:09Z

What this PR does / why we need it:
Adds MachinePool controller implementation described in the MachinePool API proposal.

juan-lee · 2019-12-20T20:39:50Z

FYI - I'll add tests before removing the [WIP] flag.

ncdc · 2020-01-09T20:09:56Z

@juan-lee is this ready for an initial round of reviews?

juan-lee · 2020-01-10T15:04:15Z

@juan-lee is this ready for an initial round of reviews?

Yes, please. @ncdc

controllers/machinepool_controller.go

vincepri

First pass, looking good so far!

controllers/machinepool_controller.go

controllers/machinepool_controller_phases.go

ncdc

Soooo sorry for the delay in reviewing this!

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

controllers/machinepool_controller.go

controllers/machinepool_controller_phases.go

controllers/machinepool_controller_noderef.go

controllers/machinepool_controller_phases.go

ncdc · 2020-01-27T21:30:17Z

@juan-lee @CecileRobertMichon will you all have time over the next week or so to work on review comments & unit tests? Anything else we can do to help?

juan-lee · 2020-01-28T15:41:17Z

@juan-lee @CecileRobertMichon will you all have time over the next week or so to work on review comments & unit tests? Anything else we can do to help?

@ncdc I just addressed the comments sans validation and unit tests. I should have cycles today or tomorrow to get those wrapped up. Thanks for the review @ncdc @vincepri @CecileRobertMichon @JoelSpeed

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

ncdc

@juan-lee it looks like there's still a decent amount of outstanding comments from my earlier review.

controllers/machinepool_controller.go

ncdc · 2020-01-28T19:41:38Z

controllers/machinepool_controller.go

@@ -57,13 +76,181 @@ func (r *MachinePoolReconciler) SetupWithManager(mgr ctrl.Manager, options contr
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("machinepool-controller")
 	r.config = mgr.GetConfig()
-
 	r.scheme = mgr.GetScheme()
 	return nil


Pending resolution of https://github.com/kubernetes-sigs/cluster-api/pull/2101/files#r371995294, may need to add a watch map function from Cluster -> MachinePool

We should likely also port over the immutability check on ClusterName as well.

The immutability check is part of a validation webhook @detiber? Would it be okay to enable the validation webhooks and immutability check in a follow-up PR?

I'm not sure I fully understand what's required here. @vincepri @detiber could either of you clarify?

I'm not totally sure (the link to the discussion is not working), but do we need to receive reconcile requests here when an Cluster object is updated?

I have not encountered the need for triggering when cluster is updated during my testing, but I might not be exercising the scenario ya'll have in mind. @vincepri @detiber @ncdc Could I get some clarification or can we address this in a subsequent PR?

controllers/machinepool_controller.go

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

controllers/machinepool_controller.go

juan-lee · 2020-02-09T00:10:01Z

@ncdc @vincepri @detiber All comments have been addressed (pending clarification on one) and validation/unit tests are done. Would love a final pass over the PR.

vincepri · 2020-02-12T15:25:48Z

Taking a look today

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

vincepri · 2020-02-14T03:30:56Z

controllers/machinepool_controller.go

@@ -57,13 +76,181 @@ func (r *MachinePoolReconciler) SetupWithManager(mgr ctrl.Manager, options contr
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("machinepool-controller")
 	r.config = mgr.GetConfig()
-
 	r.scheme = mgr.GetScheme()
 	return nil


I'm not totally sure (the link to the discussion is not working), but do we need to receive reconcile requests here when an Cluster object is updated?

controllers/machinepool_controller.go

controllers/machinepool_controller_noderef.go

Signed-off-by: Juan-Lee Pang <[email protected]>

juan-lee · 2020-02-16T15:50:00Z

/test pull-cluster-api-capd-e2e

vincepri · 2020-02-16T16:49:17Z

/milestone v0.3.0-rc.1

vincepri

/approve

k8s-ci-robot · 2020-02-16T16:49:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juan-lee, vincepri

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~~ [vincepri]

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

juan-lee · 2020-02-16T22:42:21Z

/assign @davidewatson

juan-lee · 2020-02-16T23:11:51Z

/retest

CecileRobertMichon

lgtm

vincepri · 2020-02-18T22:04:46Z

/lgtm

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 Dec 20, 2019

k8s-ci-robot requested review from justinsb and ncdc December 20, 2019 20:35

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 20, 2019

juan-lee mentioned this pull request Dec 20, 2019

[WIP] ✨ MachinePool API (provider agnostic scale types) #1665

Closed

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

juan-lee force-pushed the machinepool-controller branch from 6cfbdce to cab374c Compare January 10, 2020 15:02

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

juan-lee force-pushed the machinepool-controller branch from cab374c to 988b198 Compare January 10, 2020 15:10

juan-lee changed the title ~~[WIP] ⚠️ MachinePool API Controller Implementation~~ ⚠️ MachinePool API Controller Implementation Jan 13, 2020

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 Jan 13, 2020

CecileRobertMichon reviewed Jan 13, 2020

View reviewed changes

controllers/machinepool_controller.go Show resolved Hide resolved

ncdc added this to the v0.3.0 milestone Jan 16, 2020

JoelSpeed reviewed Jan 20, 2020

View reviewed changes

controllers/machinepool_controller.go Show resolved Hide resolved

controllers/machinepool_controller.go Outdated Show resolved Hide resolved

vincepri reviewed Jan 21, 2020

View reviewed changes

juan-lee force-pushed the machinepool-controller branch from 988b198 to be50ed6 Compare January 27, 2020 15:14

ncdc reviewed Jan 27, 2020

View reviewed changes

juan-lee force-pushed the machinepool-controller branch from be50ed6 to ce2bab2 Compare January 28, 2020 15:38

ncdc reviewed Jan 28, 2020

View reviewed changes

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Jan 28, 2020

View reviewed changes

juan-lee force-pushed the machinepool-controller branch from ce2bab2 to 5dd9d65 Compare January 29, 2020 15:26

detiber reviewed Jan 29, 2020

View reviewed changes

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go Outdated Show resolved Hide resolved

detiber reviewed Jan 29, 2020

View reviewed changes

controllers/machinepool_controller.go Outdated Show resolved Hide resolved

juan-lee force-pushed the machinepool-controller branch from 5dd9d65 to b8faf17 Compare January 30, 2020 04:00

juan-lee force-pushed the machinepool-controller branch 4 times, most recently from b3cb727 to efd7f2b Compare February 8, 2020 23:52

vincepri reviewed Feb 14, 2020

View reviewed changes

juan-lee force-pushed the machinepool-controller branch from efd7f2b to e236dfc Compare February 15, 2020 23:20

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

juan-lee force-pushed the machinepool-controller branch 2 times, most recently from 7b54bff to d0754e1 Compare February 16, 2020 02:36

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

juan-lee force-pushed the machinepool-controller branch from d0754e1 to fd38a74 Compare February 16, 2020 02:39

MachinePool API Controller Implementation

d3dcb27

Signed-off-by: Juan-Lee Pang <[email protected]>

juan-lee force-pushed the machinepool-controller branch from fd38a74 to d3dcb27 Compare February 16, 2020 15:41

k8s-ci-robot modified the milestones: v0.3.0, v0.3.0-rc.1 Feb 16, 2020

vincepri approved these changes Feb 16, 2020

View reviewed changes

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

k8s-ci-robot assigned davidewatson Feb 16, 2020

CecileRobertMichon approved these changes Feb 18, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Feb 18, 2020

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

k8s-ci-robot merged commit 4ff9ae5 into kubernetes-sigs:master Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ MachinePool API Controller Implementation #1952

⚠️ MachinePool API Controller Implementation #1952

juan-lee commented Dec 20, 2019

juan-lee commented Dec 20, 2019

ncdc commented Jan 9, 2020

juan-lee commented Jan 10, 2020

vincepri left a comment

ncdc left a comment

ncdc commented Jan 27, 2020

juan-lee commented Jan 28, 2020

ncdc left a comment

ncdc Jan 28, 2020

detiber Jan 29, 2020

juan-lee Feb 7, 2020

juan-lee Feb 9, 2020

vincepri Feb 14, 2020

juan-lee Feb 16, 2020

juan-lee commented Feb 9, 2020

vincepri commented Feb 12, 2020

vincepri Feb 14, 2020

juan-lee commented Feb 16, 2020

vincepri commented Feb 16, 2020

vincepri left a comment

k8s-ci-robot commented Feb 16, 2020

juan-lee commented Feb 16, 2020

juan-lee commented Feb 16, 2020

CecileRobertMichon left a comment

vincepri commented Feb 18, 2020

⚠️ MachinePool API Controller Implementation #1952

⚠️ MachinePool API Controller Implementation #1952

Conversation

juan-lee commented Dec 20, 2019

juan-lee commented Dec 20, 2019

ncdc commented Jan 9, 2020

juan-lee commented Jan 10, 2020

vincepri left a comment

Choose a reason for hiding this comment

ncdc left a comment

Choose a reason for hiding this comment

ncdc commented Jan 27, 2020

juan-lee commented Jan 28, 2020

ncdc left a comment

Choose a reason for hiding this comment

ncdc Jan 28, 2020

Choose a reason for hiding this comment

detiber Jan 29, 2020

Choose a reason for hiding this comment

juan-lee Feb 7, 2020

Choose a reason for hiding this comment

juan-lee Feb 9, 2020

Choose a reason for hiding this comment

vincepri Feb 14, 2020

Choose a reason for hiding this comment

juan-lee Feb 16, 2020

Choose a reason for hiding this comment

juan-lee commented Feb 9, 2020

vincepri commented Feb 12, 2020

vincepri Feb 14, 2020

Choose a reason for hiding this comment

juan-lee commented Feb 16, 2020

vincepri commented Feb 16, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 16, 2020

juan-lee commented Feb 16, 2020

juan-lee commented Feb 16, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

vincepri commented Feb 18, 2020