Async bind feature #625

jmrodri · 2018-01-10T06:14:32Z

The async bind feature is used to allow binds that need more time to execute than the 60 seconds response time allotted in the Open Service Broker API spec. Async bind will spawn a binding job and return the job token immediately. The catalog will use the last_operation to monitor the state of the running job until either successful completion or a failure.

There are no new dependencies for this feature.

There is a new binding_subscriber to monitor the JobMsgs sent from the new binding_job. The Bind method in the broker was updated to take in the async flag and determine if it should spawn an APB for bind or simply return what was already stored like it did before.

Another set of critical additions was the new getserviceinstance and getbind calls added to the handler. These are to implement the required getters for async bind, particularly the getbind. Prior to async bind, when bind was called we would return the credentials at the end of the request. Since it is now asynchronous, we return the job token as the operation, but we will need a way to get the bind that was created once the job is done.

Aysnc bind is gated by the LaunchApbOnBind broker configuration setting. This MUST be set to True for the async bind to even be an option. Also, accepts_incomplete=true must accompany all PUT requests on the bind endpoint for async to be used. Both of those conditions need to be satisfied before using the async feature.

jmrodri · 2018-01-10T06:16:22Z

I will remove or cleanup some of the debug logging statements. They're still needed as I continue testing.

coveralls · 2018-01-10T06:56:19Z

Changes Unknown when pulling 6db4ec3 on async-bind into ** on master**.

rthallisey · 2018-01-10T14:22:25Z

pkg/broker/binding_job.go

