From f50696adda97e01b25cca56029ff663cf208a5e8 Mon Sep 17 00:00:00 2001 From: Joel Smith Date: Thu, 11 Apr 2019 11:21:58 -0600 Subject: [PATCH] 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 --- .../podautoscaler/horizontal_test.go | 89 +++++++++++-------- .../podautoscaler/legacy_horizontal_test.go | 25 +++--- 2 files changed, 66 insertions(+), 48 deletions(-) diff --git a/pkg/controller/podautoscaler/horizontal_test.go b/pkg/controller/podautoscaler/horizontal_test.go index 524f6e7c8a..bf78265d7d 100644 --- a/pkg/controller/podautoscaler/horizontal_test.go +++ b/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() + 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") + 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 <- "" diff --git a/pkg/controller/podautoscaler/legacy_horizontal_test.go b/pkg/controller/podautoscaler/legacy_horizontal_test.go index 8b42fe9a8e..a0621a36d3 100644 --- a/pkg/controller/podautoscaler/legacy_horizontal_test.go +++ b/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 := 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") + 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