Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

published_node_ids has different requirements in ListVolumes vs ControlerGetVolume #486

Closed
bswartz opened this issue Jul 21, 2021 · 0 comments · Fixed by #487
Closed

published_node_ids has different requirements in ListVolumes vs ControlerGetVolume #486

bswartz opened this issue Jul 21, 2021 · 0 comments · Fixed by #487

Comments

@bswartz
Copy link
Contributor

bswartz commented Jul 21, 2021

In ListVolumesResponse the docs read:

// A list of all `node_id` of nodes that the volume in this entry
// is controller published on.
// This field is OPTIONAL. If it is not specified and the SP has
// the LIST_VOLUMES_PUBLISHED_NODES controller capability, the CO
// MAY assume the volume is not controller published to any nodes.
// If the field is not specified and the SP does not have the
// LIST_VOLUMES_PUBLISHED_NODES controller capability, the CO MUST
// not interpret this field.
// published_node_ids MAY include nodes not published to or
// reported by the SP. The CO MUST be resilient to that.
repeated string published_node_ids = 1;

In ControllerGetVolume, the docs read:

// A list of all the `node_id` of nodes that this volume is
// controller published on.
// This field is OPTIONAL.
// This field MUST be specified if the PUBLISH_UNPUBLISH_VOLUME
// controller capability is supported.
// published_node_ids MAY include nodes not published to or
// reported by the SP. The CO MUST be resilient to that.
repeated string published_node_ids = 1;

For a driver wants to implement LIST_VOLUMES, GET_VOLUME, and PUBLISH_UNPUBLISH_VOLUME, but not LIST_VOLUMES_PUBLISHED_NODES, this creates an impossible situation because you have to populate the field but you don't have anything to populate it with.

I think we we simply changed the wording in the ControllerGetVolume case to refer to the LIST_VOLUMES_PUBLISHED_NODES capability instead of the PUBLISH_UNPUBLISH_VOLUME capability, then the conflict would be avoided. In that case LIST_VOLUMES_PUBLISHED_NODES would be the capability that specified COs can rely on published_node_ids in both ControllerGetVolume and ListVolumes. It's unfortunate that the capability has the word "LIST" in the name, but it predates the new Get call so unless we want to add a whole other capability, it's best to just reuse this capability and apologize for the slightly confusing name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant