🐛 Fix a race condition between leader election and recorder

This change introduces better syncronization between the leader election
code and the event recorder. Running tests with -race flag, we often saw
a panic on a closed channel, the channel was the one that the event
recorder was using internally.

After digging more through the code, it seems that we weren't properly
waiting for leader election code to stop completely, but instead we were
only calling the cancel() function asking the leader election to stop.

With this change, during a shutdown, we now wait for leader election to
finish up any internal task before we return and close an internal
channel. Only after leader election signals that the channel has been
closed, and  Run(...) has properly returned, we return execution to the
stop procedure, where the event recorder is then stopped.

Signed-off-by: Vince Prignano <[email protected]>

Author: vincepri

pkg/manager/internal.go

@@ -121,6 +121,10 @@ type controllerManager struct {
 	// it must be deferred until after gracefulShutdown is done.
 	leaderElectionCancel context.CancelFunc
 
+	// leaderElectionStopped is an internal channel used to signal the stopping procedure that the
+	// LeaderElection.Run(...) function has returned and the shutdown can proceed.
+	leaderElectionStopped chan struct{}
+
 	// stop procedure engaged. In other words, we should not add anything else to the manager
 	stopProcedureEngaged bool
 
@@ -557,7 +561,12 @@ func (cm *controllerManager) waitForRunnableToEnd(shutdownCancel context.CancelF
 	// Cancel leader election only after we waited. It will os.Exit() the app for safety.
 	defer func() {
 		if cm.leaderElectionCancel != nil {
+			// After asking the context to be cancelled, make sure
+			// we wait for the leader stopped channel to be closed, otherwise
+			// we might encounter race conditions between this code
+			// and the event recorder, which is used within leader election code.
 			cm.leaderElectionCancel()
+			<-cm.leaderElectionStopped
 		}
 	}()
 
@@ -660,7 +669,11 @@ func (cm *controllerManager) startLeaderElection() (err error) {
 	}
 
 	// Start the leader elector process
-	go l.Run(ctx)
+	go func() {
+		l.Run(ctx)
+		<-ctx.Done()
+		close(cm.leaderElectionStopped)
+	}()
 	return nil
 }
 

pkg/manager/manager.go

@@ -359,6 +359,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		livenessEndpointName:    options.LivenessEndpointName,
 		gracefulShutdownTimeout: *options.GracefulShutdownTimeout,
 		internalProceduresStop:  make(chan struct{}),
+		leaderElectionStopped:   make(chan struct{}),
 	}, nil
 }