search

container-storage-interface / spec

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

Pass Topology Information for DeleteVolume #408

Open leakingtapan opened 4 years ago

leakingtapan commented 4 years ago

With latest CSI spec, topology information is passed into CreateVolume call, so that CSI driver can provision a volume by the given TopologyRequirement. The topology requirement could be region, zones, or racks. This enables CSI driver to provision volume in different regions. Eg. a CSI driver could provision volume is both us-west-2 and us-east-1. However, due to topology information is missing from DeleteVolume call, a CSI driver won't be able to delete a volume for different regions.

In order to keep API symmetric between CreateVolume and DeleteVolume, topology information should be passed into DeleteVolume call. So that a CSI driver controller service could manage volumes from different regions for clusters that have worker nodes in many different regions.

/cc @gnufied @msau42

msau42 commented 4 years ago

Generally, the volumedelete call cannot pass in information other than ID because it may not always available in the teardown case.

The way that we implemented it in the GCP PD driver is to encode zone/region information in the volume id.

leakingtapan commented 4 years ago

The way that we implemented it in the GCP PD driver is to encode zone/region information in the volume id.

Yep. that's something could be achieved at driver side.

Generally, the volumedelete call cannot pass in information other than ID because it may not always available in the teardown case

Could you give some example of when the topology info is not available? One case I can think of is pre-provisioned volume that doesn't have nodeAffinity constraint. But in that case, driver could still fall back to some default region that the driver thinks appropriate.

And where do we have such doc about encoding region in volumeHandle from the CSI spec? This feels more of a workaround than best practice for me.

vitalif commented 3 years ago

Same problem with VolumeContext missing in DeleteVolume, this is really inconvenient and a lot of drivers end up with encoding all the information into volume name... which is of course ugly as hell

andyzhangx commented 2 years ago

Same problem with VolumeContext missing in DeleteVolume, this is really inconvenient and a lot of drivers end up with encoding all the information into volume name... which is of course ugly as hell

@vitalif agree on this, 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. added a new feature request here: https://github.com/container-storage-interface/spec/issues/507

jdef commented 2 years ago

https://github.com/container-storage-interface/spec/issues/507#issuecomment-1101947052