Fix potential test flakes in HPA tests TestEventNotCreated and TestAvoidUncessaryUpdates

Also, re-work the code so that the lock is never held while writing to the chan

Author: joelsmith

pkg/controller/podautoscaler/horizontal_test.go

@@ -330,38 +330,43 @@ func (tc *testCase) prepareTestClient(t *testing.T) (*fake.Clientset, *metricsfa
 	})
 
 	fakeClient.AddReactor("update", "horizontalpodautoscalers", func(action core.Action) (handled bool, ret runtime.Object, err error) {
-		tc.Lock()
-		defer tc.Unlock()
-
-		obj := action.(core.UpdateAction).GetObject().(*autoscalingv1.HorizontalPodAutoscaler)
-		assert.Equal(t, namespace, obj.Namespace, "the HPA namespace should be as expected")
-		assert.Equal(t, hpaName, obj.Name, "the HPA name should be as expected")
-		assert.Equal(t, tc.expectedDesiredReplicas, obj.Status.DesiredReplicas, "the desired replica count reported in the object status should be as expected")
-		if tc.verifyCPUCurrent {
-			if assert.NotNil(t, obj.Status.CurrentCPUUtilizationPercentage, "the reported CPU utilization percentage should be non-nil") {
-				assert.Equal(t, tc.CPUCurrent, *obj.Status.CurrentCPUUtilizationPercentage, "the report CPU utilization percentage should be as expected")
+		handled, obj, err := func() (handled bool, ret *autoscalingv1.HorizontalPodAutoscaler, err error) {
+			tc.Lock()
+			defer tc.Unlock()
+
+			obj := action.(core.UpdateAction).GetObject().(*autoscalingv1.HorizontalPodAutoscaler)
+			assert.Equal(t, namespace, obj.Namespace, "the HPA namespace should be as expected")
+			assert.Equal(t, hpaName, obj.Name, "the HPA name should be as expected")
+			assert.Equal(t, tc.expectedDesiredReplicas, obj.Status.DesiredReplicas, "the desired replica count reported in the object status should be as expected")
+			if tc.verifyCPUCurrent {
+				if assert.NotNil(t, obj.Status.CurrentCPUUtilizationPercentage, "the reported CPU utilization percentage should be non-nil") {
+					assert.Equal(t, tc.CPUCurrent, *obj.Status.CurrentCPUUtilizationPercentage, "the report CPU utilization percentage should be as expected")
+				}
 			}
+			var actualConditions []autoscalingv1.HorizontalPodAutoscalerCondition
+			if err := json.Unmarshal([]byte(obj.ObjectMeta.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation]), &actualConditions); err != nil {
+				return true, nil, err
+			}
+			// TODO: it's ok not to sort these becaues statusOk
+			// contains all the conditions, so we'll never be appending.
+			// Default to statusOk when missing any specific conditions
+			if tc.expectedConditions == nil {
+				tc.expectedConditions = statusOkWithOverrides()
+			}
+			// clear the message so that we can easily compare
+			for i := range actualConditions {
+				actualConditions[i].Message = ""
+				actualConditions[i].LastTransitionTime = metav1.Time{}
+			}
+			assert.Equal(t, tc.expectedConditions, actualConditions, "the status conditions should have been as expected")
+			tc.statusUpdated = true
+			// Every time we reconcile HPA object we are updating status.
+			return true, obj, nil
+		}()
+		if obj != nil {
+			tc.processed <- obj.Name
 		}
-		var actualConditions []autoscalingv1.HorizontalPodAutoscalerCondition
-		if err := json.Unmarshal([]byte(obj.ObjectMeta.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation]), &actualConditions); err != nil {
-			return true, nil, err
-		}
-		// TODO: it's ok not to sort these becaues statusOk
-		// contains all the conditions, so we'll never be appending.
-		// Default to statusOk when missing any specific conditions
-		if tc.expectedConditions == nil {
-			tc.expectedConditions = statusOkWithOverrides()
-		}
-		// clear the message so that we can easily compare
-		for i := range actualConditions {
-			actualConditions[i].Message = ""
-			actualConditions[i].LastTransitionTime = metav1.Time{}
-		}
-		assert.Equal(t, tc.expectedConditions, actualConditions, "the status conditions should have been as expected")
-		tc.statusUpdated = true
-		// Every time we reconcile HPA object we are updating status.
-		tc.processed <- obj.Name
-		return true, obj, nil
+		return handled, obj, err
 	})
 
 	fakeScaleClient := &scalefake.FakeScaleClient{}
