⚠️ Remove RequeueAfterError

controllers/machine_controller.go

@@ -36,7 +36,6 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/controllers/remote"
-	capierrors "sigs.k8s.io/cluster-api/errors"
 	kubedrain "sigs.k8s.io/cluster-api/third_party/kubernetes-drain"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -333,10 +332,12 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 				return ctrl.Result{}, errors.Wrap(err, "failed to patch Machine")
 			}
 
-			if err := r.drainNode(ctx, cluster, m.Status.NodeRef.Name); err != nil {
-				conditions.MarkFalse(m, clusterv1.DrainingSucceededCondition, clusterv1.DrainingFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
-				r.recorder.Eventf(m, corev1.EventTypeWarning, "FailedDrainNode", "error draining Machine's node %q: %v", m.Status.NodeRef.Name, err)
-				return ctrl.Result{}, err
+			if result, err := r.drainNode(ctx, cluster, m.Status.NodeRef.Name); !result.IsZero() || err != nil {
+				if err != nil {
+					conditions.MarkFalse(m, clusterv1.DrainingSucceededCondition, clusterv1.DrainingFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
+					r.recorder.Eventf(m, corev1.EventTypeWarning, "FailedDrainNode", "error draining Machine's node %q: %v", m.Status.NodeRef.Name, err)
+				}
+				return result, err
 			}
 
 			conditions.MarkTrue(m, clusterv1.DrainingSucceededCondition)
@@ -489,28 +490,28 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, cluster *cl
 	}
 }
 
-func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, nodeName string) error {
+func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, nodeName string) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx, "cluster", cluster.Name, "node", nodeName)
 
 	restConfig, err := remote.RESTConfig(ctx, MachineControllerName, r.Client, util.ObjectKey(cluster))
 	if err != nil {
 		log.Error(err, "Error creating a remote client while deleting Machine, won't retry")
-		return nil
+		return ctrl.Result{}, nil
 	}
 	kubeClient, err := kubernetes.NewForConfig(restConfig)
 	if err != nil {
 		log.Error(err, "Error creating a remote client while deleting Machine, won't retry")
-		return nil
+		return ctrl.Result{}, nil
 	}
 
 	node, err := kubeClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			// If an admin deletes the node directly, we'll end up here.
 			log.Error(err, "Could not find node from noderef, it may have already been deleted")
-			return nil
+			return ctrl.Result{}, nil
 		}
-		return errors.Errorf("unable to get node %q: %v", nodeName, err)
+		return ctrl.Result{}, errors.Errorf("unable to get node %q: %v", nodeName, err)
 	}
 
 	drainer := &kubedrain.Helper{
@@ -543,17 +544,17 @@ func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cl
 	if err := kubedrain.RunCordonOrUncordon(ctx, drainer, node, true); err != nil {
 		// Machine will be re-reconciled after a cordon failure.
 		log.Error(err, "Cordon failed")
-		return errors.Errorf("unable to cordon node %s: %v", node.Name, err)
+		return ctrl.Result{}, errors.Errorf("unable to cordon node %s: %v", node.Name, err)
 	}
 
 	if err := kubedrain.RunNodeDrain(ctx, drainer, node.Name); err != nil {
 		// Machine will be re-reconciled after a drain failure.
-		log.Error(err, "Drain failed")
-		return &capierrors.RequeueAfterError{RequeueAfter: 20 * time.Second}
+		log.Error(err, "Drain failed, retry in 20s")
+		return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
 	}
 
 	log.Info("Drain successful")
-	return nil
+	return ctrl.Result{}, nil
 }
 
 func (r *MachineReconciler) deleteNode(ctx context.Context, cluster *clusterv1.Cluster, name string) error {

controllers/machine_controller_phases.go

@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -97,9 +96,8 @@ func (r *MachineReconciler) reconcileExternal(ctx context.Context, cluster *clus
 	obj, err := external.Get(ctx, r.Client, ref, m.Namespace)
 	if err != nil {
 		if apierrors.IsNotFound(errors.Cause(err)) {
-			return external.ReconcileOutput{}, errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: externalReadyWait},
-				"could not find %v %q for Machine %q in namespace %q, requeuing",
-				ref.GroupVersionKind(), ref.Name, m.Name, m.Namespace)
+			log.Info("could not find external ref, requeueing", "RefGVK", ref.GroupVersionKind(), "RefName", ref.Name, "Machine", m.Name, "Namespace", m.Namespace)
+			return external.ReconcileOutput{RequeueAfter: externalReadyWait}, nil
 		}
 		return external.ReconcileOutput{}, err
 	}
