search

kubernetes-retired / service-catalog

Consume services in Kubernetes using the Open Service Broker API
https://svc-cat.io
Apache License 2.0
1.05k stars 385 forks source link

Instances that fail provision are never deprovisioned #1461

Closed pmorie closed 6 years ago

pmorie commented 6 years ago

Currently, if an instance fails provision, when that instance is deleted, we skip the deprovision call. This is due to an assumption that we made during development that the broker is responsible for cleaning up after failed provisions. However, this assumption was wrong; in fact, the spec has a total gap in the area of what the broker should do when provision fails.

CF's behavior in this situation is to report to the user that the instance failed provision. The user still has to indicate that the instance should be deprovisioned in order for CF to send a deprovision request.

The bug here is that since we make the assumption that the broker has cleaned up and do not send a deprovision request that we will leak resources when this happens. I think we should match the behavior of CF in this regard and send a deprovision request even if an instance has failed. If the broker has already cleaned up after the failed provision request, it can just return 'GONE' which is counted as a success response to the deprovision call.

vaikas commented 6 years ago

If a resource fails to create, there should be nothing to delete? I think the spec says what to do in error cases, but if a resource fails to get created, there should be nothing to clean up because the resource doesn't exist. Would be good to get more clarity from the spec side, but I think this is the correct behaviour.

pmorie commented 6 years ago

That might be the case, but I think parity with CF wins the day here. If we do not change this behavior, it means that existing brokers don't work correctly with kube.

I'm all for clarifying the spec or correcting it if it's not correct, but this is a blocker for me for the moment.

On Mon, Oct 23, 2017 at 12:25 PM Ville Aikas notifications@github.com wrote:

If a resource fails to create, there should be nothing to delete? I think the spec says what to do in error cases, but if a resource fails to get created, there should be nothing to clean up because the resource doesn't exist. Would be good to get more clarity from the spec side, but I think this is the correct behaviour.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kubernetes-incubator/service-catalog/issues/1461#issuecomment-338715803, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWXmHr4CXQmsXrkUlYTRcEDj85APKdDks5svL3lgaJpZM4QC6rF .

kibbles-n-bytes commented 6 years ago

Per the OSB working group discussion today, we decided that platforms should send a deprovision request for every resource that has been attempted to be provisioned, failed or not.

staebler commented 6 years ago

I missed a bit of the latter end of the call. Did we decide that the deprovision should be sent when the provision fails? Or should we wait until the resource is deleted to send the deprovision?

pmorie commented 6 years ago

Did we decide that the deprovision should be sent when the provision fails? Or should we wait until the resource is deleted to send the deprovision?

@vaikas-google @kibbles-n-bytes @jpeeler where did we come down on this in the call?

vaikas commented 6 years ago

That's really up to us. At the call we agreed that the deprovision will be sent, it's up to us to decide when to do it.

staebler commented 6 years ago

OK. Unless I hear other objections, I will opt for the latter option, where the deprovision request is sent when the service-catalog resource is deleted.

pmorie commented 6 years ago

I will opt for the latter option, where the deprovision request is sent when the service-catalog resource is deleted.

I am +1 on doing that, go for it.

kibbles-n-bytes commented 6 years ago

Yeah, I am +1 on that option as well.