spec: Add efficient volume cloning support #244

Akrog · 2018-05-31T08:43:13Z

With the new CSI snapshot feature it's now possible to create volumes
from a snapshots. This may lead to users abusing it for unintended
functionality, such as having a golden image to source new volumes.

Most storage backends support efficient volume cloning, so it would be
benefitial to expose this functionality in the CSI spec.

By supporting cloning we avoid users creating a volume, and then a
snapshot of that volume, just so it can be used as the source for new
volumes.

The VolumeContentSource, that was added by the snapshots feature, will
be used for this feature. New VolumeSource message is added to the
VolumeContentSource.

jdef · 2018-05-31T11:38:54Z

spec.md

@@ -602,6 +602,12 @@ This RPC will be called by the CO to provision a new volume on behalf of a user
 This operation MUST be idempotent.
 If a volume corresponding to the specified volume `name` already exists and is compatible with the specified `capacity_range`, `volume_capabilities` and `parameters` in the `CreateVolumeRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateVolumeResponse`.

+Plugins MAY create 3 types of volumes:
+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` capability.


nit: the next two are "optional capability" but this is simply "capability" -- but it's also optional. why the difference?

My bad, I'll update it.

jdef · 2018-05-31T11:39:07Z

spec.md

+Plugins MAY create 3 types of volumes:
+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability .


nit: extra space prior to the period

jdef · 2018-05-31T11:41:18Z

spec.md

@@ -796,6 +810,8 @@ The CO MUST implement the specified error recovery behavior when it encounters t
 | Operation pending for volume | 10 ABORTED | Indicates that there is a already an operation pending for the specified volume. In general the Cluster Orchestrator (CO) is responsible for ensuring that there is no more than one call "in-flight" per volume at a given time. However, in some circumstances, the CO MAY lose state (for example when the CO crashes and restarts), and MAY issue multiple calls simultaneously for the same volume. The Plugin, SHOULD handle this as gracefully as possible, and MAY return this error code to reject secondary calls. | Caller SHOULD ensure that there are no other calls pending for the specified volume, and then retry with exponential back off. |
 | Unsupported `capacity_range` | 11 OUT_OF_RANGE | Indicates that the capacity range is not allowed by the Plugin, for example when trying to create a volume smaller than the source snapshot. More human-readable information MAY be provided in the gRPC `status.message` field. | Caller MUST fix the capacity range before retrying. |
 | Call not implemented | 12 UNIMPLEMENTED | CreateVolume call is not implemented by the plugin or disabled in the Plugin's current mode of operation. | Caller MUST NOT retry. Caller MAY call `ControllerGetCapabilities` or `NodeGetCapabilities` to discover Plugin capabilities. |
+| Source incompatible or not supported | 3 INVALID_ARGUMENT | Besides the general cases, this code MUST also be used to indicate when plugin supporting CREATE_DELETE_VOLUME cannot create a volume from the requested source (`SnapshotSource` or `VolumeSource`).  Failure may be caused by not supporting the source (CO shouldn't have provided that source) or incompatibility between `parameters` from the source and the ones requested for the new volume. More human-readable information SHOULD be provided in the gRPC `status.message` field if the problem is the source. | On source related issues, caller MUST use different parameters, a different source, or no source at all. |
+| Source does not exist | 5 NOT_FOUND | Indicates that the source specified exist. | Caller MUST verify that the `volume_content_source` is correct, the source is accessible, and has not been deleted, before retrying with exponential back off. |


Typo? suggest: "Indicates that the specified source does not exist."

Also, the comma before "before" seems strange.

jdef · 2018-05-31T11:43:00Z

spec.md

@@ -1125,6 +1141,11 @@ message ControllerServiceCapability {
      // with the snapshot_id as the filter to query whether the
      // uploading process is complete or not.
      LIST_SNAPSHOTS = 6;
+      // Plugins supporting efficient volume cloning MAY report this
+      // capability.  Source volume must be managed by the same plugin.


we only use a single space between sentences.

also, suggest "A source volume..."

Sorry, I've been writing too much on RST lately and I use double space there for readability.

jdef · 2018-05-31T11:44:11Z

spec.md

@@ -1125,6 +1141,11 @@ message ControllerServiceCapability {
      // with the snapshot_id as the filter to query whether the
      // uploading process is complete or not.
      LIST_SNAPSHOTS = 6;
+      // Plugins supporting efficient volume cloning MAY report this
+      // capability.  Source volume must be managed by the same plugin.
+      // Not all volume sources and parameters combinations MAY work.


MAY indicates a recommendation, or "soft" requirement - does it make sense in this context? or would "may" (non-RFC verbage) suffice?

Normal may will suffice in my opinion. Changing it.

jdef · 2018-05-31T11:45:28Z

spec.md

+      // Plugins supporting efficient volume cloning MAY report this
+      // capability.  Source volume must be managed by the same plugin.
+      // Not all volume sources and parameters combinations MAY work.
+      // CLONE_VOLUME is NOT REQUIRED.


"... is NOT REQUIRED" seems redundant. It's a capability field because it's optional. I don't think there's a need to state that here.

I agree, I just copied from the CREATE_DELETE_SNAPSHOT in case this was something that should be done in the specs. Removing it.

cpuguy83 · 2018-05-31T12:30:20Z

spec.md

+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability .
+- From an existing volume. When plugin supports efficient cloning, and reports the optional capability `CLONE_VOLUME`.


This seems like an implementation detail. Should we support naive copies and leave it up to the plugin to handle it?

I wanted to indicate that the idea is for efficient cloning. Not an attach vol1, attach vol2, then use dd.
Plugins can ignore it and internally do that, that's why I didn't add any MUST or REQUIRED.
But plugins using the host assisted cloning mechanism (dd) would give terrible performance and in my opinion would be frustrating.
Do you think I should remove the "efficient" adjective?

jdef · 2018-05-31T14:10:04Z

Are any COs currently in a position to benefit from this?

cpuguy83 · 2018-05-31T14:15:58Z

Not here, but it's something we've discussed. I think it makes sense to support this at the CSI level at least.

Akrog · 2018-06-06T11:55:03Z

Looks like some non CSI providers are currently implementing this via PVC annotations: kubernetes/cloud-provider-openstack#183

aglitke · 2018-06-11T21:19:06Z

In addition to openstack/cinder, I am aware of at least two other kubernetes storage providers which have some implementation: Gluster and NetApp Trident. On the kubernetes side we are in early discussions with the storage-SIG on how to standardize on an approach. Once this is done, kubernetes would be able to support this API by converting PVC annotations into a volume_source.

j-griffith · 2018-08-17T17:21:47Z

@Akrog interested in dusting this off and seeing if we can get it merged? I'm happy to help out if you like. We should be able to just model the create-from-snapshot efforts exactly like you're doing and make the change pretty minimal.

Akrog · 2018-08-20T08:40:30Z

@j-griffith Yes, I'm interested in dusting this one off. I hadn't noticed the patch was in conflict, fixed now.

jdef · 2018-08-22T19:28:52Z

spec.md

+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` optional capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability.
+- From an existing volume. When plugin supports efficient cloning, and reports the optional capability `CLONE_VOLUME`.