@@ -192,6 +190,9 @@ func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clu
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+	if externalResult.RequeueAfter > 0 {
+		return ctrl.Result{RequeueAfter: externalResult.RequeueAfter}, nil
+	}
 	if externalResult.Paused {
 		return ctrl.Result{}, nil
 	}
@@ -240,14 +241,18 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster
 	// Call generic external reconciler.
 	infraReconcileResult, err := r.reconcileExternal(ctx, cluster, m, &m.Spec.InfrastructureRef)
 	if err != nil {
-		if m.Status.InfrastructureReady && strings.Contains(err.Error(), "could not find") {
-			// Infra object went missing after the machine was up and running
+		return ctrl.Result{}, err
+	}
+	if infraReconcileResult.RequeueAfter > 0 {
+		// Infra object went missing after the machine was up and running
+		if m.Status.InfrastructureReady {
 			log.Error(err, "Machine infrastructure reference has been deleted after being ready, setting failure state")
 			m.Status.FailureReason = capierrors.MachineStatusErrorPtr(capierrors.InvalidConfigurationMachineError)
 			m.Status.FailureMessage = pointer.StringPtr(fmt.Sprintf("Machine infrastructure resource %v with name %q has been deleted after being ready",
 				m.Spec.InfrastructureRef.GroupVersionKind(), m.Spec.InfrastructureRef.Name))
+			return ctrl.Result{}, errors.Errorf("could not find %v %q for Machine %q in namespace %q, requeueing", m.Spec.InfrastructureRef.GroupVersionKind().String(), m.Spec.InfrastructureRef.Name, m.Name, m.Namespace)
 		}
-		return ctrl.Result{}, err
+		return ctrl.Result{RequeueAfter: infraReconcileResult.RequeueAfter}, nil
 	}
 	// if the external object is paused, return without any further processing
 	if infraReconcileResult.Paused {

controllers/machine_controller_phases_test.go

@@ -587,9 +587,9 @@ func TestReconcileBootstrap(t *testing.T) {
 		name            string
 		bootstrapConfig map[string]interface{}
 		machine         *clusterv1.Machine
+		expectResult    ctrl.Result
 		expectError     bool
 		expected        func(g *WithT, m *clusterv1.Machine)
-		result          *ctrl.Result
 	}{
 		{
 			name: "new machine, bootstrap config ready with data",
@@ -606,7 +606,8 @@ func TestReconcileBootstrap(t *testing.T) {
 					"dataSecretName": "secret-data",
 				},
 			},
-			expectError: false,
+			expectResult: ctrl.Result{},
+			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeTrue())
 				g.Expect(m.Spec.Bootstrap.DataSecretName).ToNot(BeNil())
@@ -627,7 +628,8 @@ func TestReconcileBootstrap(t *testing.T) {
 					"ready": true,
 				},
 			},
-			expectError: true,
+			expectResult: ctrl.Result{},
+			expectError:  true,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 				g.Expect(m.Spec.Bootstrap.DataSecretName).To(BeNil())
@@ -645,8 +647,8 @@ func TestReconcileBootstrap(t *testing.T) {
 				"spec":   map[string]interface{}{},
 				"status": map[string]interface{}{},
 			},
-			expectError: false,
-			result:      &ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 			},
@@ -663,7 +665,8 @@ func TestReconcileBootstrap(t *testing.T) {
 				"spec":   map[string]interface{}{},
 				"status": map[string]interface{}{},
 			},
-			expectError: true,
+			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 			},
@@ -680,7 +683,8 @@ func TestReconcileBootstrap(t *testing.T) {
 				"spec":   map[string]interface{}{},
 				"status": map[string]interface{}{},
 			},
-			expectError: true,
+			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectError:  false,
 		},
 		{
 			name: "existing machine, bootstrap data should not change",
@@ -716,7 +720,8 @@ func TestReconcileBootstrap(t *testing.T) {
 					BootstrapReady: true,
 				},
 			},
-			expectError: false,
+			expectResult: ctrl.Result{},
+			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeTrue())
 				g.Expect(*m.Spec.Bootstrap.DataSecretName).To(BeEquivalentTo("secret-data"))
@@ -763,8 +768,8 @@ func TestReconcileBootstrap(t *testing.T) {
 					BootstrapReady: true,
 				},
 			},
-			expectError: false,
-			result:      &ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.GetOwnerReferences()).NotTo(ContainRefOfGroupKind("cluster.x-k8s.io", "MachineSet"))
 			},
@@ -810,7 +815,8 @@ func TestReconcileBootstrap(t *testing.T) {
 					BootstrapReady: true,
 				},
 			},
