search

container-storage-interface / spec

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

[feature request] provide VolumeContext in DeleteVolume, ControllerExpandVolume request #507

Closed andyzhangx closed 2 years ago

andyzhangx commented 2 years ago

VolumeContext should also be in DeleteVolumeRequest, currently we have to encoding all info as a volumeID, now the volumeID is longer and longer, that's really ugly design. if there is VolumeContext in DeleteVolumeRequest, we could inject volume.Spec.ClaimRef.Namespace in external-provisioner by providing flag --extra-delete-metadata , similar to --extra-create-metadata: https://github.com/kubernetes-csi/external-provisioner#recommended-optional-arguments.

jdef commented 2 years ago

please no. we've discussed this all before, and it should be possible to delete a volume if CO loses its mind and vol metadata is lost. if vol ID is getting "longer and longer" then this suggests arch/design issues elsewhere (because non-identifying information is being stuffed into the ID field for convenience).

andyzhangx commented 2 years ago

please no. we've discussed this all before, and it should be possible to delete a volume if CO loses its mind and vol metadata is lost. if vol ID is getting "longer and longer" then this suggests arch/design issues elsewhere (because non-identifying information is being stuffed into the ID field for convenience).

@jdef we need secretNamespace in DeleteVolume, so what's your suggestion in this case, append secretNamespace into volumeID?

jdef commented 2 years ago

@andyzhangx maybe i misunderstand the use case. if you need secrets to delete a volume, doesn't DeleteVolumeRequest already offer that API? CSI doesn't know about k8s namespaces, nor should it. that said, if namespace is a reasonable (read: essential) coordinate for locating a volume in some storage backend, then it probably belongs in vol ID.

andyzhangx commented 2 years ago

@andyzhangx maybe i misunderstand the use case. if you need secrets to delete a volume, doesn't DeleteVolumeRequest already offer that API? CSI doesn't know about k8s namespaces, nor should it. that said, if namespace is a reasonable (read: essential) coordinate for locating a volume in some storage backend, then it probably belongs in vol ID.

@jdef actually external-provisioner would inject pvcNamespace now on CreateVolume by using --extra-create-metadata: https://github.com/kubernetes-csi/external-provisioner#recommended-optional-arguments, I would like to get same functionality for DeleteVolume, current workaround is append pvcNamespace to volumeID, now our volumeID already has resoucegroup#accountname#sharename#uuid, this would make volumeID longer and longer, and also we need to consider volumeID format comparability issue. To make life easier, why not providing a VolumeContext in DeleteVolume, so CSI driver would write any data to VolumeContext, that won't change the volumeID format, it would make the design clearer. Next time if we would like to inject other metadata from external-provisioner, we could just write data to VolumeContext, I think that would benefit a lot CSI drivers.

bswartz commented 2 years ago

DeleteVolume is expected to succeed without a volume context because it might be called as part of an emergency cleanup operation, when the CO has lost other relevant information and has only the volume ID. The alternative is that DeleteVolume might not get called at all, and resources could leak, in a scenario where the CO has lost data. The spec writers decided it was preferable to put the responsibility on the SP for retaining enough relevant knowledge so that it can delete a volume given nothing but the volume ID and delete secrets.

andyzhangx commented 2 years ago

@bswartz thanks for the clarification!