nit: why is "efficient" a prerequisite for this capability? either a plugin has the cap or it doesn't.

A plugin could do host assisted "cloning" by creating a new volume, attaching both volumes, and dd-ing contents from source to destination.

I don't think it would be a good idea mixing both concepts (fast storage backend cloning and slow host assisted cloning) under the same capability. That would probably lead to unfulfilled expectations from the caller side.

We could add two capabilities instead of one, CLONE_VOLUME and EFFICIENT_CLONING.
That way plugins for backends supporting efficient cloning would report them both, and those doing host assisted cloning would just report the CLONE_VOLUME capability. But my initial though is that this would be more confusing/troublesome.

So I think that in discussions we agreed that we didn't need/want to specify here and wanted to keep this generic in terms of the implementation with the caveat that if we need to we could consider additions for host assisted type cloning in the future? If folks agree it'd be great to remove the specifics here and see if we can move forward with getting this merged.

Thanks, I removed "efficient" from here, and added a comment below on the attaching and copying as agreed in the meeting.

j-griffith

Sounds like if we remove the specifics regarding different implementations we can get this going again?

j-griffith · 2018-11-02T20:55:21Z

spec.md

+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` optional capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability.
+- From an existing volume. When plugin supports efficient cloning, and reports the optional capability `CLONE_VOLUME`.


So I think that in discussions we agreed that we didn't need/want to specify here and wanted to keep this generic in terms of the implementation with the caveat that if we need to we could consider additions for host assisted type cloning in the future? If folks agree it'd be great to remove the specifics here and see if we can move forward with getting this merged.

j-griffith · 2018-11-02T20:57:46Z

spec.md

@@ -1009,6 +1023,8 @@ The CO MUST implement the specified error recovery behavior when it encounters t
 | Operation pending for volume | 10 ABORTED | Indicates that there is a already an operation pending for the specified volume. In general the Cluster Orchestrator (CO) is responsible for ensuring that there is no more than one call "in-flight" per volume at a given time. However, in some circumstances, the CO MAY lose state (for example when the CO crashes and restarts), and MAY issue multiple calls simultaneously for the same volume. The Plugin, SHOULD handle this as gracefully as possible, and MAY return this error code to reject secondary calls. | Caller SHOULD ensure that there are no other calls pending for the specified volume, and then retry with exponential back off. |
 | Unsupported `capacity_range` | 11 OUT_OF_RANGE | Indicates that the capacity range is not allowed by the Plugin, for example when trying to create a volume smaller than the source snapshot. More human-readable information MAY be provided in the gRPC `status.message` field. | Caller MUST fix the capacity range before retrying. |
 | Call not implemented | 12 UNIMPLEMENTED | CreateVolume call is not implemented by the plugin or disabled in the Plugin's current mode of operation. | Caller MUST NOT retry. Caller MAY call `ControllerGetCapabilities` or `NodeGetCapabilities` to discover Plugin capabilities. |
+| Source incompatible or not supported | 3 INVALID_ARGUMENT | Besides the general cases, this code MUST also be used to indicate when plugin supporting CREATE_DELETE_VOLUME cannot create a volume from the requested source (`SnapshotSource` or `VolumeSource`).  Failure may be caused by not supporting the source (CO shouldn't have provided that source) or incompatibility between `parameters` from the source and the ones requested for the new volume. More human-readable information SHOULD be provided in the gRPC `status.message` field if the problem is the source. | On source related issues, caller MUST use different parameters, a different source, or no source at all. |
+| Source does not exist | 5 NOT_FOUND | Indicates that the specified source does not exist. | Caller MUST verify that the `volume_content_source` is correct, the source is accessible, and has not been deleted before retrying with exponential back off. |


I think we can leverage this to solve the potential of some devices only allowing RO volumes to be cloned by @humblec on the Kubernetes implementation side. IE even if we don't add caveats like RO or optimized etc to the spec we can respond with a reasonable error message if that's a requirement and it's not satisfied. In other words this LGTM and provides what we'd need.

jdef · 2018-11-07T23:49:55Z

spec.md

@@ -1012,6 +1026,8 @@ The CO MUST implement the specified error recovery behavior when it encounters t
 | Unable to provision in `accessible_topology` | 8 RESOURCE_EXHAUSTED | Indicates that although the `accessible_topology` field is valid, a new volume can not be provisioned with the specified topology constraints. More human-readable information MAY be provided in the gRPC `status.message` field. | Caller MUST ensure that whatever is preventing volumes from being provisioned in the specified location (e.g. quota issues) is addressed before retrying with exponential backoff. |
 | Unsupported `capacity_range` | 11 OUT_OF_RANGE | Indicates that the capacity range is not allowed by the Plugin, for example when trying to create a volume smaller than the source snapshot. More human-readable information MAY be provided in the gRPC `status.message` field. | Caller MUST fix the capacity range before retrying. |
 | Call not implemented | 12 UNIMPLEMENTED | CreateVolume call is not implemented by the plugin or disabled in the Plugin's current mode of operation. | Caller MUST NOT retry. Caller MAY call `ControllerGetCapabilities` or `NodeGetCapabilities` to discover Plugin capabilities. |
+| Source incompatible or not supported | 3 INVALID_ARGUMENT | Besides the general cases, this code MUST also be used to indicate when plugin supporting CREATE_DELETE_VOLUME cannot create a volume from the requested source (`SnapshotSource` or `VolumeSource`).  Failure may be caused by not supporting the source (CO shouldn't have provided that source) or incompatibility between `parameters` from the source and the ones requested for the new volume. More human-readable information SHOULD be provided in the gRPC `status.message` field if the problem is the source. | On source related issues, caller MUST use different parameters, a different source, or no source at all. |


nit: please re-order the new error row entries here so that the error codes flow from lowest to highest

Thanks, I failed to notice that errors were ordered.

saad-ali · 2018-11-07T23:45:56Z

spec.md

+Plugins MAY create 3 types of volumes:
+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` optional capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability.


