Issue-10544 ignore unreachable cluster while deleting machinePool

Signed-off-by: melserngawy <[email protected]>

Author: serngawy

exp/internal/controllers/machinepool_controller.go

@@ -253,33 +253,52 @@ func (r *MachinePoolReconciler) reconcile(ctx context.Context, cluster *clusterv
 	return res, kerrors.NewAggregate(errs)
 }
 
-func (r *MachinePoolReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster, mp *expv1.MachinePool) error {
-	if ok, err := r.reconcileDeleteExternal(ctx, mp); !ok || err != nil {
-		// Return early and don't remove the finalizer if we got an error or
-		// the external reconciliation deletion isn't ready.
-		return err
-	}
+func (r *MachinePoolReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster, machinepool *expv1.MachinePool) error {
+	deleteAllowed, clusterClient, err := r.isDeleteMachinePoolAllowed(ctx, cluster)
+
+	// Check for cluster allowing delete or machinePool delete timeout.
+	if deleteAllowed || r.isMachinePoolDeleteTimeoutPassed(machinepool) {
+		if ok, err := r.reconcileDeleteExternal(ctx, machinepool); !ok || err != nil {
+			// Return early and don't remove the finalizer if we got an error or
+			// the external reconciliation deletion isn't ready.
+			return err
+		}
 
-	if err := r.reconcileDeleteNodes(ctx, cluster, mp); err != nil {
-		// Return early and don't remove the finalizer if we got an error.
-		return err
+		// Delete nodes when cluster accessor available & there are nodes to delete.
+		if len(machinepool.Status.NodeRefs) > 0 && clusterClient != nil {
+			if err = r.deleteRetiredNodes(ctx, clusterClient, machinepool.Status.NodeRefs, machinepool.Spec.ProviderIDList); err != nil {
+				return err
+			}
+		}
+		// Remove finalizer to delete the resource.
+		controllerutil.RemoveFinalizer(machinepool, expv1.MachinePoolFinalizer)
+
+		return nil
 	}
 
-	controllerutil.RemoveFinalizer(mp, expv1.MachinePoolFinalizer)
-	return nil
+	return err
 }
 
-func (r *MachinePoolReconciler) reconcileDeleteNodes(ctx context.Context, cluster *clusterv1.Cluster, machinepool *expv1.MachinePool) error {
-	if len(machinepool.Status.NodeRefs) == 0 {
-		return nil
+// isMachinePoolDeleteTimeoutPassed check the machinePool node delete time out.
+func (r *MachinePoolReconciler) isMachinePoolDeleteTimeoutPassed(machinepool *expv1.MachinePool) bool {
+	if !machinepool.DeletionTimestamp.IsZero() {
+		return machinepool.DeletionTimestamp.Add(machinepool.Spec.Template.Spec.NodeDeletionTimeout.Duration).After(time.Now())
 	}
+	return false
+}
 
-	clusterClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster))
-	if err != nil {
-		return err
+// isDeleteMachinePoolAllowed check the cluster status and target cluster client accessor state.
+func (r *MachinePoolReconciler) isDeleteMachinePoolAllowed(ctx context.Context, cluster *clusterv1.Cluster) (bool, client.Client, error) {
+	// Check if cluster is been deleted
+	if !cluster.DeletionTimestamp.IsZero() {
+		// return true when cluster is been deleted to delete external resources and skip delete nodes refs.
+		return true, nil, nil
 	}
 
-	return r.deleteRetiredNodes(ctx, clusterClient, machinepool.Status.NodeRefs, machinepool.Spec.ProviderIDList)
+	// Check if the target cluster client is reachable.
+	clusterClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster))
+
+	return err == nil, clusterClient, err
 }
 
 // reconcileDeleteExternal tries to delete external references, returning true if it cannot find any.

exp/internal/controllers/machinepool_controller_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -31,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/controllers/remote"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 	"sigs.k8s.io/cluster-api/util"
@@ -426,6 +428,7 @@ func TestReconcileMachinePoolRequest(t *testing.T) {
 			r := &MachinePoolReconciler{
 				Client:    clientFake,
 				APIReader: clientFake,
+				Tracker:   remote.NewTestClusterCacheTracker(ctrl.LoggerFrom(context.TODO()), clientFake, clientFake, clientFake.Scheme(), client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}),
 			}
 
 			result, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: util.ObjectKey(&tc.machinePool)})
@@ -591,8 +594,10 @@ func TestRemoveMachinePoolFinalizerAfterDeleteReconcile(t *testing.T) {
 		},
 	}
 	key := client.ObjectKey{Namespace: m.Namespace, Name: m.Name}
+	clientFake := fake.NewClientBuilder().WithObjects(testCluster, m).WithStatusSubresource(&expv1.MachinePool{}).Build()
 	mr := &MachinePoolReconciler{
-		Client: fake.NewClientBuilder().WithObjects(testCluster, m).WithStatusSubresource(&expv1.MachinePool{}).Build(),
+		Client:  clientFake,
+		Tracker: remote.NewTestClusterCacheTracker(ctrl.LoggerFrom(context.TODO()), clientFake, clientFake, clientFake.Scheme(), client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}),
 	}
 	_, err := mr.Reconcile(ctx, reconcile.Request{NamespacedName: key})
 	g.Expect(err).ToNot(HaveOccurred())
@@ -864,6 +869,7 @@ func TestMachinePoolConditions(t *testing.T) {
 			r := &MachinePoolReconciler{
 				Client:    clientFake,
 				APIReader: clientFake,
+				Tracker:   remote.NewTestClusterCacheTracker(ctrl.LoggerFrom(context.TODO()), clientFake, clientFake, clientFake.Scheme(), client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}),
 			}
 
 			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: util.ObjectKey(machinePool)})