From 7b9f8bd470031d9506cddb5928a665b9d53f9687 Mon Sep 17 00:00:00 2001 From: Matthew Staebler Date: Fri, 13 Jul 2018 16:50:32 -0400 Subject: [PATCH] Re-work TestCreateServiceInstanceWithProvisionFailure to mitigate its flake (#2153) * Re-work the TestCreateServiceInstanceWithProvisionFailure integration test to mitigate its flake * Revert change to AssertServiceInstanceCondition --- test/integration/controller_instance_test.go | 227 ++++++++++++------- test/util/util.go | 13 ++ 2 files changed, 159 insertions(+), 81 deletions(-) diff --git a/test/integration/controller_instance_test.go b/test/integration/controller_instance_test.go index a170e1db5c9..5d56b7335f9 100644 --- a/test/integration/controller_instance_test.go +++ b/test/integration/controller_instance_test.go @@ -1005,72 +1005,70 @@ func (e TimeoutError) Error() string { // TestCreateServiceInstanceWithProvisionFailure tests creating a ServiceInstance // with various failure results in response to the provision request. -// TODO(carolynvs): I'm disabling this test because it's a flake that fails so much that I'm seeing 🔥 -func FlakeTestCreateServiceInstanceWithProvisionFailure(t *testing.T) { +func TestCreateServiceInstanceWithProvisionFailure(t *testing.T) { cases := []struct { - name string - statusCode int - nonHTTPResponseError error - conditionReason string - expectFailCondition bool + // name of the test + name string + // status code returned by failing provision calls + statusCode int + // non-HTTP error returned by failing provision calls + nonHTTPResponseError error + // expected reason used in the instance condition to indiciate that the provision failed + provisionErrorReason string + // expected reason used in the instance condiiton to indicate that the provision failed terminally + failReason string + // true if the failed provision is expected to trigger orphan mitigation triggersOrphanMitigation bool }{ { - name: "Status OK", - statusCode: http.StatusOK, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + name: "Status OK", + statusCode: http.StatusOK, + provisionErrorReason: "ProvisionCallFailed", }, { name: "Status Created", statusCode: http.StatusCreated, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + provisionErrorReason: "ProvisionCallFailed", triggersOrphanMitigation: true, }, { name: "other 2xx", statusCode: http.StatusNoContent, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + provisionErrorReason: "ProvisionCallFailed", triggersOrphanMitigation: true, }, { - name: "3XX", - statusCode: 300, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + name: "3XX", + statusCode: 300, + provisionErrorReason: "ProvisionCallFailed", }, { name: "Status Request Timeout", statusCode: http.StatusRequestTimeout, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + provisionErrorReason: "ProvisionCallFailed", triggersOrphanMitigation: false, }, { - name: "400", - statusCode: 400, - conditionReason: "ClusterServiceBrokerReturnedFailure", - expectFailCondition: true, + name: "400", + statusCode: 400, + provisionErrorReason: "ProvisionCallFailed", + failReason: "ClusterServiceBrokerReturnedFailure", }, { - name: "other 4XX", - statusCode: 403, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + name: "other 4XX", + statusCode: 403, + provisionErrorReason: "ProvisionCallFailed", }, { name: "5XX", statusCode: 500, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + provisionErrorReason: "ProvisionCallFailed", triggersOrphanMitigation: true, }, { name: "non-url transport error", nonHTTPResponseError: fmt.Errorf("non-url error"), - conditionReason: "ProvisionedSuccessfully", + provisionErrorReason: "ErrorCallingProvision", }, { name: "non-timeout url error", @@ -1079,7 +1077,7 @@ func FlakeTestCreateServiceInstanceWithProvisionFailure(t *testing.T) { URL: "https://fakebroker.com/v2/service_instances/instance_id", Err: fmt.Errorf("non-timeout error"), }, - conditionReason: "ProvisionedSuccessfully", + provisionErrorReason: "ErrorCallingProvision", }, { name: "network timeout", @@ -1088,14 +1086,26 @@ func FlakeTestCreateServiceInstanceWithProvisionFailure(t *testing.T) { URL: "https://fakebroker.com/v2/service_instances/instance_id", Err: TimeoutError("timeout error"), }, - conditionReason: "ProvisionedSuccessfully", - expectFailCondition: false, + provisionErrorReason: "ErrorCallingProvision", triggersOrphanMitigation: true, }, } for _, tc := range cases { tc := tc t.Run(tc.name, func(t *testing.T) { + + provisionSuccessChan := make(chan bool, 2) + deprovisionSuccessChan := make(chan bool, 2) + deprovisionBlockChan := make(chan bool, 2) + + // Ensure that broker requests respond successfully after running + // the core of the test so that the resource cleanup can proceed. + defer func() { + provisionSuccessChan <- true + deprovisionSuccessChan <- true + deprovisionBlockChan <- false + }() + //t.Parallel() ct := &controllerTest{ t: t, @@ -1111,72 +1121,127 @@ func FlakeTestCreateServiceInstanceWithProvisionFailure(t *testing.T) { Description: strPtr("response description"), } } + respondSuccessfullyToProvision := false ct.osbClient.ProvisionReaction = fakeosb.DynamicProvisionReaction( - getProvisionResponseByPollCountReactions(2, []fakeosb.ProvisionReaction{ - fakeosb.ProvisionReaction{ - Error: reactionError, - }, - fakeosb.ProvisionReaction{ - Response: &osb.ProvisionResponse{}, - }, - })) + func(r *osb.ProvisionRequest) (*osb.ProvisionResponse, error) { + select { + case respondSuccessfullyToProvision = <-provisionSuccessChan: + default: + } + if respondSuccessfullyToProvision { + return &osb.ProvisionResponse{}, nil + } else { + return nil, reactionError + } + }) + respondSuccessfullyToDeprovision := false + blockDeprovision := true ct.osbClient.DeprovisionReaction = fakeosb.DynamicDeprovisionReaction( - getDeprovisionResponseByPollCountReactions(2, []fakeosb.DeprovisionReaction{ - fakeosb.DeprovisionReaction{ - Error: osb.HTTPStatusCodeError{ + func(r *osb.DeprovisionRequest) (*osb.DeprovisionResponse, error) { + for blockDeprovision { + blockDeprovision = <-deprovisionBlockChan + } + select { + case respondSuccessfullyToDeprovision = <-deprovisionSuccessChan: + default: + } + if respondSuccessfullyToDeprovision { + return &osb.DeprovisionResponse{}, nil + } else { + return nil, osb.HTTPStatusCodeError{ StatusCode: 500, ErrorMessage: strPtr("temporary deprovision error"), - }, - }, - fakeosb.DeprovisionReaction{ - Response: &osb.DeprovisionResponse{}, - }, - })) + } + } + }) }, } ct.run(func(ct *controllerTest) { - var condition v1beta1.ServiceInstanceCondition - if tc.expectFailCondition { - // Instance should get stuck in a Failed condition - condition = v1beta1.ServiceInstanceCondition{ - Type: v1beta1.ServiceInstanceConditionFailed, - Status: v1beta1.ConditionTrue, - Reason: tc.conditionReason, - } - } else { - // Instance provisioning should be retried and succeed - condition = v1beta1.ServiceInstanceCondition{ - Type: v1beta1.ServiceInstanceConditionReady, - Status: v1beta1.ConditionTrue, - Reason: tc.conditionReason, - } + // Wait for the provision to fail + condition := v1beta1.ServiceInstanceCondition{ + Type: v1beta1.ServiceInstanceConditionReady, + Status: v1beta1.ConditionFalse, + Reason: tc.provisionErrorReason, + } + if tc.triggersOrphanMitigation { + condition.Reason = "StartingInstanceOrphanMitigation" } if err := util.WaitForInstanceCondition(ct.client, testNamespace, testInstanceName, condition); err != nil { - t.Fatalf("error waiting for instance condition: %v", err) + t.Fatalf("error waiting for provision to fail: %v", err) } - if tc.expectFailCondition { - if err := util.WaitForInstanceProcessedGeneration(ct.client, ct.instance.Namespace, ct.instance.Name, 1); err != nil { - t.Fatalf("error waiting for instance reconciliation to complete: %v", err) - } + // Assert that the latest generation has been observed + instance, err := ct.client.ServiceInstances(testNamespace).Get(testInstanceName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance: %v", err) + } + if e, a := int64(1), instance.Status.ObservedGeneration; e != a { + t.Fatalf("unexpected observed generation: expected %v, got %v", e, a) } - brokerActions := ct.osbClient.Actions() - fmt.Printf("%#v", brokerActions) + // If the provision failed with a terminating failure + if tc.failReason != "" { + util.AssertServiceInstanceCondition(t, instance, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, tc.failReason) + if e, a := 0, len(findBrokerActions(t, ct.osbClient, fakeosb.DeprovisionInstance)); e != a { + t.Fatalf("unexpected calls to deprovision instance: expected %v, got %v", e, a) + } + return + } - // Ensure that we meet expectations on deprovision requests for orphan mitigation - deprovisionActions := findBrokerActions(t, ct.osbClient, fakeosb.DeprovisionInstance) + // Assert that the orphan mitigation reason was set correctly if tc.triggersOrphanMitigation { - if len(deprovisionActions) == 0 { - t.Fatal("expected orphan mitigation deprovision request to occur") + util.AssertServiceInstanceCondition(t, instance, v1beta1.ServiceInstanceConditionOrphanMitigation, v1beta1.ConditionTrue, tc.provisionErrorReason) + if !instance.Status.OrphanMitigationInProgress { + t.Fatalf("expected orphan mitigation to be in progress") } } else { - if len(deprovisionActions) != 0 { - t.Fatal("unexpected deprovision requests") + util.AssertServiceInstanceConditionFalseOrAbsent(t, instance, v1beta1.ServiceInstanceConditionOrphanMitigation) + if instance.Status.OrphanMitigationInProgress { + t.Fatalf("expected orphan mitigation to not be in progress") + } + } + + // Now that the provision error conditions have been asserted, allow the broker to send its response to the deprovision request + deprovisionBlockChan <- false + + if tc.triggersOrphanMitigation { + // Assert that the ready condition is set to Unknown when the deprovision request fails + if err := util.WaitForInstanceCondition(ct.client, testNamespace, testInstanceName, v1beta1.ServiceInstanceCondition{ + Type: v1beta1.ServiceInstanceConditionReady, + Status: v1beta1.ConditionUnknown, + }); err != nil { + t.Fatalf("error waiting for instance deprovision to fail: %v", err) } } - // All instances should eventually succeed + // Now that everything surround the failed provision has been asserted, allow provision requests + // to succeed. Also, allow orphan mitigation to complete by allowing deprovision requests to succeed. + provisionSuccessChan <- true + deprovisionSuccessChan <- true + + // Wait for the instance to be provisioned successfully + if err := util.WaitForInstanceCondition(ct.client, testNamespace, testInstanceName, v1beta1.ServiceInstanceCondition{ + Type: v1beta1.ServiceInstanceConditionReady, + Status: v1beta1.ConditionTrue, + }); err != nil { + t.Fatalf("error waiting for instance to be provisioned: %v", err) + } + + // Assert that the observed generation is up-to-date, that orphan mitigation is not in progress, + // and that the instance is not in a failed state. + instance, err = ct.client.ServiceInstances(testNamespace).Get(testInstanceName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance: %v", err) + } + if g, og := instance.Generation, instance.Status.ObservedGeneration; g != og { + t.Fatalf("latest generation not observed: generation: %v, observed: %v", g, og) + } + if instance.Status.OrphanMitigationInProgress { + t.Fatalf("unexpected orphan mitigation in progress") + } + util.AssertServiceInstanceConditionFalseOrAbsent(t, instance, v1beta1.ServiceInstanceConditionFailed) + + // Assert that the last broker action was a provision-instance request. getLastBrokerAction(t, ct.osbClient, fakeosb.ProvisionInstance) }) }) diff --git a/test/util/util.go b/test/util/util.go index c3a81a1ef89..b84fc4395fc 100644 --- a/test/util/util.go +++ b/test/util/util.go @@ -369,3 +369,16 @@ func AssertServiceBindingCondition(t *testing.T, binding *v1beta1.ServiceBinding t.Fatalf("%v condition not found", conditionType) } } + +// AssertServiceInstanceConditionFalseOrAbsent asserts that the instance's status +// either contains the given condition type with a status of False or does not +// contain the given condition. +func AssertServiceInstanceConditionFalseOrAbsent(t *testing.T, instance *v1beta1.ServiceInstance, conditionType v1beta1.ServiceInstanceConditionType) { + for _, condition := range instance.Status.Conditions { + if condition.Type == conditionType { + if e, a := v1beta1.ConditionFalse, condition.Status; e != a { + t.Fatalf("%v condition had unexpected status; expected %v, got %v", conditionType, e, a) + } + } + } +}