Add clarity around concurrent updates #300

duglin · 2017-08-29T13:46:43Z

From the f2f:
Proposal: In the “Blocking Operations” section change it to say “...that mutate the same resource…” instead of “...that act on the same set of resources...”

&& creating Binding is not necessary considered mutating an Instance, it’s a Broker’s choice

&& move “Blocking Operations” out from under Async

&& change 422 in createBinding response status code table to include this concurrency stuff

Signed-off-by: Doug Davis [email protected]

cfdreddbot · 2017-08-29T13:46:44Z

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 · 2017-08-29T18:03:31Z

LGTM

jmrodri · 2017-08-30T02:50:38Z

@duglin PR title has a TYPO: conurrent should be concurrent

duglin · 2017-08-30T11:07:44Z

@jmrodri fixed! thanks

avade · 2017-09-04T14:07:57Z

LGTM

mattmcneeney · 2017-09-05T06:50:42Z

LGTM

duglin · 2017-09-05T11:51:05Z

just one more review needed

jmrodri · 2017-09-05T12:31:42Z

Not sure I count but LGTM :)

duglin · 2017-09-10T19:44:35Z

Added text about orphan mitigation per the weekly meeting.

duglin · 2017-09-10T19:53:27Z

I changed the error code from 422 to "409 Conflict" - seemed more appropriate, but what do others think?

jmrodri · 2017-09-11T01:50:50Z

@duglin 409 Conflict does sound more appropriate. 👍

angarg12 · 2017-09-11T07:29:05Z

LGTM

duglin · 2017-09-12T16:06:53Z

reviews needed

vaikas · 2017-09-12T16:36:23Z

spec.md

