✨ KCP adopts existing machines #2489

sethp-nr · 2020-02-29T00:25:12Z

What this PR does / why we need it:

A mostly-functional prototype for having the KCP controller identify
Machines that belong to the control plane of an existing cluster and
adopt them.

Additionally, we set set a "best guess" value for the
kubeadm.controlplane.cluster.x-k8s.io/hash on the adopted machine as if
it were generated by a KCP in the past.

Co-authored-by: mnguyen mnguyen@newrelic.com

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

TODO

The bits of this work that are still in flight are:

(critical) Handling the adoption of cluster PKI secrets (see [kubeadm control plane] upgrade: etcd CA was regenerated #2455)
(critical) Validating that the machines being adopted are actually Kubeadm-managed
Re-working the adoption pieces to make better use of the FilterableMachineCollection
Writing safety checks around upgrade versions, i.e.
- ~~a validating webhook that rejects two major version bumps to a KCP~~ See KCP: validate version changes are no further than one minor version #2578
- a fail-fast path in the adoption if we'd be adopting machines that are "too old")
Write an e2e test, especially around the Secret adoption behavior (it's kind of a pain to test in unit)

/hold
/milestone v0.3.0

k8s-ci-robot · 2020-02-29T00:25:14Z

@sethp-nr: You must be a member of the kubernetes-sigs/cluster-api-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

What this PR does / why we need it:

A mostly-functional prototype for having the KCP controller identify
Machines that belong to the control plane of an existing cluster and
adopt them.

Additionally, we set set a "best guess" value for the
kubeadm.controlplane.cluster.x-k8s.io/hash on the adopted machine as if
it were generated by a KCP in the past.

Co-authored-by: mnguyen mnguyen@newrelic.com

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

TODO

The bits of this work that are still in flight are:

(critical) Handling the adoption of cluster PKI secrets (see [kubeadm control plane] upgrade: etcd CA was regenerated #2455)

(critical) Validating that the machines being adopted are actually Kubeadm-managed

Re-working the adoption pieces to make better use of the FilterableMachineCollection

Writing safety checks around upgrade versions, i.e.

a validating webhook that rejects two major version bumps to a KCP

a fail-fast path in the adoption if we'd be adopting machines that are "too old")

/hold
/milestone v0.3.0

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.

vincepri · 2020-03-05T00:52:43Z

/milestone v0.3.0

vincepri · 2020-03-05T00:57:39Z

/assign

to review later

sethp-nr · 2020-03-07T00:39:06Z

OK, I think this is RFR – I left the hold label on there so we can make a judgement call about whether it needs to be feature-gated after a review cycle or two.

vincepri · 2020-03-09T15:05:53Z

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

+	// We do an uncached full quorum read against the KCP to avoid re-adopting Machines the garbage collector just intentionally orphaned
+	// See https://github.com/kubernetes/kubernetes/issues/42639
+	uncached := controlplanev1.KubeadmControlPlane{}
+	err := r.uncachedClient.Get(ctx, client.ObjectKey{Namespace: kcp.Namespace, Name: kcp.Name}, &uncached)


If we adopt a Machine that's going to be deleted, wouldn't we get a notification and KCP reconciles again afterwards?

I guess what I'm trying to understand is the actual problem this extra client is trying to solve, the comment explains part of it, but I'm missing why this would be a problem given that the controller will keep reconciling the resource

There's an in-depth explanation of the flow in the test suite, but the issue has to do with a deleted KCP possibly re-adopting (and then deleting) dependents that we don't want cleaned up.

There is another, different race with orphaning dependents (see #2505) but from my brief survey of k/k's adoption code it all does a check like this one before it actually takes ownership of anything.

If this is a common pattern for adoption, we likely should create an issue to track standardizing on this approach. I also wonder if it would be worth it to try and get support for this more directly in controller-runtime somehow.

Good call – post-release when I file the follow-up "let's talk about deletes" issue for 2505 there I'll make sure to include this part.

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

vincepri · 2020-03-09T15:14:46Z

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

+		err := r.Client.Update(ctx, &ss)
+		if err != nil {


Any reason not to use Patch instead?

+1 to using patch here

The Patch that's implemented is strategic merge. We'd need to use a different strategy to do a targeted remove of the old controller reference before adding the new one.

vincepri · 2020-03-09T15:15:56Z

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

 	if err != nil {
 		logger.Error(err, "failed to retrieve control plane machines for cluster")
 		return ctrl.Result{}, err
 	}

+	adoptableMachines := controlPlaneMachines.Filter(internal.AdoptableControlPlaneMachines(cluster.Name))


I wonder if this behavior should be opt-in rather than having it run by default, wdyt?

One would already need to create KCP resource to trigger the adoption, or do you think that an additional safeguard would be needed?

Not sure yet, that's a good point though, it's opt-in if you create and link the object

Yeah, that felt like the opt-in edge to me. Otherwise, the KCP creates a new, separate control plane that's "for" the same cluster as the old one but otherwise independent.

util/util.go

detiber · 2020-03-09T15:42:09Z

controlplane/kubeadm/internal/machine_filters.go

@@ -93,10 +94,34 @@ func OwnedControlPlaneMachines(controlPlaneName string) MachineFilter {
 		if controllerRef == nil {
 			return false
 		}
-		return controllerRef.Kind == "KubeadmControlPlane" && controllerRef.Name == controlPlaneName
+		return controllerRef.Kind == "KubeadmControlPlane" && controllerRef.Name == owner.GetName() && controllerRef.UID == owner.GetUID()


I'm assuming that the addition of UID here isn't introducing any edge cases around management cluster pivoting and that adoption should reconcile the UID.

That is a very good point. I don't think the adoption code will reconcile the UID properly as it's currently written, but maybe clusterctl move knows how to handle that already? Otherwise, I'd expect the GC to cause trouble for pivoting anyway.

It appears that clusterctl move does update owner references as objects are moved across:

cluster-api/cmd/clusterctl/client/cluster/mover.go

Line 458 in 5154da4

UID: ownerNode.newUID, // Use the owner's newUID read from the target management cluster (instead of the UID read during discovery).

That is an impressive piece of code!

I think what that means for us is that UIDs are valid in post-clusterctl v2 workflows. Is there a support matrix where we might need to consider a clusterctl v1 pivot with a KCP?

I don't think we necessarily need to consider the v1 pivot, but I do think that we need to consider the use case of someone backing up and restoring resources from backup, which I'm not sure that tools in that space would necessarily do the right thing with the UID there.

It seems like the controller-runtime code handles overwriting the controller ref if the api version or the UID are out of date, and I suspect we should likely do the same (even if part of a different workflow than the comparison here)

detiber · 2020-03-09T15:46:19Z

controlplane/kubeadm/internal/machine_filters.go

-// Usage: managementCluster.GetMachinesForCluster(ctx, cluster, OwnedControlPlaneMachines(controlPlane.Name))
-func OwnedControlPlaneMachines(controlPlaneName string) MachineFilter {
+// Usage: managementCluster.GetMachinesForCluster(ctx, cluster, OwnedControlPlaneMachines(controlPlane))
+func OwnedControlPlaneMachines(owner metav1.Object) func(machine *clusterv1.Machine) bool {


nit: based on the behavior, I'm wondering if this should be renamed OwnedMachines or should also apply the ControlPlaneMachines filter added below

detiber · 2020-03-09T15:49:34Z

controlplane/kubeadm/internal/machine_filters.go

+		},
+		ControlPlaneMachines(clusterName),
+		func(machine *clusterv1.Machine) bool {
+			return metav1.GetControllerOf(machine) == nil


Because of the UID check introduced above, I suspect that this should also take into account the case of the controller referencing the kind/name of the controller ref matching the kcp but the UID not matching.

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

detiber · 2020-03-09T16:03:59Z

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

+		err := r.Client.Update(ctx, &ss)
+		if err != nil {


+1 to using patch here

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

controlplane/kubeadm/internal/machine_filters.go

sethp-nr · 2020-05-12T21:31:42Z

I can rebase past the e2e removal easily enough.

For api changes, it sounds like I should be adding new methods & deprecating old ones?

vincepri · 2020-05-12T22:03:42Z

Yeah that seems the way to go

sethp-nr · 2020-05-12T22:52:57Z

Ok, I introduced new methods in 891f4e8 – if that looks ok, I'll squash it in.

randomvariable · 2020-05-13T09:49:06Z

Looking good.

vincepri · 2020-05-13T14:27:35Z

Doing another pass

bootstrap/kubeadm/api/equality/doc.go

vincepri · 2020-05-13T14:43:45Z

controllers/cluster_controller_test.go

+		TypeMeta: metav1.TypeMeta{
+			APIVersion: clusterv1.GroupVersion.String(),
+			Kind:       "Cluster",
+		},


Why is this needed now?

Because PointsToObject checks Kind and group (though not version) per Jason's suggestion to have it match on the object kind / group / name triple.

vincepri · 2020-05-13T14:44:41Z

controlplane/kubeadm/controllers/controller.go

 	managementClusterUncached internal.ManagementCluster
+
+	uncachedClient client.Reader


Can we dedupe uncachedClient with managementClusterUncached? We can add more methods to internal.ManagementCluster if needed

controlplane/kubeadm/controllers/controller.go

vincepri · 2020-05-13T14:56:13Z

test/framework/convenience.go

+// TypeToKindChecked returns the Kind without the package prefix. Pass in a pointer to a struct
+// This will panic if used incorrectly.
+func TypeToKindChecked(i runtime.Object) string {
+	return reflect.ValueOf(i).Elem().Type().Name()
+}


Suggested change

// TypeToKindChecked returns the Kind without the package prefix. Pass in a pointer to a struct

// This will panic if used incorrectly.

func TypeToKindChecked(i runtime.Object) string {

return reflect.ValueOf(i).Elem().Type().Name()

}

// ObjectToKind returns the Kind without the package prefix. Pass in a pointer to a struct

// This will panic if used incorrectly.

func ObjectToKind(i runtime.Object) string {

return reflect.ValueOf(i).Elem().Type().Name()

}

util/util.go

vincepri · 2020-05-13T15:01:47Z

util/util.go

+}
+
+// Returns true if ref refers to obj
+func refersTo(ref *metav1.OwnerReference, obj Object) bool {


This internal method and the other one are almost the same :/

Yeah, interface vs struct in the second position. I'll move 'em next to each other at least.

vincepri · 2020-05-13T15:02:38Z

util/util.go

+type Object interface {
+	metav1.Object
+	GroupVersionKind() schema.GroupVersionKind
+}


Can we make this interface private? There is kubernetes-sigs/controller-runtime#898 upstream that we might be able to use at some point, although I don't want to expose this interface just yet (and make it an API)

vincepri · 2020-05-13T15:03:25Z

util/util.go

+// PointsToObject returns true if any of the owner references point to the given target
+func PointsToObject(obj metav1.Object, target Object) bool {


PointsToObject is a little too generic, should this be IsOwnedByObject?

util/util.go

k8s-ci-robot · 2020-05-14T20:04:51Z

@sethp-nr: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-capd-e2e	`6c5538d`	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.

vincepri · 2020-05-26T14:26:14Z

util/util.go

+type object interface {
+	metav1.Object
+	GroupVersionKind() schema.GroupVersionKind
+}


If we rebase on master, we should be able to consume controllerutil.Object from kubernetes-sigs/controller-runtime#898

vincepri · 2020-05-26T16:57:43Z

/milestone v0.3.7

The KCP controller identifies Machines that belong to the control plane of an existing cluster and adopts them, including finding PKI materials that may be owned by the machine's bootstrap config and pivoting their ownership to the KCP as well. Prior to adopting machines (which, if unsuccessful, will block the KCP from taking any management actions), it runs a number of safety checks including: - Ensuring the KCP has not been deleted (to prevent re-adoption of orphans, though this process races with the garbage collector) - Checking that the machine's bootstrap provider was KubeadmConfig - Verifying that the Machine is no further than one minor version off of the KCP's spec Additionally, we set set a "best guess" value for the kubeadm.controlplane.cluster.x-k8s.io/hash on the adopted machine as if it were generated by a KCP in the past. The intent is that a KCP will adopt machines matching its "spec" (to the best of its ability) without modification, which in practice works well for adopting machines with the same spec'd version. We make an educated guess at what parts of the kubeadm config the operator considers important, and feed that into the hash function. The goal here is to be able to: 1. Create a KCP over existing machines with ~the same configuration (for then new api/equality's package definition of the same) and have the KCP make no changes 2. Create a KCP over existing machines with a different configuration and have the KCP upgrade those machines to the new config Finally, replaces PointsTo and introduces IsControlledBy, both of which search through the owner reference list of their first argument (similar to metav1.IsControlledBy). Unlike the metav1 function, they're looking for a match on name plus api group (not version) and kind. PointsTo returns true if any OwnerReference matches the pointee, whereas IsControlledBy only returns true if the sole permitted (by the API) controller reference matches. fix: handle JoinConfiguration hashing refactor: util should not check UIDs This is a behavior change not least for tests, which typically do not set either Kind or GroupVersion on various objects that end up acting as referents. Co-Authored-By: mnguyen <mnguyen@newrelic.com> Co-Authored-By: Jason DeTiberus <detiberusj@vmware.com> Co-authored-by: Vince Prignano <vince@vincepri.com>

vincepri

/approve

k8s-ci-robot · 2020-05-26T18:16:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, sethp-nr, 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

vincepri · 2020-05-27T01:12:46Z

/lgtm

k8s-ci-robot requested review from CecileRobertMichon and ncdc February 29, 2020 00:25

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

k8s-ci-robot added this to the v0.3.0 milestone Mar 5, 2020

k8s-ci-robot assigned vincepri Mar 5, 2020

sethp-nr mentioned this pull request Mar 6, 2020

KCP: validate version changes are no further than one minor version #2578

Closed

k8s-ci-robot added 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 Mar 7, 2020

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 6c81d4a to c37afb5 Compare March 7, 2020 00:37

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

sethp-nr changed the title ~~[wip] ✨ KCP adopts existing machines~~ ✨ KCP adopts existing machines Mar 7, 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 Mar 7, 2020

sethp-nr force-pushed the feat/kcp-machine-adoption branch 2 times, most recently from cb435e1 to 17da6e3 Compare March 7, 2020 01:08

vincepri reviewed Mar 9, 2020

View reviewed changes

detiber reviewed Mar 9, 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 Mar 9, 2020

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 8ced727 to 5b0e5e9 Compare March 9, 2020 18:09

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed 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. labels Mar 9, 2020

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 5b0e5e9 to ab53134 Compare March 9, 2020 18:21

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 6c5538d to 891f4e8 Compare May 12, 2020 22:52

vincepri reviewed May 13, 2020

View reviewed changes

sethp-nr force-pushed the feat/kcp-machine-adoption branch 3 times, most recently from 5a11862 to 22d7846 Compare May 14, 2020 18:09

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

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 22d7846 to 78b1795 Compare May 14, 2020 19:51

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

sethp-nr force-pushed the feat/kcp-machine-adoption branch from 78b1795 to bf58224 Compare May 15, 2020 16:27

vincepri reviewed May 26, 2020

View reviewed changes

nrb mentioned this pull request May 26, 2020

Re-test restore of CAPI clusters with upstream fix to allow machine adoption vmware-tanzu/velero#2554

Closed

k8s-ci-robot modified the milestones: v0.3.x, v0.3.7 May 26, 2020

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

sethp-nr force-pushed the feat/kcp-machine-adoption branch from bf58224 to f792d74 Compare May 26, 2020 18:14

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

vincepri approved these changes May 26, 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 May 26, 2020

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

k8s-ci-robot merged commit b00dfde into kubernetes-sigs:master May 27, 2020

		managementClusterUncached internal.ManagementCluster

		uncachedClient client.Reader

		// PointsToObject returns true if any of the owner references point to the given target
		func PointsToObject(obj metav1.Object, target Object) bool {

✨ KCP adopts existing machines #2489

✨ KCP adopts existing machines #2489

Conversation

sethp-nr commented Feb 29, 2020 • edited by vincepri Loading

TODO

k8s-ci-robot commented Feb 29, 2020

TODO

vincepri commented Mar 5, 2020

vincepri commented Mar 5, 2020

sethp-nr commented Mar 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethp-nr commented May 12, 2020

vincepri commented May 12, 2020

sethp-nr commented May 12, 2020 • edited Loading

randomvariable commented May 13, 2020

vincepri commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 14, 2020 • edited Loading

Choose a reason for hiding this comment

vincepri commented May 26, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 26, 2020

vincepri commented May 27, 2020

sethp-nr commented Feb 29, 2020 •

edited by vincepri

Loading

sethp-nr commented May 12, 2020 •

edited

Loading

k8s-ci-robot commented May 14, 2020 •

edited

Loading