Avoid double-resyncs without leader election. (#2252)

* Avoid double-resyncs without leader election.

tl;dr Without leader election enabled, we've been suffering from double resyncs, this fixes that.

Essentially when the informers are started, prior to starting the controllers, they fill the controller's workqueue with ~the world.  Once we start the controllers, the leader election code calls `Promote` on a `Bucket` so that it starts processing them as their leader, and then is requeues every key that falls into that `Bucket`.

When leader election is disabled, we use the `UniversalBucket`, which always owns everything.  This means that the first pass through the workqueue was already hitting every key, so re-enqueuing every key results in superfluous processing of those keys.

This change makes the `LeaderAwareFuncs` elide the call to `PromoteFunc` when the `enq` function passed is `nil`, and makes the `unopposedElector` have a `nil` `enq` field when we explicitly pass it the `UniversalBucket` because leader election is disabled.

* Add more unit test coverage

controller/controller_test.go

@@ -941,15 +941,15 @@ func (cr *countingLeaderAwareReconciler) Reconcile(ctx context.Context, key stri
 }
 
 func TestStartAndShutdownWithLeaderAwareNoElection(t *testing.T) {
-	promoted := make(chan struct{})
 	r := &countingLeaderAwareReconciler{
 		LeaderAwareFuncs: reconciler.LeaderAwareFuncs{
 			PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
-				close(promoted)
+				t.Error("Promote should not be called when no leader election is enabled.")
 				return nil
 			},
 		},
 	}
+
 	impl := NewContext(context.TODO(), r, ControllerOptions{
 		Logger:        TestLogger(t),
 		WorkQueueName: "Testing",
@@ -968,13 +968,10 @@ func TestStartAndShutdownWithLeaderAwareNoElection(t *testing.T) {
 	})
 
 	select {
-	case <-promoted:
-		// We expect to be promoted immediately, since there is no
-		// ElectorBuilder attached to the context.
 	case <-doneCh:
 		t.Fatal("StartAll finished early.")
 	case <-time.After(10 * time.Second):
-		t.Error("Timed out waiting for StartAll.")
+		// Give it some time to run.
 	}
 
 	cancel()

leaderelection/context.go

@@ -94,7 +94,11 @@ func BuildElector(ctx context.Context, la reconciler.LeaderAware, queueName stri
 	return &unopposedElector{
 		la:  la,
 		bkt: reconciler.UniversalBucket(),
-		enq: enq,
+		// The UniversalBucket owns everything, so there is never a need to
+		// enqueue things (no possible state change).  We pass nil here to
+		// avoid filling the queue for an extra resynce at startup along
+		// this path.
+		enq: nil,
 	}, nil
 }
 

leaderelection/context_test.go

@@ -323,3 +323,35 @@ func TestWithStatefulSetBuilder(t *testing.T) {
 		t.Fatal("Timed out waiting for promotion.")
 	}
 }
+
+func TestWithUnopposedElector(t *testing.T) {
+	laf := &reconciler.LeaderAwareFuncs{
+		PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
+			t.Error("Unexpected call to PromoteFunc with unopposedElector")
+			return nil
+		},
+	}
+
+	ctx := context.Background()
+	if HasLeaderElection(ctx) {
+		t.Error("HasLeaderElection() = true, wanted false")
+	}
+
+	le, err := BuildElector(ctx, laf, "name", func(reconciler.Bucket, types.NamespacedName) {
+		t.Error("Unexpected call to enqueue function.")
+	})
+	if err != nil {
+		t.Fatal("BuildElector() =", err)
+	}
+
+	if _, ok := le.(*unopposedElector); !ok {
+		t.Fatalf("BuildElector() = %T, wanted an unopposedElector", le)
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	go le.Run(ctx)
+
+	// Wait to see if PromoteFunc is called with nil or our enq function.
+	<-time.After(time.Second)
+}

reconciler/leader.go

@@ -97,7 +97,7 @@ func (laf *LeaderAwareFuncs) Promote(b Bucket, enq func(Bucket, types.Namespaced
 		laf.buckets[b.Name()] = b
 	}()
 
-	if promote := laf.PromoteFunc; promote != nil {
+	if promote := laf.PromoteFunc; promote != nil && enq != nil {
 		return promote(b, enq)
 	}
 	return nil

reconciler/leader_test.go

@@ -42,6 +42,11 @@ func TestLeaderAwareFuncs(t *testing.T) {
 	}
 
 	laf.PromoteFunc = func(bkt Bucket, gotFunc func(Bucket, types.NamespacedName)) error {
+		if gotFunc == nil {
+			t.Error("PromoteFunc should not be called with nil gotFunc")
+			return nil
+		}
+
 		gotFunc(bkt, wantKey)
 		if !called {
 			t.Error("gotFunc didn't call wantFunc!")
@@ -79,4 +84,7 @@ func TestLeaderAwareFuncs(t *testing.T) {
 	if laf.IsLeaderFor(wantKey) {
 		t.Error("IsLeaderFor() = true, wanted false")
 	}
+
+	// We should elide calls with a nil enq
+	laf.Promote(wantBkt, nil)
 }