Check rollout status when compositions change

internal/controllers/reconciliation/integration_test.go

@@ -22,6 +22,8 @@ import (
 	"github.com/Azure/eno/internal/testutil"
 )
 
+// TODO: Sometimes the resource is patched to the create phase after the external update but before the update
+
 type crudTestCase struct {
 	Name                         string
 	Empty, Initial, Updated      client.Object
@@ -245,18 +247,6 @@ func setImage(t *testing.T, upstream client.Client, syn *apiv1.Synthesizer, comp
 		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 {

internal/controllers/synthesis/integration_test.go

@@ -203,10 +203,10 @@ func TestControllerSynthesizerRollout(t *testing.T) {
 	require.NoError(t, err)
 
 	// The first synthesizer update should be applied to the composition
-	// TODO: Flake
+	// TODO: Flake because sometimes the pod updates the slice refs but the synth generation somehow still doesn't match this one
 	testutil.Eventually(t, func() bool {
 		require.NoError(t, client.IgnoreNotFound(cli.Get(ctx, client.ObjectKeyFromObject(comp), comp)))
-		return comp.Status.CurrentState != nil && comp.Status.CurrentState.ObservedSynthesizerGeneration == syn.Generation
+		return comp.Status.CurrentState != nil && comp.Status.CurrentState.ObservedSynthesizerGeneration >= syn.Generation
 	})
 
 	// Wait for the informer cache to know about the last update

internal/controllers/synthesis/rollout.go

@@ -31,6 +31,7 @@ func NewRolloutController(mgr ctrl.Manager, cooldownPeriod time.Duration) error
 	}
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&apiv1.Synthesizer{}).
+		Watches(&apiv1.Composition{}, manager.NewCompositionToSynthesizerHandler(c.client)).
 		WithLogConstructor(manager.NewLogConstructor(mgr, "rolloutController")).
 		Complete(c)
 }
@@ -48,7 +49,7 @@ func (c *rolloutController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	if syn.Status.LastRolloutTime != nil {
 		remainingCooldown := c.cooldown - time.Since(syn.Status.LastRolloutTime.Time)
 		if remainingCooldown > 0 {
-			return ctrl.Result{RequeueAfter: remainingCooldown}, nil
+			return ctrl.Result{RequeueAfter: remainingCooldown}, nil // not ready to continue rollout yet
 		}
 	}
 
@@ -61,6 +62,7 @@ func (c *rolloutController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	}
 
 	// randomize list to avoid always rolling out changes in the same order
+	// TODO: Consider a more efficient approach here
 	rand.Shuffle(len(compList.Items), func(i, j int) { compList.Items[i] = compList.Items[j] })
 
 	for _, comp := range compList.Items {
@@ -107,7 +109,7 @@ func (c *rolloutController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		if len(compList.Items) > 0 { // log doesn't make sense if the synthesizer wasn't actually rolled out
 			logger.Info("finished rolling out latest synthesizer version")
 		}
-		return ctrl.Result{}, nil // TODO: Consider leaving this loop open in case new compositions fell through the cracks earlier
+		return ctrl.Result{}, nil
 	}
 
 	return ctrl.Result{}, nil

internal/manager/manager.go

@@ -6,9 +6,13 @@ import (
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/util/workqueue"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -76,3 +80,32 @@ func NewLogConstructor(mgr ctrl.Manager, controllerName string) func(*reconcile.
 		return l
 	}
 }
+
+func NewCompositionToSynthesizerHandler(cli client.Client) handler.EventHandler {
+	return &handler.Funcs{
+		CreateFunc: func(ctx context.Context, ce event.CreateEvent, rli workqueue.RateLimitingInterface) {
+			comp, ok := ce.Object.(*apiv1.Composition)
+			if !ok {
+				logr.FromContextOrDiscard(ctx).V(0).Info("unexpected type given to NewCompositionToSynthesizerHandler")
+				return
+			}
+			rli.Add(reconcile.Request{NamespacedName: types.NamespacedName{Name: comp.Spec.Synthesizer.Name}})
+		},
+		UpdateFunc: func(ctx context.Context, ue event.UpdateEvent, rli workqueue.RateLimitingInterface) {
+			comp, ok := ue.ObjectNew.(*apiv1.Composition)
+			if !ok {
+				logr.FromContextOrDiscard(ctx).V(0).Info("unexpected type given to NewCompositionToSynthesizerHandler")
+				return
+			}
+			rli.Add(reconcile.Request{NamespacedName: types.NamespacedName{Name: comp.Spec.Synthesizer.Name}})
+		},
+		DeleteFunc: func(ctx context.Context, de event.DeleteEvent, rli workqueue.RateLimitingInterface) {
+			comp, ok := de.Object.(*apiv1.Composition)
+			if !ok {
+				logr.FromContextOrDiscard(ctx).V(0).Info("unexpected type given to NewCompositionToSynthesizerHandler")
+				return
+			}
+			rli.Add(reconcile.Request{NamespacedName: types.NamespacedName{Name: comp.Spec.Synthesizer.Name}})
+		},
+	}
+}