container-storage-interface / spec

Container Storage Interface (CSI) Specification.
Apache License 2.0
1.34k stars 373 forks source link

ControllerUnpublishVolume from all nodes - confusing spec #473

Open michael-db opened 3 years ago

michael-db commented 3 years ago

According to the CSI specification, a ControllerUnpublishVolume() with an empty string for the node_id field must unpublish the volume from all nodes.

This is exceptional: there is no corresponding global "node unstage" or "node unpublish" operation. All of the rest of the text in the CSI spec. assumes that ControllerUnpublishVolume() is called for a specific node.

The specification is rather confusing. ControllerUnpublishVolume() "MUST be called after all NodeUnstageVolume and NodeUnpublishVolume on the volume are called and succeed." This makes little sense if it is interpreted as meaning that it must be called whenever a volume that was staged and node-published is no longer staged, since there is no useful reason to require the CO to go through a ControllerUnpublish-ControllerPublish cycle before staging the volume again on the same node. If it is interpreted as meaning that it can only be called after the volume is no longer staged and node-published then it conflicts with the optative mood in "the CO SHOULD issue all NodeUnpublishVolume (as specified above) before calling ControllerUnpublishVolume" - unless the implication is that out-of-order Unpublish/Unstage calls are allowed; but that conflicts with the lifecycle or state diagram; also, taking into account other statements, it implies that the CO must Unstage and NodeUnpublish volumes even if they have already been ControllerUnpublished, which again makes no sense considering that the calls at that point should have no effect. On the other hand, it is consistent with there being no defined error for trying to ControllerUnpublish a volume which is still staged.

I am working with K8s which does not appear to require support for this global unpublish function, so pragmatically I am currently ignoring the global unpublish feature. I am also requiring that NodeUnstageVolume is called before ControllerUnpublishVolume.