search

openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 434 forks source link

Resolve 409/422 conflict on concurrent updates #413

Closed duglin closed 6 years ago

duglin commented 6 years ago

Closes: #408

Moved the "Additionally..." sentence to the 422 section.

Signed-off-by: Doug Davis dug@us.ibm.com

cfdreddbot commented 6 years ago

Hey duglin!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

angarg12 commented 6 years ago

Would this be a breaking change since we add MUST to the error code?

duglin commented 6 years ago

We have an inconsistency in the spec right now (saying MUST use 422 in one spot and MUST use 409 in another spot for the same condition), so yes something will technically break but we can't avoid it.

mattmcneeney commented 6 years ago

This is going to conflict with #409... Let's get that one through first!

duglin commented 6 years ago

rebased

angarg12 commented 6 years ago

LGTM

Approved with PullApprove

duglin commented 6 years ago

3 needed

vaikas commented 6 years ago

LGTM

Approved with PullApprove

mattmcneeney commented 6 years ago

LGTM

Approved with PullApprove

avade commented 6 years ago

LGTM

Approved with PullApprove

zrob commented 6 years ago

I went back to the 2.7 spec and noticed the text actually indicated that it should be a 400. It seems we've previously introduced a breaking change to get into this ambiguous state of 422/409.

The Cloud Controller ensures that service brokers do not receive requests for an instance while an asynchronous operation is in progress. For example, if a broker is in the process of provisioning an instance asynchronously, the Cloud Controller will not allow any update, bind, unbind, or deprovision requests to be made through the platform. A user who attempts to perform one of these actions while an operation is already in progress will get an HTTP 400 with error message “Another operation for this service instance is in progress.”

Given the current state I'm in favor of moving forward to the now suggested 422, but it is concerning that our "refactor only" changes are resulting in breaking changes.

duglin commented 6 years ago

@zrob the change was made here: https://github.com/openservicebrokerapi/servicebroker/pull/193/files If I'm remembering correctly the change was made because the old text talked about two things: 1 - what the end user should see, not what the REST API from the Broker should return - which is technically out of scope for the spec 2 - how the platform needs to block concurrent requests to the Broker

The 2nd one is actually the critical piece here because it means that the spec was written with the assumption that concurrent requests would never make it to the broker, so the spec was basically silent on this point. Which means we didn't technically break anything other than to go from "undefined" to "well-defined semantics" in an error situation. I guess you could claim that the Broker was free to do anything before and now we're forcing a particular error and that's a breaking change, but I guess at the time we decided interop was more important.

I believe, at the same time, we decided that having Platform block concurrent requests was an unnecessary restriction since some brokers might be able to deal with it just fine, and it may not always be possible for a Platform to serialize all requests for the same instance if its a multi-threaded/process/component infrastructure.