is the numbering here and blow correct?

markdown expands it to different levels, i.e. this "1." is rendered as "a.", see "rich diff" at https://github.com/kubernetes/kubernetes/pull/30285/files?short_path=3cb6e02#diff-3cb6e02c12e0f637a83317ce28eb2c8c

Please report any problems with wrong cross-references, I might have missed some.. And yes, it's hard to review from looking at the .md file, better review rendered markdown or rich diff.

should the provisioner be blacklisted if it keeps trashing?

I would leave such optimization to future when we see they're needed. The first implementation would IMO work well without it.

There are a couple of other race conditions here, I think, as hinted at by the paragraph below. Specifically,

1. The claim can be changed to point to a storage class that uses a different provisioner.
2. The claim can be resized to request more storage than it originally did.
3. The claim can be resized to request less storage than it originally did.

Item 3 is a non-issue (the claim will bind to the PVC), but items 1 and 2 will likely cause problems. I'm unsure what the desired semantics for item 1 should be, as there'll be nothing to stop the binding from occurring, even though the user no longer wants to use the external provisioner. In the case of item 2, the binding won't happen, and the incorrectly-sized volume should probably be deleted and the binding retried.

1. this is indeed possible in rare cases, however if two provisioners create a PV for one PVC, the PV controller will bind the PVC to one of them and it will mark the "loosing" one for deletion.

2-3) we do not support PVC resizing (yet), PVC are be immutable after creation.

Should there be an explicit mechanism for doing this that all external provisioners should support, like an annotation in the PVC? Should this also apply to internal provisioners?

We're not there yet, maybe in the future.

This seems to contradict line 260, which implies that other reclaim options should be possible. In cases where the policy is set to "Retain", a useful approach would be to retain the storage asset until the PV itself was deleted, rather than the PVC. "Recycle" should probably have similar semantics to its current behavior, although the recycling process could be left to the plugin itself, possibly.

good point, the delete should check reclaim policy first. I'll add a note about it.

@jsafrane are you referring deleter of the external provisioner here ? if yes, we may require the storage class parameters at time of deleter() invocation. Isnt it ?

I'd like to move away from StorageClass to be present at the time of deletion. The only reason we need it there is for internal provisioners and their handling of secrets. External provisioners can have secrets configured in a different way, e.g. as Secret instance attached to the pod where the provisioner runs. So, let's suggest that the class may be missing at the time when a deleter is invoked. I'll add some text about that.

@jsafrane thanks, but then, if there are more than one storage class, the external provisioner need to keep track of the parameters of each SC internally and fetch and use it whenever a delete request comes in. iic on this assumption, it will be difficult to maintain the pointers to SC and PVs when deleter is invoked in the external provisioners . Isnt it ?

the provisioner still can pass parameters to deleter via PV annotations. Just don't pass pointers to secret this way.

@jsafrane thanks for the clarification ! that make sense :)

@jsafrane if its running as a pod, do we need to make sure the pod has certain privileges to serve the claims ? I mean, the access to the controller or the api server or something of that sort?

The pod's privileges are up to the storage admin & external provisioner author and shouldn't need accounting for in the binding controller.

Thanks @childsb .

I think we could relax or undefine the behavior for label selectors and storage classes for out of tree provisioners. The only thing that needs to be enforced is that PVC.size < PV.size, PV.storageClass = PVC.storageClass and PVC.selector matches PV.labels.

None of the in-tree provisioners support labels+storageClass at this time, but there's no real way we can enforce out-of-tree behavior for storageClass and label selectors for provisioners.

Possible update: "For the in-tree storage provisioners, a PVC with labelSelectors is ignored for provisioning. For out of tree provisioners, the PV created by the provisioner must be labeled such that it matches the PVC selector."

done, thanks for a nice sentence

The specification says "for in-tree provisioners a PVC with labelSelectors is ignored for provisioning" and "claim.Spec.Selector and claim.Spec.Class are mutually exclusive. User can either match existing volumes with Selector XOR match existing volumes with Class and get dynamic provisioning by using Class." However, external provisioners "MUST parse arguments in the StorageClass and claim.Spec.Selector and provisions appropriate storage asset that matches both the parameters and the selector." Why is there a distinction between in-tree and external provisioners? Can external provisioners ignore PVCs with labelSelectors?

You are right, the text is different for internal and external provisioners, however the result is the same: if there is a selector in a PVC, the provisioner must provision something that matches both the class and the selector. In-tree provisioners are stupid and don't parse selectors right now (this will change in future, 1.6?) and therefore refuse to provision anything with any selector. I would advise external provisoners do the same - either refuse to provision a claim with any selector or explicitly select few labels that are allowed in a PVC selector and document those as additional way how to configure a volume. Therefore "MUST parse claim.Spec.Selector" may be just if pvc.Spec.Selector != nil { return Error("can't parse selector!") }

I'll try to capture this idea in the proposal...

I added a new item here: https://github.com/kubernetes/kubernetes/pull/30285/files#diff-3cb6e02c12e0f637a83317ce28eb2c8cR80

Thanks for the clarification! Is using selector as a means to passing arguments to provisioners an interim capability (say to support dynamic provisioning for releases before 1.4 where StorageClass abstraction didn't exist) or a capability planned for the long term?

For internal provisioners (GCE/AWS/Cinder) it's a feature we'd like introduce eventually, maybe in 1.6 (patches welcome!), external provisioner can go wild and parse selectors as they want, Kubernetes will not prevent them in doing so in any release.