When plugin supports CREATE_DELETE_VOLUME and CREATE_DELETE_SNAPSHOT optional capabilities.

saad-ali · 2018-11-07T23:46:25Z

spec.md

+
+- Empty volumes. When plugin supports `CREATE_DELETE_VOLUME` optional capability.
+- From an existing snapshot. When plugin supports `CREATE_DELETE_SNAPSHOT` optional capability.
+- From an existing volume. When plugin supports cloning, and reports the optional capability `CLONE_VOLUME`.


When plugin supports cloning, and reports the optional capabilities CREATE_DELETE_VOLUME and CLONE_VOLUME.

saad-ali · 2018-11-07T23:46:58Z

spec.md

@@ -691,8 +697,16 @@ message VolumeContentSource {
    string id = 1;
  }

+  message VolumeSource {
+    // Contains identity information for the existing source volume.
+    // This field is REQUIRED.  Plugins reporting CLONE_VOLUME


nit: be consistent about one or two spaces after period.

Sorry about that. I always leave two, but I know in this spec one is preferred, so I try to, but muscle memory sometimes gets the best of me. ;-)

saad-ali · 2018-11-07T23:51:46Z

spec.md

+      // combinations may work. Plugins MUST NOT implement this feature
+      // by attaching 2 volumes and copying the content, but COs may
+      // implement it that way for plugins that don't support this
+      // feature.


