✨ KubeadmControlPlane scale up serially #2193

dlipovetsky · 2020-01-29T03:13:12Z

What this PR does / why we need it:
The control plane needs to be scaled one replica at a time. Also, before a replica is added with, all etcd endpoint should be healthy, and all existing members should be started.

This PR is a collaborative effort. I started the etcd health check work in the scale up function itself. @chuckha factored that out. More importantly, he added abstractions for the management and target cluster, and wired up the etcd client (merged in #2237).

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 #2016, #2243

/hold
/assign @randomvariable @detiber

chuckha

sweet, this looks good, i wanted to get rid of that for loop for a while 😄

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

detiber · 2020-01-30T17:05:32Z

Would you agree with this:

The controller should create a new replica only if:
(a) for every existing replica (i.e. ownedMachine), there is a healthy etcd member (which implies that the member is listed and started), and
(b) the number of etcd members (including unstarted) is equal to the number of ownedMachines

yes

k8s-ci-robot · 2020-02-01T00:41:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dlipovetsky
To complete the pull request process, please assign chuckha
You can assign the PR to them by writing /assign @chuckha in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

dlipovetsky · 2020-02-01T00:49:26Z

(The delete-related unit tests are failing, because they relied on the earlier, permissive, scale-up functionality to create a control plane for testing delete. Will fix)

dlipovetsky · 2020-02-05T23:37:26Z

@chuckha All unit tests pass, but that's because they inject a fake etcd client getter into KubeadmControlPlaneReconcile. Because the "get an etcd client for the etcd Pod of this Machine" isn't implemented, this will break actual use.

How do you want to proceed? Do you want me to pull out the etcd healthcheck code into its own PR and merge that, then come back to this once you've merged your "etcd client getter"?

chuckha · 2020-02-06T15:03:03Z

I'll PR the etcd client getter then we an revisit this

chuckha · 2020-02-06T15:36:37Z

@dlipovetsky actually i'm going to PR against your branch so I can work it in seamlessly

chuckha · 2020-02-07T18:52:19Z

controlplane/kubeadm/internal/etcd/util/set.go

+limitations under the License.
+*/
+
+// Modified copy of k8s.io/apimachinery/pkg/util/sets/int64.go


What got modified? Curious why we can't use this out of the box

hmm, is it mainly modifying for uint64? If so, can we leverage the set-gen code generator instead of a hand-edited copy here?

Yes, upstream is Int64.

If so, can we leverage the set-gen code generator instead of a hand-edited copy here?

Funny you should ask. I wanted to do this for a different type months ago; it didn't work, and I filed an issue back then: kubernetes/code-generator#74. Not sure if that problem was fixed since, though.

Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

Signed-off-by: Chuck Ha <chuckh@vmware.com>

detiber

Awesome work @dlipovetsky and @chuckha, a few comments, otherwise this is looking great.

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

controlplane/kubeadm/internal/cluster.go

detiber · 2020-02-07T19:11:33Z

controlplane/kubeadm/internal/etcd/util/set.go

+limitations under the License.
+*/
+
+// Modified copy of k8s.io/apimachinery/pkg/util/sets/int64.go


hmm, is it mainly modifying for uint64? If so, can we leverage the set-gen code generator instead of a hand-edited copy here?

- Pass context to cluster client - Fix formatting - Fix linter errors - Also rename FilterMachines to FilterMachine - Remove unused code - Remove whitespace Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

dlipovetsky · 2020-02-07T20:09:36Z

/hold cancel

controlplane/kubeadm/internal/cluster.go

Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

k8s-ci-robot · 2020-02-07T21:24:25Z

@dlipovetsky: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-test	`a1fdb3e`	link	`/test pull-cluster-api-test`
pull-cluster-api-capd-e2e	`a1fdb3e`	link	`/test pull-cluster-api-capd-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

dlipovetsky · 2020-02-11T02:03:56Z

This PR organically grew too large. We split this into multiple PRs, and reviewers were nice enough to copy over the unresolved comments. Closing this PR.

k8s-ci-robot assigned chuckha, detiber and randomvariable Jan 29, 2020

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2020

k8s-ci-robot requested review from ncdc and vincepri January 29, 2020 03:13

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

chuckha approved these changes Jan 29, 2020

View reviewed changes

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Show resolved Hide resolved

detiber reviewed Jan 29, 2020

View reviewed changes

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2020

dlipovetsky changed the title ~~✨ KubeadmControlPlane scale up serially~~ [WIP / Do not review] ✨ KubeadmControlPlane scale up serially Feb 3, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2020

dlipovetsky force-pushed the control-plane-scale-serially branch from 34473ac to 33c7cac Compare February 5, 2020 20:13

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

dlipovetsky force-pushed the control-plane-scale-serially branch from 33c7cac to 5486644 Compare February 5, 2020 23:27

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2020

dlipovetsky changed the title ~~[WIP / Do not review] ✨ KubeadmControlPlane scale up serially~~ ✨ KubeadmControlPlane scale up serially Feb 5, 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 Feb 5, 2020

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

chuckha reviewed Feb 7, 2020

View reviewed changes

dlipovetsky and others added 2 commits February 7, 2020 11:02

✨ KubeadmControlPlane scale up serially

bbd09e4

Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

Refactor healthcheck logic into new package

068d438

Signed-off-by: Chuck Ha <chuckh@vmware.com>

detiber reviewed Feb 7, 2020

View reviewed changes

dlipovetsky force-pushed the control-plane-scale-serially branch 2 times, most recently from 1f4e55d to 5fc8b1c Compare February 7, 2020 19:27

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

dlipovetsky force-pushed the control-plane-scale-serially branch from 5fc8b1c to 3ff0876 Compare February 7, 2020 19:44

Misc. fixes

225f97b

- Pass context to cluster client - Fix formatting - Fix linter errors - Also rename FilterMachines to FilterMachine - Remove unused code - Remove whitespace Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

dlipovetsky force-pushed the control-plane-scale-serially branch from 3ff0876 to 225f97b Compare February 7, 2020 20:06

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2020

detiber reviewed Feb 7, 2020

View reviewed changes

controlplane/kubeadm/internal/cluster.go Show resolved Hide resolved

controlplane/kubeadm/internal/cluster.go Show resolved Hide resolved

Inject fake managementCluster client into unit tests

a1fdb3e

Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>

This was referenced Feb 8, 2020

✨ Adds healthcheck for workload clusters #2295

Merged

🏃 KCP cleanup #2303

Merged

dlipovetsky closed this Feb 11, 2020

dlipovetsky mentioned this pull request Feb 13, 2020

Change set-gen to respect command-line arguments like input and output directory kubernetes/code-generator#74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ KubeadmControlPlane scale up serially #2193

✨ KubeadmControlPlane scale up serially #2193

dlipovetsky commented Jan 29, 2020 •

edited

Loading

chuckha left a comment

detiber commented Jan 30, 2020

k8s-ci-robot commented Feb 1, 2020

dlipovetsky commented Feb 1, 2020

dlipovetsky commented Feb 5, 2020

chuckha commented Feb 6, 2020

chuckha commented Feb 6, 2020

chuckha Feb 7, 2020

detiber Feb 7, 2020

dlipovetsky Feb 7, 2020 •

edited

Loading

detiber left a comment

detiber Feb 7, 2020

dlipovetsky commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

dlipovetsky commented Feb 11, 2020

✨ KubeadmControlPlane scale up serially #2193

✨ KubeadmControlPlane scale up serially #2193

Conversation

dlipovetsky commented Jan 29, 2020 • edited Loading

chuckha left a comment

Choose a reason for hiding this comment

detiber commented Jan 30, 2020

k8s-ci-robot commented Feb 1, 2020

dlipovetsky commented Feb 1, 2020

dlipovetsky commented Feb 5, 2020

chuckha commented Feb 6, 2020

chuckha commented Feb 6, 2020

chuckha Feb 7, 2020

Choose a reason for hiding this comment

detiber Feb 7, 2020

Choose a reason for hiding this comment

dlipovetsky Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

detiber left a comment

Choose a reason for hiding this comment

detiber Feb 7, 2020

Choose a reason for hiding this comment

dlipovetsky commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

dlipovetsky commented Feb 11, 2020

dlipovetsky commented Jan 29, 2020 •

edited

Loading

dlipovetsky Feb 7, 2020 •

edited

Loading