CreateVolumeRequest: add requested AccessMode

[jsafrane]

User should be able to specify (via CO) what kind of volume should be created.

Some applications running on CO may require a SINGLE_NODE_WRITER volume for a database, while others may want MULTI_NODE_MULTI_WRITER for say sharing of pictures. The Plugin should 1) see what the application needs and 2) be able to raise an error if it's not able to provision a volume with requested access mode.

[cpuguy83]

+1, this LGTM

[jdef]

This came up a few times while we wrote the spec, but I forget the reason why we pushed access mode to the *Publish calls. /cc @saad-ali @thockin @jieyu do you remember the reasoning here?

[jieyu]

Thanks for the PR! This PR made me think: should we move AccessMode to VolumeCapability?
I was asking myself why AccessMode is any different than thing like Block/Mount? It looks like they are the same: all representing some capability of a volume. In CreateVolumeRequest, a CO can request a volume with certain capabilities, and the volume create MUST have those capabilities. AccessMode is just one of those capabilities.

For the sake of consistency, we should either add VolumeCapability to VolumeInfo, or remove AccessMode from VolumeInfo.

Something like the following:

message VolumeCapability {
  ...
  message AccessMode {
    ...
  }

  // One of the following fields MUST be specified.
  oneof value {
    BlockVolume block = 1; 
    MountVolume mount = 2; 
    AccessMode access_mode = 3;
  }
}

message VolumeInfo {
  uint64 capacity_bytes = 1;
  VolumeID id = 4;
  VolumeMetadata metadata = 5;
  repeated VolumeCapability volume_capabilities = 6;
}

Another nice thing about this proposal is that we now can get the AccessMode that a Plugin supports by calling ControllerGetCapabilities.

[jsafrane]

I did not want to put it into VolumeCapabilities because it's not useful in NodePublishVolumeRequest - there already is readonly flag and eventual MULTI_NODE / SINGLE_NODE is IMO irrelevant here, it does not change the way how the plugin mounts the volume.

Also, what would be in NodeGetCapabilitiesResponse and ControllerGetCapabilitiesResponse? If a plugin supports SINGLE_NODE_WRITER, SINGLE_NODE_READER_ONLY and MULTI_NODE_READER_ONLY and ext2, ext3, ext4, xfs, btrfs and zfs, would it return 3 x 6 VolumeCapabilities? That does not seem to be useful.

[jieyu]

@jsafrane good point on AccessMode is not necessary for NodePublishVolumeRequest. Currently, given that VolumeCapability is part of NodePublishVolumeRequest, according to your comment above, that means we should not have any volume capability that's not node related? That sounds a bit unfortunate.

RE: would it return 3 x 6 VolumeCapabilities? That does not seem to be useful.

Why 3*6, not 3+6?

[jieyu]

We need to clearly define what "restrictive" means if we want to go with this route. If we define the order in AccessMode based on "restrictiveness", I don't think that forms a total order (it's a partial order meaning that there might not be an order between two enums).

[jsafrane]

RE: would it return 3 x 6 VolumeCapabilities? That does not seem to be useful.

Why 3*6, not 3+6?

My interpretation of repeated NodeServiceCapability capabilities is that all possible combinations of fs_type and access_mode should be returned. Most filesystems like ext4 support single mount read/write mount or multiple read only mounts with no writer (i.e. SINGLE_NODE_WRITER, SINGLE_NODE_READER_ONLY and MULTI_NODE_READER_ONLY), but there are some filesystems that allow multiple writers, e.g. gfs2. So it's not enough to return one VolumeCapability for each fs_type + one VolumeCapability for each:

[
  {fs_type: "ext4"},
  {fs_type: "xfs"},
  {fs_type: "gfs2"},
  {access_mode: "SINGLE_NODE_WRITER"},
  {access_mode: "SINGLE_NODE_READER_ONLY"},
  {access_mode: "MULTI_NODE_READER_ONLY"},
  {access_mode: "MULTI_NODE_SINGLE_WRITER"},
  {access_mode: "MULTI_NODE_MULTI_WRITER"}
]

How CO knows what fs supports which access mode? You must return the allowed combinations:

[
  {fs_type: "ext4", access_mode: "SINGLE_NODE_WRITER"},
  {fs_type: "ext4", access_mode: "SINGLE_NODE_READER_ONLY"},
  {fs_type: "ext4", access_mode: "MULTI_NODE_READER_ONLY"},
  {fs_type: "xfs", access_mode: "SINGLE_NODE_WRITER"},
  {fs_type: "xfs", access_mode: "SINGLE_NODE_READER_ONLY"},
  {fs_type: "xfs", access_mode: "MULTI_NODE_READER_ONLY"},
  {fs_type: "gfs2", access_mode: "SINGLE_NODE_WRITER"},
  {fs_type: "gfs2", access_mode: "SINGLE_NODE_READER_ONLY"},
  {fs_type: "gfs2", access_mode: "MULTI_NODE_READER_ONLY"},
  {fs_type: "gfs2", access_mode: "MULTI_NODE_SINGLE_WRITER"},
  {fs_type: "gfs2", access_mode: "MULTI_NODE_MULTI_WRITER"}
]

And that's too ugly I think.

[jieyu]

@jsafrane I thought about it a bit more. I agree with you that asking the plugin to return a complete combination of VolumeCapability that it supports in ControllerGetCapabilities probably does not make sense and not scalable. There probably will be a lot of filesystems that a plugin supports. Not to mention the fact that one might consider building a meta plugin that supports multiple backends.

I think a more practical way is to let the CO drive the volume capability check. So here is my proposal:

1. Remove volume_capability in ControllerServiceCapability. For a plugin that manages multiple pools of storage space that have different volume capabilities, probably make more sense to integrate with GetCapacity (related to Proposal: GetCapacity MAY report capacity w/ respect to capabilities + parameters #59)
2. Make AccessMode be one of the VolumeCapability. Remove oneof for the fields in VolumeCapability. That means a VolumeCapability can represent a combination (e.g., (MountVolume(xfs, noquota), "SINGLE_NODE_WRITER")). Certain combination will be disallowed. For instance, either 'block' or 'mount' can be set, but not both.

message VolumeCapability {
  ...
  message AccessMode {
    ...
  }

  BlockVolume block = 1; 
  MountVolume mount = 2; 
  AccessMode access_mode = 3;
}

3. Remove access_mode in VolumeInfo. For pre-existing volumes, the CO can use ValidateVolumeCapabilities to check if a pre-existing volume has certain volume capabilities it intends to use.

message VolumeInfo {
  uint64 capacity_bytes = 1;
  VolumeID id = 4;
  VolumeMetadata metadata = 5;
}

4. Add VolumeCapability to ControllerPublishVolumeRequest. Similar to NodePublishVolumeRequest. This tells the Plugin about how the CO is about to use the volume and what volume capability that the CO expect. This should be one of the volume capabilities that the CO specifies in CreateVolumeRequest (or in ValidateVolumeCapabilities for pre-existing volumes).

message ControllerPublishVolumeRequest {
  ...
  VolumeCapability volume_capability = 6;
}

@jsafrane let me know if that makes sense to you or not. If you agree, let me know if you're interested in updating the spec. If not, I'm happy to prepare a PR for this.

cc @saad-ali @jdef @cpuguy83 @julian-hj

[jsafrane]

Just thinking out loud...

For pre-existing volumes, the CO can use ValidateVolumeCapabilities to check if a pre-existing volume has certain volume capabilities it intends to use.

I could imagine a GUI where CO asks user what kind of FS should be created on a volume. There should be a way how CO asks the plugin "give me list of everything you support" so the GUI provides a list of filesystems. This is not possible with ValidateVolumeCapabilities, where CO needs to know all filesystems in the world in advance and then check if the plugin supports each of them.

Is it a valid use case for CSI?

The rest of the proposal looks solid to me. Feel free to prepare a PR on my behalf, thanks in advance :-).

[jdef]

Is the intent to also modify NodeServiceCapability by removing its volume_capability field?

Perhaps we amend Jie's point (1) such that supported filesystems (not filesystem/mount-option combos) can still be queried?

message ControllerServiceCapability {
  message RPC {
    enum Type {
      UNKNOWN = 0;
      ... // RPC types here
    }

    Type type = 1;
  }
  
  oneof type {
    // RPC that the controller supports.
    RPC rpc = 1;
    
    // File system type that the Plugin supports. Empty strings not allowed.
    string fs_type = 2;
  }
}

[cpuguy83]

Does it make sense to move the mount/block fields into access mode at this point?
It still seems like it would need to be a OneOf for block vs mount.

[jieyu]

@cpuguy83 this is pretty much what I did in the PR (except the naming). Take a look at #82

[jieyu]

Close this one as #82 is merged.