+// Run - run the binding job.
+func (p *BindingJob) Run(token string, msgBuffer chan<- JobMsg) {
+	metrics.BindingJobStarted()
+	var podName string


Probably don't need to declare extCreds, podName, and err ahead of time because if apb.Bind(p.serviceInstance, p.params) fails in anyway, these variables will still get set.

podName, extCreds, err := ...

totally, I copied it from the broker.go where there was a conditional hence the variables being defined before hand. Will fix.

rthallisey · 2018-01-10T14:23:50Z

pkg/broker/binding_job.go

+	var extCreds *apb.ExtractedCredentials
+	var err error
+
+	log.Debug("BJ: binding job started, calling apb.Bind")


Can you spell out Bind Job:

I will for the statements I plan to keep. Thanks.

rthallisey · 2018-01-10T15:25:45Z

pkg/broker/broker.go

@@ -769,8 +770,38 @@ func (a AnsibleBroker) isJobInProgress(instance *apb.ServiceInstance,
 	return len(methodJobs) > 0, token, nil
 }

+func (a AnsibleBroker) GetBind(instance apb.ServiceInstance, bindingUUID uuid.UUID) (*BindResponse, error) {
+
+	log.Debug("XXX entered GetBind")


Is 'XXX' suppose to be substituted for something? Or is this to log that "we're running GetBind"?

rthallisey · 2018-01-10T15:36:41Z

pkg/broker/broker.go

+
+	log.Debug("XXX entered GetBind")
+
+	// this might not be required


I like your honesty. Why might this not be required? Is this to verify that a provision has completed?

rthallisey · 2018-01-10T15:53:04Z

pkg/handler/handler.go

+	var async bool
+
+	// ignore the error, if async can't be parsed it will be false
+	async, _ = strconv.ParseBool(r.FormValue("accepts_incomplete"))


Should we turn on accepts_incomplete if LaunchApbOnBind is enabled and accepts_incomplete is not explicitly set to false? Also, should we have a warning if we have LaunchApbOnBind=true and accepts_incomplete=false ?

Yeah, I'll add a warning.

Actually, accepts_incomplete will be given to us from the service-catalog. We can not change its value once we get it since the caller needs to know how to monitor the last_operation endpoint.

I will add a warning when LaunchApbOnBind=true and accepts_incomplete=false condition occurs. Worst case it means that during synchronous binds we WILL launch the apb, which COULD timeout.

rthallisey · 2018-01-10T15:58:41Z

@jmrodri nice work. Mostly nits.

shawn-hurley

1 change that I think needs to happen

shawn-hurley · 2018-01-10T15:43:20Z

pkg/handler/handler.go

+	si, err := h.broker.GetServiceInstance(instanceUUID)
+	if err != nil {
+		// return 404 or 422
+		log.Debugf("service instance %v", si)


I think this needs to return from here with the status code of 404 or 422

coveralls · 2018-01-10T18:23:46Z

Changes Unknown when pulling 06af68d on async-bind into ** on master**.

coveralls · 2018-01-10T21:52:15Z

Changes Unknown when pulling 1a385b0 on async-bind into ** on master**.

* async bind update * reformat comments

* update MockBroker to match new broker interface

* Update binding job to use JobMsg

jmrodri · 2018-01-11T13:57:44Z

[jesusr@speed3 ansible-service-broker{async-bind}]$ INSTANCE_ID=`oc get serviceinstances --all-namespaces -o jsonpath='{.items[*].spec.externalID}{"\n"}'`; echo $INSTANCE_ID                                      
86db03b8-4664-4f29-89a9-2a6870aaef50                
[jesusr@speed3 ansible-service-broker{async-bind}]$ BIND_ID=`uuidgen`; echo $BIND_ID                     
b644b678-da77-41ec-9bd6-fa5780f019b6                
[jesusr@speed3 ansible-service-broker{async-bind}]$ ./bind.sh $INSTANCE_ID $BIND_ID                      
{                                                   
  "operation": "16bf0e98-38d5-4619-a986-51b1c6ca1e0a"                                                    
}                                                   
[jesusr@speed3 ansible-service-broker{async-bind}]$ ./last_operation.sh $INSTANCE_ID 16bf0e98-38d5-4619-a986-51b1c6ca1e0a                                                                                          
{                                                   
  "state": "in progress"                            
}                                                   
[jesusr@speed3 ansible-service-broker{async-bind}]$ ./last_operation.sh $INSTANCE_ID 16bf0e98-38d5-4619-a986-51b1c6ca1e0a                                                                                          
{                                                   
  "state": "succeeded"                              
}                                                   
[jesusr@speed3 ansible-service-broker{async-bind}]$ ./getbind.sh $INSTANCE_ID $BIND_ID                   
{                                                   
  "credentials": {                                  
    "DB_HOST": "postgresql",                        
    "DB_NAME": "ktzdkptahabencn",                   
    "DB_PASSWORD": "RHs1ftbUI7NYGrhne8EZ",          
    "DB_PORT": "5432",                              
    "DB_TYPE": "postgres",                          
    "DB_USER": "ktzdkptahabencn"                    
  }                                                 
}                                                   
[jesusr@speed3 ansible-service-broker{async-bind}]$ ./getinstance.sh $INSTANCE_ID                        
{                                                   
  "service_id": "86db03b8-4664-4f29-89a9-2a6870aaef50",                                                  
  "plan_id": "dev",                                 
  "parameters": {                                   
    "_apb_plan_id": "dev",                          
    "_apb_service_class_id": "1dda1477cace09730bd8ed7a6505607e",                                         
    "_apb_service_instance_id": "86db03b8-4664-4f29-89a9-2a6870aaef50",                                  
    "postgresql_database": "admin",                 
    "postgresql_password": "password",              
    "postgresql_user": "admin",                     
    "postgresql_version": "9.6"                     
  }                                                 
}

jmrodri · 2018-01-11T13:57:56Z

bind.sh used in testing

$ cat bind.sh
INSTANCE_ID=$1
BINDING_ID=$2
PLAN_UUID="7f4a5e35e4af2beb70076e72fab0b7ff"
SERVICE_UUID="dh-postgresql-apb-s964m"

req="{
  \"plan_id\": \"$PLAN_UUID\",
  \"service_id\": \"$SERVICE_UUID\",
  \"context\": \"blog-project\",
  \"app_guid\":\"\",
  \"bind_resource\":{},
  \"parameters\": {}
}"

curl \
    -k \
    -X PUT \
    -H "Authorization: bearer $(oc whoami -t)" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -H "X-Broker-API-Originating-Identity: " \
    -d "$req" \
    "https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID/service_bindings/$BINDING_ID?accepts_incomplete=true"

jmrodri · 2018-01-11T13:58:06Z

last_operation.sh used in testing

$ cat last_operation.sh 
INSTANCE_ID=$1
OPERATION=$2
PLAN_UUID="7f4a5e35e4af2beb70076e72fab0b7ff"
SERVICE_UUID="dh-postgresql-apb-s964m"

curl \
    -k \
    -X GET \
    -H "Authorization: bearer $(oc whoami -t)" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -H "X-Broker-API-Originating-Identity: " \
    "https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID/last_operation?operation=$OPERATION&service_id=$SERVICE_UUID&plan_id=$PLAN_UUID"

jmrodri · 2018-01-11T13:58:15Z

getbind.sh used in testing

$ cat getbind.sh 
INSTANCE_ID=$1
BINDING_ID=$2
PLAN_UUID="7f4a5e35e4af2beb70076e72fab0b7ff"
SERVICE_UUID="dh-postgresql-apb-s964m"

req="{
  \"plan_id\": \"$PLAN_UUID\",
  \"service_id\": \"$SERVICE_UUID\",
  \"context\": \"blog-project\",
  \"app_guid\":\"\",
  \"bind_resource\":{},
  \"parameters\": {}
}"

curl \
    -k \
    -X GET \
    -H "Authorization: bearer $(oc whoami -t)" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -H "X-Broker-API-Originating-Identity: " \
    "https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID/service_bindings/$BINDING_ID"

jmrodri · 2018-01-11T13:58:26Z

getinstance.sh used in testing

$ cat getinstance.sh 
INSTANCE_ID=$1

curl \
    -k \
    -X GET \
    -H "Authorization: bearer $(oc whoami -t)" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -H "X-Broker-API-Originating-Identity: " \
    "https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID"

coveralls · 2018-01-11T14:57:16Z

Changes Unknown when pulling 0590138 on async-bind into ** on master**.

coveralls · 2018-01-11T15:07:28Z

Changes Unknown when pulling 0590138 on async-bind into ** on master**.

jmrodri · 2018-01-11T15:13:54Z

Test setup

I modified catasb my_vars to use my image for the broker:

dockerhub_org: jmrodri
asb_template_url: file:///home/jesusr/dev/src/github.com/openshift/ansible-service-broker/templates/deploy-ansible-service-broker.template.yaml
origin_image_tag: v3.9.0-alpha.0
broker_launch_apb_on_bind: true
broker_auto_escalate: true
broker_image_name: docker.io/jmrodri/origin-ansible-service-broker
broker_tag: "async"
local_oc_client: true

I built my image as follows:

make build-image ORG=jmrodri TAG=async
docker push docker.io/jmrodri/origin-ansible-service-broker:async

You should probably use your OWN organization or just use the image I already pushed up.

jmrodri · 2018-01-11T15:43:04Z

TEST

bring up broker with async code in it (see above)
ensure you have an apb with bind in it: I'm using docker.io/mhrivnak/postgresql-apb. I actually pushed a copy to my own org and configured catasb and broker to use it.

Deploy PostgreSQL APB
Here we will create the service instance we need to create the binding.

Launch openshift web console
create a project i.e. blog-project
click the PostgreSQL (APB)
select Development plan, click Next
select the blog-project
enter password for database, click Next
DO NOT CREATE A BINDING!
Click Create
Click Close
10 Monitor the deployment to ensure the database has come up.

Create async bind
Here we will create an async bind using the CLI to talk directly to the broker's API. The commands will use scripts that were defined in previous comments.

Get instance uuid from deployed PostgreSQL service

INSTANCE_ID=`oc get serviceinstances --all-namespaces -o jsonpath='{.items[*].spec.externalID}{"\n"}'`; echo $INSTANCE_ID

Generate a new binding uuid

BIND_ID=`uuidgen`; echo $BIND_ID

Kick off an async bind, NOTE: the job id will be returned by this call, it is required for the NEXT call

./bind.sh $INSTANCE_ID $BIND_ID

Check the binding job status. Depending on how long it took you to run this command you may see "in progress" or "succeeded". If you see "in progress" run the command again until you see "succeeded". If you see "failed" I will be hiding in a corner.

./last_operation.sh $INSTANCE_ID 16bf0e98-38d5-replacewithidfrompreviouscall-51b1c6ca1e0a

Call get bind to see the binding created by the async bind job:

./getbind.sh $INSTANCE_ID $BIND_ID

See the service instance as well

./getinstance.sh $INSTANCE_ID

jmrodri · 2018-01-11T15:43:42Z

Also consider testing the getters with different ids. Ones that don't exist, invalid ones to see how the broker responds. We are trying to avoid PANICs :)

shawn-hurley

nits but overall LGTM

shawn-hurley · 2018-01-11T15:12:44Z

pkg/broker/binding_subscriber.go

+					Token:   bmsg.JobToken,
+					State:   apb.StateFailed,
+					Podname: bmsg.PodName,
+					Method:  apb.JobMethodBind,


I think that we might need a follow-on PR to pass errors back when the last operation is called.

I don't think this needs to be solved in the PR, but want to make a note of it.

shawn-hurley · 2018-01-11T15:48:15Z

pkg/broker/unbinding_subscriber.go

+				})
+			} else {
+				json.Unmarshal([]byte(bmsg.Msg), &extCreds)
+				b.dao.SetState(bmsg.InstanceUUID, apb.JobState{


Might want to handle the errors here

shawn-hurley · 2018-01-11T15:48:54Z

pkg/broker/unbinding_subscriber.go

+					Podname: bmsg.PodName,
+					Method:  apb.JobMethodBind,
+				})
+				b.dao.SetExtractedCredentials(bmsg.BindingUUID, extCreds)


Especially want to handle the error if the setting of the extracted credentials fail

shawn-hurley · 2018-01-11T15:49:51Z

pkg/handler/handler.go

@@ -219,6 +222,42 @@ func (h handler) catalog(w http.ResponseWriter, r *http.Request, params map[stri
 	writeDefaultResponse(w, http.StatusOK, resp, err)
 }

+func (h handler) getinstance(w http.ResponseWriter, r *http.Request, params map[string]string) {


nit: getinstance -> getInstance

but it's ugly and isn't all lower like the others in handler :) I really wanted to use get_instance :)

shawn-hurley · 2018-01-11T15:53:27Z

pkg/handler/handler.go

+		switch err {
+		case broker.ErrorNotFound:
+			log.Debug("handler: bind not found")
+			writeResponse(w, http.StatusNotFound, broker.ErrorResponse{Description: err.Error()})


need to return after the writeResponse otherwise, you will get multiple writes to the response writer.

shawn-hurley · 2018-01-11T15:55:15Z

pkg/handler/handler.go

+			log.Debug("handler: something went bad during the getbind")
+			writeResponse(w, http.StatusBadRequest, broker.ErrorResponse{Description: err.Error()})
+		}
+	} else {


I think that this should not be in an else block

It needs to be in the else block otherwise we'll write TWO responses out. It's consistent with some of the other calls. Probably a better alternative would be to do a return after the writeResponse in the if block.

I'll add a return and remove the else, I like that better.

jmrodri · 2018-01-11T17:11:45Z

unbind.sh script

INSTANCE_ID=$1
BINDING_ID=$2
PLAN_UUID="7f4a5e35e4af2beb70076e72fab0b7ff"

curl \
    -k \
    -X DELETE \
    -H "Authorization: bearer $(oc whoami -t)" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -H "X-Broker-API-Originating-Identity: " \
     "https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID/service_bindings/$BINDING_ID?accepts_incomplete=true&plan_id=$PLAN_UUID"

coveralls · 2018-01-11T17:24:46Z

Changes Unknown when pulling b2429ef on async-bind into ** on master**.

coveralls · 2018-01-11T17:59:30Z

Changes Unknown when pulling efd42b3 on async-bind into ** on master**.

coveralls · 2018-01-11T20:01:02Z

Changes Unknown when pulling 7e1592f on async-bind into ** on master**.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2018

jmrodri requested review from eriknelson, shawn-hurley and rthallisey January 10, 2018 06:15

rthallisey suggested changes Jan 10, 2018

View reviewed changes

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 10, 2018

shawn-hurley requested changes Jan 10, 2018

View reviewed changes

jmrodri added 14 commits January 10, 2018 23:21

initial async binding work

f13ce7c

* async bind update * reformat comments

move LaunchApbOnBind around

d5f8d3a

handle merge with master

b1a0605

* update MockBroker to match new broker interface

fix subscriber

7b11653

switch to using the common JobMsg

6cc54a6

* Update binding job to use JobMsg

fix up the async bind

8d3e98b

start the binding subscriber

14df8f1

debug logging

dabd294

implement getbind and getserviceinstance

b6c6e47

make MockBroker conform to the Broker interface

ac71a87

fix lint errors

1d0322f

add supporting code for async unbinding

1cbce7d

enable unbinding subscriber

fd664fc

enable get service instance

ed50c14

rthallisey approved these changes Jan 11, 2018

View reviewed changes

jmrodri added 2 commits January 11, 2018 09:12

change BJ to (un)bindjob

87eea55

remove error logs used for debug

0590138

shawn-hurley requested changes Jan 11, 2018

View reviewed changes

jmrodri added 4 commits January 11, 2018 11:41

add async unbind

b2429ef

handle errors in the subscribers

4af0085

move writeDefaultResponse out of the else block

0e254e8

remove debug statements from getbind

efd42b3

shawn-hurley approved these changes Jan 11, 2018

View reviewed changes

jmrodri added 3 commits January 11, 2018 13:42

allow unbind job to start

26eb7bb

allow bind cleanup

368feb2

fix unbind job. binds now deleted

7e1592f

jmrodri removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2018

jmrodri removed the request for review from jianzhangbjz January 11, 2018 19:26

jmrodri merged commit eb4db6e into master Jan 11, 2018

jmrodri deleted the async-bind branch January 11, 2018 20:05


		log.Debug("XXX entered GetBind")

		// this might not be required

Async bind feature #625

Async bind feature #625

Conversation

jmrodri commented Jan 10, 2018

jmrodri commented Jan 10, 2018

coveralls commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Jan 10, 2018

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

jmrodri commented Jan 11, 2018

jmrodri commented Jan 11, 2018

jmrodri commented Jan 11, 2018

jmrodri commented Jan 11, 2018

jmrodri commented Jan 11, 2018

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018

jmrodri commented Jan 11, 2018

Test setup

jmrodri commented Jan 11, 2018

TEST

jmrodri commented Jan 11, 2018

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri commented Jan 11, 2018 • edited

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018 • edited

coveralls commented Jan 11, 2018

jmrodri commented Jan 11, 2018 •

edited

coveralls commented Jan 11, 2018 •

edited