🐛 Wait for all descendants when deleting a cluster #1650

ncdc · 2019-10-24T18:31:07Z

What this PR does / why we need it:

Instead of waiting for all direct descendants to be deleted and then
allowing cluster deletion to proceed (by removing the finalizer), wait
for all descendants (both direct and indirect) to be removed before
allowing cluster deletion to proceed. There can be race conditions where
there are indirect descendants (machines belonging to a machine set)
that still exist, and they need the cluster to remain so they can be
deleted properly.

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

Alternative to #1644

k8s-ci-robot · 2019-10-24T18:31:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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

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

ncdc · 2019-10-24T18:33:54Z

If y'all are 👍 on this, I'll see about adding some unit tests

vincepri · 2019-10-24T18:43:01Z

controllers/cluster_controller.go

+}
+
+// empty returns true if all the lists are empty.
+func (c *clusterDescendants) empty() bool {


vincepri · 2019-10-24T18:46:31Z

controllers/cluster_controller.go

+}
+
+// listAllClusterDescendants returns a list of all MachineDeployments, MachineSets, and Machines for the cluster.
+func (r *ClusterReconciler) listAllClusterDescendants(ctx context.Context, cluster *clusterv1.Cluster) (clusterDescendants, error) {


Suggested change

func (r *ClusterReconciler) listAllClusterDescendants(ctx context.Context, cluster *clusterv1.Cluster) (clusterDescendants, error) {

func (r *ClusterReconciler) listDescendants(ctx context.Context, cluster *clusterv1.Cluster) (clusterDescendants, error) {

Given that this is the cluster reconciler, cluster can probably be omitted from the name

vincepri · 2019-10-24T18:47:14Z

controllers/cluster_controller.go

+	return descendants, nil
+}
+
+func (r *ClusterReconciler) extractDirectClusterDescendants(input clusterDescendants, cluster *clusterv1.Cluster) ([]runtime.Object, error) {


Suggested change

func (r *ClusterReconciler) extractDirectClusterDescendants(input clusterDescendants, cluster *clusterv1.Cluster) ([]runtime.Object, error) {

func (r *ClusterReconciler) filterOwnedDescendants(input clusterDescendants, cluster *clusterv1.Cluster) ([]runtime.Object, error) {

?

Let's also add a godoc for this method

Should this be a method on clusterDescendants?

probably yeah

vincepri · 2019-10-24T18:47:56Z

controllers/cluster_controller.go

+	logger := r.Log.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)
+
+	// Split machines into control plane and worker machines so we make sure we delete control plane machines last
+	controlPlaneMachines, machines := splitMachineList(&input.machines)

 	var children []runtime.Object


Suggested change

var children []runtime.Object

var ownedDescendants []runtime.Object

ncdc · 2019-10-24T18:49:35Z

WDYT of the overall approach? @detiber @liztio @vincepri @xrmzju?

vincepri · 2019-10-24T18:55:10Z

I'm +1, seems pretty straightforward, the review I did is mostly nits. The logic looks good

detiber · 2019-10-24T18:54:34Z

controllers/cluster_controller.go

+	return descendants, nil
+}
+
+func (r *ClusterReconciler) extractDirectClusterDescendants(input clusterDescendants, cluster *clusterv1.Cluster) ([]runtime.Object, error) {


Should this be a method on clusterDescendants?

detiber · 2019-10-24T18:59:04Z

controllers/cluster_controller.go

+	logger := r.Log.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)
+
+	// Split machines into control plane and worker machines so we make sure we delete control plane machines last
+	controlPlaneMachines, machines := splitMachineList(&input.machines)


Maybe split these within clusterDescendants as well instead of here?

ncdc · 2019-10-24T20:27:47Z

controllers/cluster_controller.go

+}
+
+func (r *ClusterReconciler) extractDirectClusterDescendants(input clusterDescendants, cluster *clusterv1.Cluster) ([]runtime.Object, error) {
+	logger := r.Log.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)


How much do y'all care about logging an error if we couldn't create the meta.Accessor?

I'm fine removing it

ncdc · 2019-10-24T20:31:28Z

controllers/cluster_controller.go

-func (r *ClusterReconciler) listChildren(ctx context.Context, cluster *clusterv1.Cluster) ([]runtime.Object, error) {
-	logger := r.Log.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)
+type clusterDescendants struct {
+	machineDeployments clusterv1.MachineDeploymentList


Should these lists be pointers or is it ok for them not to be?

They don't have to be afaict

+1, worse comes to worse it becomes a performance concern down the line, but that would require some really large clusters anyway.

vincepri

One nit, other than that changes LGTM.

Seems you need to run make modules, if that doesn't change any file locally, try using Go 1.12

vincepri · 2019-10-25T14:13:59Z

controllers/cluster_controller.go

+	workerMachines       clusterv1.MachineList
+}
+
+// isEmpty returns true if all the lists are isEmpty.


Suggested change

// isEmpty returns true if all the lists are isEmpty.

// isEmpty returns true if all the lists are empty.

Thanks, my find and replace was too aggressive.

ncdc · 2019-10-25T14:18:19Z

Yeah I'm working on the modules. There's no diff. Hence my new commit to figure out what it's complaining about. And I'm using 1.12.9.

ncdc · 2019-10-25T15:31:52Z

/hold

Would like @xrmzju to review

xrmzju · 2019-10-27T02:55:06Z

LGTM expect a little log suggestion

controllers/cluster_controller.go

ncdc · 2019-10-28T14:32:28Z

If this looks ok, I will squash. @vincepri

vincepri · 2019-10-28T14:39:33Z

go for it, lgtm

Instead of waiting for all direct descendants to be deleted and then allowing cluster deletion to proceed (by removing the finalizer), wait for all descendants (both direct and indirect) to be removed before allowing cluster deletion to proceed. There can be race conditions where there are indirect descendants (machines belonging to a machine set) that still exist, and they need the cluster to remain so they can be deleted properly. Signed-off-by: Andy Goldstein <goldsteina@vmware.com>

vincepri

/hold cancel
/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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 24, 2019

k8s-ci-robot requested review from chuckha and detiber October 24, 2019 18:31

ncdc mentioned this pull request Oct 24, 2019

machine remains undeleted while cluster has been deleted #1643

Closed

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

ncdc mentioned this pull request Oct 24, 2019

:running delete child with foreground policy to ensure resource deleted #1644

Closed

vincepri reviewed Oct 24, 2019

View reviewed changes

detiber reviewed Oct 24, 2019

View reviewed changes

ncdc commented Oct 24, 2019

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 24, 2019

ncdc force-pushed the delete-cluster-wait-for-all-descendants branch from 91b48cd to fb02873 Compare October 25, 2019 14:08

ncdc changed the title ~~[WIP] 🐛 Wait for all descendants when deleting a cluster~~ 🐛 Wait for all descendants when deleting a cluster Oct 25, 2019

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 Oct 25, 2019

ncdc added this to the v0.3.0 milestone Oct 25, 2019

vincepri reviewed Oct 25, 2019

View reviewed changes

ncdc force-pushed the delete-cluster-wait-for-all-descendants branch 3 times, most recently from 5d6c608 to 63388cc Compare October 25, 2019 15:24

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

xrmzju suggested changes Oct 27, 2019

View reviewed changes

controllers/cluster_controller.go Outdated Show resolved Hide resolved

xrmzju approved these changes Oct 28, 2019

View reviewed changes

ncdc force-pushed the delete-cluster-wait-for-all-descendants branch from 485cbbc to a193a17 Compare October 28, 2019 14:43

vincepri approved these changes Oct 28, 2019

View reviewed changes

k8s-ci-robot assigned vincepri Oct 28, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 28, 2019

k8s-ci-robot merged commit 3a9008d into kubernetes-sigs:master Oct 28, 2019

ncdc deleted the delete-cluster-wait-for-all-descendants branch December 3, 2019 20:31

ncdc mentioned this pull request Dec 11, 2019

🐛 [0.2] Wait for all descendants when deleting a cluster #1877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Wait for all descendants when deleting a cluster #1650

🐛 Wait for all descendants when deleting a cluster #1650

ncdc commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

ncdc commented Oct 24, 2019

vincepri Oct 24, 2019

vincepri Oct 24, 2019

vincepri Oct 24, 2019

vincepri Oct 24, 2019

detiber Oct 24, 2019

vincepri Oct 24, 2019

vincepri Oct 24, 2019

ncdc commented Oct 24, 2019

vincepri commented Oct 24, 2019

detiber Oct 24, 2019

detiber Oct 24, 2019

ncdc Oct 24, 2019

vincepri Oct 24, 2019

ncdc Oct 24, 2019

vincepri Oct 24, 2019

detiber Oct 24, 2019

vincepri left a comment

vincepri Oct 25, 2019

ncdc Oct 25, 2019

ncdc commented Oct 25, 2019

ncdc commented Oct 25, 2019

xrmzju commented Oct 27, 2019

ncdc commented Oct 28, 2019

vincepri commented Oct 28, 2019

vincepri left a comment

	func (r ClusterReconciler) listAllClusterDescendants(ctx context.Context, cluster clusterv1.Cluster) (clusterDescendants, error) {
	func (r ClusterReconciler) listDescendants(ctx context.Context, cluster clusterv1.Cluster) (clusterDescendants, error) {

	func (r ClusterReconciler) extractDirectClusterDescendants(input clusterDescendants, cluster clusterv1.Cluster) ([]runtime.Object, error) {
	func (r ClusterReconciler) filterOwnedDescendants(input clusterDescendants, cluster clusterv1.Cluster) ([]runtime.Object, error) {

	var children []runtime.Object
	var ownedDescendants []runtime.Object

	// isEmpty returns true if all the lists are isEmpty.
	// isEmpty returns true if all the lists are empty.

🐛 Wait for all descendants when deleting a cluster #1650

🐛 Wait for all descendants when deleting a cluster #1650

Conversation

ncdc commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

ncdc commented Oct 24, 2019

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

ncdc commented Oct 24, 2019

vincepri commented Oct 24, 2019

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

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 25, 2019

ncdc commented Oct 25, 2019

xrmzju commented Oct 27, 2019

ncdc commented Oct 28, 2019

vincepri commented Oct 28, 2019

vincepri left a comment

Choose a reason for hiding this comment