We should just drop:

// Plugins MUST NOT implement this feature // by attaching 2 volumes and copying the content, but COs may // implement it that way for plugins that don't support this // feature.

Seems like an implementation detail. Good plugins won't do it, we don't need to spell it out.

I'll remove it, but after the "efficient cloning" discussion on this PR, we decided in last weeks meeting (if I understood it right) to spell it out in the comments to ensure that plugins and COs were clear on this point, and users could expect a consistent behavior across plugins: cloning is a fast operation.

I'll remove it, but after the "efficient cloning" discussion on this PR, we decided in last weeks meeting (if I understood it right) to spell it out in the comments to ensure that plugins and COs were clear on this point, and users could expect a consistent behavior across plugins: cloning is a fast operation.

We did talk about some comments but I think having it in the form of "MUST NOT" is too strong here, and it is an implementation detail so if someone wants to do it then hey, that's up to them.

We did talk about some comments but I think having it in the form of "MUST NOT" is too strong here, and it is an implementation detail so if someone wants to do it then hey, that's up to them.

That is the situation I wanted to avoid, unmet expectations of clone on some plugins/backends. If a backend doesn't support efficient cloning and they report they do, then they are not following the specs, and users can call them out on it.

But maybe it's not such a big deal to people as I'm making it to be.

