🐛 Fix node deletion under managed control planes #3673

dthorsen · 2020-09-22T00:22:23Z

What this PR does / why we need it:
This PR allows the Machine controller to drain and delete Nodes when running under managed control planes.

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

k8s-ci-robot · 2020-09-22T00:22:31Z

Welcome @dthorsen!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. 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 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. 😃

k8s-ci-robot · 2020-09-22T00:22:32Z

Hi @dthorsen. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

dthorsen · 2020-09-22T00:27:09Z

@vincepri As far as I know node objects for control plane are only used under a kubeadm control plane. EKS and GKE don't list control plane nodes.

controllers/machine_controller.go

vincepri · 2020-09-25T20:57:45Z

/ok-to-test

controllers/machine_controller.go

controllers/machine_helpers.go

controllers/machine_controller.go

vincepri

/approve
/assign @ncdc

k8s-ci-robot · 2020-09-25T21:40:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

dthorsen · 2020-09-28T17:53:45Z

@vincepri or @ncdc It occurred to me that maybe returning an error here:

https://github.com/kubernetes-sigs/cluster-api/pull/3673/files#diff-7ea69dfdf2e12f7bff0fb38e0c59e0b8R384

may also be a breaking change for non-externally-managed control plane clusters in the case of a 404. I think if this error is a 404 we should simply skip the call to util.IsExternalManagedControlPlane(controlPlane) and only return the error if it is any error other than a 404. This will probably avoid some edge cases where things could get stuck unable to drain correctly.

vincepri · 2020-09-28T18:11:43Z

Good catch, we should fallback to the old logic (checking machines) if the error is NotFound

dthorsen · 2020-09-29T12:47:38Z

@vincepri @ncdc I added the check for a NotFound error.

dthorsen · 2020-09-29T13:00:46Z

/test pull-cluster-api-test

vincepri · 2020-09-29T13:26:38Z

/milestone v0.3.10

vincepri · 2020-09-29T14:44:06Z

/assign @CecileRobertMichon

fabriziopandini

lgtm for me, only a small thing that can be also addressed in a follow-up PR

fabriziopandini · 2020-09-29T15:50:27Z

util/util.go

