Ensure status updates succeed

Signed-off-by: Chuck Ha <chuckh@vmware.com>

controllers/BUILD.bazel

@@ -48,6 +48,7 @@ go_test(
         "//api/v1alpha2:go_default_library",
         "//vendor/github.com/onsi/ginkgo:go_default_library",
         "//vendor/github.com/onsi/gomega:go_default_library",
+        "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/client-go/kubernetes/scheme:go_default_library",
         "//vendor/k8s.io/client-go/rest:go_default_library",

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,10 +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(clusterReconciler.SetupWithManager(mgr)).NotTo(HaveOccurred())
 
 	By("starting the manager")
 	go func() {

pkg/controller/machine/machine_controller.go

@@ -116,19 +116,7 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (_ reconcile.Res
 	// Always issue a Patch for the Machine object and its status after each reconciliation.
 	// TODO(vincepri): Figure out if we should bubble up the errors from Patch to the controller.
 	defer func() {
-		gvk := m.GroupVersionKind()
-		if err := r.Client.Patch(ctx, m, patchMachine); err != nil {
-			klog.Errorf("Error Patching Machine %q in namespace %q: %v", m.Name, m.Namespace, err)
-			if reterr == nil {
-				reterr = err
-			}
-			return
-		}
-		// TODO(vincepri): This is a hack because after a Patch, the object loses TypeMeta information.
-		// Remove when https://github.com/kubernetes-sigs/controller-runtime/issues/526 is fixed.
-		m.SetGroupVersionKind(gvk)
-		if err := r.Client.Status().Patch(ctx, m, patchMachine); err != nil {
-			klog.Errorf("Error Patching Machine status %q in namespace %q: %v", m.Name, m.Namespace, err)
+		if err := r.patchMachine(ctx, m, patchMachine); err != nil {
 			if reterr == nil {
 				reterr = err
 			}
@@ -325,3 +313,16 @@ func (r *ReconcileMachine) isDeleteReady(ctx context.Context, m *v1alpha2.Machin
 func shouldAdopt(m *v1alpha2.Machine) bool {
 	return !util.HasOwner(m.OwnerReferences, v1alpha2.GroupVersion.String(), []string{"MachineSet", "Cluster"})
 }
+
+func (r *ReconcileMachine) patchMachine(ctx context.Context, machine *clusterv1.Machine, patch client.Patch) error {
+	// Always patch the status before the spec
+	if err := r.Client.Status().Patch(ctx, machine, patch); err != nil {
+		klog.Errorf("Error Patching Machine status %q in namespace %q: %v", machine.Name, machine.Namespace, err)
+		return err
+	}
+	if err := r.Client.Patch(ctx, machine, patch); err != nil {
+		klog.Errorf("Error Patching Machine %q in namespace %q: %v", machine.Name, machine.Namespace, err)
+		return err
+	}
+	return nil
+}