make deprovisioning an instance asynchronously not fall-through to sy…

…nchronous deprovision (#1067)

pkg/controller/controller_broker.go

@@ -108,7 +108,7 @@ const (
 	successUnboundReason             string = "UnboundSuccessfully"
 	asyncProvisioningReason          string = "Provisioning"
 	asyncProvisioningMessage         string = "The instance is being provisioned asynchronously"
-	asyncDeprovisioningReason        string = "Derovisioning"
+	asyncDeprovisioningReason        string = "Deprovisioning"
 	asyncDeprovisioningMessage       string = "The instance is being deprovisioned asynchronously"
 )
 

pkg/controller/controller_instance.go

@@ -254,10 +254,14 @@ func (c *controller) reconcileInstanceDelete(instance *v1alpha1.Instance) error
 			if err != nil {
 				return err
 			}
-		} else {
-			glog.V(5).Infof("Deprovision call to broker succeeded for Instance %v/%v, finalizing", instance.Namespace, instance.Name)
+
+			c.recorder.Eventf(instance, api.EventTypeNormal, asyncDeprovisioningReason, asyncDeprovisioningMessage)
+
+			return nil
 		}
 
+		glog.V(5).Infof("Deprovision call to broker succeeded for Instance %v/%v, finalizing", instance.Namespace, instance.Name)
+
 		setInstanceCondition(
 			toUpdate,
 			v1alpha1.InstanceConditionReady,
@@ -318,6 +322,10 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 		return nil
 	}
 
+	if instance.Status.AsyncOpInProgress {
+		return c.pollInstanceInternal(instance)
+	}
+
 	// If there's no async op in progress, determine whether the checksum
 	// has been invalidated by a change to the object. If the instance's
 	// checksum matches the calculated checksum, there is no work to do.
@@ -334,38 +342,31 @@ func (c *controller) reconcileInstance(instance *v1alpha1.Instance) error {
 	// communicating with the broker.  In the future the same logic will
 	// result in an instance that requires update being processed by the
 	// controller.
-	if !instance.Status.AsyncOpInProgress {
-		if instance.Status.Checksum != nil && instance.DeletionTimestamp == nil {
-			instanceChecksum := checksum.InstanceSpecChecksum(instance.Spec)
-			if instanceChecksum == *instance.Status.Checksum {
-				glog.V(4).Infof(
-					"Not processing event for Instance %v/%v because checksum showed there is no work to do",
-					instance.Namespace,
-					instance.Name,
-				)
-				return nil
-			}
+	if instance.Status.Checksum != nil && instance.DeletionTimestamp == nil {
+		instanceChecksum := checksum.InstanceSpecChecksum(instance.Spec)
+		if instanceChecksum == *instance.Status.Checksum {
+			glog.V(4).Infof(
+				"Not processing event for Instance %v/%v because checksum showed there is no work to do",
+				instance.Namespace,
+				instance.Name,
+			)
+			return nil
 		}
 	}
 
 	glog.V(4).Infof("Processing Instance %v/%v", instance.Namespace, instance.Name)
 
-	// if the instance is marked for deletion, handle that first.
 	if instance.ObjectMeta.DeletionTimestamp != nil {
 		return c.reconcileInstanceDelete(instance)
 	}
 
+	glog.V(4).Infof("Adding/Updating Instance %v/%v", instance.Namespace, instance.Name)
+
 	serviceClass, servicePlan, brokerName, brokerClient, err := c.getServiceClassPlanAndBroker(instance)
 	if err != nil {
 		return err
 	}
 
-	if instance.Status.AsyncOpInProgress {
-		return c.pollInstance(serviceClass, servicePlan, brokerName, brokerClient, instance)
-	}
-
-	glog.V(4).Infof("Adding/Updating Instance %v/%v", instance.Namespace, instance.Name)
-
 	// we will definitely update the instance's status - make a deep copy now
 	// for use later in this method.
 	clone, err := api.Scheme.DeepCopy(instance)

pkg/controller/controller_instance_test.go

@@ -779,6 +779,79 @@ func TestReconcileInstanceDelete(t *testing.T) {
 	}
 }
 
+func TestReconcileInstanceDeleteAsynchronous(t *testing.T) {
+	key := osb.OperationKey(testOperation)
+	fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{
+		DeprovisionReaction: &fakeosb.DeprovisionReaction{
+			Response: &osb.DeprovisionResponse{
+				Async:        true,
+				OperationKey: &key,
+			},
+		},
+	})
+
+	sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker())
+	sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass())
+
+	instance := getTestInstance()
+	instance.ObjectMeta.DeletionTimestamp = &metav1.Time{}
+	instance.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog}
+	// we only invoke the broker client to deprovision if we have a checksum set
+	// as that implies a previous success.
+	checksum := checksum.InstanceSpecChecksum(instance.Spec)
+	instance.Status.Checksum = &checksum
+
+	fakeCatalogClient.AddReactor("get", "instances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+		return true, instance, nil
+	})
+
+	if testController.pollingQueue.Len() != 0 {
+		t.Fatalf("Expected the polling queue to be empty")
+	}
+
+	err := testController.reconcileInstance(instance)
+	if err != nil {
+		t.Fatalf("This should not fail")
+	}
+
+	// The item should've been added to the pollingQueue for later processing
+
+	// TODO: add a way to peek into rate-limited adds that are still pending,
+	// then uncomment.
+	// if testController.pollingQueue.Len() != 1 {
+	// 	t.Fatalf("Expected the asynchronous instance to end up in the polling queue")
+	// }
+
+	brokerActions := fakeBrokerClient.Actions()
+	assertNumberOfBrokerActions(t, brokerActions, 1)
+	assertDeprovision(t, brokerActions[0], &osb.DeprovisionRequest{
+		AcceptsIncomplete: true,
+		InstanceID:        instanceGUID,
+		ServiceID:         serviceClassGUID,
+		PlanID:            planGUID,
+	})
+
+	// Verify no core kube actions occurred
+	kubeActions := fakeKubeClient.Actions()
+	assertNumberOfActions(t, kubeActions, 0)
+
+	actions := fakeCatalogClient.Actions()
+	// The action should be:
+	// 0. Updating the ready condition
+	assertNumberOfActions(t, actions, 1)
+
+	updatedInstance := assertUpdateStatus(t, actions[0], instance)
+	assertInstanceReadyFalse(t, updatedInstance)
+
+	events := getRecordedEvents(testController)
+	assertNumEvents(t, events, 1)
+
+	expectedEvent := api.EventTypeNormal + " " + asyncDeprovisioningReason + " " + "The instance is being deprovisioned asynchronously"
+	if e, a := expectedEvent, events[0]; e != a {
+		t.Fatalf("Received unexpected event: %v", a)
+	}
+}
+
 // TestReconcileInstanceDeleteFailedInstance tests that a failed instance will
 // be finalized, but no deprovision request will be sent to the broker.
 func TestReconcileInstanceDeleteFailedInstance(t *testing.T) {