-			expectError: true,
+			expectResult: ctrl.Result{},
+			expectError:  true,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.GetOwnerReferences()).NotTo(ContainRefOfGroupKind("cluster.x-k8s.io", "MachineSet"))
 			},
@@ -839,6 +845,7 @@ func TestReconcileBootstrap(t *testing.T) {
 			}
 
 			res, err := r.reconcileBootstrap(ctx, defaultCluster, tc.machine)
+			g.Expect(res).To(Equal(tc.expectResult))
 			if tc.expectError {
 				g.Expect(err).ToNot(BeNil())
 			} else {
@@ -848,10 +855,6 @@ func TestReconcileBootstrap(t *testing.T) {
 			if tc.expected != nil {
 				tc.expected(g, tc.machine)
 			}
-
-			if tc.result != nil {
-				g.Expect(res).To(Equal(*tc.result))
-			}
 		})
 	}
 }
@@ -889,14 +892,14 @@ func TestReconcileInfrastructure(t *testing.T) {
 	}
 
 	testCases := []struct {
-		name               string
-		bootstrapConfig    map[string]interface{}
-		infraConfig        map[string]interface{}
-		machine            *clusterv1.Machine
-		expectError        bool
-		expectChanged      bool
-		expectRequeueAfter bool
-		expected           func(g *WithT, m *clusterv1.Machine)
+		name            string
+		bootstrapConfig map[string]interface{}
+		infraConfig     map[string]interface{}
+		machine         *clusterv1.Machine
+		expectResult    ctrl.Result
+		expectError     bool
+		expectChanged   bool
+		expected        func(g *WithT, m *clusterv1.Machine)
 	}{
 		{
 			name: "new machine, infrastructure config ready",
@@ -933,6 +936,7 @@ func TestReconcileInfrastructure(t *testing.T) {
 					},
 				},
 			},
+			expectResult:  ctrl.Result{},
 			expectError:   false,
 			expectChanged: true,
 			expected: func(g *WithT, m *clusterv1.Machine) {
@@ -985,8 +989,8 @@ func TestReconcileInfrastructure(t *testing.T) {
 				"apiVersion": "infrastructure.cluster.x-k8s.io/v1alpha4",
 				"metadata":   map[string]interface{}{},
 			},
-			expectError:        true,
-			expectRequeueAfter: true,
+			expectResult: ctrl.Result{},
+			expectError:  true,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.InfrastructureReady).To(BeTrue())
 				g.Expect(m.Status.FailureMessage).ToNot(BeNil())
@@ -1023,6 +1027,7 @@ func TestReconcileInfrastructure(t *testing.T) {
 					},
 				},
 			},
+			expectResult:  ctrl.Result{},
 			expectError:   false,
 			expectChanged: false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
@@ -1052,8 +1057,9 @@ func TestReconcileInfrastructure(t *testing.T) {
 					).Build(),
 			}
 
-			_, err := r.reconcileInfrastructure(ctx, defaultCluster, tc.machine)
+			result, err := r.reconcileInfrastructure(ctx, defaultCluster, tc.machine)
 			r.reconcilePhase(ctx, tc.machine)
+			g.Expect(result).To(Equal(tc.expectResult))
 			if tc.expectError {
 				g.Expect(err).ToNot(BeNil())
 			} else {

controlplane/kubeadm/controllers/controller.go

@@ -36,7 +36,6 @@ import (
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/machinefilters"
-	capierrors "sigs.k8s.io/cluster-api/errors"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
@@ -161,13 +160,6 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 	}
 
 	defer func() {
-		if requeueErr, ok := errors.Cause(reterr).(capierrors.HasRequeueAfterError); ok {
-			if res.RequeueAfter == 0 {
-				res.RequeueAfter = requeueErr.GetRequeueAfter()
-				reterr = nil
-			}
-		}
-
 		// Always attempt to update status.
 		if err := r.updateStatus(ctx, kcp, cluster); err != nil {
 			var connFailure *internal.RemoteClusterConnectionError
@@ -265,9 +257,11 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
 	}
 
 	// Generate Cluster Kubeconfig if needed
-	if err := r.reconcileKubeconfig(ctx, cluster, kcp); err != nil {
-		log.Error(err, "failed to reconcile Kubeconfig")
-		return ctrl.Result{}, err
+	if result, err := r.reconcileKubeconfig(ctx, cluster, kcp); !result.IsZero() || err != nil {
+		if err != nil {
+			log.Error(err, "failed to reconcile Kubeconfig")
+		}
+		return result, err
 	}
 
 	controlPlaneMachines, err := r.managementClusterUncached.GetMachinesForCluster(ctx, util.ObjectKey(cluster), machinefilters.ControlPlaneMachines(cluster.Name))