Meh... I don't know. Anyway, LGTM; I'm still not a contributor so you just get my moral support in the form of a +1

saad-ali

LGTM

saad-ali · 2018-11-08T23:19:48Z

Please rebase and we will merge this.

jieyu

LGTM. please rebase

xing-yang · 2018-11-09T00:47:27Z

LGTM but need a rebase.

With the new CSI snapshot feature it's now possible to create volumes from a snapshots. This may lead to users abusing it for unintended functionality, such as having a golden image to source new volumes. Most storage backends support efficient volume cloning, so it would be benefitial to expose this functionality in the CSI spec. By supporting cloning we avoid users creating a volume, and then a snapshot of that volume, just so it can be used as the source for new volumes. The VolumeContentSource, that was added by the snapshots feature, will be used for this feature. New VolumeSource message is added to the VolumeContentSource.

j-griffith · 2018-11-09T15:00:56Z

LGTM

xing-yang · 2018-11-09T15:23:43Z

LGTM

jdef reviewed May 31, 2018

View reviewed changes

cpuguy83 reviewed May 31, 2018

View reviewed changes

Akrog force-pushed the feature/volume-clone branch from f26ef70 to 35ec9e8 Compare May 31, 2018 13:13

Akrog force-pushed the feature/volume-clone branch from 35ec9e8 to 7f3b61d Compare June 1, 2018 08:49

Akrog force-pushed the feature/volume-clone branch from 7f3b61d to f45b2eb Compare August 20, 2018 08:36

jdef reviewed Aug 22, 2018

View reviewed changes

j-griffith mentioned this pull request Nov 2, 2018

[WIP] Create cloning.md kubernetes/community#2533

Closed

j-griffith suggested changes Nov 2, 2018

View reviewed changes

Akrog force-pushed the feature/volume-clone branch from f45b2eb to 973a0e7 Compare November 5, 2018 13:46

j-griffith approved these changes Nov 6, 2018

View reviewed changes

Akrog force-pushed the feature/volume-clone branch from 973a0e7 to 27ced57 Compare November 7, 2018 17:10

saad-ali added this to the v1.0 milestone Nov 7, 2018

saad-ali added the priority/p2 label Nov 7, 2018

jdef reviewed Nov 7, 2018

View reviewed changes

saad-ali reviewed Nov 7, 2018

View reviewed changes

Akrog force-pushed the feature/volume-clone branch from 27ced57 to 65e1843 Compare November 8, 2018 09:06

saad-ali approved these changes Nov 8, 2018

View reviewed changes

jieyu approved these changes Nov 8, 2018

View reviewed changes

Akrog force-pushed the feature/volume-clone branch from 65e1843 to 9cf5611 Compare November 9, 2018 09:30

jieyu merged commit 7a8d2d5 into container-storage-interface:master Nov 9, 2018

spec: Add efficient volume cloning support #244

spec: Add efficient volume cloning support #244

Conversation

Akrog commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akrog May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdef commented May 31, 2018

cpuguy83 commented May 31, 2018

Akrog commented Jun 6, 2018

aglitke commented Jun 11, 2018

j-griffith commented Aug 17, 2018

Akrog commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akrog Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali commented Nov 8, 2018

jieyu left a comment

Choose a reason for hiding this comment

xing-yang commented Nov 9, 2018

j-griffith commented Nov 9, 2018

xing-yang commented Nov 9, 2018

Akrog May 31, 2018 •

edited

Loading

Akrog Nov 8, 2018 •

edited

Loading