Merge pull request #466 from abursavich/dontpanic

🐛 Fix runnable race in manager

Author: k8s-ci-robot

pkg/manager/internal.go

@@ -288,19 +288,7 @@ func (cm *controllerManager) startNonLeaderElectionRunnables() {
 	cm.mu.Lock()
 	defer cm.mu.Unlock()
 
-	// Start the Cache. Allow the function to start the cache to be mocked out for testing
-	if cm.startCache == nil {
-		cm.startCache = cm.cache.Start
-	}
-	go func() {
-		if err := cm.startCache(cm.internalStop); err != nil {
-			cm.errChan <- err
-		}
-	}()
-
-	// Wait for the caches to sync.
-	// TODO(community): Check the return value and write a test
-	cm.cache.WaitForCacheSync(cm.internalStop)
+	cm.waitForCache()
 
 	// Start the non-leaderelection Runnables after the cache has synced
 	for _, c := range cm.nonLeaderElectionRunnables {
@@ -311,14 +299,13 @@ func (cm *controllerManager) startNonLeaderElectionRunnables() {
 			cm.errChan <- ctrl.Start(cm.internalStop)
 		}()
 	}
-
-	cm.started = true
 }
 
 func (cm *controllerManager) startLeaderElectionRunnables() {
-	// Wait for the caches to sync.
-	// TODO(community): Check the return value and write a test
-	cm.cache.WaitForCacheSync(cm.internalStop)
+	cm.mu.Lock()
+	defer cm.mu.Unlock()
+
+	cm.waitForCache()
 
 	// Start the leader election Runnables after the cache has synced
 	for _, c := range cm.leaderElectionRunnables {
@@ -329,7 +316,26 @@ func (cm *controllerManager) startLeaderElectionRunnables() {
 			cm.errChan <- ctrl.Start(cm.internalStop)
 		}()
 	}
+}
+
+func (cm *controllerManager) waitForCache() {
+	if cm.started {
+		return
+	}
 
+	// Start the Cache. Allow the function to start the cache to be mocked out for testing
+	if cm.startCache == nil {
+		cm.startCache = cm.cache.Start
+	}
+	go func() {
+		if err := cm.startCache(cm.internalStop); err != nil {
+			cm.errChan <- err
+		}
+	}()
+
+	// Wait for the caches to sync.
+	// TODO(community): Check the return value and write a test
+	cm.cache.WaitForCacheSync(cm.internalStop)
 	cm.started = true
 }
 

pkg/manager/manager_test.go

@@ -206,15 +206,15 @@ var _ = Describe("manger.Manager", func() {
 				Expect(err).NotTo(HaveOccurred())
 				c1 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
-					defer close(c1)
 					defer GinkgoRecover()
+					close(c1)
 					return nil
 				}))).To(Succeed())
 
 				c2 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
-					defer close(c2)
 					defer GinkgoRecover()
+					close(c2)
 					return nil
 				}))).To(Succeed())
 
@@ -257,21 +257,21 @@ var _ = Describe("manger.Manager", func() {
 				c1 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
 					defer GinkgoRecover()
-					defer close(c1)
+					close(c1)
 					return nil
 				}))).To(Succeed())
 
 				c2 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
 					defer GinkgoRecover()
-					defer close(c2)
+					close(c2)
 					return fmt.Errorf("expected error")
 				}))).To(Succeed())
 
 				c3 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
 					defer GinkgoRecover()
-					defer close(c3)
+					close(c3)
 					return nil
 				}))).To(Succeed())
 
@@ -441,8 +441,8 @@ var _ = Describe("manger.Manager", func() {
 				// Add one component before starting
 				c1 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
-					defer close(c1)
 					defer GinkgoRecover()
+					close(c1)
 					return nil
 				}))).To(Succeed())
 
@@ -457,8 +457,8 @@ var _ = Describe("manger.Manager", func() {
 				// Add another component after starting
 				c2 := make(chan struct{})
 				Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
-					defer close(c2)
 					defer GinkgoRecover()
+					close(c2)
 					return nil
 				}))).To(Succeed())
 				<-c1
@@ -483,8 +483,8 @@ var _ = Describe("manger.Manager", func() {
 
 			c1 := make(chan struct{})
 			Expect(m.Add(RunnableFunc(func(s <-chan struct{}) error {
-				defer close(c1)
 				defer GinkgoRecover()
+				close(c1)
 				return nil
 			}))).To(Succeed())
 			<-c1