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

Add Snapshot Support in CSI Spec #224

Merged
merged 8 commits into from May 16, 2018

Conversation

Projects
None yet
7 participants
@xing-yang
Contributor

xing-yang commented Apr 25, 2018

This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

  1. Create volume from snapshot: In this case, a new volume will be
    created on the storage backend and the data in the snapshot will
    be copied to the new volume. This case will be addressed by this
    proposal.

  2. Revert snapshot: In this case, data in the original volume
    will be erased and replaced with data in the snapshot. This case
    is out of scope for this proposal and should be handled by a
    function of its own (not to be mixed with create volume from
    snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:

  • Add CreateSnapshot
  • Add DeleteSnapshot
  • Add ListSnapshots
  • Modify CreateVolumeRequest to support creating a new volume from
    snapshot.
  • Modify ControllerGetCapabilitiesResponse to return
    CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
  • Creating volume from snapshot is a required functionality for plugins
    supporting CREATE_DELETE_SNAPSHOT.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big (possibly due to the large number of comments) and some people can't view it on Github, so close that one and submit a new PR here instead.

Closes #208

@cpuguy83

LGTM

@j-griffith

/LGTM
/APPROVE

@saad-ali

This comment has been minimized.

Member

saad-ali commented Apr 25, 2018

Woo I can actually open this without unicorn of death, thanks @xing-yang !

@j-griffith

LGTM thanks Xing!

@saad-ali

Largely LGTM. A few nit comments.

CO SHALL permit passing through the required secrets.
A CO MAY pass the same secrets to all RPCs, therefore the keys for all unique secrets that an SP expects must be unique across all CSI operations.
This information is sensitive and MUST be treated as such (not logged, etc.) by the CO.

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit: I prefer having this information on each field instead of in a common place because it makes it easy for a reader to discover the requirements when it is a comment on the field rather then an independent paragraph that they have to find.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

I moved this info to a common place to address comments from @julian-hj. He said the comments are getting very lengthy for each field and it's better to put the comments in a common place. I'm fine doing it either way.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

I actually like having them here, honestly.

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

I prefer having them here too :)

This comment has been minimized.

@jdef

jdef May 10, 2018

Member

@saad-ali would it help if the secrets field in the protos referred back to this section for requirements?

This comment has been minimized.

@saad-ali

saad-ali May 15, 2018

Member

@jdef That's a fair compromise. I would like the proto file to be self-documenting. If there is a requirement for some field, I want that to be quickly apparent from reading the comment rather then require user to go read the entire document. If we can have a pointer comment saying "see section x of spec for requirements on y", that should be fine -- since the reader will be aware that there are additional requirements that apply.

