Skip to content

Clarify the state of an instance after a failed update or delete #570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

Clarify the state of an instance after a failed update or delete #570

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6d21bf
Merge pull request #1 from openservicebrokerapi/master
fmui Aug 6, 2018
0ce0e67
Added clarification of the state of an instance after a failed update…
fmui Aug 7, 2018
0c1308a
Clarified default of the instance_corrupt field.
fmui Aug 17, 2018
a8bc00f
Added section about corrupt instances
fmui Aug 31, 2018
550c6e5
Fixed CI failure
fmui Sep 18, 2018
549b855
Replaced instance_corrupt with status_code
fmui Oct 5, 2018
546cd23
Merge branch 'master' into instance-state
fmui Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,7 @@ For success responses, the following fields are defined:
| --- | --- | --- |
| state* | string | Valid values are `in progress`, `succeeded`, and `failed`. While `"state": "in progress"`, the Platform SHOULD continue polling. A response with `"state": "succeeded"` or `"state": "failed"` MUST cause the Platform to cease polling. |
| description | string | A user-facing message that can be used to tell the user details about the status of the operation. If present, MUST be a non-empty string. |
| instance_corrupt | boolean | For failed update and deprovisioning operations, this field indicates whether the instance is still usable or not. If the value is `true`, the Service Instance MUST be considered corrupt and the Platform SHOULD NOT allow the creation of new bindings. If the value is `false`, the Service Instance is in an unmodified and usable state. The default is true. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last sentence: When not specified, the default value is true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we head down this path, it seems like we'd need to support returning this value in the sync case too, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I would like to have the spec be precise about the state of the resource upon failure, for backwards compatibility can we? I'm wondering if the last sentence should be:
If not specified, the state of the resource is unspecified by this specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sync case is covered by HTTP status codes. 4xx means the instance is still usable, 5xx means the instance is corrupt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right with the last sentence. But from a practical point of view, it makes no difference. The platform must treat the instance as corrupt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I missed the correlation to the 400/500 response codes. Makes sense. thanks


\* Fields with an asterisk are REQUIRED.

Expand Down Expand Up @@ -1190,9 +1191,13 @@ $ 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). Additionally, a `422 Unprocessable Entity` MUST 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 response body MUST contain a error code `"AsyncRequired"` (see [Service Broker Errors](#service-broker-errors)). The error response MAY include a helpful error message in the `description` field such as `"This Service Plan requires client support for asynchronous service operations."`. |

Responses with any other status code MUST be interpreted as a failure.
When the response includes a 4xx status code, the Service Broker MUST NOT
apply any of the requested changes to the Service Instance.
apply any of the requested changes to the Service Instance and the
Service Instance MUST be in an unmodified and usable state.

Responses with any other status code MUST be interpreted as a failure.
The Service Instance MUST be considered corrupt and the Platform SHOULD NOT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a section on corrupt instances that explains:

  • what the meaning of 'corrupt' is
  • when you should set instance_corrupt as a broker author
  • whether corrupt instances can be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "Responses with any other status code..." is kind of worrisome. If it said "Any other 5xx error..." then I agree. But what about some 3xx status code? If people agree I can open a new PR to address this since this isn't really part of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmui did you want to address these comments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmui ^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, does "any other" mean non-4xx or does it mean "not mentioned above in the table or previous paragraph" ? I think we need to clarify this.

Copy link
Contributor

@Samze Samze Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that if a Service Broker has an intermittent failure, and returns 500s for a period of time, then instances that receive update calls during this time will be marked as corrupt, leaving them unusable.

I would prefer the broker be explicit about when an instance is corrupt rather than inferring it. It seems kinda risky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree with @Samze. I'm also starting to question how prevalent this problem is in the wild. Do service brokers ever leave instances as corrupt today? Do they allow this to happen or have they built preventative measures?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this call into question the entire orphan mitigation strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Samze I agree with you, but that would be an incompatible change. The PR just clarifies the current spec, it doesn't change it. The current spec says, a status code 500 must be interpreted as a failure. Following the spec, orphan mitigation must kick in. Depending on the platform, there is shorter or longer period of time between the failure and orphan mitigation. Within this timeframe, the platform must already today consider the service instance as unusable. Otherwise, why should it trigger the orphan mitigation process later? So, the PR just spells out that platform should treat this instance as broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattmcneeney There are brokers that update in multiple steps. If one step fails, they can’t move forwards or backwards and then they wait for orphan mitigation. Sure, they could immediately clean up, but from a platform perspective this is not any different.

allow the creation of new bindings.

#### Body

Expand Down Expand Up @@ -1672,8 +1677,13 @@ $ curl 'http://username:password@service-broker-url/v2/service_instances/:instan
| 410 Gone | MUST be returned if the Service Instance does not exist. |
| 422 Unprocessable Entity | MUST be returned if the Service Broker only supports asynchronous deprovisioning for the requested plan and the request did not include `?accepts_incomplete=true`. The response body MUST contain error code `"AsyncRequired"` (see [Service Broker Errors](#service-broker-errors)). The error response MAY include a helpful error message in the `description` field such as `"This Service Plan requires client support for asynchronous service operations."`. |

When the response includes a 4xx status code other than 410 Gone, the
Service Instance MUST be in an unmodified and usable state.

Responses with any other status code MUST be interpreted as a failure and the
Platform MUST remember the Service Instance.
Platform MUST remember the Service Instance. The Service Instance MUST be
considered corrupt and the Platform SHOULD NOT allow the creation of
new bindings.

#### Body

Expand Down