From f4f51a8f38c118c70166e6e89501ac62a4a32e29 Mon Sep 17 00:00:00 2001 From: Chuck Ha Date: Thu, 15 Aug 2019 10:25:57 -0400 Subject: [PATCH] Ensure status updates succeed Signed-off-by: Chuck Ha --- controllers/cluster_controller.go | 24 ++++--- controllers/cluster_controller_test.go | 89 ++++++++++++++++++++++++++ controllers/suite_test.go | 22 +++---- 3 files changed, 114 insertions(+), 21 deletions(-) diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go index efe1095fbd49..598d66542539 100644 --- a/controllers/cluster_controller.go +++ b/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 +} diff --git a/controllers/cluster_controller_test.go b/controllers/cluster_controller_test.go index fa62a2cea746..9b53b89d1d96 100644 --- a/controllers/cluster_controller_test.go +++ b/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()) + }) }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 299095741153..8305988a83d5 100644 --- a/controllers/suite_test.go +++ b/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() {