Exit without updating status if suspended

The convention is now to simply exit, when a GOTK object is
suspended. Previously this would update the status to indicate that it
was unready; now it just leaves it in whichever state it was before.

This also applies to the reconcile request annotation; it will _not_
be marked as seen if the object is suspended. The effect of this is
any change to the object will be passed by the predicate and therefore
reach Reconcile, until the object is unsuspended. Since it will _also_
exit early until unsuspended, this is harmless except for some extra
log lines. (But changing that ordering might be worth considering in
the future.)

This change required a few changes to tests:

 - to check that suspend makes the reconciliation exit without doing
   anything, explicitly run `r.Reconcile(...)`.

 - to avoid waiting for the reconciler's caching client to see
   changes, use an uncached client.

 - (fix a problem caused by comparing a time pointer with its alias)

Signed-off-by: Michael Bridgen <[email protected]>

Author: squaremo

controllers/imageupdateautomation_controller.go

@@ -87,6 +87,11 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	if auto.Spec.Suspend {
+		log.Info("ImageUpdateAutomation is suspended, skipping automation run")
+		return ctrl.Result{}, nil
+	}
+
 	// whatever else happens, we've now "seen" the reconcile
 	// annotation if it's there
 	if token, ok := meta.ReconcileAnnotationValue(auto.GetAnnotations()); ok {
@@ -96,21 +101,6 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 		}
 	}
 
