fix async deprovision retry (openshift#1832)

pkg/controller/controller_instance.go

@@ -746,21 +746,17 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro
 		switch {
 		case deleting:
 			// For deprovisioning only, we should reattempt even on failure
-			//
-			// TODO(mkibbe): This is actually broken today, as we
-			// will not actually be retrying the deprovision, but
-			// instead constantly re-hitting the "last_operation"
-			// endpoint with the same operation, which will forever
-			// be stuck on failure.
 			msg := "Deprovision call failed: " + description
 			readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionUnknown, errorDeprovisionCalledReason, msg)
 
 			if c.reconciliationRetryDurationExceeded(instance.Status.OperationStartTime) {
 				return c.processServiceInstancePollingFailureRetryTimeout(instance, readyCond)
 			}
 
-			c.processServiceInstanceOperationError(instance, readyCond)
-			return c.continuePollingServiceInstance(instance)
+			clearServiceInstanceAsyncOsbOperation(instance)
+			c.finishPollingServiceInstance(instance)
+
+			return c.processServiceInstanceOperationError(instance, readyCond)
 		case provisioning:
 			reason := errorProvisionCallFailedReason
 			message := "Provision call failed: " + description
@@ -790,6 +786,15 @@ func (c *controller) pollServiceInstance(instance *v1beta1.ServiceInstance) erro
 	}
 }
 
+// clearServiceInstanceAsyncOsbOperation will reset the given instance's
+// asynchronous OSB operation status fields. Note: This does not clear the
+// Service Catalog operation, only the concept of "operation" as part of the
+// OSB async flow.
+func clearServiceInstanceAsyncOsbOperation(instance *v1beta1.ServiceInstance) {
+	instance.Status.AsyncOpInProgress = false
+	instance.Status.LastOperation = nil
+}
+
 // isServiceInstanceProcessedAlready returns true if there is no further processing
 // needed for the instance based on ObservedGeneration
 func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool {

pkg/controller/controller_instance_test.go

@@ -2460,12 +2460,12 @@ func TestPollServiceInstanceFailureDeprovisioning(t *testing.T) {
 	}
 
 	err := testController.pollServiceInstance(instance)
-	if err != nil {
-		t.Fatalf("pollServiceInstance failed: %s", err)
+	if err == nil {
+		t.Fatalf("Expected pollServiceInstance to return an error but there was none")
 	}
 
-	if testController.instancePollingQueue.NumRequeues(instanceKey) != 1 {
-		t.Fatalf("Expected polling queue to have record of seeing test instance once")
+	if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 {
+		t.Fatalf("Expected polling queue to not have any record of test instance as polling should have completed")
 	}
 
 	brokerActions := fakeClusterServiceBrokerClient.Actions()

test/integration/controller_instance_test.go

@@ -1544,3 +1544,33 @@ func TestPollServiceInstanceLastOperation(t *testing.T) {
 		})
 	}
 }
+
+// TestRetryAsyncDeprovision tests whether asynchronous deprovisioning retries
+// by attempting a new deprovision after failing.
+func TestRetryAsyncDeprovision(t *testing.T) {
+	hasPollFailed := false
+	ct := &controllerTest{
+		t:        t,
+		broker:   getTestBroker(),
+		instance: getTestInstance(),
+		setup: func(ct *controllerTest) {
+			ct.osbClient.DeprovisionReaction = fakeosb.DynamicDeprovisionReaction(
+				func(_ *osb.DeprovisionRequest) (*osb.DeprovisionResponse, error) {
+					response := &osb.DeprovisionResponse{Async: true}
+					if hasPollFailed {
+						response.Async = false
+					}
+					return response, nil
+				})
+
+			ct.osbClient.PollLastOperationReaction = fakeosb.DynamicPollLastOperationReaction(
+				func(_ *osb.LastOperationRequest) (*osb.LastOperationResponse, error) {
+					hasPollFailed = true
+					return &osb.LastOperationResponse{
+						State: osb.StateFailed,
+					}, nil
+				})
+		},
+	}
+	ct.run(func(_ *controllerTest) {})
+}