+// IsExternalManagedControlPlane returns a bool indicating whether the control plane referenced
+// in the passed Unstructured resource is an externally managed control plane such as AKS, EKS, GKE, etc.
+func IsExternalManagedControlPlane(controlPlane *unstructured.Unstructured) bool {
+	managed, found, err := unstructured.NestedBool(controlPlane.Object, "status", "externalManagedControlPlane")


Should we document somewhere that manage control plane implementation should have this field (might be in https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane.html#required-status-fields)

+1 this isn't part of the contract currently AFAIK

cc @alexeldeib

+1 updating the documentation, just a note though the field is completely optional and there is a fallback to the current behavior during delete

Makes sense. What are thoughts on making this new field be an optional part of the contract to avoid a breaking change?

This is an unusual case in that the field is really required for managed control planes like AKS, EKS, GKE, etc otherwise the Machine controller will suffer this pretty bad bug (instance termination without draining nodes), but not required at all for KubeadmControlPlane. No other optional status fields currently appear to modify functionality and are purely informational. If we make it a new required status field though, I think this would need to wait for a new minor version to be released. I'd like to get the bugfix released sooner rather than later if possible.

I'm 👍 to optional. Please update the control plane architecture doc linked above and add it to the list of optional status fields, assuming everyone else agrees.

Added a blurb to the docs:
https://github.com/kubernetes-sigs/cluster-api/pull/3673/files#diff-cd671ed1b58bf2cf9d0a69033c9a764dR181-R183

CecileRobertMichon · 2020-09-29T19:07:59Z

docs/book/src/developer/architecture/controllers/control-plane.md


 * `failureReason` - is a string that explains why an error has occurred, if possible.
 * `failureMessage` - is a string that holds the message contained by the error.
+* `externalManagedControlPlane` - is a bool that should be set to true if the Node objects do not
+  exist in the cluster. For example, managed control plane providers for AKS, EKS, GKE, etc, should


One small note: the AKS implementation doesn't have a separate control plane provider, see https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/19e80de4a7e92082ca0c5eb672e486801da23766/templates/cluster-template-aks.yaml

Would that still work as a field on the AzureManagedControlPlane https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/995da71c822c201cfa72cf99ae1b96652544e352/exp/api/v1alpha3/azuremanagedcontrolplane_types.go#L79 as part of the Azure infra provider?

(I think it should since we're using controlPlaneRef the same way)

@CecileRobertMichon Yes, as long as you reference the AzureManagedControlPlane in the cluster's controlPlaneRef field you should be able to use the status in your resource as well. Since the check here in CAPI is using the unstructured API, the underlying type is not important. Whatever object is referenced by controlPlaneRef just needs to have the status field externalManagedControlPlane if there are no control plane Machine resources in the cluster.

CecileRobertMichon

lgtm

fabriziopandini · 2020-09-30T09:55:08Z

/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 Sep 22, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 22, 2020

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 22, 2020

k8s-ci-robot requested review from justinsb and vincepri September 22, 2020 00:22

vincepri reviewed Sep 22, 2020

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

dthorsen changed the title ~~🐛 Always delete nodes except under kubeadm control plane~~ 🐛 Always delete nodes except under kubeadm control plane Sep 22, 2020

dthorsen force-pushed the issue-3631 branch from fe93b72 to 35a0ef6 Compare September 25, 2020 20:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 25, 2020

dthorsen changed the title ~~🐛 Always delete nodes except under kubeadm control plane~~ 🐛 Fix node deletion under managed control planes Sep 25, 2020

dthorsen force-pushed the issue-3631 branch 2 times, most recently from 8dd9af0 to 64eda6d Compare September 25, 2020 20:43

dthorsen marked this pull request as ready for review September 25, 2020 20:45

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 Sep 25, 2020

dthorsen requested a review from vincepri September 25, 2020 20:45

dthorsen mentioned this pull request Sep 25, 2020

CAPI won't delete Nodes in "managed" control planes because there are no control plane machines #3631

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2020

vincepri reviewed Sep 25, 2020

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

controllers/machine_controller.go Outdated Show resolved Hide resolved

controllers/machine_helpers.go Outdated Show resolved Hide resolved

vincepri reviewed Sep 25, 2020

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

dthorsen force-pushed the issue-3631 branch from bca0fa3 to 1398f8c Compare September 25, 2020 21:34

vincepri reviewed Sep 25, 2020

View reviewed changes

controllers/machine_controller.go Outdated Show resolved Hide resolved

controllers/machine_controller.go Outdated Show resolved Hide resolved

vincepri approved these changes Sep 25, 2020

View reviewed changes

k8s-ci-robot assigned ncdc Sep 25, 2020

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

dthorsen force-pushed the issue-3631 branch from 1b5e83a to 6a9472b Compare September 29, 2020 12:45

k8s-ci-robot added this to the v0.3.10 milestone Sep 29, 2020

k8s-ci-robot assigned CecileRobertMichon Sep 29, 2020

fabriziopandini reviewed Sep 29, 2020

View reviewed changes

dthorsen force-pushed the issue-3631 branch 2 times, most recently from 11ae6a8 to 717e5e4 Compare September 29, 2020 17:19

🐛 Fix node deletion under managed control planes

cddd57b

dthorsen force-pushed the issue-3631 branch from 717e5e4 to cddd57b Compare September 29, 2020 17:32

CecileRobertMichon reviewed Sep 29, 2020

View reviewed changes

CecileRobertMichon approved these changes Sep 29, 2020

View reviewed changes

k8s-ci-robot assigned fabriziopandini Sep 30, 2020

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

k8s-ci-robot merged commit 5754973 into kubernetes-sigs:master Sep 30, 2020

dthorsen mentioned this pull request Oct 14, 2020

REQUEST: New membership for dthorsen kubernetes/org#2266

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix node deletion under managed control planes #3673

🐛 Fix node deletion under managed control planes #3673

dthorsen commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020

dthorsen commented Sep 22, 2020

vincepri commented Sep 25, 2020

vincepri left a comment

k8s-ci-robot commented Sep 25, 2020

dthorsen commented Sep 28, 2020

vincepri commented Sep 28, 2020

dthorsen commented Sep 29, 2020

dthorsen commented Sep 29, 2020

vincepri commented Sep 29, 2020

vincepri commented Sep 29, 2020

fabriziopandini left a comment •

edited

fabriziopandini Sep 29, 2020

CecileRobertMichon Sep 29, 2020

vincepri Sep 29, 2020

dthorsen Sep 29, 2020

dthorsen Sep 29, 2020

ncdc Sep 29, 2020

dthorsen Sep 29, 2020

CecileRobertMichon Sep 29, 2020

CecileRobertMichon Sep 29, 2020

dthorsen Sep 29, 2020

CecileRobertMichon left a comment

fabriziopandini commented Sep 30, 2020

🐛 Fix node deletion under managed control planes #3673

🐛 Fix node deletion under managed control planes #3673

Conversation

dthorsen commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020

dthorsen commented Sep 22, 2020

vincepri commented Sep 25, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2020

dthorsen commented Sep 28, 2020

vincepri commented Sep 28, 2020

dthorsen commented Sep 29, 2020

dthorsen commented Sep 29, 2020

vincepri commented Sep 29, 2020

vincepri commented Sep 29, 2020

fabriziopandini left a comment • edited

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

CecileRobertMichon left a comment

Choose a reason for hiding this comment

fabriziopandini commented Sep 30, 2020

fabriziopandini left a comment •

edited