CSI snapshot design #2335

xing-yang · 2018-07-02T03:18:17Z

This document proposes to add Kubernetes snapshot support for CSI Volume Drivers. It proposes standardized snapshot APIs in the form of CRD, external snapshot controller to handle snapshotting operations, and a way to restore volumes from snapshots.

k8s-ci-robot · 2018-07-02T03:18:20Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jsafrane · 2018-07-02T07:14:39Z

/ok-to-test

jsafrane · 2018-07-02T13:28:42Z

contributors/design-proposals/storage/csi-snapshot.md

+
+**Authors:** [Jing Xu](https://github.com/jingxu97), [Xing Yang](https://github.com/xing-yang), [Tomas Smetana](https://github.com/tsmetana), [Huamin Chen ](https://github.com/rootfs)
+
+## Background


There should be some relation to the previous snapshot implementation. Will there be a supported path how to move from the old implementation to the new one?

The previous snapshot implementation is not for CSI. Is there a way to move PV/PVC from non-CSI drivers to CSI drivers? If there's a way, then maybe we can do similar move for snapshots. Maybe some kind of export/import?

I'll take a look of the migration of in-tree plugin to CSI spec. Maybe we can do something similar here.

I am not advocating for full backward compatibility, I think a paragraph or two about relation of these two implementations and perhaps high-level list of (manual) steps necessary to move from the old to the new one would be enough.

@jsafrane I added a section "Transition to the New Snapshot Support" to address your comments. Please take a look. Thanks.

jsafrane · 2018-07-02T13:37:57Z

contributors/design-proposals/storage/csi-snapshot.md

+}
+
+// Represents the actual location and type of the snapshot. Only one of its members may be specified.
+type VolumeSnapshotDataSource struct {


Since only CSI is going to be supported, this struct should probably contain only CSISnapshotSource:

type VolumeSnapshotDataSource struct { CSI *CSISnapshotSource } type CSISnapshotSource struct { ID string }

jsafrane · 2018-07-02T13:38:50Z

contributors/design-proposals/storage/csi-snapshot.md

+
+```
+
+type SnapshotClass struct {


Missing TypeMeta and ObjectMeta?

You are right.

jsafrane · 2018-07-02T13:40:28Z

contributors/design-proposals/storage/csi-snapshot.md

+
+type SnapshotClass struct {
+	// This value may not be empty.
+	Provisioner string


Since only CSI is going to be supported, should it just contain CSIDriver instead of Provisioner?

In addition, Provisioner is already used to define provisioner that provisions PVs. Should it be Snapshotter or something similar?

Ok, we can call it Snapshotter.

jsafrane · 2018-07-02T13:41:21Z

contributors/design-proposals/storage/csi-snapshot.md

+
+* External snapshotter is running in the sidecar along with external-attacher and external-provisioner for each CSI Volume Driver.
+
+* For dynamically created snapshot, it should have a SnapshotClass associated with it. User can explicitly specify a SnapshotClass in the VolumeSnapshot API object. If not, a default SnapshotClass will be used.


"a default SnapshotClass will be used." - how is the default SnapshotClass marked?

A default SnapshotClass should still be created ahead of time by the admin, similar to a default StorageClass. We'll add some clarifications.

jsafrane · 2018-07-02T13:44:01Z

contributors/design-proposals/storage/csi-snapshot.md

+
+* For dynamically created snapshot, it should have a SnapshotClass associated with it. User can explicitly specify a SnapshotClass in the VolumeSnapshot API object. If not, a default SnapshotClass will be used.
+
+* For statically binding snapshot, user/admin must specify the pointers in both directions (VolumeSnapshot → VolumeSnapshotData, and VolumeSnapshotData → VolumeSnapshot) so that controller will bind them.


"so that controller will bind them" - if both pointers are set, what exactly will the controller do to "bind them"? In PV-PVC case, these pointers are the binding and PV controller won't do anything when both pointers are set (except updating PV/PVC status to "Bound").

Ok. This will be re-worded.

jingxu97 · 2018-07-10T18:06:31Z

contributors/design-proposals/storage/csi-snapshot.md

+To use the snapshot implementation in [external storage repo](https://github.com/kubernetes-incubator/external-storage/tree/master/snapshot), an external snapshot controller and external provisioner need to bedeployed.
+
+To use the new snapshot implementation for CSI, a sidecar container for the external snapshot controller needs to be deployed in addition to the sidecar container for the external provisioner.
+


Maybe put the deployment differences you mentioned at the beginning into the following section "New design" "Old design" too?

Just say something like "The following lists the main differences between this snapshot proposal and the the old snapshot support in external storage repo"?

Or we have two part, the first part talks about the user facing differences, the second part talks about deployment differences.

wackxu · 2018-07-11T01:50:46Z

contributors/design-proposals/storage/csi-snapshot.md

+// the VolumeSnapshotSpec
+type VolumeSnapshot struct {
+	metav1.TypeMeta `json:",inline"`
+	Metadata        metav1.ObjectMeta `json:"metadata"`


maybe

metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

wackxu · 2018-07-11T01:51:44Z

contributors/design-proposals/storage/csi-snapshot.md

+
+type VolumeSnapshotList struct {
+	metav1.TypeMeta `json:",inline"`
+	Metadata        metav1.ListMeta  `json:"metadata"`


metav1.ListMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

wackxu · 2018-07-11T01:57:34Z

contributors/design-proposals/storage/csi-snapshot.md

+const (
+        // VolumeSnapshotConditionCreating means the snapshot is being created but
+        // it is not cut yet.
+        VolumeSnapshotConditionCreating VolumeSnapshotConditionType = "Creating"


In CSI spec, we only have four type for SnapshotStatus_Type,

SnapshotStatus_UNKNOWN SnapshotStatus_Type = 0 // A snapshot is ready for use. SnapshotStatus_READY SnapshotStatus_Type = 1 // A snapshot is cut and is now being uploaded. // Some cloud providers and storage systems uploads the snapshot // to the cloud after the snapshot is cut. During this phase, // `thaw` can be done so the application can be running again if // `freeze` was done before taking the snapshot. SnapshotStatus_UPLOADING SnapshotStatus_Type = 2 // An error occurred during the snapshot uploading process. // This error status is specific for uploading because // `CreateSnaphot` is a blocking call before the snapshot is // cut and therefore it SHOULD NOT come back with an error // status when an error occurs. Instead a gRPC error code SHALL // be returned by `CreateSnapshot` when an error occurs before // a snapshot is cut. SnapshotStatus_ERROR_UPLOADING SnapshotStatus_Type = 3

what type can map to Creating?

API needs to return immediately while CSI driver call is blocking until the snapshot is cut. That's why we need a "Creating" status for API. It might map to UNKNOWN in CSI.

wackxu · 2018-07-11T01:58:50Z

contributors/design-proposals/storage/csi-snapshot.md

+type VolumeSnapshotData struct {
+	metav1.TypeMeta `json:",inline"`
+	// +optional
+	Metadata metav1.ObjectMeta `json:"metadata"`


metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

wackxu · 2018-07-11T02:03:48Z

contributors/design-proposals/storage/csi-snapshot.md

+        Items           []VolumeSnapshotData `json:"items"`
+}
+
+// The desired state of the volume snapshot


s/volume snapshot/volume snapshot data/

wackxu · 2018-07-11T02:06:43Z

contributors/design-proposals/storage/csi-snapshot.md

+
+```GO
+
+apiVersion: volumesnapshot.external-storage.k8s.io/v1


maybe volumesnapshot.csi.k8s.io/v1alpha1

This is an example of the old design from external-storage.

wackxu · 2018-07-11T02:08:56Z

contributors/design-proposals/storage/csi-snapshot.md

+
+	// Spec represents the desired state of the snapshot
+	// +optional
+	Spec VolumeSnapshotDataSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`


Do we need status for VolumeSnapshotData ?

Since we are not binding in the in-tree controller any more, we don't need a status for VolumeSnapshotData any more because external-snapshotter will be handling the status creation and binding.

wackxu · 2018-07-11T02:09:51Z

contributors/design-proposals/storage/csi-snapshot.md

+type SnapshotClass struct {
+        metav1.TypeMeta `json:",inline"`
+        // +optional
+        Metadata metav1.ObjectMeta `json:"metadata"`


metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

wackxu · 2018-07-11T02:11:56Z

contributors/design-proposals/storage/csi-snapshot.md

+kind: SnapshotClass
+metadata:
+  name: csi-hostpath-snapclass
+  snapshotter: csi-hostpath


the format maybe wrong, remove the white space

Ok, I'll check.

humblec · 2018-07-16T16:10:52Z

contributors/design-proposals/storage/csi-snapshot.md

+// +nonNamespaced=true
+
+// VolumeSnapshotData represents the actual "on-disk" snapshot object
+type VolumeSnapshotData struct {


As discussed in today's design meeting, VolumeSnapshotContent looks better than VolumeSnapshotData.

jsafrane · 2018-07-17T06:35:43Z

contributors/design-proposals/storage/csi-snapshot.md

+	// If the timestamp CreateAt is set, it means the snapshot was created;
+	// Otherwise the snapshot was not created.
+        // +optional
+        Created VolumeSnapshotCreated


Since it's optional, should it be a pointer?

if snap.Status.Created == nil { }

vs.

if snap.Status.Created.CreatedAt.IsZero() { }

Why do we need this to be a struct at all? The "IsZero()" test would work with the plain timestamp too and should be sufficient to figure out if the snapshot exists and since when.

Since we only have timestamp in the struct now, it is possible to get rid of the struct. A struct would allow us to add more parameters to it later if needed.

I think that's a good suggestion. We can make it like this

Type VolumeSnapshotStatus struct { CreatedAt *metav1.Time AvailableAt *metav1.Time FailedAt *metav1.Time Reason string Message string }

garthy · 2018-07-23T10:10:21Z

contributors/design-proposals/storage/csi-snapshot.md

+        // +optional
+        PVCDataSourceRef *core_v1.LocalObjectReference `json:"dataSourceRef" protobuf:"bytes,2,opt,name=dataSourceRef"`
+
+}


Would a v1.ObjectReference be better? This also allows sources to be in other namespaces.

The intention is to only allow sources in the same namespace to be used.

Why is this restriction in place? Is it a technical reason or something else?

It is for technical reason. VolumeSnapshot itself is name-spaced.

garthy · 2018-07-23T10:11:22Z

contributors/design-proposals/storage/csi-snapshot.md

+```
+
+In the first version, only VolumeSnapshot will be a supported `Type` for data source object reference. PersistentVolumeClaim will be added in a future version. If unsupported `Type` is used, the PV Controller SHALL bail out of the operation.
+


bail out -> fail

garthy · 2018-07-23T10:13:11Z

contributors/design-proposals/storage/csi-snapshot.md

+#### Add `DataSource` to `StorageClass`
+
+DataSource will be added as an optional parameter in `StorageClass` to allow volumes belonging to the same `StorageClass` to be created with prepopulated data.
+


This leans towards making the LocalObjectReference an ObjectReference as it supports shared sources in a public namespace usable my other in multiple other namespaces.

You are right. We'll need to discuss about this one. May be we need to add some limitations here.

garthy · 2018-07-23T10:18:23Z

contributors/design-proposals/storage/csi-snapshot.md

+ListSnapshots can be an expensive operation because it will try to list all snapshots on the storage system. For a storage system that takes nightly periodic snapshots, the total number of snapshots on the system can be huge. Kubernetes should try to avoid this call if possible. Instead, calling ListSnapshots with a specific snapshot_id as filtering to query the status of the snapshot will be more desirable and efficient.
+
+CreateSnapshot is a synchronous function and it must be blocking until the snapshot is cut. For cloud providers that support the uploading of a snapshot as part of the creating snapshot operation, CreateSnapshot function must also be blocking until the snapshot is cut and after that it shall return an operation pending gRPC error code until the uploading process is complete.
+


You mention that is is a "synchronous function" but then state that operation pending will be returned until the upload is complete.
Does this mean that while a snapshot is in progress on a volume you can't take another snapshot until the current one has finished? (In some cases uploadin to cloud?)
This seems overly restrictive.

"Synchronous" refers to the CSI driver interface part. Actually all the volume operations such as create snapshot, create volume are synchronous in CSI. You can still take another snapshot while the current one is in progress. If you call create snapshot with the same input parameters while the current one is in progress (pending), the call will be pending as well because it is idempotent.

thockin

First pass is done. I am sure there will be more once this is all resolved.

thockin · 2018-07-30T23:57:51Z

contributors/design-proposals/storage/csi-snapshot.md

+
+```GO
+
+// The volume snapshot object accessible to the user. Upon successful creation of the actual


Comment wise this doesn't explain enough. I hate to nit-pick on language, but this has to be clear.

Now changed it to

// VolumeSnapshot is a user's request for taking a snapshot. Upon successful creation of the actual
// snapshot by the volume provider it is bound to the corresponding VolumeSnapshotContent.
// Only the VolumeSnapshot object is accessible to the users in their namespaces.

After we add snapshot document files, we can also have something like this

//More info: https://kubernetes.io/docs/concepts/storage/volumesnapshot

thockin · 2018-07-30T23:58:12Z

contributors/design-proposals/storage/csi-snapshot.md

+	// Spec represents the desired state of the snapshot
+	Spec VolumeSnapshotSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`
+
+	// Status represents the latest observer state of the snapshot


s/observer/observed

thockin · 2018-07-31T00:01:33Z

contributors/design-proposals/storage/csi-snapshot.md

+
+// VolumeSnapshotSpec is the desired state of the volume snapshot
+type VolumeSnapshotSpec struct {
+        // PersistentVolumeClaimName is the name of the PVC being snapshotted


Missing:

What happens if this is not specified when creating a VolumeSnapshot?

What happens if the named PVC does not exist?

Does this check authz? You need to be sure that the calling user is allowed to read the PVC.

volumesnapshot creations supports two types: dynamically creation of volumesnapshot and statically binding of snapshot and snapshotContent. In case of dynamically creation, PVCName must be provided. For statically binding, PVC can be nil, and user has to create VolumeSnapshotContent and bind it with VolumeSnapshot. The details is explained in controller design section.

thockin · 2018-07-31T00:04:45Z

contributors/design-proposals/storage/csi-snapshot.md

+        // +optional
+        PersistentVolumeClaimName string `json:"persistentVolumeClaimName" protobuf:"bytes,1,opt,name=persistentVolumeClaimName"`
+
+        // SnapshotContentName binds the VolumeSnapshot object with the VolumeSnapshotContent


What if the user specifies this when creating? Is it allowed? What does it mean? Can multiple snapshots bind to the same content?

What if the named object doesn't exist?

Does this check authz? What if I am not allowed to read a content? Can I restore from it? That seems bad.

It is allowed for user to provide SnapshotContentName to support statically binding. Snapshot and SnapshoContent is 1 to 1 mapping which is similar to PVC and PV. SnapshotContent is non-namespaced. The details is explained in controller design section.

thockin · 2018-07-31T00:06:00Z

contributors/design-proposals/storage/csi-snapshot.md

+        // +optional
+        SnapshotContentName string `json:"snapshotContentName" protobuf:"bytes,2,opt,name=snapshotContentName"`
+
+        // Name of the VolumeSnapshotClass required by the volume snapshot.


This says "required by the volume snapshot" and then says it is optional.

What if it is not specified when creating?

What if the named class doesn't exist?

The comments here follows the one from StorageClass. I changed it to "// Name of the VolumeSnapshotClass used by the VolumeSnapshot. If not specified, a default snapshot class will be used if it is available.

thockin · 2018-07-31T20:58:21Z

contributors/design-proposals/storage/csi-snapshot.md

+        // not empty. The maximum number of parameters is
+        // 512, with a cumulative max size of 256K
+        // +optional
+	Parameters map[string]string


thockin · 2018-07-31T20:59:26Z

contributors/design-proposals/storage/csi-snapshot.md

+```
+
+type PersistentVolumeClaimSpec struct {
+        // If specified, volume will be prepopulated with data from the PVCDataSourceRef.


If specified when creating...

Document and validate that updates to this field are not allowed

comment says "PVCDataSourceRef" but field is "DataSourceRef"

thockin · 2018-07-31T21:01:40Z

contributors/design-proposals/storage/csi-snapshot.md

+        DataSourceRef *TypedLocalObjectReference `json:"dataSourceRef" protobuf:"bytes,2,opt,name=dataSourceRef"`
+}
+
+type PersistentVolumeSpec struct {


Do we really need this? Is it not sufficient for it to be in the PVC?

if so, same comments as above

What if the PVC and PV have different sources?

Is this going to be used as part of the PV<->PVC match logic?

We put this in both PVC and PV so that controller can check if there is any inconsistency. One use case is that if the provisioner is the older version and cannot recognize the datasource field, it will create an empty volume. The controller could check that PVC has a datasource and PV does not have it and put an error to it.

thockin · 2018-07-31T21:01:48Z

contributors/design-proposals/storage/csi-snapshot.md

+type TypedLocalObjectReference struct {
+        // Name of the object reference.
+        Name string
+	// Kind indicates the type of the object reference.


thockin · 2018-07-31T21:05:30Z

contributors/design-proposals/storage/csi-snapshot.md

+type StorageClass struct {
+        // If specified, volume will be prepouplated with data from the DataSource
+        // +optional
+        DataSource TypedLocalObjectReference


Do we really need this? How common is it?

What if this and the PVC both specify source, and they disagree?

For all of these source fields - what happens if I point them at a nonsensical kind, like "Service" or at a kind that doesn't exist? What if I point them at a valid Kind but the named instance doesn't exist. they ALL need to document this.

I think in the first version, we don't need to add it in StorgeClass. We can add it later when there is a need for it.

garthy · 2018-08-07T10:33:50Z

contributors/design-proposals/storage/csi-snapshot.md

+	// Setting Bound to true means that the snapshot is created (and uploaded for cloud providers) sucuessfully, and 
+	// VolumeSnapshotContent is created and bound with the VolumeSnapshot. After Bound becomes true, users
+	// can start to use VolumeSnapshot to provision new volumes.
+	Bound bool `json:"bound" protobuf:"varint,4,opt,name=bound"`


Can we rename this to ready? Arguably the snapshot is bound when it is created it is just not read to be use.

bound to me implies that something is associated which is true once the snapshot is created. ready seem more appropriate as the volume is ready to use.

I am totally fine with "Ready". In snapshot meeting, we talked about this name. Propose to use "Bound" to be consistent with PVC/PV states. But since snapshot has this extra phase of from being created and ready, I think it is ok to use "ready" to be more clear

This PR proposes to add snapshot support for CSI Volume Drivers and provide a way to restore volumes from snapshots.

change VolumeSnapshotClassName as pointer

thockin · 2018-08-24T17:56:29Z

contributors/design-proposals/storage/csi-snapshot.md

+        DataSource *TypedLocalObjectReference `json:"dataSource" protobuf:"bytes,2,opt,name=dataSource"`
+}
+
+type PersistentVolumeSpec struct {


I thought we agreed we didn't need this any more?

yes, just removed it. Thank you!

thockin · 2018-08-27T16:34:29Z

contributors/design-proposals/storage/csi-snapshot.md

+	// +optional
+	Source *TypedLocalObjectReference `json:"source" protobuf:"bytes,1,opt,name=source"`
+
+	// SnapshotContentName binds the VolumeSnapshot object with the VolumeSnapshotContent


We will need detailed docs for what happens in all the various corner-cases. e.g. what if a user specifies this field?

thockin · 2018-08-27T16:36:42Z

contributors/design-proposals/storage/csi-snapshot.md

+
+```
+type PersistentVolumeClaimSpec struct {
+        // If specified when creating, volume will be prepopulated with data from the DataSource.


In code this has to be more detailed. What types are supported, what happens if the type specified is not supported? Who decides what is supported...

thockin · 2018-08-27T16:37:22Z

/lgtm
/approve

k8s-ci-robot · 2018-08-27T16:37:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contributors/design-proposals/storage/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

CSI snapshot design

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 2, 2018

k8s-ci-robot requested review from childsb and saad-ali July 2, 2018 03:18

xing-yang force-pushed the csi_snapshot branch from 2761b5f to 79ba906 Compare July 2, 2018 03:33

This was referenced Jul 2, 2018

Add Volume Snapshot Support #1662

Closed

Snapshot Support for Kubernetes (CRD + External Controller) kubernetes/enhancements#543

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 2, 2018

jsafrane reviewed Jul 2, 2018

View reviewed changes

jingxu97 reviewed Jul 10, 2018

View reviewed changes

wackxu reviewed Jul 11, 2018

View reviewed changes

xing-yang force-pushed the csi_snapshot branch 2 times, most recently from a9528e3 to b331712 Compare July 15, 2018 16:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 15, 2018

humblec reviewed Jul 16, 2018

View reviewed changes

xing-yang force-pushed the csi_snapshot branch 3 times, most recently from 2a14bdf to 0b6954c Compare July 17, 2018 03:01

jsafrane reviewed Jul 17, 2018

View reviewed changes

xing-yang force-pushed the csi_snapshot branch 3 times, most recently from 34da5bb to cc6b041 Compare July 19, 2018 07:29

jdumars mentioned this pull request Jul 20, 2018

API Review: storage snapshot CRD binding review kubernetes-retired/architecture-tracking#44

Closed

garthy reviewed Jul 23, 2018

View reviewed changes

xing-yang force-pushed the csi_snapshot branch 3 times, most recently from 83218d4 to aa139f3 Compare July 23, 2018 19:11

jingxu97 mentioned this pull request Jul 28, 2018

Snapshot / Restore Volume Support for Kubernetes (CRD + External Controller) kubernetes/enhancements#177

Closed

thockin reviewed Jul 31, 2018

View reviewed changes

garthy reviewed Aug 7, 2018

View reviewed changes

This was referenced Aug 7, 2018

Add DataSource and TypedLocalObjectReference kubernetes/kubernetes#67087

Merged

Add Snapshot APIs kubernetes-csi/external-snapshotter#6

Merged

xing-yang force-pushed the csi_snapshot branch 2 times, most recently from 97b863c to f2291ef Compare August 11, 2018 20:18

jingxu97 mentioned this pull request Aug 20, 2018

Add data source for volume provisioning #2495

Merged

xing-yang and others added 4 commits August 22, 2018 18:06

Add Design Spec for CSI Snapshot

5fcba27

This PR proposes to add snapshot support for CSI Volume Drivers and provide a way to restore volumes from snapshots.

Update csi-snapshot.md

6268b50

Update csi-snapshot.md

766ec70

Fixed some typos.

4b0fb4c

xing-yang force-pushed the csi_snapshot branch from 619cf13 to 4b0fb4c Compare August 23, 2018 01:12

jingxu97 added 2 commits August 23, 2018 12:02

Update csi-snapshot.md

127a5b7

change VolumeSnapshotClassName as pointer

Update csi-snapshot.md

7eb005c

thockin reviewed Aug 24, 2018

View reviewed changes

jingxu97 added 2 commits August 24, 2018 11:30

Update csi-snapshot.md

71b354d

Update csi-snapshot.md

7724986

thockin reviewed Aug 27, 2018

View reviewed changes

k8s-ci-robot assigned thockin Aug 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2018

k8s-ci-robot merged commit 9adb191 into kubernetes:master Aug 27, 2018

xing-yang mentioned this pull request Jan 21, 2019

REQUEST: New membership for xing-yang kubernetes/org#393

Closed

6 tasks

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#2335 from xing-yang/csi_snapshot

db02180

CSI snapshot design


		Authors: [Jing Xu](https://github.com/jingxu97), [Xing Yang](https://github.com/xing-yang), [Tomas Smetana](https://github.com/tsmetana), [Huamin Chen ](https://github.com/rootfs)

		## Background


		* External snapshotter is running in the sidecar along with external-attacher and external-provisioner for each CSI Volume Driver.

		* For dynamically created snapshot, it should have a SnapshotClass associated with it. User can explicitly specify a SnapshotClass in the VolumeSnapshot API object. If not, a default SnapshotClass will be used.


		* For dynamically created snapshot, it should have a SnapshotClass associated with it. User can explicitly specify a SnapshotClass in the VolumeSnapshot API object. If not, a default SnapshotClass will be used.

		* For statically binding snapshot, user/admin must specify the pointers in both directions (VolumeSnapshot → VolumeSnapshotData, and VolumeSnapshotData → VolumeSnapshot) so that controller will bind them.

		To use the snapshot implementation in [external storage repo](https://github.com/kubernetes-incubator/external-storage/tree/master/snapshot), an external snapshot controller and external provisioner need to bedeployed.

		To use the new snapshot implementation for CSI, a sidecar container for the external snapshot controller needs to be deployed in addition to the sidecar container for the external provisioner.

		```

		In the first version, only VolumeSnapshot will be a supported `Type` for data source object reference. PersistentVolumeClaim will be added in a future version. If unsupported `Type` is used, the PV Controller SHALL bail out of the operation.

		#### Add `DataSource` to `StorageClass`

		DataSource will be added as an optional parameter in `StorageClass` to allow volumes belonging to the same `StorageClass` to be created with prepopulated data.

		ListSnapshots can be an expensive operation because it will try to list all snapshots on the storage system. For a storage system that takes nightly periodic snapshots, the total number of snapshots on the system can be huge. Kubernetes should try to avoid this call if possible. Instead, calling ListSnapshots with a specific snapshot_id as filtering to query the status of the snapshot will be more desirable and efficient.

		CreateSnapshot is a synchronous function and it must be blocking until the snapshot is cut. For cloud providers that support the uploading of a snapshot as part of the creating snapshot operation, CreateSnapshot function must also be blocking until the snapshot is cut and after that it shall return an operation pending gRPC error code until the uploading process is complete.


		```GO

		// The volume snapshot object accessible to the user. Upon successful creation of the actual

CSI snapshot design #2335

CSI snapshot design #2335

Conversation

xing-yang commented Jul 2, 2018

k8s-ci-robot commented Jul 2, 2018

jsafrane commented Jul 2, 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

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

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

thockin 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

jingxu97 Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 Aug 2, 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

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

jingxu97 Aug 2, 2018 •

edited

Loading

jingxu97 Aug 2, 2018 •

edited

Loading