Fix last op endpoint unbind response (openshift#765)

pkg/broker/broker.go

@@ -64,7 +64,9 @@ var (
 	ErrorPlanUpdateNotPossible = errors.New("plan update not possible")
 	// ErrorNoUpdateRequested - Error for when no valid updates are requested
 	ErrorNoUpdateRequested = errors.New("no valid updates requested")
-	log                    = logutil.NewLog()
+	// ErrorUnbindingInProgress - Error when unbind is called that has an unbinding job in progress
+	ErrorUnbindingInProgress = errors.New("unbinding in progress")
+	log                      = logutil.NewLog()
 )
 
 const (
@@ -961,6 +963,16 @@ func (a AnsibleBroker) Unbind(
 		return nil, false, errors.New(errMsg)
 	}
 
+	jobInProgress, jobToken, err := a.isJobInProgress(&instance, apb.JobMethodUnbind)
+	if err != nil {
+		log.Errorf("An error occurred while trying to determine if a unbind job is already in progress for instance: %s", instance.ID)
+		return nil, false, err
+	}
+	if jobInProgress {
+		log.Infof("Unbind requested for instance %s, but job is already in progress", instance.ID)
+		return &UnbindResponse{Operation: jobToken}, false, ErrorUnbindingInProgress
+	}
+
 	params := make(apb.Parameters)
 	provExtCreds, err := a.dao.GetExtractedCredentials(instance.ID.String())
 	if err != nil && !client.IsKeyNotFound(err) {
@@ -1017,6 +1029,7 @@ func (a AnsibleBroker) Unbind(
 		}); err != nil {
 			log.Errorf("failed to set initial jobstate for %v, %v", token, err.Error())
 		}
+		return &UnbindResponse{Operation: token}, true, nil
 
 	} else if a.brokerConfig.LaunchApbOnBind {
 		// only launch apb if we are always launching the APB.
@@ -1033,29 +1046,12 @@ func (a AnsibleBroker) Unbind(
 		log.Warning("Broker configured to *NOT* launch and run APB unbind")
 	}
 
-	//TODO should these not be handled in the subscriber if the subscriber is handling state
-	if bindExtCreds != nil {
-		err = a.dao.DeleteExtractedCredentials(bindInstance.ID.String())
-		if err != nil {
-			return nil, false, err
-		}
-	}
-
-	err = a.dao.DeleteBindInstance(bindInstance.ID.String())
-	if err != nil {
-		return nil, false, err
-	}
+	err = cleanupUnbind(&bindInstance, &serviceInstance, bindExtCreds, a.dao)
 
-	serviceInstance.RemoveBinding(bindInstance.ID)
-	err = a.dao.SetServiceInstance(instance.ID.String(), &serviceInstance)
 	if err != nil {
 		return nil, false, err
 	}
 
-	if token != "" {
-		return &UnbindResponse{Operation: token}, true, nil
-	}
-
 	return &UnbindResponse{}, false, nil
 }
 

pkg/broker/unbinding_subscriber.go

@@ -21,6 +21,7 @@
 package broker
 
 import (
+	"github.com/openshift/ansible-service-broker/pkg/apb"
 	"github.com/openshift/ansible-service-broker/pkg/dao"
 )
 
@@ -42,8 +43,67 @@ func (b *UnbindingWorkSubscriber) Subscribe(msgBuffer <-chan JobMsg) {
 		for msg := range msgBuffer {
 			log.Debug("Processed binding message from buffer")
 			if _, err := b.dao.SetState(msg.InstanceUUID, msg.State); err != nil {
-				log.Errorf("failed to set state after deprovision %v", err)
+				log.Errorf("failed to set state after unbind %v", err)
+				continue
+			}
+
+			if msg.State.State == apb.StateSucceeded {
+				svcInstance, err := b.dao.GetServiceInstance(msg.InstanceUUID)
+				if err != nil {
+					log.Errorf("Error getting service instance [ %s ] after unbind job",
+						msg.InstanceUUID)
+					setFailedUnbindJob(b.dao, msg)
+					continue
+				}
+
+				bindInstance, err := b.dao.GetBindInstance(msg.BindingUUID)
+				if err != nil {
+					log.Errorf("Error getting bind instance [ %s ] after unbind job",
+						msg.BindingUUID)
+					setFailedUnbindJob(b.dao, msg)
+					continue
+				}
+
+				if err := cleanupUnbind(bindInstance, svcInstance, &msg.ExtractedCredentials, b.dao); err != nil {
+					log.Errorf("Failed cleaning up unbind after job, error: %v", err)
+					setFailedUnbindJob(b.dao, msg)
+					continue
+				}
 			}
 		}
 	}()
 }
+
+func setFailedUnbindJob(dao dao.Dao, dmsg JobMsg) {
+	// have to set the state here manually as the logic that triggers this is in the subscriber
+	dmsg.State.State = apb.StateFailed
+	if _, err := dao.SetState(dmsg.InstanceUUID, dmsg.State); err != nil {
+		log.Errorf("failed to set state after unbind %v", err)
+	}
+}
+
+func cleanupUnbind(bindInstance *apb.BindInstance, serviceInstance *apb.ServiceInstance, bindExtCreds *apb.ExtractedCredentials, dao dao.Dao) error {
+	var err error
+	id := bindInstance.ID.String()
+
+	if bindExtCreds != nil {
+		if err = dao.DeleteExtractedCredentials(id); err != nil {
+			log.Errorf("failed to delete extracted credentials - %v", err)
+			return err
+		}
+	}
+
+	if err = dao.DeleteBindInstance(id); err != nil {
+		log.Errorf("failed to delete bind instance - %v", err)
+		return err
+	}
+
+	serviceInstance.RemoveBinding(bindInstance.ID)
+	if err = dao.SetServiceInstance(serviceInstance.ID.String(), serviceInstance); err != nil {
+		log.Errorf("failed to set service instance [ %s ] during unbind of [ %s ]",
+			serviceInstance.ID.String(), id)
+		return err
+	}
+	log.Infof("Clean up of binding instance [ %s ] done. Unbinding successful", id)
+	return nil
+}

pkg/handler/handler.go

@@ -671,6 +671,8 @@ func (h handler) unbind(w http.ResponseWriter, r *http.Request, params map[strin
 	case err == broker.ErrorNotFound: // return 404
 		log.Debugf("Binding not found.")
 		writeResponse(w, http.StatusNotFound, broker.ErrorResponse{Description: err.Error()})
+	case err == broker.ErrorUnbindingInProgress:
+		writeResponse(w, http.StatusAccepted, broker.ErrorResponse{Description: err.Error()})
 	case err != nil: // return 500
 		log.Errorf("Unknown error: %v", err)
 		writeResponse(w, http.StatusInternalServerError, broker.ErrorResponse{Description: err.Error()})