Change SnapshotStatus enum to a boolean #307

xing-yang · 2018-11-08T03:03:06Z

This PR changes SnapshotStatus enum to a boolean is_ready_to_use
and tries to use "processing" instead of "uploading" if possible
to make it less specific.

fixes #302

julian-hj

👍

xing-yang · 2018-11-08T17:54:24Z

Thanks @julian-hj .
@saad-ali @jdef Please take a look.

saad-ali · 2018-11-08T18:15:45Z

spec.md

+      // LIST_SNAPSHOTS is NOT REQUIRED. For plugins that need to
+      // process a snapshot after it is being cut, LIST_SNAPSHOTS
+      // COULD be used with the snapshot_id as the filter to query
+      // whether the processing is complete or not.


For plugins that need to a snapshot after it is being cut, LIST_SNAPSHOTS COULD be used with the snapshot_id as the filter to query whether the processing is complete or not.

^^This is no longer required right? CO should call CreateVolume again to poll the is_ready_to_use to use state.
If so, let's remove this.

saad-ali · 2018-11-08T18:27:37Z

spec.md

@@ -1405,12 +1405,12 @@ A Controller Plugin MUST implement this RPC call if it has `CREATE_DELETE_SNAPSH
 This RPC will be called by the CO to create a new snapshot from a source volume on behalf of a user.

 This operation MUST be idempotent.
-If a snapshot corresponding to the specified snapshot `name` is already successfully cut and uploaded (if upload is part of the process) and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.
+If a snapshot corresponding to the specified snapshot `name` is already successfully cut and processed (i.e., if upload is part of the process) and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.


Change this to:

If a snapshot corresponding to the specified snapshot name is successfully cut and ready to use (meaning it may be specified as a volume_content_source in a CreateSnapshotRequest) , the Plugin MUST reply 0 OK with the corresponding CreateSnapshotResponse.

saad-ali · 2018-11-08T18:34:39Z

spec.md


 If an error occurs before a snapshot is cut, `CreateSnapshot` SHOULD return a corresponding gRPC error code that reflects the error condition.

-For plugins that implement snapshot uploads, `CreateSnapshot` SHOULD return `10 ABORTED`, a gRPC code that indicates the operation is pending for snapshot, during the snapshot uploading processs.
-If an error occurs during the uploading process, `CreateSnapshot` SHOULD return a corresponding gRPC error code that reflects the error condition.
+For plugins that supports snapshot post processing such as uploading, `CreateSnapshot` SHOULD return `0 OK` and `is_ready_to_use` SHOULD be set to `false` after the snapshot is cut but still being processed.


Add the following:

CO SHOULD then reissue the same CreateSnapshotRequest periodically until boolean isReadyToUse flips to true indicating the volume has been "processed" and is ready to use to create new Snapshots.

saad-ali · 2018-11-08T18:53:52Z

spec.md

-  // Additional information to describe why a snapshot ended up in the
-  // `ERROR_UPLOADING` status. This field is OPTIONAL.
-  string details = 2;
+  // Indicates if a snapshot is ready to use. The default value is


Indicates if a snapshot is ready to use. -> Indicates if a snapshot is ready to use as a volume_content_source in a CreateSnapshotRequest.

saad-ali · 2018-11-08T19:03:00Z

spec.md

-A controller plugin COULD implement the `LIST_SNAPSHOTS` capability and call it repeatedly with the `snapshot_id` as a filter to query whether the uploading process is complete or not if it needs to upload a snapshot after it is being cut.
-
-##### Snapshot Statuses
+A controller plugin COULD implement the `LIST_SNAPSHOTS` capability and call it repeatedly with the `snapshot_id` as a filter to query whether the process is complete or not if it needs to do post processing such as uploading a snapshot after it is being cut.


Let's drop this line. We should recommend calling CreateSnapshot again instead of List, so they can get an error code.

saad-ali · 2018-11-08T19:07:48Z

spec.md

-If the cloud provider or storage system needs to upload the snapshot after the snapshot is cut, the status returned by CreateSnapshot SHALL be `UPLOADING`.
-CO MAY continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.
+If the cloud provider or storage system does not need to process the snapshot (i.e., upload the snapshot) after it is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `true`.
+If the cloud provider or storage system needs to process the snapshot after the snapshot is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `false`.


Move this line to right after lines 1646-1647

saad-ali · 2018-11-08T19:08:33Z

spec.md

-CO MAY continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.
+If the cloud provider or storage system does not need to process the snapshot (i.e., upload the snapshot) after it is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `true`.
+If the cloud provider or storage system needs to process the snapshot after the snapshot is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `false`.
+CO MAY continue to call CreateSnapshot while waiting for the process to complete until `is_ready_to_use` becomes `true`.


This line is very similar to 1654. Delete and combine with that.

saad-ali · 2018-11-08T19:09:14Z

spec.md

 Note that CreateSnapshot no longer blocks after the snapshot is cut.

-Alternatively, ListSnapshots can be called repeatedly with snapshot_id as filtering to wait for the upload to complete.
+Alternatively, ListSnapshots can be called repeatedly with snapshot_id as filtering to wait for the process to complete.


I would not recommend this. Suggest dropping this line.

saad-ali · 2018-11-08T19:09:53Z

spec.md

 ListSnapshots SHALL return with current information regarding the snapshots on the storage system.
-When upload is complete, the status of the snapshot from ListSnapshots SHALL become `READY`.
+When the process is complete, the `is_ready_to_use` parameter of the snapshot from ListSnapshots SHALL become `true`.


When the process is complete -> When processing is complete

saad-ali · 2018-11-08T19:11:27Z

spec.md


-A snapshot could have the following statusus: UPLOADING, READY, and ERROR.
+##### The is_ready_to_use Parameter


I would move this section (everything from line 1635 to 1658) up to the CreateSnapshot section of the doc. It doesn't make sense under ListSnapshot.

Hmm, I'm still seeing this under ListSnapshots? Move everything from here to line 1656

So I'm moving the entire "##### The is_ready_to_use Parameter" section until line 1656 to the "#### CreateSnapshot" section. I believed that is what you meant? This current section is called "##### CreateSnapshot, DeleteSnapshot, ListSnapshots". So I am a little confused when you said "under ListSnapshot".

xing-yang

Addressed review comments. Thanks.

xing-yang · 2018-11-08T20:15:59Z

spec.md

+      // LIST_SNAPSHOTS is NOT REQUIRED. For plugins that need to
+      // process a snapshot after it is being cut, LIST_SNAPSHOTS
+      // COULD be used with the snapshot_id as the filter to query
+      // whether the processing is complete or not.


xing-yang · 2018-11-08T20:16:07Z

spec.md

@@ -1405,12 +1405,12 @@ A Controller Plugin MUST implement this RPC call if it has `CREATE_DELETE_SNAPSH
 This RPC will be called by the CO to create a new snapshot from a source volume on behalf of a user.

 This operation MUST be idempotent.
