✨ Cleanup cluster reconciler normal/delete

Signed-off-by: Vince Prignano <vincepri@vmware.com>

controllers/cluster_controller.go

@@ -91,25 +91,26 @@ func (r *ClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e
 		return ctrl.Result{}, err
 	}
 
-	// Initialize the patch helper
+	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(cluster, r)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	// Always attempt to Patch the Cluster object and status after each reconciliation.
+
 	defer func() {
+		// Always reconcile the Status.Phase field.
+		r.reconcilePhase(ctx, cluster)
+
+		// Always attempt to Patch the Cluster object and status after each reconciliation.
 		if err := patchHelper.Patch(ctx, cluster); err != nil {
 			if reterr == nil {
 				reterr = err
 			}
 		}
 	}()
 
-	// If object hasn't been deleted and doesn't have a finalizer, add one.
-	if cluster.ObjectMeta.DeletionTimestamp.IsZero() {
-		if !util.Contains(cluster.Finalizers, clusterv1.ClusterFinalizer) {
-			cluster.Finalizers = append(cluster.ObjectMeta.Finalizers, clusterv1.ClusterFinalizer)
-		}
+	if !cluster.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(ctx, cluster)
 	}
 
 	if err := r.reconcile(ctx, cluster); err != nil {
@@ -120,15 +121,33 @@ func (r *ClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e
 		return ctrl.Result{}, err
 	}
 
-	if !cluster.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, cluster)
+	return ctrl.Result{}, nil
+}
+
+// reconcile handles cluster reconciliation.
+func (r *ClusterReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster) (err error) {
+	// If object doesn't have a finalizer, add one.
+	if !util.Contains(cluster.Finalizers, clusterv1.ClusterFinalizer) {
+		cluster.Finalizers = append(cluster.ObjectMeta.Finalizers, clusterv1.ClusterFinalizer)
 	}
 
-	return ctrl.Result{}, nil
+	// TODO(vincepri): These can be generalized with an interface and possibly a for loop.
+	errors := []error{}
+	errors = append(errors, r.reconcileInfrastructure(ctx, cluster))
+
+	// Determine the return error, giving precedence to the first non-nil and non-requeueAfter errors.
+	for _, e := range errors {
+		if e == nil {
+			continue
+		}
+		if err == nil || capierrors.IsRequeueAfter(err) {
+			err = e
+		}
+	}
+	return err
 }
 
 // reconcileDelete handles cluster deletion.
-// TODO(ncdc): consolidate all deletion logic in here.
 func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) {
 	children, err := r.listChildren(ctx, cluster)
 	if err != nil {
@@ -172,7 +191,7 @@ func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 	}
 
 	if cluster.Spec.InfrastructureRef != nil {
-		_, err := external.Get(r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
+		obj, err := external.Get(r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
 		switch {
 		case apierrors.IsNotFound(err):
 			// All good - the infra resource has been deleted
@@ -181,14 +200,20 @@ func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 				path.Join(cluster.Spec.InfrastructureRef.APIVersion, cluster.Spec.InfrastructureRef.Kind),
 				cluster.Spec.InfrastructureRef.Name, cluster.Namespace, cluster.Name)
 		default:
-			// The infra resource still exists. Once it's been deleted, the cluster will get processed again.
+			// Issue a deletion request for the infrastructure object.
+			// Once it's been deleted, the cluster will get processed again.
+			if err := r.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.
 			return ctrl.Result{}, nil
 		}
 	}
 
 	cluster.Finalizers = util.Filter(cluster.Finalizers, clusterv1.ClusterFinalizer)
-
 	return ctrl.Result{}, nil
 }
 

controllers/cluster_controller_phases.go

@@ -37,25 +37,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 )
 
