search

container-storage-interface / spec

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

Change required capability for published_node_ids in GetVolume #487

Closed bswartz closed 3 years ago

bswartz commented 3 years ago

Instead of PUBLISH_UNPUBLISH_VOLUME, use the LIST_VOLUMES_PUBLISHED_NODES capability to control the published_node_ids field in ControllerGetVolume. This change is to be more consistent with the existing behavior. It is a backwards-incompatible change, but the API is alpha.

Also clarify the LIST_VOLUMES_PUBLISHED_NODES docs.

Fixes: #486

bswartz commented 3 years ago

@saad-ali @xing-yang please take a look at this

xing-yang commented 3 years ago

LGTM

jdef commented 3 years ago

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

bswartz commented 3 years ago

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

I agree with both these.

bswartz commented 3 years ago

@jdef Updated, PTAL.

bswartz commented 3 years ago

This is ready to merge IMO

saad-ali commented 3 years ago

/lgtm /approve

I wonder if it's also worth clarifying (in the docs for the capability) that LIST_VOLUMES_PUBLISHED_NODES requires PUBLISH_UNPUBLISH_VOLUME. While the careful reader may already observe this relationship, it might be worth calling out explicitly for the sake of clarity.

Also, given the confusing name, maybe also mention ControllerGetVolumeReponse in the docs for LIST_VOLUMES_PUBLISHED_NODES ?

I agree with both these.

Opened https://github.com/container-storage-interface/spec/issues/489 to track that.