Skip to content

spec: Move AccessMode to VolumeCapability #82

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

Merged
merged 2 commits into from
Aug 23, 2017

Conversation

jieyu
Copy link
Member

@jieyu jieyu commented Aug 14, 2017

This patch moved AccessMode to VolumeCapability, because access mode
is strongly related to the filesystem type. For instance, a plugin might
support SINGLE_NODE_WRITER for xfs, and MULTI_NODE_READER_ONLY for
gfs2.

This patch also removed VolumeCapability fields from
ControllerGetCapabilities and NodeGetCapabilities RPCs. It becomes
very tedious to ask the plugins to enumerate all volume capabilities
that a plugin supports, and probably provides less value. To check the
capablities of a volume, the CO should use ValidateVolumeCapabilities.
This applies to both newly provisioned volumes and pre-existing volumes.

See discussion in #61.

jieyu added 2 commits August 14, 2017 14:01
This patch fixed some style issues in the spec according to the
contribution guide.
This patch moved `AccessMode` to `VolumeCapability`, because access mode
is strongly related to the filesystem type. For instance, a plugin might
support `SINGLE_NODE_WRITER` for `xfs`, and `MULTI_NODE_READER_ONLY` for
`gfs2`.

This patch also removed `VolumeCapability` fields from
`ControllerGetCapabilities` and `NodeGetCapabilities` RPCs. It becomes
very tedious to ask the plugins to enumerate all volume capabilities
that a plugin supports, and probably provides less value. To check the
capablities of a volume, the CO should use `ValidateVolumeCapabilities`.
This applies to both newly provisioned volumes and pre-existing volumes.

See discussion in container-storage-interface#61.
@jieyu jieyu requested review from saad-ali and jdef August 14, 2017 21:15
@jieyu
Copy link
Member Author

jieyu commented Aug 14, 2017

cc @jsafrane

@jdef
Copy link
Member

jdef commented Aug 15, 2017

Thanks for this, posting this list of impacted RPCs for sanity checks:

  • CreateVolume
    • added AccessMode via VolumeCapability for CreateVolumeRequest
    • removed AccessMode via VolumeInfo for CreateVolumeResponse
  • ValidateVolumeCapabilities
    • added AccessMode via VolumeCapability for ValidateVolumeCapabilitiesRequest
    • removed AccessMode via VolumeInfo for ValidateVolumeCapabilitiesRequest
  • ListVolumes
    • removed AccessMode via VolumeInfo
  • ControllerPublishVolume
    • added AccessMode via VolumeCapability
  • NodePublishVolume
    • added AccessMode via VolumeCapability
  • ControllerGetCapabilities
    • removed VolumeCapability
  • NodeGetCapabilities
    • removed VolumeCapability

Copy link
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -498,10 +531,6 @@ message VolumeInfo {
// NFS share). If set, it MUST be non-zero.
uint64 capacity_bytes = 1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Access mode should still be in the returned VolumeInfo no? That way SP can satisfy with access_mode equal to or better than what was asked for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If access mode is part of VolumeCapability as well as VolumeInfo then ValidateVolumeCapabilities seems confusing because it has both data types, and each would have an AccessMode.

If access mode is NOT part of VolumeInfo what prevents the SP from satisfying the request with a mode equal to or better than what was requested? If the SP upgrades the access mode in a CreateVolume request, does the CO really care as long as it gets the minimum behavior it needs for the workload it wants to run?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the use case I am trying to understand is this:

  1. CreateVolume(VolumeCapability...) where VolumeCapability contains access_mode set to SINGLE_NODE_READER_ONLY.
  2. The volume plugin satisfies the request by provisioning a volume that is MULTI_NODE_READER_ONLY.
  3. The CreateVolumeResponse does not contain the actual access_mode, because it was removed from VolumeInfo.

How would the CO figure out the actual access_mode of the newly created volume? By issuing a follow up ValidateVolumeCapabilities call?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the above workflow, why does the CO care that the provisioned volume is actually MULTI_NODE_READER_ONLY vs. what was requested? Is it trying to make decisions about other workloads on other nodes that may be trying to use the same volume (and if so, shouldn't it stop them from doing so because step 1 requested SINGLE_NODE_READER_ONLY to begin with..)?

Copy link
Member Author

@jieyu jieyu Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saad-ali, thanks for the feedback. As @jdef pointed out, if the CO requested certain AccessMode in the request, it probably shouldn't care about what is the actual capability of the volume (which might have a larger set of capabilities). For instance, to map to k8s concepts. If a user specified a PVC with access mode SINGLE_NODE_READER_ONLY, the PV dynamically provisioned for this PVC should probably have access mode SINGLE_NODE_READER_ONLY because that's what the user expects the volume to have. Whether the PV actually has MULTI_NODE_READER_ONLY capability or not doesn't matter to the user.

Also, I think @jsafrane made an excellent point in #61 that access_mode is usually tied to filesystem type. access_mode might be different depending on the underlying filesystem. So, returning just access_mode probably does not make sense.

I'd be OK that CreateVolumeResponse also returns a set of VolumeCapabilities that a volume actually has. But as @jsafrane pointed in #61, there might be a lot of combinations. That's the reason I remove those in the spec and always ask the CO to initiate the check using ValidateVolumeCapabilities based on what capabilities the user actually want the volume to have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification guys. I can't think of a good reason the CO might care other than for binding, which doesn't matter in the dynamic provisioning case. So further objection.

@saad-ali
Copy link
Member

/lgtm
/approve

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 this pull request may close these issues.

3 participants