-func (r *ClusterReconciler) reconcile(ctx context.Context, cluster *v1alpha2.Cluster) (err error) {
-	// TODO(vincepri): These can be generalized with an interface and possibly a for loop.
-	errors := []error{}
-	errors = append(errors, r.reconcileInfrastructure(ctx, cluster))
-	errors = append(errors, r.reconcilePhase(ctx, cluster))
-
-	// Determine the return error, giving precedence to the first non-nil and non-requeueAfter errors.
-	for _, e := range errors {
-		if e == nil {
-			continue
-		}
-		if err == nil || capierrors.IsRequeueAfter(err) {
-			err = e
-		}
-	}
-	return err
-}
-
-func (r *ClusterReconciler) reconcilePhase(ctx context.Context, cluster *v1alpha2.Cluster) error {
+func (r *ClusterReconciler) reconcilePhase(ctx context.Context, cluster *v1alpha2.Cluster) {
 	// Set the phase to "pending" if nil.
 	if cluster.Status.Phase == "" {
 		cluster.Status.SetTypedPhase(v1alpha2.ClusterPhasePending)
@@ -80,17 +62,13 @@ func (r *ClusterReconciler) reconcilePhase(ctx context.Context, cluster *v1alpha
 	if !cluster.DeletionTimestamp.IsZero() {
 		cluster.Status.SetTypedPhase(v1alpha2.ClusterPhaseDeleting)
 	}
-
-	return nil
 }
 
 // reconcileExternal handles generic unstructured objects referenced by a Cluster.
 func (r *ClusterReconciler) reconcileExternal(ctx context.Context, cluster *v1alpha2.Cluster, ref *corev1.ObjectReference) (*unstructured.Unstructured, error) {
 	obj, err := external.Get(r.Client, ref, cluster.Namespace)
 	if err != nil {
-		if apierrors.IsNotFound(err) && !cluster.DeletionTimestamp.IsZero() {
-			return nil, nil
-		} else if apierrors.IsNotFound(err) {
+		if apierrors.IsNotFound(err) {
 			return nil, errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: 30 * time.Second},
 				"could not find %v %q for Cluster %q in namespace %q, requeuing",
 				ref.GroupVersionKind(), ref.Name, cluster.Name, cluster.Namespace)
@@ -100,16 +78,6 @@ func (r *ClusterReconciler) reconcileExternal(ctx context.Context, cluster *v1al
 
 	objPatch := client.MergeFrom(obj.DeepCopy())
 
-	// Delete the external object if the Cluster is being deleted.
-	if !cluster.DeletionTimestamp.IsZero() {
-		if err := r.Delete(ctx, obj); err != nil {
-			return nil, errors.Wrapf(err,
-				"failed to delete %v %q for Cluster %q in namespace %q",
-				obj.GroupVersionKind(), ref.Name, cluster.Name, cluster.Namespace)
-		}
-		return obj, nil
-	}
-
 	// Set external object OwnerReference to the Cluster.
 	ownerRef := metav1.OwnerReference{
 		APIVersion: clusterv1.GroupVersion.String(),
@@ -165,13 +133,11 @@ func (r *ClusterReconciler) reconcileInfrastructure(ctx context.Context, cluster
 
 	// Call generic external reconciler.
 	infraConfig, err := r.reconcileExternal(ctx, cluster, cluster.Spec.InfrastructureRef)
-	if infraConfig == nil && err == nil {
-		return nil
-	} else if err != nil {
+	if err != nil {
 		return err
 	}
 
-	if cluster.Status.InfrastructureReady || !infraConfig.GetDeletionTimestamp().IsZero() {
+	if cluster.Status.InfrastructureReady {
 		return nil
 	}
 

vendor/modules.txt

@@ -344,13 +344,13 @@ k8s.io/apimachinery/pkg/api/errors
 k8s.io/apimachinery/pkg/util/wait
 k8s.io/apimachinery/pkg/util/yaml
 k8s.io/apimachinery/pkg/types
-k8s.io/apimachinery/pkg/apis/meta/v1/unstructured
-k8s.io/apimachinery/pkg/runtime/serializer/streaming
 k8s.io/apimachinery/pkg/api/equality
 k8s.io/apimachinery/pkg/api/meta
+k8s.io/apimachinery/pkg/apis/meta/v1/unstructured
 k8s.io/apimachinery/pkg/util/errors
 k8s.io/apimachinery/pkg/util/rand
 k8s.io/apimachinery/pkg/util/sets
+k8s.io/apimachinery/pkg/runtime/serializer/streaming
 k8s.io/apimachinery/pkg/conversion
 k8s.io/apimachinery/pkg/conversion/queryparams
 k8s.io/apimachinery/pkg/util/json