Merge pull request #56446 from ironcladlou/gc-test-flakes

pkg/controller/garbagecollector/garbagecollector.go

@@ -173,21 +173,14 @@ func (gc *GarbageCollector) Sync(discoveryClient discovery.DiscoveryInterface, p
 		// Get the current resource list from discovery.
 		newResources := GetDeletableResources(discoveryClient)
 
-		// Detect first or abnormal sync and try again later.
-		if oldResources == nil || len(oldResources) == 0 {
-			oldResources = newResources
-			return
-		}
-
 		// Decide whether discovery has reported a change.
 		if reflect.DeepEqual(oldResources, newResources) {
 			glog.V(5).Infof("no resource updates from discovery, skipping garbage collector sync")
 			return
 		}
 
-		// Something has changed, so track the new state and perform a sync.
+		// Something has changed, time to sync.
 		glog.V(2).Infof("syncing garbage collector with updated resources from discovery: %v", newResources)
-		oldResources = newResources
 
 		// Ensure workers are paused to avoid processing events before informers
 		// have resynced.
@@ -212,9 +205,19 @@ func (gc *GarbageCollector) Sync(discoveryClient discovery.DiscoveryInterface, p
 			utilruntime.HandleError(fmt.Errorf("failed to sync resource monitors: %v", err))
 			return
 		}
+		// TODO: WaitForCacheSync can block forever during normal operation. Could
+		// pass a timeout channel, but we have to consider the implications of
+		// un-pausing the GC with a partially synced graph builder.
 		if !controller.WaitForCacheSync("garbage collector", stopCh, gc.dependencyGraphBuilder.IsSynced) {
 			utilruntime.HandleError(fmt.Errorf("timed out waiting for dependency graph builder sync during GC sync"))
+			return
 		}
+
+		// Finally, keep track of our new state. Do this after all preceding steps
+		// have succeeded to ensure we'll retry on subsequent syncs if an error
+		// occured.
+		oldResources = newResources
+		glog.V(2).Infof("synced garbage collector")
 	}, period, stopCh)
 }