Allow delete while "create in progress" (#319)

Co-authored-by: I501080 <[email protected]>

controllers/base_controller.go

@@ -27,10 +27,12 @@ const (
 	k8sNameLabel   = "_k8sname"
 	clusterIDLabel = "_clusterid"
 
-	Created  = "Created"
-	Updated  = "Updated"
-	Deleted  = "Deleted"
-	Finished = "Finished"
+	Created        = "Created"
+	Updated        = "Updated"
+	Deleted        = "Deleted"
+	Finished       = "Finished"
+	Provisioned    = "Provisioned"
+	NotProvisioned = "NotProvisioned"
 
 	CreateInProgress = "CreateInProgress"
 	UpdateInProgress = "UpdateInProgress"
@@ -386,10 +388,10 @@ func isFailed(resource api.SAPBTPResource) bool {
 
 func getReadyCondition(object api.SAPBTPResource) metav1.Condition {
 	status := metav1.ConditionFalse
-	reason := "NotProvisioned"
+	reason := NotProvisioned
 	if object.GetReady() == metav1.ConditionTrue {
 		status = metav1.ConditionTrue
-		reason = "Provisioned"
+		reason = Provisioned
 	}
 
 	return metav1.Condition{Type: api.ConditionReady, Status: status, Reason: reason, ObservedGeneration: object.GetGeneration()}

controllers/serviceinstance_controller.go

@@ -21,7 +21,6 @@ import (
 	"crypto/md5"
 	"encoding/hex"
 	"encoding/json"
-
 	"fmt"
 
 	"net/http"
@@ -84,15 +83,15 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance)
 	}
 
+	if isDelete(serviceInstance.ObjectMeta) {
+		return r.deleteInstance(ctx, smClient, serviceInstance)
+	}
+
 	if len(serviceInstance.Status.OperationURL) > 0 {
 		// ongoing operation - poll status from SM
 		return r.poll(ctx, smClient, serviceInstance)
 	}
 
-	if isDelete(serviceInstance.ObjectMeta) {
-		return r.deleteInstance(ctx, smClient, serviceInstance)
-	}
-
 	if !controllerutil.ContainsFinalizer(serviceInstance, api.FinalizerName) {
 		controllerutil.AddFinalizer(serviceInstance, api.FinalizerName)
 		log.Info(fmt.Sprintf("added finalizer '%s' to service instance", api.FinalizerName))
@@ -396,6 +395,11 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient
 			return ctrl.Result{}, r.removeFinalizer(ctx, serviceInstance, api.FinalizerName)
 		}
 
+		if len(serviceInstance.Status.OperationURL) > 0 && serviceInstance.Status.OperationType == smClientTypes.DELETE {
+			// ongoing delete operation - poll status from SM
+			return r.poll(ctx, smClient, serviceInstance)
+		}
+
 		log.Info(fmt.Sprintf("Deleting instance with id %v from SM", serviceInstance.Status.InstanceID))
 		operationURL, deprovisionErr := smClient.Deprovision(serviceInstance.Status.InstanceID, nil, buildUserInfo(ctx, serviceInstance.Spec.UserInfo))
 		if deprovisionErr != nil {
@@ -405,35 +409,40 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient
 
 		if operationURL != "" {
 			log.Info("Deleting instance async")
-			serviceInstance.Status.OperationURL = operationURL
-			serviceInstance.Status.OperationType = smClientTypes.DELETE
-			setInProgressConditions(smClientTypes.DELETE, "", serviceInstance)
-
-			if err := r.updateStatus(ctx, serviceInstance); err != nil {
-				return ctrl.Result{}, err
-			}
+			return r.handleAsyncDelete(ctx, serviceInstance, operationURL)
+		}
 
-			return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
+		// remove our finalizer from the list and update it.
+		if err := r.removeFinalizer(ctx, serviceInstance, api.FinalizerName); err != nil {
+			return ctrl.Result{}, err
 		}
+
 		log.Info("Instance was deleted successfully")
 		serviceInstance.Status.InstanceID = ""
 		setSuccessConditions(smClientTypes.DELETE, serviceInstance)
 		if err := r.updateStatus(ctx, serviceInstance); err != nil {
 			return ctrl.Result{}, err
 		}
 
-		// remove our finalizer from the list and update it.
-		if err := r.removeFinalizer(ctx, serviceInstance, api.FinalizerName); err != nil {
-			return ctrl.Result{}, err
-		}
-
 		// Stop reconciliation as the item is being deleted
 		return ctrl.Result{}, nil
 
 	}
 	return ctrl.Result{}, nil
 }
 
+func (r *ServiceInstanceReconciler) handleAsyncDelete(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, opURL string) (ctrl.Result, error) {
+	serviceInstance.Status.OperationURL = opURL
+	serviceInstance.Status.OperationType = smClientTypes.DELETE
+	setInProgressConditions(smClientTypes.DELETE, "", serviceInstance)
+
+	if err := r.updateStatus(ctx, serviceInstance); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
+}
+
 func (r *ServiceInstanceReconciler) resyncInstanceStatus(ctx context.Context, smClient sm.Client, k8sInstance *servicesv1.ServiceInstance, smInstance *smClientTypes.ServiceInstance) {
 	log := GetLogger(ctx)
 

controllers/serviceinstance_controller_test.go

@@ -304,7 +304,7 @@ var _ = Describe("ServiceInstance controller", func() {
 						serviceInstance = createInstance(ctx, instanceSpec, false)
 						expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errorMessage)
 						fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil)
-						waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey)
+						waitForInstanceToBeReady(ctx, defaultLookupKey)
 					})
 				})
 
