Async Binding Design

[kibbles-n-bytes]

Async Binding Implementation Design

In anticipation of async bindings making it into the OSB API spec, I'd like to get a jump on an initial implementation and wanted to toss some proposals out there and see what sticks. The proposed last operation flow for async binding/unbinding (viewable here) is exactly the same as that of async provisioning/deprovisioning, except the endpoint being hit is /v2/service_instances/:instance_id/service_bindings/:binding_id/last_operation, as opposed to /v2/service_instances/:instance_id/last_operation.

In order to prevent endless bikeshedding, rebasing, and churn over a PR that ends up sitting unmerged for weeks, I thought it'd be a good idea to have a discussion as to what the most appropriate implementation of async bindings within our repo is, now that polling is no longer an instance-only operation. This way, hopefully, when I implement it, it can go in smoother.

Here are a few possible implementations I came up with for architecting our last operation polling logic.

Proposals

1) Duplicate the entire last operation flow

We create a method called pollServiceInstanceCredential(credential *v1alpha1.ServiceInstanceCredential) that lives in pkg/controller/controller_binding.go. This new method would have an identical logical flow as pollServiceInstance(instance *v1alpha1.ServiceInstance) from pkg/controller/controller_instance.go, except its updates modify the status of an instance credential instead of an instance.

This would be the easiest, but adds a lot of duplicate code and polling architectural changes will have to occur in two spots.

2) Add if/else branching to polling logic

We move pollServiceInstance(instance *v1alpha1.ServiceInstance) to a new file pkg/controller/polling.go, and refactor it to instead take a generic object: pollLastOperation(obj interface{}). This method would then determine whether the resource is an instance or an instance credential, and at every point it needs to update the status, it branches depending on the resource type.

We already do quite a bit of branching depending on whether the current operation is a creation vs. a deletion, so I am wary of introducing even more branching. We would ideally refactor each branching section to instead call a helper function. For example, if we receive a "succeeded" state from the broker, the code would look like the following:

func pollLastOperation(obj interface{}) error {
	...
	case osb.StateSucceeded:
		if deleting {
			return pollingResourceDeletedSuccess(obj)
		}
	
		return pollingResourceAddedSuccess(obj)
	...
}

func pollingResourceDeletedSuccess(obj interface{}) error {
	if instance, ok := obj.(*v1alpha1.ServiceInstance); ok {
		// Instance deprovision status setting
		...
		return nil
	}

	if instanceCredential, ok := obj.(*v1alpha1.ServiceInstanceCredential); ok {
		// Instance credential deletion status setting
		...
		return nil
	}

	return fmt.Errorf("Couldn't determine resource type")
}

func pollingResourceAddedSuccess(obj interface{}) {
	...

With this approach, we do not duplicate the overall flow of the last operation poll, but suffer from a large amount of branching calls. We would also have to implement a non-trivial amount of functions to handle the branching.

3) Special PollingResource struct

Similar to the if/else branching proposal, except with a dedicated resource to avoid the explicit branching. We create a resource similar to the following:

type PollingResource struct {
	resource 	interface{}
	AddedSuccess	func(obj interface{}) error
	DeletedSuccess	func(obj interface{}) error
	...
}

func getPollingResourceForServiceInstance(instance *v1alpha1.ServiceInstance) *PollingResource {
	return &PollingResource {
		resource: instance,
		AddedSuccess: pollingServiceInstanceAddedSuccess,
		DeletedSuccess: pollingServiceInstanceDeletedSuccess,
		...
	}
}

func pollingServiceInstanceAddedSuccess(obj interface{}) error {
	instance, ok := obj.(*v1alpha1.ServiceInstance)
	
	// Instance provision status setting
	...
}

func pollingServiceInstanceDeletedSuccess(obj interface{}) error {
	...
}

The main polling logic would then look like the following:

func pollLastOperation(p *PollingResource) error {
	...
	case osb.StateSucceeded:
		if deleting {
			return p.DeletedSuccess(p.resource)
		}
	
		return p.AddedSuccess(p.resource)
	...
}

With this approach we keep separation of the code twiddling instances vs. instance credentials, though the tradeoff is a bit more boilerplate code.

4) Pollable interface

Similar to the PollingResource struct, except the implementation is via an interface. We create an interface similar to the following:

type Pollable interface {
	AddedSuccess() error
	DeletedSuccess() error
	...
}

We then implement it with concrete versions for instances and instance credentials:

type PollableServiceInstance struct {
	instance *v1alpha1.ServiceInstance
} 

func (instance *PollableServiceInstance) AddedSuccess() error {
	// Instance provision status setting
}

func (instance *PollableServiceInstance) DeletedSuccess() error {
	...
}

The polling flow is then similar to the previous proposal:

func pollLastOperation(p *Pollable) error {
	...
	case osb.StateSucceeded:
		if deleting {
			return p.DeletedSuccess()
		}
	
		return p.AddedSuccess()
	...
}

This approach has a bit less boilerplate than a single PollingResource struct, and overall my favorite, though I would like opinions on the stylistically how it fits with our code and Go programming in general.

Conclusion

Personally, I feel that the Pollable interface is the cleanest approach. However, I wanted to hear what you all have to think, and whether there is a different ideal implementation I have not thought of.

[pmorie]

I vote for number 1 initially - nothing fancy. We can make things fancy and factored later after we feel like things are relatively stable. I would be fine with a better factoring once we feel like things are stable. Introducing a large factoring in the first step is going to be far more expensive than duplicating the logic.

[kibbles-n-bytes]

@pmorie and I talked offline, summarizing here:

There are quite a few stakeholders who are blocked on async bindings, so something quick and simple that'll get us functionally there ASAP is a priority. Duplicating the flow is the best with that in mind.

[nilebox]

@kibbles-n-bytes just to clarify from the OSB API point of view: currently credentials is part of the synchronous Binding response (see https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#body-7). In case of asynchronous binding it would be moved to the last_operation response (if the binding succeeds), right? Current last operation response (see https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#body-1) doesn't have a place for arbitrary data (suitable for credentials object), so we probably need to extend it.

I am thinking of supporting instance data in the same way in the future (see openservicebrokerapi/servicebroker#295)

[kibbles-n-bytes]

@nilebox At the moment, the async binding proposal does not include credentials in the last operation response. Instead, what the platform does is wait until it gets a "state": "succeeded" in the last operation response, and then immediately do a GET on the binding, which returns the credentials as part of its payload. The async binding proposal is built off of the proposal for stateful brokers.

[carolynvs]

Can this be closed now that #1512 is in? Or are there more issues out there that should be linked to the 1.0 milestone?

[kibbles-n-bytes]

@carolynvs I'll close it. We can always refactor behind-the-scenes if we don't like the duplicated flow.