-	if auto.Spec.Suspend {
-		msg := "ImageUpdateAutomation is suspended, skipping automation run"
-		imagev1.SetImageUpdateAutomationReadiness(
-			&auto,
-			metav1.ConditionFalse,
-			meta.SuspendedReason,
-			msg,
-		)
-		if err := r.Status().Update(ctx, &auto); err != nil {
-			return ctrl.Result{Requeue: true}, err
-		}
-		log.Info(msg)
-		return ctrl.Result{}, nil
-	}
-
 	// failWithError is a helper for bailing on the reconciliation.
 	failWithError := func(err error) (ctrl.Result, error) {
 		r.event(auto, events.EventSeverityError, err.Error())

controllers/suite_test.go

@@ -44,6 +44,7 @@ import (
 var cfg *rest.Config
 var k8sClient client.Client
 var k8sManager ctrl.Manager
+var imageAutoReconciler *ImageUpdateAutomationReconciler
 var testEnv *envtest.Environment
 
 func TestAPIs(t *testing.T) {
@@ -81,19 +82,22 @@ var _ = BeforeSuite(func(done Done) {
 	})
 	Expect(err).ToNot(HaveOccurred())
 
-	err = (&ImageUpdateAutomationReconciler{
+	imageAutoReconciler = &ImageUpdateAutomationReconciler{
 		Client: k8sManager.GetClient(),
 		Log:    ctrl.Log.WithName("controllers").WithName("ImageUpdateAutomation"),
 		Scheme: scheme.Scheme,
-	}).SetupWithManager(k8sManager)
-	Expect(err).ToNot(HaveOccurred())
+	}
+	Expect(imageAutoReconciler.SetupWithManager(k8sManager)).To(Succeed())
 
 	go func() {
 		err = k8sManager.Start(ctrl.SetupSignalHandler())
 		Expect(err).ToNot(HaveOccurred())
 	}()
 
-	k8sClient = k8sManager.GetClient()
+	// Specifically an uncached client. Use <reconciler>.Get if you
+	// want to see what the reconcilers see.
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
+	Expect(err).ToNot(HaveOccurred())
 	Expect(k8sClient).ToNot(BeNil())
 
 	close(done)

controllers/update_test.go

@@ -36,9 +36,10 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/otiai10/copy"
 	corev1 "k8s.io/api/core/v1"
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
+	//	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1alpha1"
@@ -252,23 +253,36 @@ var _ = Describe("ImageUpdateAutomation", func() {
 				updatePatch.Namespace = updateKey.Namespace
 				updatePatch.Spec.Suspend = true
 				Expect(k8sClient.Patch(context.Background(), &updatePatch, client.Merge)).To(Succeed())
+				// wait for the suspension to reach the cache
+				var newUpdate imagev1.ImageUpdateAutomation
 				Eventually(func() bool {
-					if err := k8sClient.Get(context.Background(), updateKey, updateBySetters); err != nil {
+					if err := imageAutoReconciler.Get(context.Background(), updateKey, &newUpdate); err != nil {
 						return false
 					}
-					ready := apimeta.FindStatusCondition(updateBySetters.Status.Conditions, meta.ReadyCondition)
-					return ready != nil && ready.Status == metav1.ConditionFalse && ready.Reason == meta.SuspendedReason
+					return newUpdate.Spec.Suspend
 				}, timeout, time.Second).Should(BeTrue())
+				// run the reconciliation explicitly, and make sure it
+				// doesn't do anything
+				result, err := imageAutoReconciler.Reconcile(ctrl.Request{
+					NamespacedName: updateKey,
+				})
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(ctrl.Result{})) // this ought to fail, since it should be rescheduled; but if not, additional checks lie below
+
+				var checkUpdate imagev1.ImageUpdateAutomation
+				Expect(k8sClient.Get(context.Background(), updateKey, &checkUpdate)).To(Succeed())
+				Expect(checkUpdate.Status.ObservedGeneration).NotTo(Equal(checkUpdate.ObjectMeta.Generation))
 			})
 
 			It("runs when the reconcile request annotation is added", func() {
+				println("[DEBUG]", updateKey.String())
 				// the automation has run, and is not expected to run
 				// again for 2 hours. Make a commit to the git repo
 				// which needs to be undone by automation, then add
 				// the annotation and make sure it runs again.
 				Expect(k8sClient.Get(context.Background(), updateKey, updateBySetters)).To(Succeed())
-				lastRun := updateBySetters.Status.LastAutomationRunTime
-				Expect(lastRun).ToNot(BeNil())
+				Expect(updateBySetters.Status.LastAutomationRunTime).ToNot(BeNil())
+				lastRunTime := updateBySetters.Status.LastAutomationRunTime.Time
 
 				commitInRepo(repoURL, "Revert image update", func(tmp string) {
 					// revert the change made by copying the old version
@@ -277,7 +291,7 @@ var _ = Describe("ImageUpdateAutomation", func() {
 					copy.Copy("testdata/appconfig/deploy.yaml", filepath.Join(tmp, "deploy.yaml"))
 					replaceMarker(tmp, policyKey)
 				})
-				// check that it was reverted
+				// check that it was reverted correctly
 				compareRepoWithExpected(repoURL, "testdata/appconfig", func(tmp string) {
 					replaceMarker(tmp, policyKey)
 				})
@@ -291,15 +305,18 @@ var _ = Describe("ImageUpdateAutomation", func() {
 				}
 				Expect(k8sClient.Patch(context.Background(), &updatePatch, client.Merge)).To(Succeed())
 
+				// ... this is where the reconciler is supposed to do its work ...
+
+				var newUpdate imagev1.ImageUpdateAutomation
 				Eventually(func() bool {
-					if err := k8sClient.Get(context.Background(), updateKey, updateBySetters); err != nil {
+					if err := k8sClient.Get(context.Background(), updateKey, &newUpdate); err != nil {
 						return false
 					}
-					newLastRun := updateBySetters.Status.LastAutomationRunTime
-					return newLastRun != nil && newLastRun.Time.After(lastRun.Time)
+					newLastRun := newUpdate.Status.LastAutomationRunTime
+					return newLastRun != nil && newLastRun.Time.After(lastRunTime)
 				}, timeout, time.Second).Should(BeTrue())
 				// check that the annotation was recorded as seen
-				Expect(updateBySetters.Status.LastHandledReconcileAt).To(Equal(ts))
+				Expect(newUpdate.Status.LastHandledReconcileAt).To(Equal(ts))
 
 				// check that a new commit was made
 				compareRepoWithExpected(repoURL, "testdata/appconfig-setters-expected", func(tmp string) {