review

controllers/base_controller.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -344,7 +345,8 @@ func isTransientStatusCode(StatusCode int) bool {
 
 func (r *BaseReconciler) handleError(ctx context.Context, operationType smClientTypes.OperationCategory, err error, resource api.SAPBTPResource) (ctrl.Result, error) {
 	log := GetLogger(ctx)
-	smError, ok := err.(*sm.ServiceManagerError)
+	var smError *sm.ServiceManagerError
+	ok := errors.As(err, &smError)
 	if !ok {
 		log.Info("unable to cast error to SM error, will be treated as non transient")
 		return r.markAsNonTransientError(ctx, operationType, err.Error(), resource)

controllers/serviceinstance_controller.go

@@ -80,6 +80,21 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}
 
+	if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) &&
+		serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) {
+		operation := smClientTypes.CREATE
+		if len(serviceInstance.Status.InstanceID) > 0 {
+			operation = smClientTypes.UPDATE
+		}
+		setInProgressConditions(ctx, operation, "", serviceInstance)
+		conditions := serviceInstance.GetConditions()
+		if len(conditions) > 0 {
+			meta.RemoveStatusCondition(&conditions, api.ConditionFailed)
+		}
+		serviceInstance.SetConditions(conditions)
+		r.Status().Update(ctx, serviceInstance)
+	}
+
 	if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) {
 		if len(serviceInstance.Status.HashedSpec) == 0 {
 			updateHashedSpecValue(serviceInstance)
@@ -558,10 +573,7 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan
 		log.Info("instance is not in final state, sync operation is in progress")
 		return false
 	}
-	if isInRetry(serviceInstance, log, nonTransientTimeout) {
-		log.Info("instance is not in final state, IgnoreNonTransientErrorAnnotation exist")
-		return false
-	}
+
 	if sharingUpdateRequired(serviceInstance) {
 		log.Info("instance is not in final state, need to sync sharing status")
 		if len(serviceInstance.Status.HashedSpec) == 0 {
@@ -593,9 +605,7 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger
 	if cond != nil && cond.Reason == UpdateInProgress { //in case of transient error occurred
 		return true
 	}
-	if serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) {
-		return true
-	}
+
 	return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec
 }
 

controllers/serviceinstance_controller_test.go

@@ -251,18 +251,18 @@ var _ = Describe("ServiceInstance controller", func() {
 				Context("first with 400 status then success", func() {
 					errMessage := "failed to provision instance"
 					BeforeEach(func() {
-						fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{
+						fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{
 							StatusCode:  http.StatusBadRequest,
 							Description: errMessage,
 						})
-						fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil)
+						fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil)
 					})
 
 					It("should have failure condition", func() {
 						serviceInstance = createInstance(ctx, instanceSpec, false)
 						waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage)
 					})
-					It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() {
+					It("ignoreNonTransientErrorAnnotation should remove the annotation once succeeded", func() {
 						serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false)
 						waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "")
 						waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation)
@@ -280,6 +280,7 @@ var _ = Describe("ServiceInstance controller", func() {
 						serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false)
 						waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage)
 						waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation)
+						waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage)
 						sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time)
 						Expect(sinceCreate > ignoreNonTransientTimeout)
 					})

controllers/suite_test.go

@@ -67,7 +67,7 @@ const (
 	interval                  = time.Millisecond * 50
 	syncPeriod                = time.Millisecond * 250
 	pollInterval              = time.Millisecond * 250
-	ignoreNonTransientTimeout = time.Second * 1
+	ignoreNonTransientTimeout = time.Second * 10
 
 	fakeBindingID        = "fake-binding-id"
 	bindingTestNamespace = "test-namespace"