-If a snapshot corresponding to the specified snapshot `name` is already successfully cut and uploaded (if upload is part of the process) and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.
+If a snapshot corresponding to the specified snapshot `name` is already successfully cut and processed (i.e., if upload is part of the process) and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.


xing-yang · 2018-11-08T20:16:13Z

spec.md


 If an error occurs before a snapshot is cut, `CreateSnapshot` SHOULD return a corresponding gRPC error code that reflects the error condition.

-For plugins that implement snapshot uploads, `CreateSnapshot` SHOULD return `10 ABORTED`, a gRPC code that indicates the operation is pending for snapshot, during the snapshot uploading processs.
-If an error occurs during the uploading process, `CreateSnapshot` SHOULD return a corresponding gRPC error code that reflects the error condition.
+For plugins that supports snapshot post processing such as uploading, `CreateSnapshot` SHOULD return `0 OK` and `is_ready_to_use` SHOULD be set to `false` after the snapshot is cut but still being processed.


xing-yang · 2018-11-08T20:16:20Z

spec.md

-  // Additional information to describe why a snapshot ended up in the
-  // `ERROR_UPLOADING` status. This field is OPTIONAL.
-  string details = 2;
+  // Indicates if a snapshot is ready to use. The default value is


xing-yang · 2018-11-08T20:16:28Z

spec.md

-A controller plugin COULD implement the `LIST_SNAPSHOTS` capability and call it repeatedly with the `snapshot_id` as a filter to query whether the uploading process is complete or not if it needs to upload a snapshot after it is being cut.
-
-##### Snapshot Statuses
+A controller plugin COULD implement the `LIST_SNAPSHOTS` capability and call it repeatedly with the `snapshot_id` as a filter to query whether the process is complete or not if it needs to do post processing such as uploading a snapshot after it is being cut.


xing-yang · 2018-11-08T20:16:43Z

spec.md

-If the cloud provider or storage system does not need to upload the snapshot after it is cut, the status returned by CreateSnapshot SHALL be `READY`.
-If the cloud provider or storage system needs to upload the snapshot after the snapshot is cut, the status returned by CreateSnapshot SHALL be `UPLOADING`.
-CO MAY continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.
+If the cloud provider or storage system does not need to process the snapshot (i.e., upload the snapshot) after it is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `true`.


