From 369242c5b78fe49299e69959103e7b3e0d71507a Mon Sep 17 00:00:00 2001 From: TalShorSap <108805932+TalShorSap@users.noreply.github.com> Date: Thu, 24 Aug 2023 10:17:29 +0300 Subject: [PATCH] Allow delete while "create in progress" (#319) Co-authored-by: I501080 --- controllers/base_controller.go | 14 +++--- controllers/serviceinstance_controller.go | 45 +++++++++++------- .../serviceinstance_controller_test.go | 47 ++++++++++++++----- 3 files changed, 69 insertions(+), 37 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 954b7efd..734f5874 100644 --- a/controllers/base_controller.go +++ b/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()} diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index cbba3536..eabd57f7 100644 --- a/controllers/serviceinstance_controller.go +++ b/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,16 +409,14 @@ 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) @@ -422,11 +424,6 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient 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 @@ -434,6 +431,18 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient 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) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index aa1b489b..27d504c4 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/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,