@@ -701,15 +706,25 @@ func (tc *testCase) runTestWithController(t *testing.T, hpaController *Horizonta
 	go hpaController.Run(stop)
 
 	tc.Lock()
-	if tc.verifyEvents {
-		tc.Unlock()
+	shouldWait := tc.verifyEvents
+	tc.Unlock()
+
+	if shouldWait {
 		// We need to wait for events to be broadcasted (sleep for longer than record.sleepDuration).
-		time.Sleep(2 * time.Second)
+		timeoutTime := time.Now().Add(2 * time.Second)
+		for now := time.Now(); timeoutTime.After(now); now = time.Now() {
+			sleepUntil := timeoutTime.Sub(now)
+			select {
+			case <-tc.processed:
+				// drain the chan of any sent events to keep it from filling before the timeout
+			case <-time.After(sleepUntil):
+				// timeout reached, ready to verifyResults
+			}
+		}
 	} else {
-		tc.Unlock()
+		// Wait for HPA to be processed.
+		<-tc.processed
 	}
-	// Wait for HPA to be processed.
-	<-tc.processed
 	tc.verifyResults(t)
 }
 
@@ -2418,7 +2433,9 @@ func TestAvoidUncessaryUpdates(t *testing.T) {
 			// wait a tick and then mark that we're finished (otherwise, we have no
 			// way to indicate that we're finished, because the function decides not to do anything)
 			time.Sleep(1 * time.Second)
+			tc.Lock()
 			tc.statusUpdated = true
+			tc.Unlock()
 			tc.processed <- "test-hpa"
 		}()
 
@@ -2493,8 +2510,6 @@ func TestAvoidUncessaryUpdates(t *testing.T) {
 		return true, objv1, nil
 	})
 	testClient.PrependReactor("update", "horizontalpodautoscalers", func(action core.Action) (handled bool, ret runtime.Object, err error) {
-		tc.Lock()
-		defer tc.Unlock()
 		assert.Fail(t, "should not have attempted to update the HPA when nothing changed")
 		// mark that we've processed this HPA
 		tc.processed <- ""

pkg/controller/podautoscaler/legacy_horizontal_test.go

@@ -332,19 +332,22 @@ func (tc *legacyTestCase) prepareTestClient(t *testing.T) (*fake.Clientset, *sca
 	})
 
 	fakeClient.AddReactor("update", "horizontalpodautoscalers", func(action core.Action) (handled bool, ret runtime.Object, err error) {
-		tc.Lock()
-		defer tc.Unlock()
-
-		obj := action.(core.UpdateAction).GetObject().(*autoscalingv1.HorizontalPodAutoscaler)
-		assert.Equal(t, namespace, obj.Namespace, "the HPA namespace should be as expected")
-		assert.Equal(t, hpaName, obj.Name, "the HPA name should be as expected")
-		assert.Equal(t, tc.desiredReplicas, obj.Status.DesiredReplicas, "the desired replica count reported in the object status should be as expected")
-		if tc.verifyCPUCurrent {
-			if assert.NotNil(t, obj.Status.CurrentCPUUtilizationPercentage, "the reported CPU utilization percentage should be non-nil") {
-				assert.Equal(t, tc.CPUCurrent, *obj.Status.CurrentCPUUtilizationPercentage, "the report CPU utilization percentage should be as expected")
+		obj := func() *autoscalingv1.HorizontalPodAutoscaler {
+			tc.Lock()
+			defer tc.Unlock()
+
+			obj := action.(core.UpdateAction).GetObject().(*autoscalingv1.HorizontalPodAutoscaler)
+			assert.Equal(t, namespace, obj.Namespace, "the HPA namespace should be as expected")
+			assert.Equal(t, hpaName, obj.Name, "the HPA name should be as expected")
+			assert.Equal(t, tc.desiredReplicas, obj.Status.DesiredReplicas, "the desired replica count reported in the object status should be as expected")
+			if tc.verifyCPUCurrent {
+				if assert.NotNil(t, obj.Status.CurrentCPUUtilizationPercentage, "the reported CPU utilization percentage should be non-nil") {
+					assert.Equal(t, tc.CPUCurrent, *obj.Status.CurrentCPUUtilizationPercentage, "the report CPU utilization percentage should be as expected")
+				}
 			}
-		}
-		tc.statusUpdated = true
+			tc.statusUpdated = true
+			return obj
+		}()
 		// Every time we reconcile HPA object we are updating status.
 		tc.processed <- obj.Name
 		return true, obj, nil