Add text around what brokers should do upon failures #354

duglin · 2017-10-27T12:48:05Z

Provide more guidance for what should happen when things go wrong
so we can ensure a more consistent semantics and interop.

This should only be reviewed after #353 - so only look at the 2nd commit.

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

cfdreddbot · 2017-10-27T12:48:06Z

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.

duglin · 2017-11-07T14:31:49Z

On hold until we resolve #353

mattmcneeney · 2017-11-07T15:04:38Z

spec.md

@@ -718,6 +718,12 @@ For success responses, the following fields are valid.
 }
 ```

+If the successful response includes a `state` of `failed` then the Platform
+MUST send a deprovision request to the Service Broker to prevent an orphan
+being created on the Service Broker.  However, while the platform will attempt


I'd capitalise Platform (multiple occurences in this PR) now to save conflicts when #356 is merged

mattmcneeney · 2017-11-07T15:06:05Z

spec.md

-binding will remain in the marketplace database. Service brokers can include a
-user-facing message in the `description` field; for details see [Service
-Broker Errors](#service-broker-errors).
+binding MUST remain in the marketplace database.


Is this the first time we've used "marketplace database"? Can we rephrase this to something like:

Responses with any other status code will be interpreted as a failure and the Platform MUST continue to remember the Service Binding.

its not the only time we talk about DBs for the marketplace. Let me see if I can sneak in that change....

actually, let me do that in a new PR since its more than just a one word change and might require some thinking by the reviewers.

Thank you! :)

See openservicebrokerapi#354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

mattmcneeney · 2017-11-07T15:27:42Z

LGTM

angarg12 · 2017-11-07T21:21:54Z

@duglin this PR needs rebase now. I will give my LG after it :)

duglin · 2017-11-08T02:00:59Z

rebased

angarg12 · 2017-11-08T02:19:56Z

LGTM

duglin · 2017-11-14T15:05:16Z

I know there was some concern about this text:

However, while the Platform will attempt
to send a deprovision request, Service Brokers MAY automatically delete
any resources associated with the failed bind request on their own.

so, if those concerns are still there would using this text (someplace in the spec) help?

When the creation of a resource fails, or when a resource is deleted, the Service Broker
SHOULD eventually clean up all artifacts related to that resource. The exact timing of when
that occurs is an implementation detail. However, from the perspective of the OSB APIs
the clean up SHOULD be immediate.

IMO this leaves it open for the resource to live on for a while if the broker has a good reason
too - but per the OSB API it really needs to appear like it was deleted, otherwise the API is
just lying to the platform.

Thoughts?

mattmcneeney · 2017-11-14T15:10:07Z

Regarding this text:

However, while the Platform will attempt
to send a deprovision request, Service Brokers MAY automatically delete
any resources associated with the failed bind request on their own.

As a platform author, my interpretation of that is:

I should send a deprovision request when something fails
But in reality, that deprovision request may well fail as the broker may have already done some cleanup
I'm fairly confident that this means that the cleanup has completed, but if that deprovision request I make fails, I should probably check that it failed because the cleanup has already happened, not because something has gone wrong

duglin · 2017-11-14T15:27:36Z

I'm ok with that interpretation :-)
I would think most platforms would treat a 404/410 from a DELETE as a good thing and move on.

n3wscott · 2017-12-13T22:27:43Z

spec.md

@@ -962,7 +966,12 @@ $ curl http://username:password@service-broker-url/v2/service_instances/:instanc
 | 400 Bad Request | MUST be returned if the request is malformed or missing mandatory data. |
 | 422 Unprocessable entity | MUST be returned if the requested change is not supported or if the request cannot currently be fulfilled due to the state of the Service Instance (e.g. Service Instance utilization is over the quota of the requested plan). Service Brokers SHOULD include a user-facing message in the body; for details see [Service Broker Errors](#service-broker-errors). Additionally, a `422 Unprocessable Entity` can also be returned if the Service Broker only supports asynchronous update for the requested plan and the request did not include `?accepts_incomplete=true`; in this case the expected response body is: `{ "error": "AsyncRequired", "description": "This Service Plan requires client support for asynchronous service operations." }` (see [Service Broker Errors](#service-broker-errors). |

-Responses with any other status code will be interpreted as a failure. Service Brokers can include a user-facing message in the `description` field; for details see [Service Broker Errors](#service-broker-errors).
+Responses with any other status code will be interpreted as a failure and
+the Service Broker MUST NOT apply any of the requested changes to the


How does the service broker deal with a long running, multi step update that fails near the end? It had to modify things to keep track of state in the migration, but was unable to complete the update. Do we require that the broker rolls all the steps back?

Perhaps this MUST NOT should be a SHOULD NOT ?

My concern with SHOULD NOT is that it then becomes indeterminate and we can't know what state the broker left things. Which, sort of, means we can't use the instance upon failure and will be forced to delete it.

I have a concern about the case where the broker returns a 500, as the resource could have been put into an unknown state. The same goes for timeout scenarios.

For 4xx I agree that we should state MUST NOT.

per our last call, I made the MUST NOT linked to a 4xx status code. I'll open a new issue to discuss if we can say anything about 5xx's.

n3wscott · 2018-01-11T22:39:32Z

LGTM

Note this PR will conflict with #409.

angarg12 · 2018-01-16T13:32:38Z

Which one do you think will be easier to merge? This one last?

duglin · 2018-01-16T13:34:22Z

I'm ok with #409 going first since it has more edits

duglin · 2018-01-19T15:24:08Z

rebased

angarg12 · 2018-01-22T10:58:15Z

Travis is failing

tools/../spec.md - 726: should only have a single space after a period

@duglin the check is working as intended 😄

Provide more guidance for what should happen when things go wrong so we can ensure a more consistent semantics and interop. This should only be reviewed after openservicebrokerapi#353 Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2018-01-22T11:25:10Z

:-)
fixed

duglin · 2018-01-23T14:42:17Z

reviews needed

mattmcneeney · 2018-01-23T17:44:08Z

LGTM

avade · 2018-01-24T10:53:31Z

Happy with these changes.

LGTM

duglin · 2018-01-25T14:54:33Z

2 more needed. ping @fmui @angarg12

duglin · 2018-01-29T13:26:13Z

2 more needed

fmui · 2018-01-30T09:12:13Z

LGTM

vaikas · 2018-01-30T17:35:53Z

LGTM

duglin force-pushed the cleanOnFail branch from aa85f77 to 8115cae Compare October 31, 2017 17:20

mattmcneeney reviewed Nov 7, 2017

View reviewed changes

This was referenced Nov 7, 2017

Clarify status of an Instance when Updating fails #358

Closed

Clarify the expectations of a platform when a resource fails to get created #348

Closed

duglin force-pushed the cleanOnFail branch from 8115cae to 691f100 Compare November 7, 2017 15:16

duglin pushed a commit to duglin/servicebroker that referenced this pull request Nov 7, 2017

Remove reference to "marketplace database"

de6af71

See openservicebrokerapi#354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin mentioned this pull request Nov 7, 2017

Remove reference to "marketplace database" #362

Merged

angarg12 added the needs rebase/work label Nov 7, 2017

duglin force-pushed the cleanOnFail branch from 691f100 to 357cfe8 Compare November 8, 2017 01:59

duglin removed the needs rebase/work label Nov 8, 2017

duglin force-pushed the cleanOnFail branch 2 times, most recently from c9b8178 to 53193b6 Compare November 21, 2017 14:22

n3wscott reviewed Dec 13, 2017

View reviewed changes

duglin force-pushed the cleanOnFail branch from 53193b6 to 0ad40e8 Compare January 8, 2018 14:16

duglin mentioned this pull request Jan 8, 2018

What's the expectation of a 500's error code on update #405

Closed

duglin force-pushed the cleanOnFail branch from 0ad40e8 to bdfd428 Compare January 9, 2018 17:20

duglin force-pushed the cleanOnFail branch from bdfd428 to a882d60 Compare January 19, 2018 15:23

Add text around what brokers should do upon failures

af53c22

Provide more guidance for what should happen when things go wrong so we can ensure a more consistent semantics and interop. This should only be reviewed after openservicebrokerapi#353 Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the cleanOnFail branch from a882d60 to af53c22 Compare January 22, 2018 11:24

vaikas merged commit 076f4d3 into openservicebrokerapi:master Jan 30, 2018

duglin deleted the cleanOnFail branch March 2, 2018 03:26

gberche-orange mentioned this pull request Aug 23, 2018

Fix an inconsistency introduced in v2.14 #579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add text around what brokers should do upon failures #354

Add text around what brokers should do upon failures #354

duglin commented Oct 27, 2017

cfdreddbot commented Oct 27, 2017

duglin commented Nov 7, 2017

mattmcneeney Nov 7, 2017

duglin Nov 7, 2017

mattmcneeney Nov 7, 2017

duglin Nov 7, 2017

duglin Nov 7, 2017

duglin Nov 7, 2017

mattmcneeney Nov 7, 2017

mattmcneeney commented Nov 7, 2017 •

edited by chipchilders

Loading

angarg12 commented Nov 7, 2017

duglin commented Nov 8, 2017

angarg12 commented Nov 8, 2017 •

edited by chipchilders

Loading

duglin commented Nov 14, 2017

mattmcneeney commented Nov 14, 2017

duglin commented Nov 14, 2017

n3wscott Dec 13, 2017

duglin Dec 19, 2017

avade Dec 19, 2017

duglin Jan 8, 2018

n3wscott commented Jan 11, 2018 •

edited by avade

Loading

angarg12 commented Jan 16, 2018

duglin commented Jan 16, 2018

duglin commented Jan 19, 2018

angarg12 commented Jan 22, 2018

duglin commented Jan 22, 2018

duglin commented Jan 23, 2018

mattmcneeney commented Jan 23, 2018 •

edited by avade

Loading

avade commented Jan 24, 2018 •

edited

Loading

duglin commented Jan 25, 2018

duglin commented Jan 29, 2018

fmui commented Jan 30, 2018 •

edited by avade

Loading

vaikas commented Jan 30, 2018 •

edited by avade

Loading

Add text around what brokers should do upon failures #354

Add text around what brokers should do upon failures #354

Conversation

duglin commented Oct 27, 2017

cfdreddbot commented Oct 27, 2017

duglin commented Nov 7, 2017

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

mattmcneeney commented Nov 7, 2017 • edited by chipchilders Loading

angarg12 commented Nov 7, 2017

duglin commented Nov 8, 2017

angarg12 commented Nov 8, 2017 • edited by chipchilders Loading

duglin commented Nov 14, 2017

mattmcneeney commented Nov 14, 2017

duglin commented Nov 14, 2017

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

n3wscott commented Jan 11, 2018 • edited by avade Loading

angarg12 commented Jan 16, 2018

duglin commented Jan 16, 2018

duglin commented Jan 19, 2018

angarg12 commented Jan 22, 2018

duglin commented Jan 22, 2018

duglin commented Jan 23, 2018

mattmcneeney commented Jan 23, 2018 • edited by avade Loading

avade commented Jan 24, 2018 • edited Loading

duglin commented Jan 25, 2018

duglin commented Jan 29, 2018

fmui commented Jan 30, 2018 • edited by avade Loading

vaikas commented Jan 30, 2018 • edited by avade Loading

mattmcneeney commented Nov 7, 2017 •

edited by chipchilders

Loading

angarg12 commented Nov 8, 2017 •

edited by chipchilders

Loading

n3wscott commented Jan 11, 2018 •

edited by avade

Loading

mattmcneeney commented Jan 23, 2018 •

edited by avade

Loading

avade commented Jan 24, 2018 •

edited

Loading

fmui commented Jan 30, 2018 •

edited by avade

Loading

vaikas commented Jan 30, 2018 •

edited by avade

Loading