external-provisioner should NOT issue delete calls between retries.

[saad-ali]

The CSI external-provisioner relies on the external-provisioner library https://github.com/kubernetes-incubator/external-storage/blob/master/lib/controller/controller.go

In that library the provisioning code will attempt to provision a volume, if provisioning fails it will try to delete the volume to cleanup (see https://github.com/kubernetes-incubator/external-storage/blob/master/lib/controller/controller.go#L935).

If the failure was due to the volume provisioning taking too long, this just makes the problem: Volume provisioning is triggered, it takes too long so the call times out, the volume is deleted, we still want the volume so provisioning is triggered again but timesout before it is complete, and then is deleted, so every retry it will have to start over.

Instead if we fail due to error, we should not delete the volume. We should only delete once the request for provisioning is gone (i.e. PVC is deleted).

CC @sbezverk thoughts?

Reported by @cduchesne

[sbezverk]

@saad-ali I might be mistaken but the code gets to Line 935 only if disk provisioning succeeded. If provisioning fails then provisionClaimOperation will exit on line: https://github.com/kubernetes-incubator/external-storage/blob/master/lib/controller/controller.go#L897
It looks like line 935 will be executed when PV object fails to be created after successful provision of disk. In this condition, deleting disk imho makes as there is no corresponding PV to use it.
I might be wrong but happy to discuss.

[saad-ali]

Right so the volume is created but the kube API server is rejecting requests to create PV object (e.g. due to throttling). If we delete the volume, we still come back and try again because PVC object still exists and force the storage backend to create another volume, and then hit kube API throttling and then force the storage backend to delete the volume, etc. It causes unnecessary work on the storage backend. Maybe the correct answer is we try harder/longer to create PV object? And/or we fix #68 so that it is much harder for API to throttle.

I think there is also another bug in that code (https://github.com/kubernetes-incubator/external-storage/blob/master/lib/controller/controller.go#L958). After deleting the volume the provisionClaimOperation(...) method returns nil instead of the failed to create PV error so even though provisioning failed, binding will incorrectly succeed.

[sbezverk]

Right it seems if disk provisioning was successful, does not matter if PV creation fails, it returns nil. Probably last return should be return err?

[wongma7]

The code is the same in kubernetes internal so we should fix it in both places. https://github.com/kubernetes/kubernetes/blob/f1bfde49002ed3e44d3b47b6737536b35dca8f55/pkg/controller/volume/persistentvolume/pv_controller.go#L1521 Keeping track of volumes when there is no PV referencing them is hard so we are just cleaning them up immediately. I am not sure how common API throttling is, if #68 is the typical cause then yeah that is the easier fix.

For bug 2 the return value only affects goroutinemap's exponential retrying logic so the PV & PVC binding will be fine.

[sbezverk]

@saad-ali Here is a proposed change WDYT
kubernetes/kubernetes#65100
I think similar behaviour could be used for external provisioner as well.

[jsafrane]

Work in progress: kubernetes-sigs/sig-storage-lib-external-provisioner#33