Proposes solutions for tracking state of BindInstance creation

[mhrivnak]

Please provide feedback plus any other potential solutions you think are worth consideration.

Of the three I described, my least-favorite is the "JobState.Resource" option.

[shawn-hurley]

I would prefer to go with option 2. I think that it is the most efficient and clear way of implementing this.

[maleck13]

For options 2 would you need to modify SetState to return the state key which would then be appended to the BindingInstance. If so this could work in the current pattern as the BindingID is sent through to the subscriber. So after the state has been set, the BindingInstance could have a statekey set and persisted.

As an alternative, what if we were to merge the Job State into the BindingInstance (and other Instances) BindingingInstance.State{...} and remove the need for the separate JobState all together. The subscribers would then be set a copy of the instance and persist that as the Job progresses. (probably needs a little more thinking about but would remove the need for creating a relationship)

[mhrivnak]

I haven't tested it, but looking further, I think this affects unbind also. If two unbind requests come in that are identical, we need to return the same "operation" token with both 202 responses. Not to mention, we don't want to start a second APB. We need a way to know that there is already an unbind job for a given BindInstance.

So for option 2, which seems popular so far (I also like it), we would need two new fields on the BindInstance. It could look something like this:

// BindInstance - Binding Instance describes a completed binding
type BindInstance struct {
    ID           uuid.UUID   `json:"id"`
    ServiceID    uuid.UUID   `json:"service_id"`
    Parameters   *Parameters `json:"parameters"`
    CreateJobKey string
    DeleteJobKey string
}

[eriknelson]

Option 2 could work, as @maleck13 pointed out it would require some moving around of signatures. It sounds even better to potentially attach job states directly to the instances. One obvious problem with this is that it breaks backwards compatibility with existing deployments. We haven't had any real conversations about whether or not this is acceptable. If we're talking about removing etcd for 3.10, obviously a 3.10 broker is not going to work with the existing broker state (in etcd) of previous brokers. I would expect use to need a migration plan from etcd to CRDs.

I like the idea of attaching the state to the instance because you don't have to worry about atomically creating the job and setting the token on the instance.

[mhrivnak]

Here is a WIP of what option 2 would look like. Sometimes seeing it can help clarify things, so let me know if this makes anyone like any particular option more or less. Note that this branch also includes fixes to the comparison of BindInstance "Parameters", and fixes to a couple of other edge cases that had to be fixed in order to test this.

https://github.com/openshift/ansible-service-broker/compare/release-1.1...mhrivnak:add-createjob-to-bindinstance?expand=1

[rthallisey]

Option 2 looks good to me. @mhrivnak is there anymore detail you want add? Or is there more discussion folks want to have around this proposal? If not I think it's ok to merge.

[eriknelson]

@rthallisey Agreed, it sounds like we're mostly in agreement around option 2. I'll merge and if we would like to update with some details let's add in a follow-up PR.