🐛 Ensure status updates succeed

controllers/cluster_controller.go

@@ -89,15 +89,7 @@ func (r *ClusterReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e
 
 	// Always issue a Patch for the Cluster object and its status after each reconciliation.
 	defer func() {
-		if err := r.Client.Patch(ctx, cluster, patchCluster); err != nil {
-			klog.Errorf("Error Patching Cluster %q in namespace %q: %v", cluster.Name, cluster.Namespace, err)
-			if reterr == nil {
-				reterr = err
-			}
-			return
-		}
-		if err := r.Client.Status().Patch(ctx, cluster, patchCluster); err != nil {
-			klog.Errorf("Error Patching Cluster status %q in namespace %q: %v", cluster.Name, cluster.Namespace, err)
+		if err := r.patchCluster(ctx, cluster, patchCluster); err != nil {
 			if reterr == nil {
 				reterr = err
 			}
@@ -241,3 +233,17 @@ func (r *ClusterReconciler) listChildren(ctx context.Context, cluster *clusterv1
 
 	return children, nil
 }
+
+func (r *ClusterReconciler) patchCluster(ctx context.Context, cluster *clusterv1.Cluster, patch client.Patch) error {
+	log := r.Log.WithValues("cluster-namespace", cluster.Namespace, "cluster-name", cluster.Name)
+	// Always patch the status before the spec
+	if err := r.Client.Status().Patch(ctx, cluster, patch); err != nil {
+		log.Error(err, "Error patching cluster status")
+		return err
+	}
+	if err := r.Client.Patch(ctx, cluster, patch); err != nil {
+		log.Error(err, "Error patching cluster")
+		return err
+	}
+	return nil
+}

controllers/cluster_controller_test.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -49,4 +50,92 @@ var _ = Describe("Cluster Reconciler", func() {
 		}, timeout).Should(BeTrue())
 	})
 
+	It("Should successfully patch a cluster object if the status diff is empty but the spec diff is not", func() {
+		// Setup
+		cluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-cluster",
+				Namespace: "default",
+			},
+		}
+		key := client.ObjectKey{Name: "test-cluster", Namespace: "default"}
+		Expect(k8sClient.Create(ctx, cluster)).To(BeNil())
+		defer k8sClient.Delete(ctx, cluster)
+
+		// Reconcile
+		Eventually(func() bool {
+			patch := client.MergeFrom(cluster.DeepCopy())
+			cluster.Spec.InfrastructureRef = &v1.ObjectReference{Name: "test"}
+			Expect(clusterReconciler.patchCluster(ctx, cluster, patch)).To(BeNil())
+			return true
+		}, timeout).Should(BeTrue())
+
+		// Assertions
+		Eventually(func() bool {
+			instance := &clusterv1.Cluster{}
+			Expect(k8sClient.Get(ctx, key, instance)).To(BeNil())
+			Expect(instance.Spec.InfrastructureRef.Name).To(BeEquivalentTo("test"))
+			return true
+		}, timeout).Should(BeTrue())
+	})
+
+	It("Should successfully patch a cluster object if the spec diff is empty but the status diff is not", func() {
+		// Setup
+		cluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-cluster",
+				Namespace: "default",
+			},
+		}
+		key := client.ObjectKey{Name: "test-cluster", Namespace: "default"}
+		Expect(k8sClient.Create(ctx, cluster)).To(BeNil())
+		defer k8sClient.Delete(ctx, cluster)
+
+		// Reconcile
+		Eventually(func() bool {
+			patch := client.MergeFrom(cluster.DeepCopy())
+			cluster.Status.InfrastructureReady = true
+			Expect(clusterReconciler.patchCluster(ctx, cluster, patch)).To(BeNil())
+			return true
+		}, timeout).Should(BeTrue())
+
+		// Assertions
+		Eventually(func() bool {
+			instance := &clusterv1.Cluster{}
+			Expect(k8sClient.Get(ctx, key, instance)).To(BeNil())
+			Expect(instance.Status.InfrastructureReady).To(BeTrue())
+			return true
+		}, timeout).Should(BeTrue())
+	})
+
+	It("Should successfully patch a cluster object if both the spec diff and status diff are non empty", func() {
+		// Setup
+		cluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-cluster",
+				Namespace: "default",
+			},
+		}
+		key := client.ObjectKey{Name: "test-cluster", Namespace: "default"}
+		Expect(k8sClient.Create(ctx, cluster)).To(BeNil())
+		defer k8sClient.Delete(ctx, cluster)
+
+		// Reconcile
+		Eventually(func() bool {
+			patch := client.MergeFrom(cluster.DeepCopy())
+			cluster.Status.InfrastructureReady = true
+			cluster.Spec.InfrastructureRef = &v1.ObjectReference{Name: "test"}
+			Expect(clusterReconciler.patchCluster(ctx, cluster, patch)).To(BeNil())
+			return true
+		}, timeout).Should(BeTrue())
+
+		// Assertions
+		Eventually(func() bool {
+			instance := &clusterv1.Cluster{}
+			Expect(k8sClient.Get(ctx, key, instance)).To(BeNil())
+			Expect(instance.Status.InfrastructureReady).To(BeTrue())
+			Expect(instance.Spec.InfrastructureRef.Name).To(BeEquivalentTo("test"))
+			return true
+		}, timeout).Should(BeTrue())
+	})
 })

controllers/suite_test.go

@@ -51,12 +51,13 @@ const (
 )
 
 var (
-	cfg       *rest.Config
-	k8sClient client.Client
-	testEnv   *envtest.Environment
-	mgr       manager.Manager
-	doneMgr   = make(chan struct{})
-	ctx       = context.Background()
+	cfg               *rest.Config
+	k8sClient         client.Client
+	testEnv           *envtest.Environment
+	mgr               manager.Manager
+	clusterReconciler *ClusterReconciler
+	doneMgr           = make(chan struct{})
+	ctx               = context.Background()
 )
 
 func TestAPIs(t *testing.T) {
@@ -90,14 +91,11 @@ var _ = BeforeSuite(func(done Done) {
 	By("setting up a new manager")
 	mgr, err = manager.New(cfg, manager.Options{Scheme: scheme.Scheme, MetricsBindAddress: "0"})
 	Expect(err).NotTo(HaveOccurred())
-	Expect((&ClusterReconciler{
+	clusterReconciler = &ClusterReconciler{
 		Client: mgr.GetClient(),
 		Log:    log.Log,
-	}).SetupWithManager(mgr)).NotTo(HaveOccurred())
-	Expect((&MachineReconciler{
-		Client: mgr.GetClient(),
-		Log:    log.Log,
-	}).SetupWithManager(mgr)).NotTo(HaveOccurred())
+	}
+	Expect(clusterReconciler.SetupWithManager(mgr)).NotTo(HaveOccurred())
 
 	By("starting the manager")
 	go func() {