xing-yang · 2018-11-08T20:16:48Z

spec.md

-If the cloud provider or storage system needs to upload the snapshot after the snapshot is cut, the status returned by CreateSnapshot SHALL be `UPLOADING`.
-CO MAY continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.
+If the cloud provider or storage system does not need to process the snapshot (i.e., upload the snapshot) after it is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `true`.
+If the cloud provider or storage system needs to process the snapshot after the snapshot is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `false`.


xing-yang · 2018-11-08T20:16:54Z

spec.md

-CO MAY continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.
+If the cloud provider or storage system does not need to process the snapshot (i.e., upload the snapshot) after it is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `true`.
+If the cloud provider or storage system needs to process the snapshot after the snapshot is cut, the `is_ready_to_use` parameter returned by CreateSnapshot SHALL be `false`.
+CO MAY continue to call CreateSnapshot while waiting for the process to complete until `is_ready_to_use` becomes `true`.


xing-yang · 2018-11-08T20:17:00Z

spec.md

 Note that CreateSnapshot no longer blocks after the snapshot is cut.

-Alternatively, ListSnapshots can be called repeatedly with snapshot_id as filtering to wait for the upload to complete.
+Alternatively, ListSnapshots can be called repeatedly with snapshot_id as filtering to wait for the process to complete.


xing-yang · 2018-11-08T20:17:06Z

spec.md

 ListSnapshots SHALL return with current information regarding the snapshots on the storage system.
-When upload is complete, the status of the snapshot from ListSnapshots SHALL become `READY`.
+When the process is complete, the `is_ready_to_use` parameter of the snapshot from ListSnapshots SHALL become `true`.


saad-ali · 2018-11-08T20:54:42Z

spec.md


-A snapshot could have the following statusus: UPLOADING, READY, and ERROR.
+##### The is_ready_to_use Parameter


Hmm, I'm still seeing this under ListSnapshots? Move everything from here to line 1656

saad-ali · 2018-11-08T20:57:29Z

spec.md

-Some cloud providers will upload the snapshot to a location in the cloud (i.e., an object store) after the snapshot is cut.
-Uploading may be a long process that could take hours.
-If a `freeze` operation was done on the application before taking the snapshot, it could be a long time before the application can be running again if we wait until the upload is complete to `thaw` the application.
+Some cloud providers will process the snapshot after the snapshot is cut, i.e., uploading the snapshot to a location in the cloud (i.e., an object store) after the snapshot is cut.


Some cloud providers will process... -> Some SPs MAY "process"...
...cut, i.e., uploading... -> ...cut, for example, maybe uploading...
...to a location in the cloud (i.e., an object store) after the snapshot is cut. -> ...somewhere after the snapshot is cut.

saad-ali · 2018-11-08T20:58:05Z

spec.md

-Uploading may be a long process that could take hours.
-If a `freeze` operation was done on the application before taking the snapshot, it could be a long time before the application can be running again if we wait until the upload is complete to `thaw` the application.
+Some cloud providers will process the snapshot after the snapshot is cut, i.e., uploading the snapshot to a location in the cloud (i.e., an object store) after the snapshot is cut.
+A process such as uploading may be a long process that could take hours.


A process such as uploading may... -> The post-cut process may...

saad-ali · 2018-11-08T21:00:22Z

spec.md

-If a `freeze` operation was done on the application before taking the snapshot, it could be a long time before the application can be running again if we wait until the upload is complete to `thaw` the application.
+Some cloud providers will process the snapshot after the snapshot is cut, i.e., uploading the snapshot to a location in the cloud (i.e., an object store) after the snapshot is cut.
+A process such as uploading may be a long process that could take hours.
+If a `freeze` operation was done on the application before taking the snapshot, it could be a long time before the application can be running again if we wait until the process is complete to `thaw` the application.


Change this line to:

The CO may freeze the application using the source volume before taking the snapshot.

saad-ali · 2018-11-08T21:02:09Z

spec.md


-For cloud providers and storage systems that don't have the uploading process, the status should be `READY` after the snapshot is cut.
-`thaw` can be done when the status is `READY` in this case.
+For cloud providers and storage systems that don't have the process, the `is_ready_to_use` parameter should be `true` after the snapshot is cut.


Find and replace all instances of
should -> SHOULD
may -> MAY
must -> MUST

At least the bits you are touching in this PR. I will open a PR addressing the rest of the spec.

saad-ali · 2018-11-08T21:04:49Z

spec.md