spec.md Outdated
// source_type fields MUST be specified. Only Snapshot is
// a supported VolumeContentSource at the moment.
message VolumeContentSource {
message Snapshot {

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

This is confusing, because there is already a top level (global) message Snapshot.

Maybe call it SnapshotSource instead of just Snapshot?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

spec.md Outdated
// This field is OPTIONAL.
map<string, string> controller_create_secrets = 5;
// Contains all attributes of the source that is used to create
// the volume from. This field is OPTIONAL.

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit:

// If specified, the new volume will be pre-populated with data from this source.
// This field is OPTIONAL.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Ok.

spec.md Outdated
@@ -699,6 +726,10 @@ message Volume {
// the same attributes. This field is OPTIONAL and when present MUST
// be passed to volume validation and publishing calls.
map<string, string> attributes = 3;
// All information about the source where the volume is created
// from. This field is OPTIONAL.

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit:

// If specified, indicates that the volume is not empty and is
// pre-populated with data from the specified source.
// This field is OPTIONAL.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Ok.

spec.md Outdated
// a volume from it. Therefore plugins supporting
// CREATE_DELETE_SNAPSHOT should also support
// CREATE_VOLUME_FROM_SNAPSHOT.
CREATE_VOLUME_FROM_SNAPSHOT = 7;

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

If this is a requirement for CREATE_DELETE_SNAPSHOT why have it as a separate capability?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

You are right. This is not necessary. I'll remove this one and change the spec accordingly.

spec.md Outdated
// The information about a provisioned snapshot.
message Snapshot {
// SnapshotSize is the disk size of the snapshot. The purpose of this

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit: disk size of the snapshot -> disk size of the restored snapshot?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Actually I changed this to "disk size of the snapshot" based on the Google Cloud API doc.

From this doc (https://cloud.google.com/compute/docs/reference/rest/v1/disks/createSnapshot), the output of createSnapshot has two sizes:
snapshot.diskSizeGb - "Size of the snapshot in GB."
snapshot.storageBytes - "A size of the storage used by the snapshot. As snapshots share storage, this number is expected to change with snapshot creation/deletion."

From here (https://cloud.google.com/compute/docs/reference/rest/v1/disks), there is a sizeGb in create volume:
sizeGb - "Size of the persistent disk, specified in GB. You can specify this field when creating a persistent disk using the sourceSnapshot parameter. The value of sizeGb must not be less than the size of the snapshot."

So our SnapshotSize is the same as snapshot.diskSizeGb - "Size of the snapshot in GB". I can change this just to "size of the snapshot".

This comment has been minimized.

@saad-ali

saad-ali Apr 27, 2018

Member

complete size of snapshot SGTM!

This comment has been minimized.

@xing-yang

xing-yang Apr 28, 2018

Contributor

Sound good! I changed it in the latest PR.

spec.md Outdated
// The status of a snapshot. A snapshot could have the following
// statuses:
// UPLOADING - A snapshot is cut and is now being uploaded.

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit: Move these detailed descriptions above each enum below -- having two sets of descriptions means that they may become out of sync over time.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

spec.md Outdated
#### `ListSnapshots`
A Controller Plugin MUST implement this RPC call if it has LIST_SNAPSHOTS capability.
The Plugin SHALL return the information about all the snapshots on the storage system regardless of how they were created.

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit: Except if snapshot_id is provided? Maybe word it so that like all snapshots on the storage system within the given parameters regardless of how they were created?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

}
message ListSnapshotsResponse {
message Entry {

This comment has been minimized.

@saad-ali

saad-ali Apr 25, 2018

Member

nit: Why wrap Snapshot object? I know ListVolumes does this as well. I can't remember why? @jdef do you remember?

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

For volumes, I think it we made it an entry in case we wanted to include additional information later on, like the currently attached node or something.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

I'll keep it this way to be consistent with ListVolumes.

@jdef

Nice job on this PR. I left a bunch of suggestions. Since I can't access the original PR I don't know if anything I'm mentioning has already been discussed and resolved - apologies in advance for any dups.

spec.md Outdated
// source_type fields MUST be specified. Only Snapshot is
// a supported VolumeContentSource at the moment.
message VolumeContentSource {
message Snapshot {

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

Maybe include a comment here (or else on the Snapshot snapshot field) that mentions controller capability CREATE_VOLUME_FROM_SNAPSHOT as a prerequisite?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

spec.md Outdated
}
// Specifies what source the volume will be created from. One of the
// source_type fields MUST be specified. Only Snapshot is

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

"Only Snapshot is a supported..." feels redundant given that its clearly the only supported type in the following message definition. Maybe remove this last sentence?

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Ok.

spec.md Outdated
LIST_SNAPSHOTS = 6;
// Currently the only way to consume a snapshot is to create
// a volume from it. Therefore plugins supporting
// CREATE_DELETE_SNAPSHOT should also support

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

"... should also ..." : if this is an official recommendation, then s/should/SHOULD/

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

spec.md Outdated
@@ -1074,6 +1058,16 @@ message ControllerServiceCapability {
PUBLISH_UNPUBLISH_VOLUME = 2;
LIST_VOLUMES = 3;
GET_CAPACITY = 4;
CREATE_DELETE_SNAPSHOT = 5;
// For storage systems that need to upload the snapshot after
// it is cut, LIST_SNAPSHOTS is a required RPC because CO needs

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

s/required/REQUIRED/

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Ok

spec.md Outdated
Secrets may be required by plugin to complete a RPC request.
A secret is a string to string map where the key identifies the name of the secret (e.g. "username" or "password"), and the value contains the secret data (e.g. "bob" or "abc123").
Each key MUST consist of alphanumeric characters, '-', '_' or '.'.
Each value MUST contain a valid string. An SP MAY choose to accept binary (non-string) data by using a binary-to-text encoding scheme, like base64.

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

nit: two sentences on the same line; please split

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

You are right. I'll split.

spec.md Outdated
// Note that CreateSnapshot no longer blocks after the snapshot is cut.
// Alternatively, another RPC ListSnapshots can be called with
// snapshot_id as filtering repeatedly to wait for the upload to
// complete. ListSnapshots should return immediately with the current

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

"... should return immediately..."

I mean, it's a blocking API. We don't know how long it will take. It shouldn't take very long, but I don't know why that's important to call out here.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

What I meant was that ListSnapshots should not wait for any snapshot to be created or deleted and will just return the snapshots it can find currently, but you are right that this could still take a long time if there are a large number of snapshots in the storage system. I'll reword this.

spec.md Outdated
A Controller Plugin MUST implement this RPC call if it has CREATE_DELETE_SNAPSHOT capability.
This RPC will be called by the CO to delete a snapshot.
If successful, the storage space associated with the snapshot MUST be released and all the data in the snapshot SHALL NOT be accessible anymore.

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

"... storage space associated with the snapshot MUST be released..." - this probably depends on how the storage backend reclaims storage space. is there a reason that we feel the need to declare this in the spec? for example, some backend systems might tombstone a snapshot, but it might not actually be deleted until some reaper (or compactor, or whatever) comes along and removes the data.

I see that we have similar language for DeleteVolume, perhaps we should create a ticket and address this language in a follow-up PR instead of in this one.

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Yes, I added this because it is also in DeleteVolume. I'll create a ticket.

This comment has been minimized.

@jdef
spec.md Outdated
#### `DeleteSnapshot`
A Controller Plugin MUST implement this RPC call if it has CREATE_DELETE_SNAPSHOT capability.

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

wrap in backticks? CREATE_DELETE_SNAPSHOT

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Ok.

spec.md Outdated
If successful, the storage space associated with the snapshot MUST be released and all the data in the snapshot SHALL NOT be accessible anymore.
This operation MUST be idempotent.
If a snapshot corresponding to the specified snapshot_id does not exist or the artifacts associated with the snapshot do not exist anymore, the Plugin MUST reply `0 OK`.

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

backticks? snapshot_id

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

spec.md Outdated
#### `ListSnapshots`
A Controller Plugin MUST implement this RPC call if it has LIST_SNAPSHOTS capability.

This comment has been minimized.

@jdef

jdef Apr 25, 2018

Member

backticks? LIST_SNAPSHOTS

This comment has been minimized.

@xing-yang

xing-yang Apr 25, 2018

Contributor

Sure.

@jdef

This comment has been minimized.

Member

jdef commented Apr 26, 2018

@jdef

This comment has been minimized.

Member

jdef commented Apr 26, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented Apr 26, 2018

@jdef I see. I'll use Int64Value then. Regarding capacity, I am aware that volume uses bytes. I actually specified "bytes" in the spec earlier but got a review comment asking me to remove it. So right now the spec just says "size of snapshot" without specifying a unit.

@jdef

This comment has been minimized.

Member

jdef commented Apr 26, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented Apr 26, 2018

@jdef Ok, I've added "bytes" in the comments for the size field. I'll address Int64Value in a followup PR.

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch from 483327c to 1f14ae4 Apr 26, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented Apr 26, 2018

@jdef @saad-ali I have addressed your comments. Please take look. Thanks!

spec.md Outdated
// Secrets required by plugin to complete snapshot creation request.
// This field is OPTIONAL.
map<string, string> create_snapshot_secrets = 3;

This comment has been minimized.

@dims

This comment has been minimized.

@xing-yang

xing-yang Apr 26, 2018

Contributor

The secret is used in many places in the CSI spec and it has always been map<string, string>. I don't know the reasoning behind map<string, string> vs map<string, bytes>. I think you can open an issue with the request to change it to map<string, bytes>.

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member
  • @saad-ali. IIRC, the rationale of using map<string, string> is to optimize the common use case where the secret is actually a string (e.g., passwd). For binary secrets, you can always encode it using base64.

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

Jie is correct. This has been debated several times in the past and the consensus among "owners" is to use string instead of bytes. This eliminates arguments over character encoding since that's defined by protobuf standards, and also addresses the common case where passwords actually consist of octets that naturally form human-readable strings.

// The suggested name for the snapshot. This field is REQUIRED for
// idempotency.
string name = 2;

This comment has been minimized.

@dims

dims Apr 26, 2018

@saad-ali @xing-yang

So here in CreateSnapshotRequest, we are REQUIRED to pass a name. and this better be a fresh one or the snapshot does not get created. Right?

If that's the case, we should be able to query the name(s) we used before .... However, ListSnapshotsResponse has a list of Snapshot object, BUT these objects do not have the name that was used when they were created.

So how do we keep track of the names that were already used?

This comment has been minimized.

@xing-yang

xing-yang Apr 26, 2018

Contributor

@dims The name in CreateSnapshotRequest works the same way as the name in CreateVolumeRequest. In create volume, name is unique and it will be saved in PV in K8S so we can find a mapping between the volume name and the id generated by the plugin in the PV source. CreateSnapshot will work the same way. Snapshot will get an ID after it is created by the plugin and when we call ListSnapshots we pass in the snapshot ID to find it.

This comment has been minimized.

@dims

dims Apr 26, 2018

@xing-yang the code that created the snapshot is the ONLY one who knows which name is mapped to which ID. in create volumes like you mentioned anyone can look in the PV to find the name and ID. that's not the case here

This comment has been minimized.

@xing-yang

xing-yang Apr 26, 2018

Contributor

@dims For snapshot, we have VolumeSnapshot which is equivalent to PVC and VolumeSnapshotData which is equivalent to PV. We also have data source for snapshot, similar to the data source for PV. However those design info will be in the design proposal for K8S, not here:). This is a CSI spec that involves other CO's in addition to K8S.

This comment has been minimized.

@dims

dims Apr 26, 2018

@xing-yang i am just pointing out that the API is not consistent. we cannot query what was a REQUIRED field during create. period. that's not a k8s problem, it's a problem with the API design. my initial comment still stands (and does not talk about k8s). I'll stop pestering :)

This comment has been minimized.

@xing-yang

xing-yang Apr 26, 2018

Contributor

@dims I heard you. Can you take a look of the "RPC Interactions" section? It explains that name may be different from resource id. Maybe you'll change your mind after reading that:)?

This comment has been minimized.

@xing-yang

xing-yang Apr 26, 2018

Contributor

From the "RPC Interactions" section, you can see that it was a design decision that the CO-generated volume name may be different from the plugin-generated volume_id. This PR for snapshot is consistent with that design.

spec.md Outdated
@@ -1103,6 +1313,46 @@ If a `CreateVolume` operation times out, leaving the CO without an ID with which
It is NOT REQUIRED for a controller plugin to implement the `LIST_VOLUMES` capability if it supports the `CREATE_DELETE_VOLUME` capability: the onus is upon the CO to take into consideration the full range of plugin capabilities before deciding how to proceed in the above scenario.
##### `CreateSnapshot`, `DeleteSnapshot`, `ListSnapshots`
The plugin-generated `snapshot_id` is a REQUIRED field for the `DeleteSnapshot` RPC, as opposed to the CO-generated snapshot `name` that is REQUIRED for the `CreateSnapshot` RPC: these fields MAY NOT contain the same value.

This comment has been minimized.

@dims

dims Apr 27, 2018

based on discussion above. may be we should make it clear

CO-generated snapshot `name` specified in CreateSnapshot cannot be looked up by `ListSnapshots`. It is a write only field and is not used anywhere else.

This comment has been minimized.

@xing-yang

xing-yang Apr 27, 2018

Contributor

Sure. I added some text to clarify this. Thanks!

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

this is a bit misleading. the text "... MAY NOT contain the same value." means this:

(a) the returned snapshot_id might be the same as the user-generated snapshot name
(b) .. or it could be completely different

to say that the ListSnapshots can't be filtered by the user-generated name only applies for snapshots that generate their own snapshot_id that's guaranteed to never match user-generated name. The sentence that was added below (SHALL NOT be used...) to resolve complaints is semantically inconsistent - please rewrite, or else remove it.

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch 2 times, most recently from 9f1381f to 103c6ea Apr 27, 2018

spec.md Outdated
}
// Specifies what source the volume will be created from. One of the
// source_type fields MUST be specified.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

nit: this field is now called type, right?

This comment has been minimized.

@xing-yang

xing-yang May 3, 2018

Contributor

Yes! Fixed.

spec.md Outdated
// create a volume from this snapshot. The size of the volume must not
// be less than the size of the source snapshot. This field is
// OPTIONAL. If this field is not set, it indicates that this size is
// unknown. The value of this field MUST NOT be negative.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

is zero a valid snapshot size? if so, what does it mean?

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

nits: Given that this field is of type SnapshotSize, please move the comment about negative or not to SnapshotSize message.

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

In the earlier version of this spec, we were also considering how much space is used by a snapshot so the used space is variable and can be zero in the beginning. To differentiate between the case when the snapshot space is not used vs the case when the storage system does not know the size, we decided to use a message to define that.

Now that we have removed the variable part and this snapshot size should be greater than zero if specified. I wonder if we should just use int64 for the size now and a size of 0 indicates the storage system does not know the size? That will be consistent with the volume size.

I'll move the comment close to the SnapshotSize message if we are keeping it.

spec.md Outdated
string source_volume_id = 3;
// Timestamp when the point-in-time snapshot is taken on the storage
// system. The format of this field should be a Unix Nanoseconds time

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

nit: why is Nanoseconds capitalized?

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

I'll make it lower case.

spec.md Outdated
// `thaw` can be done so the application can be running again if
// `freeze` was done before taking the snapshot.
UPLOADING = 2;
// A snapshot is in error status.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

is this a terminal status? as in, the snapshot can only be deleted from here?

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

If a snapshot ends up in error status, the user will need to find out what has caused the error condition and then determines what to do with it. If an error happens before the snapshot is cut, it needs to be deleted. If an error happens during the upload, may be the cloud admin has a way to fix the problem and still allow the snapshot to be used.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

is the user the only actor that can initiate recovery here? for example, are there ways that a CO could gracefully recovery from an ERROR condition and if so, what are they? (read: please document them if there are any)

This comment has been minimized.

@xing-yang

xing-yang May 10, 2018

Contributor

I checked with Jing on the upload case. If an error happens during upload, that is also a terminal status so the snapshot can be deleted. So that is consistent with the error before the snapshot is cut.

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.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

this is good information. the organization feels a bit clumsy. i might take a stab at crafting a PR against this branch to reorganize this section for better "flow"

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

Sure. Thanks:).

spec.md Outdated
Note that CreateSnapshot is a synchronous call and it must block until the snapshot is cut.
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 can continue to call CreateSnapshot while waiting for the upload to complete until the status becomes `READY`.

This comment has been minimized.

@jdef

jdef Apr 30, 2018

Member

suggest: The CO MAY continue to call...

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

Fixed.

spec.md Outdated
A Controller Plugin MUST implement this RPC call if it has CREATE_DELETE_SNAPSHOT capability.
This RPC will be called by the CO to delete a snapshot.
If successful, the storage space associated with the snapshot MUST be released and all the data in the snapshot SHALL NOT be accessible anymore.

This comment has been minimized.

@jdef
@jdef

Thanks for all the updates. Left a few more comments.

@jieyu

jieyu approved these changes May 2, 2018

Most LGTM! Modulo comments from other folks.

CO SHALL permit passing through the required secrets.
A CO MAY pass the same secrets to all RPCs, therefore the keys for all unique secrets that an SP expects must be unique across all CSI operations.
This information is sensitive and MUST be treated as such (not logged, etc.) by the CO.

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

I prefer having them here too :)

spec.md Outdated
// If specified, the new volume will be pre-populated with data from
// this source. This field is OPTIONAL.
VolumeContentSource content_source = 6;

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

Let's call it volume_content_source given that the context is not really clear that it's a content source for volume (similar to volume_capabilities in this request proto).

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

Sure. Changed.

spec.md Outdated
// snapshot.
CREATE_DELETE_SNAPSHOT = 5;
// For storage systems that need to upload the snapshot after
// it is cut, LIST_SNAPSHOTS is a REQUIRED RPC because CO needs

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

This needs a bit of rewording. It's more about that if this capabilities is not specified by the SP, the CO will simply assume that the snapshot is created (all steps, including uploading) once it receives a successful CreateSnapshotResponse.

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

Sure. Reworded a little bit. Let me know if it looks better.

spec.md Outdated
// Secrets required by plugin to complete snapshot creation request.
// This field is OPTIONAL.
map<string, string> create_snapshot_secrets = 3;

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member
  • @saad-ali. IIRC, the rationale of using map<string, string> is to optimize the common use case where the secret is actually a string (e.g., passwd). For binary secrets, you can always encode it using base64.
spec.md Outdated
message Snapshot {
// This is the complete size of the snapshot in bytes. The purpose of
// this field is to give CO guidance on how much space is needed to
// create a volume from this snapshot. The size of the volume must not

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

nits: Please use MUST NOT here.

This comment has been minimized.

@xing-yang

xing-yang May 4, 2018

Contributor

Fixed.

spec.md Outdated
// create a volume from this snapshot. The size of the volume must not
// be less than the size of the source snapshot. This field is
// OPTIONAL. If this field is not set, it indicates that this size is
// unknown. The value of this field MUST NOT be negative.

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

nits: Given that this field is of type SnapshotSize, please move the comment about negative or not to SnapshotSize message.

spec.md Outdated
// Timestamp when the point-in-time snapshot is taken on the storage
// system. The format of this field should be a Unix Nanoseconds time
// encoded as an int64. This field is REQUIRED.
int64 created_at = 4;

This comment has been minimized.

@jieyu

jieyu May 2, 2018

Member

Can you specify what will be the specification for the timestamp? Is there an RFC that we can reference?

This comment has been minimized.

@xing-yang

xing-yang May 6, 2018

Contributor

On Unix, if you enter “date +%s%N”, it returns the current time in nanoseconds since 1970-01-01 00:00:00 UTC.
$ date +%s%N
1525619388433753983
In Go, it is time.Now().UnixNano().

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch 2 times, most recently from ad73d64 to 6ccec55 May 4, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 6, 2018

/retest

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 7, 2018

@jieyu @jdef @saad-ali I have addressed your comments. Can you please take a look again? Thanks!

By the way, does anyone know how to re-trigger the CI?

@jdef

This comment has been minimized.

Member

jdef commented May 7, 2018

Re-triggering CI won't help, this needs #227 to land, and then to rebase

@jdef

additional commentary. we need resolution on what it means for a snapshot size to be zero.

spec.md Outdated
message SnapshotSize {
// The complete size of the snapshot in bytes.
// The value of this field MUST NOT be negative.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

It's still unclear to me what a value of "zero" means here. I think we need to either (a) disallow zero; (b) explain the meaning of zero, or else; (c) re-think using a message type for SnapshotSize and instead make zero == unspecified.

This comment has been minimized.

@xing-yang

xing-yang May 7, 2018

Contributor

I prefer option (c). That will be consistent with what "zero" means in volume size.

spec.md Outdated
If a snapshot corresponding to the specified snapshot `name` already exists and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.
A snapshot MAY be used as the source to provision a new volume.
CreateVolumeRequest is modified to take an OPTIONAL parameter of a source snapshot.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

nit: "is modified" only makes sense in the context of reviewing this PR. Once the PR lands, it sounds funny. Suggest instead: "A CreateVolumeRequest message may specify an OPTIONAL source snapshot parameter."

This comment has been minimized.

@xing-yang

xing-yang May 7, 2018

Contributor

Ok

spec.md Outdated
A snapshot MAY be used as the source to provision a new volume.
CreateVolumeRequest is modified to take an OPTIONAL parameter of a source snapshot.
Reverting snapshot, where data in the original volume is erased and replaced with data in the snapshot, is an advanced functionality not every storage system can support and therefore is currently out of scope.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

suggest: "Reverting a snapshot..."

This comment has been minimized.

@xing-yang

xing-yang May 7, 2018

Contributor

Ok

spec.md Outdated
// the provisioned snapshot.
string id = 2;
// Identity information for the source volume. Note that create

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

nit: s/create snapshot from snapshot/creating a snapshot from a snapshot/

This comment has been minimized.

@xing-yang

xing-yang May 7, 2018

Contributor

Ok

// Timestamp when the point-in-time snapshot is taken on the storage
// system. The format of this field should be a Unix nanoseconds time
// encoded as an int64. On Unix, the command `date +%s%N` returns the

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

thanks for this clarity

spec.md Outdated
// `thaw` can be done so the application can be running again if
// `freeze` was done before taking the snapshot.
UPLOADING = 2;
// A snapshot is in error status.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

is the user the only actor that can initiate recovery here? for example, are there ways that a CO could gracefully recovery from an ERROR condition and if so, what are they? (read: please document them if there are any)

spec.md Outdated
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` already exists and is compatible with the specified `source_volume_id` and `parameters` in the `CreateSnapshotRequest`, the Plugin MUST reply `0 OK` with the corresponding `CreateSnapshotResponse`.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

This is an important sentence: if the snapshot "exists" and the snapshot status is ERROR this RPC is still expected to return OK. Maybe we could make that more clear in the errors table that follows?

This comment has been minimized.

@xing-yang

xing-yang May 10, 2018

Contributor

If the snapshot "exists" and the snapshot status becomes ERROR, this RPC should be returning a gRPC error code, depending on what the error is. Currently we have the following error codes specified: 6 ALREADY_EXISTS; 10 ABORTED (Operation pending for snapshot); 12 UNIMPLEMENTED; and 13 RESOURCE_EXHAUSTED.

This comment has been minimized.

@xing-yang

xing-yang May 10, 2018

Contributor

The sentence you are referring to only applies if the snapshot has been successfully cut and uploaded (if upload is part of the process).

spec.md Outdated
@@ -1103,6 +1313,46 @@ If a `CreateVolume` operation times out, leaving the CO without an ID with which
It is NOT REQUIRED for a controller plugin to implement the `LIST_VOLUMES` capability if it supports the `CREATE_DELETE_VOLUME` capability: the onus is upon the CO to take into consideration the full range of plugin capabilities before deciding how to proceed in the above scenario.
##### `CreateSnapshot`, `DeleteSnapshot`, `ListSnapshots`
The plugin-generated `snapshot_id` is a REQUIRED field for the `DeleteSnapshot` RPC, as opposed to the CO-generated snapshot `name` that is REQUIRED for the `CreateSnapshot` RPC: these fields MAY NOT contain the same value.

This comment has been minimized.

@jdef

jdef May 7, 2018

Member

this is a bit misleading. the text "... MAY NOT contain the same value." means this:

(a) the returned snapshot_id might be the same as the user-generated snapshot name
(b) .. or it could be completely different

to say that the ListSnapshots can't be filtered by the user-generated name only applies for snapshots that generate their own snapshot_id that's guaranteed to never match user-generated name. The sentence that was added below (SHALL NOT be used...) to resolve complaints is semantically inconsistent - please rewrite, or else remove it.

@jdef

This comment has been minimized.

Member

jdef commented May 7, 2018

@xing-yang please stop squashing your commits down to 1 - that can be done as a last step. it's harder for reviewers to see what's changed between revisions when everything is continuously squashed. thanks!

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch from 96346c9 to be31d77 May 10, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 10, 2018

Merge conflicts are resolved now.

spec.md Outdated
ERROR = 3;
}
Type type = 1;
}

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

Is it worth adding a "message" or "details" string here that could give users additional information as to why the snapshot ended up in this failed state? Not having something like that seems like it might complicate debugging efforts. Not having such a field forces Alice to examine log files to determine why a snapshot attempt failed, instead of allowing the CO to surface the reason for the error to some more immediate UI.

I guess it's arguable that if the CO wants such a message, it should replay the CreateSnapshot RPC and examine the gRPC error that is returned. That's fine w/ me too (and doesn't require any additional changes to this PR). Thoughts?

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

I think having a "message" or "details" string would be very helpful for debugging. I'll add that.

spec.md Outdated
// Secrets required by plugin to complete snapshot creation request.
// This field is OPTIONAL.
map<string, string> create_snapshot_secrets = 3;

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

nit: wondering if we can come up with a better name for the secrets field here. create_snapshot_secrets stutters with the message type name. maybe controller_create_secrets?

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

I think someone (maybe Jie) suggested naming this field this way (as secrets for creating a snapshot). If you and him can agree with a new way of naming, I'll be happy to change it:).

spec.md Outdated
// Secrets required by plugin to complete snapshot deletion request.
// This field is OPTIONAL.
map<string, string> delete_snapshot_secrets = 2;

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

nit: ditto re: a better name for the secrets field. maybe controller_delete_secrets?

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

Same as above. I named it this way because I got an earlier comment to call this one as _snapshot_secrets.

spec.md Outdated
1. Execute the `ListSnapshots` RPC to possibly obtain a snapshot ID that may be used to execute a `DeleteSnapshot` RPC; upon success execute `DeleteSnapshot`.
2. The CO takes no further action regarding the timed out RPC, a snapshot is possibly leaked and the operator/user is expected to clean up.
It is NOT REQUIRED for a controller plugin to implement the `LIST_SNAPSHOTS` capability if it supports the CREATE_DELETE_SNAPSHOT capability: the CO needs to take into consideration the full range of plugin capabilities before deciding how to proceed in the above scenario.

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

I'm thinking about this statement more. If a plugin does not support LIST_SNAPSHOTS and also implements an "uploading" step, then .. in the event that an upload fails, there's no way for a CO to obtain a snapshot_id because the CreateSnapshot call with return a gRPC error (instead of a Snapshot w/ a SnapshotStatus). Is that acceptable?

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

In this case, CreateSnapshot can return a Snapshot with a Error status and a gRPC error code.

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch 2 times, most recently from 54b21e0 to a8911be May 11, 2018

spec.md Outdated
@@ -1183,6 +1191,10 @@ message SnapshotStatus {
ERROR = 3;
}
Type type = 1;

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

suggest: // This field is REQUIRED.

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

Done.

spec.md Outdated
@@ -1183,6 +1191,10 @@ message SnapshotStatus {
ERROR = 3;
}
Type type = 1;
// Additional information to describe why a snapshot ended up in the
// `ERROR` status.

This comment has been minimized.

@jdef

jdef May 11, 2018

Member

suggest: This field is OPTIONAL.

This comment has been minimized.

@xing-yang

xing-yang May 11, 2018

Contributor

Done.

@saad-ali

This comment has been minimized.

Member

saad-ali commented May 15, 2018

@xing-yang We spoke offline, please ping me when ready. I'd like to get this merged.

xing-yang added some commits Apr 25, 2018

Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.

@xing-yang xing-yang force-pushed the xing-yang:snapshot2 branch from b8b5c65 to 842bdbf May 15, 2018

@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 15, 2018

Rebased.

Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 15, 2018

Hi @saad-ali, I have rebased and addressed all the comments. Please take a look. Thanks!

Add Snapshot Support in CSI Spec
This PR proposes to add snapshot support in CSI specification.
It proposes to add 3 new controller service RPCs, including
CreateSnapshot, DeleteSnapshot, and ListSnapshots, and modify
the existing ControllerGetCapabilitiesResponse and
CreateVolumeRequest.

There are several cases for using the snapshot data in a volume:

1. Create volume from snapshot: In this case, a new volume will be
created on the storage backend and the data in the snapshot will
be copied to the new volume. This case will be addressed by this
proposal.

2. Revert snapshot: In this case, data in the original volume
will be erased and replaced with data in the snapshot. This case
is out of scope for this proposal and should be handled by a
function of its own (not to be mixed with create volume from
snapshot) if it is proposed in the future.

The following changes will be made in the Service Controller:
- Add CreateSnapshot
- Add DeleteSnapshot
- Add ListSnapshots
- Modify CreateVolumeRequest to support creating a new volume from
  snapshot.
- Modify ControllerGetCapabilitiesResponse to return
  CREATE_DELETE_SNAPSHOT and LIST_SNAPSHOTS support capabilities.
- Create volume from snapshot is a REQUIRED functionality if
  CREATE_DELETE_SNAPSHOT is supported.

This proposal was drafted here:
https://docs.google.com/document/d/1oXVuDTNhXerr1UJ48tQrxF9J6vbm2LHVVC3z28655Qo/edit#

The PR #207 is getting very big due to the large number of comments and some people
can't view it on Github, so close that one and submit a new PR here instead.
@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 16, 2018

@jdef @saad-ali I changed the status ERROR to ERROR_UPLOADING and added some clarifications based on what we discussed. Please take a look. Thanks!

@jdef

jdef approved these changes May 16, 2018

LGTM

@saad-ali

This comment has been minimized.

Member

saad-ali commented May 16, 2018

LGTM, and merging!!

@saad-ali saad-ali merged commit 6a1abdf into container-storage-interface:master May 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xing-yang

This comment has been minimized.

Contributor

xing-yang commented May 16, 2018

Awesome! Thanks everyone for reviewing the PR and helping us improve the design!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment