Merge pull request #2519 from vincepri/requeue-cp

🐛Requeue while cluster's control plane object is being deleted
kubernetes-sigs · Mar 4, 2020 · 3ccb9b9 · 3ccb9b9
2 parents 42ef987 + d27e4a3
commit 3ccb9b9
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 18 deletions.
diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go
@@ -206,22 +206,6 @@ func (r *ClusterReconciler) reconcileMetrics(_ context.Context, cluster *cluster
 func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) {
 	logger := r.Log.WithValues("cluster", cluster.Name, "namespace", cluster.Namespace)
 
-	// First handle the control plane
-	if cluster.Spec.ControlPlaneRef != nil {
-		obj, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Namespace)
-		switch {
-		case apierrors.IsNotFound(errors.Cause(err)):
-		case err != nil:
-			return reconcile.Result{}, err
-		case obj.GetDeletionTimestamp().IsZero():
-			if err := r.Client.Delete(ctx, obj); err != nil {
-				return ctrl.Result{}, errors.Wrapf(err,
-					"failed to delete %v %q for Cluster %q in namespace %q",
-					obj.GroupVersionKind(), obj.GetName(), cluster.Name, cluster.Namespace)
-			}
-		}
-	}
-
 	descendants, err := r.listDescendants(ctx, cluster)
 	if err != nil {
 		logger.Error(err, "Failed to list descendants")
@@ -273,6 +257,30 @@ func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 		return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil
 	}
 
+	if cluster.Spec.ControlPlaneRef != nil {
+		obj, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Namespace)
+		switch {
+		case apierrors.IsNotFound(errors.Cause(err)):
+			// All good - the control plane resource has been deleted
+		case err != nil:
+			return reconcile.Result{}, errors.Wrapf(err, "failed to get %s %q for Cluster %s/%s",
+				path.Join(cluster.Spec.ControlPlaneRef.APIVersion, cluster.Spec.ControlPlaneRef.Kind),
+				cluster.Spec.ControlPlaneRef.Name, cluster.Namespace, cluster.Name)
+		default:
+			// Issue a deletion request for the control plane object.
+			// Once it's been deleted, the cluster will get processed again.
+			if err := r.Client.Delete(ctx, obj); err != nil {
+				return ctrl.Result{}, errors.Wrapf(err,
+					"failed to delete %v %q for Cluster %q in namespace %q",
+					obj.GroupVersionKind(), obj.GetName(), cluster.Name, cluster.Namespace)
+			}
+
+			// Return here so we don't remove the finalizer yet.
+			logger.Info("Cluster still has descendants - need to requeue", "controlPlaneRef", cluster.Spec.ControlPlaneRef.Name)
+			return ctrl.Result{}, nil
+		}
+	}
+
 	if cluster.Spec.InfrastructureRef != nil {
 		obj, err := external.Get(ctx, r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
 		switch {
@@ -292,6 +300,7 @@ func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 			}
 
 			// Return here so we don't remove the finalizer yet.
+			logger.Info("Cluster still has descendants - need to requeue", "infrastructureRef", cluster.Spec.InfrastructureRef.Name)
 			return ctrl.Result{}, nil
 		}
 	}
@@ -372,8 +381,12 @@ func (r *ClusterReconciler) listDescendants(ctx context.Context, cluster *cluste
 
 	// Split machines into control plane and worker machines so we make sure we delete control plane machines last
 	controlPlaneMachines, workerMachines := splitMachineList(&machines)
-	descendants.controlPlaneMachines = *controlPlaneMachines
 	descendants.workerMachines = *workerMachines
+	// Only count control plane machines as descendants if there is no control plane provider.
+	if cluster.Spec.ControlPlaneRef == nil {
+		descendants.controlPlaneMachines = *controlPlaneMachines
+
+	}
 
 	return descendants, nil
 }

diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go
@@ -465,7 +465,7 @@ func (r *MachineReconciler) reconcileDeleteExternal(ctx context.Context, m *clus
 }
 
 func (r *MachineReconciler) shouldAdopt(m *clusterv1.Machine) bool {
-	return !util.HasOwner(m.OwnerReferences, clusterv1.GroupVersion.String(), []string{"MachineSet", "Cluster"})
+	return metav1.GetControllerOf(m) == nil && !util.HasOwner(m.OwnerReferences, clusterv1.GroupVersion.String(), []string{"Cluster"})
 }
 
 // writer implements io.Writer interface as a pass-through for klog.

diff --git a/controllers/machine_controller_test.go b/controllers/machine_controller_test.go
@@ -169,6 +169,32 @@ func TestMachineOwnerReference(t *testing.T) {
 					APIVersion: clusterv1.GroupVersion.String(),
 					Kind:       "MachineSet",
 					Name:       "valid-machineset",
+					Controller: pointer.BoolPtr(true),
+				},
+			},
+		},
+		Spec: clusterv1.MachineSpec{
+			Bootstrap: clusterv1.Bootstrap{
+				Data: &bootstrapData,
+			},
+			ClusterName: "test-cluster",
+		},
+	}
+
+	machineValidControlled := &clusterv1.Machine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "machine4",
+			Namespace: "default",
+			Labels: map[string]string{
+				clusterv1.ClusterLabelName:             "valid-cluster",
+				clusterv1.MachineControlPlaneLabelName: "",
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: "test.group",
+					Kind:       "KubeadmControlPlane",
+					Name:       "valid-controlplane",
+					Controller: pointer.BoolPtr(true),
 				},
 			},
 		},
@@ -212,6 +238,22 @@ func TestMachineOwnerReference(t *testing.T) {
 					APIVersion: clusterv1.GroupVersion.String(),
 					Kind:       "MachineSet",
 					Name:       "valid-machineset",
+					Controller: pointer.BoolPtr(true),
+				},
+			},
+		},
+		{
+			name: "should not add cluster owner reference if machine has a controller owner",
+			request: reconcile.Request{
+				NamespacedName: util.ObjectKey(machineValidControlled),
+			},
+			m: machineValidControlled,
+			expectedOR: []metav1.OwnerReference{
+				{
+					APIVersion: "test.group",
+					Kind:       "KubeadmControlPlane",
+					Name:       "valid-controlplane",
+					Controller: pointer.BoolPtr(true),
 				},
 			},
 		},
@@ -228,6 +270,7 @@ func TestMachineOwnerReference(t *testing.T) {
 					machineInvalidCluster,
 					machineValidCluster,
 					machineValidMachine,
+					machineValidControlled,
 				),
 				Log:    log.Log,
 				scheme: scheme.Scheme,