Snapshotter uses ListSnapshots on drivers which do not advertise it.

[lpabon]

Snaphotter uses a function called GetSnapshotStatus() to do the binding from a VolumeSnapshot to a VolumeSnapshotContent. GetSnapshotStatus() then uses the csi.ListSnapshot() call even when the driver does not support it. Therefore this causes an error and the binding does not occur.

[lpabon]

This affects release-0.4 and later.

[msau42]

This looks like the same issue as #108?

[xing-yang]

Yes.

[xing-yang]

This only affects statically provisioned volume snapshots. For dynamically provisioned snapshots, we use CreateSnapshot to get the status. For statically provisioned snapshots, we cannot call CreateSnapshot because it was pre-provisioned, the only way to verify its status is to call ListSnapshots with a snapshot_id to get its status.

I looked at the code in PV controller to see how it handles statically provisioned volumes. Looks like it does not verify whether the volume exists or not on the storage system and just assumes it is there. It directly updates volume phase to "VolumeAvailable":
https://github.com/kubernetes/kubernetes/blob/v1.15.0/pkg/controller/volume/persistentvolume/pv_controller.go#L524-L528

I think we can use similar approach for statically provisioned volume snapshots. We need to document that the existence of the snapshots will not be verified by the snapshot controller and it is the responsibility of the admin who statically provisions the snapshots to make sure the snapshot id is correct. For drivers that have implemented ListSnapshots, we can verify the status of the snapshot. The information we are getting out of GetSnapshotStatus (ListSnapshots with snapshot_id) is as follows:

• ReadyToUse
• CreationTime
• RestoreSize

We can directly set ReadyToUse to True for drivers that do not support ListSnapshots. For drivers that do support ListSnapshots, we can get it from the driver.
Similarly for drivers that do not support ListSnapshots, we will not set CreationTime and RestoreSize. These two parameters are optional.

[xing-yang]

Actually ListSnapshots was used to check whether snapshot is ready to use when the snapshot CRD was introduced in v0.4.0, for both dynamic and static binding (regardless of whether ListSnapshots is supported by the driver or not).

In v1.0.1, there was a fix to use CreateSnapshot to replace ListSnapshots in order to support CSI spec 1.0 changes (c01fb64). That fix allowed dynamic binding to use CreateSnapshot to replace ListSnapshots, however, it broke static binding.

This above bug in v1.0.1 was fixed by #98 in v1.1.0, but it calls ListSnapshots again because CreateSnapshot is not called for static binding.

To fix this, we should check whether ListSnapshots is supported by the driver. If it is supported, use ListSnapshots to validate snapshot's status. If it is not supported, do not call ListSnapshots and assume snapshot ID provided by admin is valid (same as PV controller's logic).

[xing-yang]

In the CSI spec, ListSnapshots is optional, but we recommend drivers that need to upload snapshot to implement ListSnapshots function because it could be a time consuming process for upload to complete and for the snapshot to become ReadyToUse.

[ggriffiths]

Working on this issue now, will open a PR when ready.