From a803a98fa135375ab8fffbfa073db471d1e291fa Mon Sep 17 00:00:00 2001
From: Jordan Olshevski <joolshev@microsoft.com>
Date: Mon, 11 Dec 2023 21:49:45 +0000
Subject: [PATCH] (hopefully) fix integration test race

---
 .../controllers/reconciliation/controller.go     |  4 ++++
 .../reconciliation/integration_test.go           | 16 ++++++++++++++--
 internal/controllers/synthesis/rollout.go        |  2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go
index 0f0c945f..277b50b2 100644
--- a/internal/controllers/reconciliation/controller.go
+++ b/internal/controllers/reconciliation/controller.go
@@ -20,6 +20,10 @@ import (
 	"github.com/go-logr/logr"
 )
 
+// TODO: Implement tombstones
+// - Synthesis collection controller diffs against previous state, creates tombstones
+// - Any tombstones present and not yet reconciled in the previous state are passed on to the next
+
 // TODO: Minimal retries for validation error
 
 type Controller struct {
diff --git a/internal/controllers/reconciliation/integration_test.go b/internal/controllers/reconciliation/integration_test.go
index f82b0a80..7b48ad71 100644
--- a/internal/controllers/reconciliation/integration_test.go
+++ b/internal/controllers/reconciliation/integration_test.go
@@ -224,7 +224,7 @@ func TestCRUD(t *testing.T) {
 			}
 
 			t.Run("update", func(t *testing.T) {
-				setSynImage(t, upstream, syn, "update")
+				setSynImage(t, upstream, syn, comp, "update")
 
 				var obj client.Object
 				testutil.Eventually(t, func() bool {
@@ -254,7 +254,7 @@ func (c *crudTestCase) Get(downstream client.Client) (client.Object, error) {
 	return obj, downstream.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)
 }
 
-func setSynImage(t *testing.T, upstream client.Client, syn *apiv1.Synthesizer, image string) {
+func setSynImage(t *testing.T, upstream client.Client, syn *apiv1.Synthesizer, comp *apiv1.Composition, image string) {
 	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		if err := upstream.Get(context.Background(), client.ObjectKeyFromObject(syn), syn); err != nil {
 			return err
@@ -263,6 +263,18 @@ func setSynImage(t *testing.T, upstream client.Client, syn *apiv1.Synthesizer, i
 		return upstream.Update(context.Background(), syn)
 	})
 	require.NoError(t, err)
+
+	// Also pin the composition to >= this synthesizer version.
+	// This is necessary to avoid deadlock in cases where incoherent cache causes the composition not to be updated on this tick of the rollout loop.
+	// It isn't a problem in production because we don't expect serialized behavior from the rollout controller.
+	err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		if err := upstream.Get(context.Background(), client.ObjectKeyFromObject(comp), comp); err != nil {
+			return err
+		}
+		comp.Spec.Synthesizer.MinGeneration = syn.Generation
+		return upstream.Update(context.Background(), comp)
+	})
+	require.NoError(t, err)
 }
 
 func newSliceBuilder(t *testing.T, scheme *runtime.Scheme, test *crudTestCase) func(c *apiv1.Composition, s *apiv1.Synthesizer) []*apiv1.ResourceSlice {
diff --git a/internal/controllers/synthesis/rollout.go b/internal/controllers/synthesis/rollout.go
index bf662bc5..fe35b11c 100644
--- a/internal/controllers/synthesis/rollout.go
+++ b/internal/controllers/synthesis/rollout.go
@@ -16,6 +16,8 @@ import (
 	"github.com/Azure/eno/internal/manager"
 )
 
+// TODO: Does controller-runtime add jitter to requeue intervals automatically?
+
 type rolloutController struct {
 	client   client.Client
 	cooldown time.Duration