Part 2 of PR #579

[duglin]

I missed this part of the spec that had similar text to what we edited
as part of #579.

So this is part 2 of that PR.

It makes it clear that upon a failed instance.update() operation, we should not do orphan mitigation.

thanks @gberche-orange

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

[cfdreddbot]

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.

[gberche-orange]

thanks @duglin for this fix. Since the last operation for service instance endpoint is called for checking status of service provisionning, update and delete operations, I believe this PR then leaves the specifications unclear about the expected behavior when a last operation for service instance response returns a 200 with a failed response body.

May be the table at https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#orphans should be updated to describe the status=failed response, and the following sentence would need to describe the update operation as well.

To mitigate orphan Service Instances and Service Bindings, the Platform SHOULD attempt to delete resources it cannot be sure were successfully created, and SHOULD keep trying to delete them until the Service Broker responds with a success.

Maybe something like the following

To mitigate orphan Service Instances and Service Bindings, the Platform SHOULD attempt to delete resources it cannot be sure to have reached the expected provisioned or deleted state, and SHOULD keep requesting their deletion until the Service Broker responds with a success. The platform MUST not attempt to delete resources it cannot be sure to have reached an expected updated state, and rather report update failures to the user that initiated the update request (this user can then take best appropriate response based on detailed update failure message)

[mattmcneeney]

@duglin before merging this, check out @gberche-orange comment. I think it's important enough to add a clarification for.

[duglin]

@gberche-orange I agree we should add text so that async responses that fail follow the same orphan mitigation rules as sync failures - I'll work on that.

Why are you suggesting that a failed update should not be automatically deleted? The spec right now is basically silent on this point as of now, so I'm curious to know why you want us to change that.

[gberche-orange]

@duglin

Why are you suggesting that a failed update should not be automatically deleted? The spec right now is basically silent on this point as of now, so I'm curious to know why you want us to change that.

As discussed into #579 (comment) the specs need such improvement to properly address #508 . The action decided in just #508 (comment) was insufficient, and I believe a explicit sentence is necessary

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. from the spec since we believe this was done in error. This was added back when we only had async provision, and we missed that the text would then apply to async binding - that was a mistake.

[duglin]

@gberche-orange I guess what you're suggesting is that we simply codify into the spec what existing platforms are probably doing today (meaning, "nothing" and leave it up to the end-user to decide). I'd like to think more about this... something about this feels odd to me but I can't put into words why... yet.

[mattmcneeney]

Ping @openservicebrokerapi/osbapi-pmc

[duglin]

Adding do-no-merge since I think I have a todo on this one

[mattmcneeney]

ping @duglin do you think you'll be able to finish this one or should we find a new owner?

[duglin]

Will try over xmas

[mattmcneeney]

Just checking @duglin - is this covered by #598 or is this something else?

[duglin]

@mattmcneeney I think that #598 goes a long way to making a solution for this a lot easier, but it's missing a few things before we could close this one:
1 - the orphan mitigation row with 200 with "state": "failed" should state that it's just for provision
2 - we'd need a new row for "instance update (w/200 + failed)" and it would say that orphan mitigation should NOT be done

@gberche-orange do I have that right?

If so, @mattmcneeney would you want to add that to #598 (and close this one) or do you want this PR to just build on top of it?

[mattmcneeney]

Let's keep them separate in an attempt to make reviews smaller and get them in quicker!

[duglin]

@gberche-orange @mattmcneeney see if this looks ok now

[duglin]

Removed the "do not merge" label.
My change is just the 2nd commit - a one liner: 966d89d
and is based on Matt's change in #598

[duglin]

Sorry - added "do not merge" back on this one. While this has 4 github approvals, its dependent PR ( #598) does not yet and merging this will cause those commit to get added as well. So, once #598 is approved then we can merge this one.

[mattmcneeney]

#598 is now merged but this has conflicts. Are you able to resolve @duglin ?

[duglin]

on it...

[mattmcneeney]

Thank you @duglin ! I've also forgotten what this PR is actually for, so maybe you could update the description whilst you're at it :)

[duglin]

Updated changes and the description

[mattmcneeney]

For consistency, could the first column be changed to:

[Polling Last Operation for Service Instances](#polling-last-operation-for-service-instances) for [Updating](#updating-a-service-instance)

[duglin]

good catch - done!

[mattmcneeney]

Do we still need this PR?

[gberche-orange]

Do we still need this PR?

I believe so, it clarifies that failed update should never trigger orphan management, which is otherwise still unspecified despites improvements brought by #661 around failed updates