From d27e4a35ba16a921d577bc0d53068f78e8949ed6 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Tue, 3 Mar 2020 17:25:08 -0800 Subject: [PATCH] :bug: Requeue while cluster's control plane object is being deleted Signed-off-by: Vince Prignano --- controllers/cluster_controller.go | 47 ++++++++++++++++---------- controllers/machine_controller.go | 2 +- controllers/machine_controller_test.go | 43 +++++++++++++++++++++++ 3 files changed, 74 insertions(+), 18 deletions(-) diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go index 4d3fbde14ea0..0d18941fd6c9 100644 --- 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 index 55b4d5da683d..14b9b3becbba 100644 --- 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 index 7b91822ffc62..ca7bcfd8cbbb 100644 --- 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,