Make node_id required in GetNodeIDResponse #148
Conversation
csi.proto
Outdated
| // know which node the volume was previously used. The Plugin SHOULD | ||
| // return an Error if this is not supported. | ||
| // The ID of the node. This field is REQUIRED. The CO SHALL set this | ||
| // field to match the node ID returned by `GetNodeID`. |
There was a problem hiding this comment.
We killed a use case here by making this REQUIRED: a CO may intentionally leave this field blank to indicate to the plugin that the volume should be unpublished from every node in the cluster. Is killing that use case really our intent here?
There was a problem hiding this comment.
Yeah. I think we deliberately made this field optional to capture the case where the CO just want to detach the volume from any node. I still believe this is useful in some reconciliation scenarios.
There was a problem hiding this comment.
Ack. I'll restore this then.
csi.proto
Outdated
| // The ID of the node as understood by the CO which MAY be returned by | ||
| // the SP in the GetNodeIDResponse if the SP does not implement a | ||
| // custom node naming scheme and understands (or internally maps) the | ||
| // CO's node id. |
spec.md
Outdated
| // The API version assumed by the CO. This is a REQUIRED field. | ||
| Version version = 1; | ||
| // The ID of the node as understood by the CO which MAY be returned by |
There was a problem hiding this comment.
cc @akutz
This is still controversial. Can we make this change a separate PR?
There was a problem hiding this comment.
Done. Will pull this in to a separate PR.
|
PTAL |
| // `ControllerPublishVolume`. | ||
| // The ID of the node as understood by the SP which SHALL be used by | ||
| // CO in subsequent `ControllerPublishVolume`. | ||
| // This is a REQUIRED field. |
There was a problem hiding this comment.
suggestion: This is an OPTIONAL field that is REQUIRED only if the controller advertises the PUBLISH_UNPUBLISH_VOLUME capability.
because not implementing the call means that GetNodeIDResponse is empty, and that violates the REQUIRED semantics as currently documented.
3 participants
node_ida required field in theGetNodeIDResponseif the plugin hasPUBLISH_UNPUBLISH_VOLUMEcontroller capability.node_ida required field in theControllerPublishVolumeandControllerUnpublishVolumecalls.node_idinGetNodeIdReqeustso that SP's that don't have their own custom internal naming scheme do not have to implement complicated logic to figure out the node name.Fixes #121