@@ -782,7 +789,7 @@ $ curl http://username:password@broker-url/v2/service_instances/:instance_id/ser
 | 201 Created | MUST be returned if the binding was created as a result of this request. The expected response body is below. |
 | 400 Bad Request | MUST be returned if the request is malformed or missing mandatory data. |
 | 409 Conflict | MUST be returned if a service binding with the same id, for the same service instance, already exists but with different parameters. The expected response body is `{}`, though the description field MAY be used to return a user-facing error message, as described in [Broker Errors](#broker-errors). |
-| 422 Unprocessable Entity | MUST be returned if the broker requires that `app_guid` be included in the request body. The expected response body is: `{ "error": "RequiresApp", "description": "This service supports generation of credentials through binding an application only." }`. |
+| 422 Unprocessable Entity | MUST be returned if the broker requires that `app_guid` be included in the request body. The expected response body is: `{ "error": "RequiresApp", "description": "This service supports generation of credentials through binding an application only." }`. Additionally, if the broker rejects the request due to a concurrent request to create a binding for the same service instance, then this error MUST be returned (see [Blocking Operations](#blocking-operations)). |


Above, we return 409 conflict when there are concurrent operations ongoing, but here we do 422 which seems a bit inconsistent.

damn - I'll fix that later today

duglin · 2017-09-18T12:20:09Z

updated and ready for reviews

mattmcneeney · 2017-09-18T13:22:18Z

LGTM

angarg12 · 2017-09-18T13:37:27Z

LGTM

duglin · 2017-09-19T17:15:53Z

spec.md

+this error response SHOULD resend the request at a later time.
+
+Brokers MAY choose to treat the creation of a binding as a mutation of
+the corresponding service instance - it is an implementation choice.


Add text about how this might cause platforms to serialize binding create() requests.

duglin · 2017-09-25T12:09:01Z

ready for next round of reviews

mattmcneeney · 2017-09-26T08:54:12Z

LGTM

fmui · 2017-09-26T10:53:30Z

The proposal defines a fixed error body. A broker can neither change the description nor add additional data that might be useful to identify why there is a conflict. Can we make this a bit more flexible?

duglin · 2017-09-26T11:17:15Z

@fmui See: https://github.com/duglin/servicebroker/blob/6cbba3d423f3d7e5ebadd2e650f245ce531e1e2f/spec.md#broker-errors it says: "For error responses, the following fields are valid. Others will be ignored. ". While it would be nice if it was a bit clearer, I view that as saying the broker may include other fields (extensions) but they might be ignored by the platform. Does that cover it?

fmui · 2017-09-26T15:02:03Z

Not really. The proposal says:
"...then the broker MUST reject the request with an HTTP 409 Conflict and the following body: ..."

This is much more restrictive than defined in the Broker Error section. It doesn't even allow to return a different description text. I don't see a reason why this error should have stricter rules than other errors.

duglin · 2017-09-26T15:08:22Z

I don't think it was meant to be more restrictive. I think an generic "error PR" to make us more consistent would be good to answer these concerns. In particular:

which fields in the error json MUST be as exactly specified or can be tweaked (e.g description)
are additional fields (extensions) allowed and how are they added? (wrapper or top-level)

and we should touch all error text to ensure consistency.

vaikas · 2017-09-26T15:16:06Z

+1 to allowing for additional fields. We have already run into some issues because brokers are built on platforms that provide some fixed fields for all error responses.

duglin · 2017-09-26T15:50:10Z

@fmui can we clarify all of this in a follow-on PR? I'd prefer to keep this one more focused on just this one usecase.

zrob · 2017-09-26T17:35:08Z

I have a few concerns here.

changing the MUST from 422 to 409 is a backwards incompatible change
similar to fmui concern, this feels like a very restrictive error message, where brokers may want to include more or different details that could be helpful in resolving the issue
I don't understand the benefits of making this change. I can see that 409 may be somewhat more correct, but do we have platforms that will perform a different behavior based on this? As long as we are returning an error and a useful message I don't see a reason to be more prescriptive than we already are.

duglin · 2017-10-03T14:40:50Z

I reverted back to 422 and decided to add the "extensibility" text into this PR instead of creating a new one. See what people think.

mattmcneeney · 2017-10-10T13:42:44Z

@duglin merge conflict!

From the f2f: Proposal: In the “Blocking Operations” section change it to say “...that mutate the same resource…” instead of “...that act on the same set of resources...” && creating Binding is not necessary considered mutating an Instance, it’s a Broker’s choice && move “Blocking Operations” out from under Async && change 422 in createBinding response status code table to include this concurrency stuff Signed-off-by: Doug Davis <[email protected]>

Signed-off-by: Doug Davis <[email protected]>

duglin · 2017-10-10T14:54:18Z

rebased

mattmcneeney · 2017-10-10T14:57:17Z

This is going to give me merge conflict fun times in the async bindings PR. But LGTM anyway!

angarg12 · 2017-10-10T15:41:44Z

LGTM

fmui · 2017-10-10T16:37:31Z

LGTM

avade · 2017-10-16T15:08:17Z

lgtm

wryun mentioned this pull request Aug 30, 2017

Validity of retrying PUT instance requests (and concurrent request behaviour) #301

Closed

duglin changed the title ~~Add clarity around conurrent updates~~ Add clarity around concurrent updates Aug 30, 2017

duglin force-pushed the asyncClarity branch from 7774d8d to d925a0d Compare September 10, 2017 19:43

duglin force-pushed the asyncClarity branch from d925a0d to ba0e50f Compare September 10, 2017 19:51

vaikas reviewed Sep 12, 2017

View reviewed changes

duglin force-pushed the asyncClarity branch from ba0e50f to e5b6da2 Compare September 18, 2017 12:20

mattmcneeney mentioned this pull request Sep 18, 2017

Add text around what to do on a deprovision when there are bindings #314

Merged

duglin commented Sep 19, 2017

View reviewed changes

duglin force-pushed the asyncClarity branch from e5b6da2 to 6cbba3d Compare September 25, 2017 12:08

duglin force-pushed the asyncClarity branch from 6cbba3d to 9913f5d Compare October 3, 2017 14:33

Doug Davis added 2 commits October 10, 2017 07:40

Allow for errors to be extensible and descriptions to be customized.

3ec6a18

Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the asyncClarity branch from 9913f5d to 3ec6a18 Compare October 10, 2017 14:53

avade merged commit 0bbe1c6 into openservicebrokerapi:master Oct 16, 2017

duglin deleted the asyncClarity branch October 26, 2017 12:34

mattmcneeney mentioned this pull request Aug 8, 2018

Preventing other operations during an ongoing async op #184

Closed

teddyking mentioned this pull request Mar 13, 2020

Add support for OSBAPI 2.14 kubernetes-retired/go-open-service-broker-client#144

Closed

Add clarity around concurrent updates #300

Add clarity around concurrent updates #300

Conversation

duglin commented Aug 29, 2017

cfdreddbot commented Aug 29, 2017

angarg12 commented Aug 29, 2017 • edited by chipchilders Loading

jmrodri commented Aug 30, 2017

duglin commented Aug 30, 2017

avade commented Sep 4, 2017 • edited by chipchilders Loading

mattmcneeney commented Sep 5, 2017 • edited by chipchilders Loading

duglin commented Sep 5, 2017

jmrodri commented Sep 5, 2017

duglin commented Sep 10, 2017

duglin commented Sep 10, 2017

jmrodri commented Sep 11, 2017

angarg12 commented Sep 11, 2017 • edited by chipchilders Loading

duglin commented Sep 12, 2017

vaikas Sep 12, 2017

Choose a reason for hiding this comment

duglin Sep 12, 2017

Choose a reason for hiding this comment

duglin Sep 18, 2017

Choose a reason for hiding this comment

duglin commented Sep 18, 2017

mattmcneeney commented Sep 18, 2017 • edited by chipchilders Loading

angarg12 commented Sep 18, 2017 • edited by chipchilders Loading

duglin Sep 19, 2017

Choose a reason for hiding this comment

duglin Sep 25, 2017

Choose a reason for hiding this comment

duglin commented Sep 25, 2017

mattmcneeney commented Sep 26, 2017 • edited by chipchilders Loading

fmui commented Sep 26, 2017

duglin commented Sep 26, 2017

fmui commented Sep 26, 2017

duglin commented Sep 26, 2017

vaikas commented Sep 26, 2017

duglin commented Sep 26, 2017

zrob commented Sep 26, 2017

duglin commented Oct 3, 2017

mattmcneeney commented Oct 10, 2017

duglin commented Oct 10, 2017

mattmcneeney commented Oct 10, 2017 • edited by chipchilders Loading

angarg12 commented Oct 10, 2017 • edited by chipchilders Loading

fmui commented Oct 10, 2017 • edited by chipchilders Loading

avade commented Oct 16, 2017 • edited by chipchilders Loading

angarg12 commented Aug 29, 2017 •

edited by chipchilders

Loading

avade commented Sep 4, 2017 •

edited by chipchilders

Loading

mattmcneeney commented Sep 5, 2017 •

edited by chipchilders

Loading

angarg12 commented Sep 11, 2017 •

edited by chipchilders

Loading

mattmcneeney commented Sep 18, 2017 •

edited by chipchilders

Loading

angarg12 commented Sep 18, 2017 •

edited by chipchilders

Loading

mattmcneeney commented Sep 26, 2017 •

edited by chipchilders

Loading

mattmcneeney commented Oct 10, 2017 •

edited by chipchilders

Loading

angarg12 commented Oct 10, 2017 •

edited by chipchilders

Loading

fmui commented Oct 10, 2017 •

edited by chipchilders

Loading

avade commented Oct 16, 2017 •

edited by chipchilders

Loading