@@ -324,6 +324,7 @@ var _ = Describe("ServiceInstance controller", func() {
 		})
 
 		Context("Async", func() {
+
 			BeforeEach(func() {
 				fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID, Location: "/v1/service_instances/fakeid/operations/1234"}, nil)
 				fakeClient.StatusReturns(&smclientTypes.Operation{
@@ -341,7 +342,7 @@ var _ = Describe("ServiceInstance controller", func() {
 						Type:  smClientTypes.CREATE,
 						State: smClientTypes.SUCCEEDED,
 					}, nil)
-					waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey)
+					waitForInstanceToBeReady(ctx, defaultLookupKey)
 				})
 			})
 
@@ -391,21 +392,33 @@ var _ = Describe("ServiceInstance controller", func() {
 				})
 			})
 
-			When("deleting during create", func() {
-				It("should be deleted", func() {
+			When("deleting while create is in progress", func() {
+				It("should be deleted successfully", func() {
 					serviceInstance = createInstance(ctx, instanceSpec, false)
-					newName := "new-name" + uuid.New().String()
-					serviceInstance.Spec.ExternalName = newName
+
+					By("waiting for instance to be CreateInProgress")
+					waitForInstanceConditionAndReason(ctx, defaultLookupKey, api.ConditionSucceeded, CreateInProgress)
+
+					fakeClient.DeprovisionReturns("/v1/service_instances/id/operations/1234", nil)
+					fakeClient.StatusReturns(&smclientTypes.Operation{
+						ID:    "1234",
+						Type:  smClientTypes.DELETE,
+						State: smClientTypes.INPROGRESS,
+					}, nil)
+
+					By("deleting instance")
 					deleteInstance(ctx, serviceInstance, false)
 
-					// stop polling state
+					By("waiting for instance to be DeleteInProgress")
+					waitForInstanceConditionAndReason(ctx, defaultLookupKey, api.ConditionSucceeded, DeleteInProgress)
+
 					fakeClient.StatusReturns(&smclientTypes.Operation{
 						ID:    "1234",
-						Type:  smClientTypes.CREATE,
+						Type:  smClientTypes.DELETE,
 						State: smClientTypes.SUCCEEDED,
 					}, nil)
 
-					// validate deletion
+					By("verify instance was deleted successfully")
 					Eventually(func() bool {
 						err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance)
 						return apierrors.IsNotFound(err)
@@ -558,7 +571,7 @@ var _ = Describe("ServiceInstance controller", func() {
 					Expect(updatedInstance.Status.Conditions[0].Message).To(ContainSubstring(errMessage))
 					fakeClient.UpdateInstanceReturns(nil, "", nil)
 					updatedInstance = updateInstance(ctx, serviceInstance)
-					waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey)
+					waitForInstanceToBeReady(ctx, defaultLookupKey)
 				})
 			})
 
@@ -1229,13 +1242,21 @@ var _ = Describe("ServiceInstance controller", func() {
 	})
 })
 
-func waitForInstanceToBeReady(instance *v1.ServiceInstance, ctx context.Context, key types.NamespacedName) {
+func waitForInstanceConditionAndReason(ctx context.Context, key types.NamespacedName, conditionType, reason string) {
+	si := &v1.ServiceInstance{}
 	Eventually(func() bool {
-		_ = k8sClient.Get(ctx, key, instance)
-		return isReady(instance)
+		if err := k8sClient.Get(ctx, key, si); err != nil {
+			return false
+		}
+		cond := meta.FindStatusCondition(si.GetConditions(), conditionType)
+		return cond != nil && cond.Reason == reason
 	}, timeout, interval).Should(BeTrue())
 }
 
+func waitForInstanceToBeReady(ctx context.Context, key types.NamespacedName) {
+	waitForInstanceConditionAndReason(ctx, key, api.ConditionReady, Provisioned)
+}
+
 func getNonTransientBrokerError(errMessage string) error {
 	return &sm.ServiceManagerError{
 		StatusCode:  http.StatusBadRequest,