search

container-storage-interface / spec

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

Consider adding publish_context or volume Capabilities to NodeUnstageVolume #362

Open codenrhoden opened 5 years ago

codenrhoden commented 5 years ago

This is closely intertwined with #361.

When doing a NodeUnstageVolume, neither the publish_context or the volume Capabilities are passed to the RPC.

This is problematic for my specific use case. When handling BlockVolume (instead of a MountVolume), the NodeUnstageVolume RPC needs to work differently (as per #361). There is no context given in the call on whether you are "unstaging" a block volume or a mount volume, so one cannot behave differently in those scenarios.

If we rely on the fact that a CO is required to create a staging-target-path (even for block volumes, which won't be used), I can detect that nothing is mounted to the target path and proceed happily. This works, and is what I will do, but seems a bit hacky to me. And if we decide in the future that a CO does not need to create a staging target dir for block volumes, this would break things. I think it would be better for NodeUnstageVolume to have the volume capabilities block so we know whether we are dealing with Mount or Block. Or alternatively, just like #361 says for NodeStageVolume make NodeUnstageVolume not be called when dealing with block.

On a secondary note (I should probably file a separate issue), I also find it problematic that PublishContext is not provided to NodeUnstageVolume while it is provided to NodeStageVolume. For my storage platform, the VolumeID is not enough to identify the underlying block device (e.g. /dev/sd{x}). The device is explicitly passed in via the PublishContext. This works fine for staging, but makes unstaging less reliable. I have to blindly unmount the staging target without being able to confirm that the requested volume is actually the one that is mounted to the staging target.

shay-berman commented 5 years ago

We also have the same issue for our CSI plugin, and it would be great to have publish_context as argument also in the NodeUnstageVolume API.

But after the last CSI meeting(1/5/2019) it looks like that publish_context is not persistent in the CO per the spec(the only persistent info is volumeID), and the best practice is that the plugin needs to store any info required for unstage inside the node itself (for example on /tmp since the info do not need to persist after reboot, or in the nodestage-target-dir).

more detail inside the meeting minutes(1/5/2019) -> https://docs.google.com/document/d/1-oiNg5V_GtS_JBAEViVBhZ3BYVFlbSz70hreyaD7c5Y/edit#heading=h.h3flg2md1zg

@saad-ali feel free to add more clarification if needed.

thanks

Akrog commented 5 years ago

Over a year ago I asked to get more data to several of the grpc calls with no luck ( https://github.com/container-storage-interface/spec/pull/30 ).

As I workaround for the unstage you can always implement your own mechanism in the stage call. For example staging the volume on "$staging_target_path/stage" and then you can check on unstage if this is a directory or a block device, thus knowing the type of volume.

If you remove stage/unstage you can always do the same check (directory or block) on the target_path.