-The SnapshotStatus parameter provides guidance to the CO on what action can be taken in the process of snapshotting.
+The `is_ready_to_use` parameter provides guidance to the CO on what action can be taken in the process of snapshotting.


Move this before line 1647.
Change on what action can be taken -> on when it can "thaw" the application

j-griffith

LGTM

But I'm coming in late on this discussion so I may have missed a detail in there

saad-ali

Sorry one last (promise no more) set of changes.

saad-ali · 2018-11-08T23:38:12Z

csi.proto

-      // a snapshot after it is being cut, LIST_SNAPSHOTS COULD be used
-      // with the snapshot_id as the filter to query whether the
-      // uploading process is complete or not.
+      // LIST_SNAPSHOTS is NOT REQUIRED.


I don't think we need this comment at all now.

saad-ali · 2018-11-08T23:39:31Z

csi.proto

+  // Indicates if a snapshot is ready to use as a
+  // `volume_content_source` in a `CreateVolumeRequest`. The default
+  // value is false. This field is REQUIRED.
+  bool is_ready_to_use = 5;


Let's change this to ready_to_use

saad-ali · 2018-11-08T23:44:59Z

spec.md

+During the snapshot processing phase, since the snapshot is already cut, a `thaw` operation can be performed so application can start running without waiting for the process to complete.
+The `is_ready_to_use` parameter of the snapshot will become `true` after the process is complete.
+
+For cloud providers and storage systems that don't have the process, the `is_ready_to_use` parameter SHOULD be `true` after the snapshot is cut.


Change to:

For SPs that do not do additional processing after cut, the is_ready_to_use parameter SHOULD be true after the snapshot is cut.

saad-ali

/lgtm
/approve

This PR changes SnapshotStatus enum to a boolean is_ready_to_use and tries to use "processing" instead of "uploading" if possible to make it less specific.

xing-yang force-pushed the snapshot_status branch from 23eed7e to d417e19 Compare November 8, 2018 03:10

julian-hj approved these changes Nov 8, 2018

View reviewed changes

xing-yang force-pushed the snapshot_status branch 2 times, most recently from 8de19ce to 35c9d13 Compare November 8, 2018 17:52

saad-ali requested changes Nov 8, 2018

View reviewed changes

xing-yang commented Nov 8, 2018

View reviewed changes

xing-yang force-pushed the snapshot_status branch from a0c3de8 to 8d99c5f Compare November 8, 2018 20:19

saad-ali requested changes Nov 8, 2018

View reviewed changes

j-griffith reviewed Nov 8, 2018

View reviewed changes

saad-ali requested changes Nov 8, 2018

View reviewed changes

xing-yang force-pushed the snapshot_status branch 3 times, most recently from 7a51cbf to 0d1fd2a Compare November 9, 2018 03:55

saad-ali approved these changes Nov 9, 2018

View reviewed changes

jieyu approved these changes Nov 9, 2018

View reviewed changes

xing-yang added 4 commits November 9, 2018 12:05

Change SnapshotStatus enum to a boolean

d9fd839

This PR changes SnapshotStatus enum to a boolean is_ready_to_use and tries to use "processing" instead of "uploading" if possible to make it less specific.

Address review comments

a90cc88

Move the section about "is_ready_to_use" to under "CreateSnapshot"

82d56dc

Rename is_ready_to_use to ready_to_use

546096b

xing-yang force-pushed the snapshot_status branch from 0d1fd2a to 546096b Compare November 9, 2018 20:07

saad-ali merged commit 5d436c6 into container-storage-interface:master Nov 9, 2018

saad-ali mentioned this pull request Nov 9, 2018

Snapshot "Uploading" is too specific #299

Closed

c0depwn mentioned this pull request Nov 24, 2021

fix & adjust snapshotting logic flowswiss/csi-driver#2

Merged


		A snapshot could have the following statusus: UPLOADING, READY, and ERROR.
		##### The is_ready_to_use Parameter


		The SnapshotStatus parameter provides guidance to the CO on what action can be taken in the process of snapshotting.
		The `is_ready_to_use` parameter provides guidance to the CO on what action can be taken in the process of snapshotting.

Change SnapshotStatus enum to a boolean #307

Change SnapshotStatus enum to a boolean #307

Conversation

xing-yang commented Nov 8, 2018

julian-hj left a comment

Choose a reason for hiding this comment

xing-yang commented Nov 8, 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

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

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

xing-yang Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

xing-yang 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

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

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

saad-ali 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

saad-ali left a comment

Choose a reason for hiding this comment

xing-yang Nov 8, 2018 •

edited

Loading