Move control plane locker from CAPA #33

SataQiu · 2019-07-17T11:02:37Z

What this PR does / why we need it:
Move control plane locker from CAPA

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

NONE

k8s-ci-robot · 2019-07-17T11:02:38Z

Welcome @SataQiu!

It looks like this is your first PR to kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

chuckha · 2019-07-17T14:05:40Z

Hmm this linter is actually failing. I need to make an issue to look into that. In the meantime can you run ./hack/verify-all.sh locally and fix the issues?

SataQiu · 2019-07-17T15:09:42Z

OK, I will do that. Thanks @chuckha

chuckha · 2019-07-17T16:39:33Z

Ping me in a comment when this is passing and ready for review! sorry about the bad signal with CI, I don't know what's going on there.

ashish-amarnath · 2019-07-17T17:10:27Z

/lgtm

chuckha · 2019-07-17T17:10:49Z

/hold

This is failing the verify scripts.

ashish-amarnath · 2019-07-17T17:14:36Z

This needs to be fixed. Automated tests are not catching this atm

Running golint...
cloudinit/cloudinit.go:32:6: exported type BaseUserData should have comment or be unexported
Found 1 lint suggestions; failing.
cloudinit/controlplane_certs.go:21:6: exported type Certificates should have comment or be unexported
Found 1 lint suggestions; failing.

fabriziopandini

Background question:
Are we implementing init lock in this project because it is something really specific for kubeadm? (otherwise managing control plane init lock does not seem to be fitting in the scope of a bootstrap provider, that should be responsible for the cloud-init generation only ...)

fabriziopandini · 2019-07-17T20:48:39Z

controllers/control_plane_init_locker.go

+	}
+}
+
+func (l *controlPlaneInitLocker) Acquire(cluster *clusterv2.Cluster) bool {


Might be I'm missing something, but I don't see the counterpart of Acquire (Release)

In the original codebase, the release part was done in the Cluster Actuator https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/pkg/cloud/actuators/cluster/actuator.go#L140-L159, we should move this as well

Reconcile is not complete yet. I think we can add this later. Right?

@SataQiu given that the Release code pointed out by @vincepri is well-scoped, IMO it would be great to include it in this PR so the control plane locker is consistent.
wdyt?

I'll move it over when I need it in the actual usage of this code. no worries here.

vincepri · 2019-07-17T21:07:16Z

@fabriziopandini This codebase originally solved the problem of generating multiple Kubeadm control-plane init configurations when a user would create an HA cluster submitting multiple Machine instances at the same time.

I see how it may touch a little outside of its primary responsibility: generating cloud-init data. That said, without coordination, we might end up generating incorrect data. Happy to chat more in Zoom or on Slack if you want to talk more about it.

/cc @ncdc

ncdc · 2019-07-17T21:40:22Z

Longer term, when we tackle control plane management (v1alpha3, perhaps), I'd expect this code to go away and be replaced by a new component that manages the full lifecycle of all control plane machines (init, scale up (join), scale down, upgrade, etc).

ncdc · 2019-07-17T21:42:46Z

Also, we need to orchestrate when the infrastructure provider is allowed to create a server:

If there is no control plane yet, we should only generate the bootstrap data for a machine if the machine is a control plane machine and we are able to acquire the control plane init lock
If there is a control plane, we can generate the bootstrap data immediately

fabriziopandini · 2019-07-18T08:21:00Z

@vincepri @ncdc thanks for the clarification! ok for me

SataQiu · 2019-07-22T05:09:46Z

I'm confused. Why did pull-cluster-api-bootstrap-provider-kubeadm-verify fail ？
Did I miss something? @chuckha

[*] Verifying whitespace...
[*] Verifying spelling...
[*] Verifying boilerplate...
[*] Verifying gofmt...
[*] Verifying golint...
[*] Verifying govet...
[*] Verifying deps...
[*] Verifying gotest...
[*] Verifying build...
All verify checks passed, congrats!

detiber · 2019-07-22T13:49:32Z

/test pull-cluster-api-bootstrap-provider-kubeadm-verify

chuckha

I think this is fine for now, I'll need this soon so I can do cleanups if necessary

I will probably be moving this to some directory other than controllers.

/lgtm

chuckha · 2019-07-22T15:07:56Z

controllers/control_plane_init_locker.go

+	return true
+}
+
+func (l *controlPlaneInitLocker) Release(cluster *clusterv2.Cluster) bool {


interesting, golint doesn't fail this, presumably because controlPlaneInitLocker is not exported.

chuckha · 2019-07-22T15:08:17Z

controllers/control_plane_init_locker.go

+	}
+}
+
+func (l *controlPlaneInitLocker) Acquire(cluster *clusterv2.Cluster) bool {


I'll move it over when I need it in the actual usage of this code. no worries here.

chuckha · 2019-07-22T15:09:34Z

/hold cancel
/approve

whoops, ignore the lgtm, this already had lgtm

k8s-ci-robot · 2019-07-22T15:09:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, SataQiu

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

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

chuckha · 2019-07-22T15:10:16Z

Thank you @SataQiu for your first PR into CABPK! Welcome aboard :) 🚂 🚀

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2019

k8s-ci-robot requested review from luxas and timothysc July 17, 2019 11:02

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2019

SataQiu force-pushed the add-init-lock branch from 9529e84 to eb431fc Compare July 17, 2019 15:26

k8s-ci-robot assigned ashish-amarnath Jul 17, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2019

fabriziopandini reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot requested a review from ncdc July 17, 2019 21:07

SataQiu force-pushed the add-init-lock branch from eb431fc to 2d85601 Compare July 18, 2019 01:46

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2019

move control plane locker from CAPA

3f073f0

SataQiu force-pushed the add-init-lock branch from 2d85601 to 3f073f0 Compare July 22, 2019 04:31

chuckha approved these changes Jul 22, 2019

View reviewed changes

k8s-ci-robot assigned chuckha Jul 22, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2019

k8s-ci-robot merged commit 1442654 into kubernetes-retired:master Jul 22, 2019

Move control plane locker from CAPA #33

Move control plane locker from CAPA #33

Uh oh!

Conversation

SataQiu commented Jul 17, 2019

Uh oh!

k8s-ci-robot commented Jul 17, 2019

Uh oh!

chuckha commented Jul 17, 2019

Uh oh!

SataQiu commented Jul 17, 2019

Uh oh!

chuckha commented Jul 17, 2019

Uh oh!

ashish-amarnath commented Jul 17, 2019

Uh oh!

chuckha commented Jul 17, 2019

Uh oh!

ashish-amarnath commented Jul 17, 2019

Uh oh!

fabriziopandini left a comment

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

vincepri Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

SataQiu Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

chuckha Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

vincepri commented Jul 17, 2019

Uh oh!

ncdc commented Jul 17, 2019

Uh oh!

ncdc commented Jul 17, 2019

Uh oh!

fabriziopandini commented Jul 18, 2019

Uh oh!

SataQiu commented Jul 22, 2019

Uh oh!

detiber commented Jul 22, 2019

Uh oh!

chuckha left a comment

Choose a reason for hiding this comment

Uh oh!

chuckha Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

chuckha Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

chuckha commented Jul 22, 2019

Uh oh!

k8s-ci-robot commented Jul 22, 2019

Uh oh!

chuckha commented Jul 22, 2019

Uh oh!

Uh oh!