split the behavior in defer to function and add test

Signed-off-by: sivchari <shibuuuu5@gmail.com>

Author: sivchari

controlplane/kubeadm/internal/controllers/controller.go

@@ -210,46 +210,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 	}
 
 	defer func() {
-		// Always attempt to update status.
-		if err := r.updateStatus(ctx, controlPlane); err != nil {
-			if errors.Is(err, &internal.RemoteClusterConnectionError{}) {
-				log.Error(err, "Could not connect to workload cluster to fetch status")
-			} else {
-				log.Error(err, "Failed to update KubeadmControlPlane status")
-				reterr = kerrors.NewAggregate([]error{reterr, err})
-			}
-		}
-
-		r.updateV1Beta2Status(ctx, controlPlane)
-
-		// Always attempt to Patch the KubeadmControlPlane object and status after each reconciliation.
-		patchOpts := []patch.Option{}
-		if reterr == nil {
-			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
-		}
-		if err := patchKubeadmControlPlane(ctx, patchHelper, kcp, patchOpts...); err != nil {
-			log.Error(err, "Failed to patch KubeadmControlPlane")
-			reterr = kerrors.NewAggregate([]error{reterr, err})
-		}
-
-		// Only requeue if there is no error, Requeue or RequeueAfter and the object does not have a deletion timestamp.
-		if reterr == nil && res.IsZero() && kcp.ObjectMeta.DeletionTimestamp.IsZero() {
-			// Make KCP requeue in case node status is not ready, so we can check for node status without waiting for a full
-			// resync (by default 10 minutes).
-			// The alternative solution would be to watch the control plane nodes in the Cluster - similar to how the
-			// MachineSet and MachineHealthCheck controllers watch the nodes under their control.
-			if !kcp.Status.Ready {
-				res = ctrl.Result{RequeueAfter: 20 * time.Second}
-			}
-
-			// Make KCP requeue if ControlPlaneComponentsHealthyCondition is false so we can check for control plane component
-			// status without waiting for a full resync (by default 10 minutes).
-			// Otherwise this condition can lead to a delay in provisioning MachineDeployments when MachineSet preflight checks are enabled.
-			// The alternative solution to this requeue would be watching the relevant pods inside each workload cluster which would be very expensive.
-			if conditions.IsFalse(kcp, controlplanev1.ControlPlaneComponentsHealthyCondition) {
-				res = ctrl.Result{RequeueAfter: 20 * time.Second}
-			}
-		}
+		res, reterr = r.deferPatch(ctx, kcp, controlPlane, patchHelper)
 	}()
 
 	if !kcp.ObjectMeta.DeletionTimestamp.IsZero() {
@@ -261,6 +222,52 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 	return r.reconcile(ctx, controlPlane)
 }
 
+func (r *KubeadmControlPlaneReconciler) deferPatch(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane, controlPlane *internal.ControlPlane, patchHelper *patch.Helper) (res ctrl.Result, reterr error) {
+	log := ctrl.LoggerFrom(ctx)
+	// Always attempt to update status.
+	if err := r.updateStatus(ctx, controlPlane); err != nil {
+		var connFailure *internal.RemoteClusterConnectionError
+		if errors.As(err, &connFailure) {
+			log.Error(err, "Could not connect to workload cluster to fetch status")
+		} else {
+			log.Error(err, "Failed to update KubeadmControlPlane status")
+			reterr = kerrors.NewAggregate([]error{reterr, err})
+		}
+	}
+
+	r.updateV1Beta2Status(ctx, controlPlane)
+
+	// Always attempt to Patch the KubeadmControlPlane object and status after each reconciliation.
+	patchOpts := []patch.Option{}
+	if reterr == nil {
+		patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
+	}
+	if err := patchKubeadmControlPlane(ctx, patchHelper, kcp, patchOpts...); err != nil {
+		log.Error(err, "Failed to patch KubeadmControlPlane")
+		reterr = kerrors.NewAggregate([]error{reterr, err})
+	}
+
+	// Only requeue if there is no error, Requeue or RequeueAfter and the object does not have a deletion timestamp.
+	if reterr == nil && res.IsZero() && kcp.ObjectMeta.DeletionTimestamp.IsZero() {
+		// Make KCP requeue in case node status is not ready, so we can check for node status without waiting for a full
+		// resync (by default 10 minutes).
+		// The alternative solution would be to watch the control plane nodes in the Cluster - similar to how the
+		// MachineSet and MachineHealthCheck controllers watch the nodes under their control.
+		if !kcp.Status.Ready {
+			res = ctrl.Result{RequeueAfter: 20 * time.Second}
+		}
+
+		// Make KCP requeue if ControlPlaneComponentsHealthyCondition is false so we can check for control plane component
+		// status without waiting for a full resync (by default 10 minutes).
+		// Otherwise this condition can lead to a delay in provisioning MachineDeployments when MachineSet preflight checks are enabled.
+		// The alternative solution to this requeue would be watching the relevant pods inside each workload cluster which would be very expensive.
+		if conditions.IsFalse(kcp, controlplanev1.ControlPlaneComponentsHealthyCondition) {
+			res = ctrl.Result{RequeueAfter: 20 * time.Second}
+		}
+	}
+	return
+}
+
 // initControlPlaneScope initializes the control plane scope; this includes also checking for orphan machines and
 // adopt them if necessary.
 // The func also returns a boolean indicating if adoptableMachine have been found and processed, but this doesn't imply those machines

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -2822,6 +2822,45 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneAndMachinesCondition
 	}
 }
 
+func TestKubeadmControlPlaneReconciler_deferPatch(t *testing.T) {
+	g := NewWithT(t)
+
+	ns, err := env.CreateNamespace(ctx, "test-reconcile-update-status")
+	g.Expect(err).ToNot(HaveOccurred())
+
+	cluster, kcp, _ := createClusterWithControlPlane(ns.Name)
+	g.Expect(env.Create(ctx, cluster)).To(Succeed())
+	g.Expect(env.Create(ctx, kcp)).To(Succeed())
+	defer func(do ...client.Object) {
+		g.Expect(env.Cleanup(ctx, do...)).To(Succeed())
+	}(kcp, ns)
+
+	r := &KubeadmControlPlaneReconciler{
+		Client:              env,
+		SecretCachingClient: secretCachingClient,
+		managementCluster: &fakeManagementCluster{
+			WorkloadErr: &internal.RemoteClusterConnectionError{
+				Name: util.ObjectKey(cluster).String(),
+				Err:  errors.New("connection error"),
+			},
+		},
+	}
+
+	controlPlane, _, err := r.initControlPlaneScope(ctx, cluster, kcp)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	patchHelper, err := patch.NewHelper(kcp, env)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	result, err := r.deferPatch(ctx, kcp, controlPlane, patchHelper)
+	// Should bump RemoteClusterConnectionError
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: 20 * time.Second}))
+
+	// calling reconcile should return error
+	g.Expect(env.CleanupAndWait(ctx, cluster)).To(Succeed())
+}
+
 type fakeClusterCache struct {
 	clustercache.ClusterCache
 	lastProbeSuccessTime time.Time

controlplane/kubeadm/internal/controllers/status.go

@@ -23,7 +23,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
@@ -94,7 +93,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro
 
 	workloadCluster, err := controlPlane.GetWorkloadCluster(ctx)
 	if err != nil {
-		return errors.Wrap(err, "failed to create remote cluster client")
+		return fmt.Errorf("failed to create remote cluster client: %w", err)
 	}
 	status, err := workloadCluster.ClusterStatus(ctx)
 	if err != nil {