fixup! 🐛 Refactor manager to avoid race conditions and provide clean shutdown

Signed-off-by: Vince Prignano <vincepri@vmware.com>

Author: vincepri

pkg/manager/internal.go

@@ -567,16 +567,16 @@ func (cm *controllerManager) engageStopProcedure(stopComplete <-chan struct{}) e
 		cm.logger.Info("Stopping and waiting for non leader election runnables")
 		cm.runnables.Others.StopAndWait(cm.shutdownCtx)
 
+		// Stop all the leader election runnables, which includes reconcilers.
+		cm.logger.Info("Stopping and waiting for leader election runnables")
+		cm.runnables.LeaderElection.StopAndWait(cm.shutdownCtx)
+
 		// Stop the caches before the leader election runnables, this is an important
 		// step to make sure that we don't race with the reconcilers by receiving more events
 		// from the API servers and enqueueing them.
 		cm.logger.Info("Stopping and waiting for caches")
 		cm.runnables.Caches.StopAndWait(cm.shutdownCtx)
 
-		// Stop all the leader election runnables, which includes reconcilers.
-		cm.logger.Info("Stopping and waiting for leader election runnables")
-		cm.runnables.LeaderElection.StopAndWait(cm.shutdownCtx)
-
 		// Webhooks should come last, as they might be still serving some requests.
 		cm.logger.Info("Stopping and waiting for webhooks")
 		cm.runnables.Webhooks.StopAndWait(cm.shutdownCtx)

pkg/manager/runnable_group.go

@@ -54,29 +54,15 @@ func newRunnables(errChan chan error) *runnables {
 // The runnables added before Start are started when Start is called.
 // The runnables added after Start are started directly.
 func (r *runnables) Add(fn Runnable, ready runnableCheck) error {
+	if ready == nil {
+		// If we don't have a readiness check, always return true.
+		ready = func(_ context.Context) bool { return true }
+	}
+
 	switch runnable := fn.(type) {
 	case hasCache:
 		return r.Caches.Add(fn, func(ctx context.Context) bool {
-			// Run the ready check for the cache a fixed number of times
-			// backing off a bit; this is to give time to the runnables
-			// to start up before their health check returns true.
-			if err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{
-				Duration: 10 * time.Millisecond,
-				Steps:    10,
-				Factor:   1.0,
-			}, func() (bool, error) {
-				for i := 0; i < 10; i++ {
-					<-time.After(time.Duration(i) * 10 * time.Millisecond)
-					if !runnable.GetCache().WaitForCacheSync(ctx) {
-						continue
-					}
-					return true, nil
-				}
-				return false, nil
-			}); err != nil {
-				return false
-			}
-			return true
+			return ready(ctx) && runnable.GetCache().WaitForCacheSync(ctx)
 		})
 	case *webhook.Server:
 		return r.Webhooks.Add(fn, ready)
@@ -259,7 +245,6 @@ func (r *runnableGroup) Add(rn Runnable, ready runnableCheck) error {
 	}
 	r.stop.RUnlock()
 
-	// If we don't have a readiness check, always return true.
 	if ready == nil {
 		ready = func(_ context.Context) bool { return true }
 	}