Bring Volume Snapshot to Beta #1133

xing-yang · 2019-07-10T03:19:18Z

This KEP is for the volume snapshot feature.

xing-yang · 2019-07-10T03:19:45Z

msau42 · 2019-07-17T01:36:53Z

keps/sig-storage/20190709-csi-snapshot.md

+
+The rest of the document includes required information missing from the original design document: test plan and graduation criteria.
+
+## Test Plan


Do we also want to add a section for changes from the original design?

Added a section for changes since the original design doc. PTAL.

msau42 · 2019-07-17T01:37:12Z

/assign @jingxu97 @saad-ali

kacole2 · 2019-07-29T15:15:11Z

Is there a window of possibility this KEP will be merged and in an implementable state by EOD tomorrow? Enhancement Freeze is 7/30 and this is going to hinder this feature from being included in 1.16. Thanks

xing-yang · 2019-07-29T15:23:57Z

@kacole2 Yes, we are working on it. Thanks.

msau42 · 2019-07-29T16:26:25Z

keps/sig-storage/20190709-csi-snapshot.md

+
+## Changes
+
+Here are the changes since the original design proposal:


We should also describe the changes to the CRD lifecycle and controller management.

msau42 · 2019-07-29T16:26:52Z

keps/sig-storage/20190709-csi-snapshot.md

+* e2e tests for creating/deleting snapshot.
+* e2e tests for creating volume from snapshot.
+
+## Graduation Criteria


Also add one more section for implementation history for alpha

xing-yang · 2019-07-29T17:05:45Z

@msau42 your comments are addressed. PTAL.

msau42 · 2019-07-29T19:58:42Z

keps/sig-storage/20190709-csi-snapshot.md

+
+## Implementation History
+
+Volume snapshot is implemented as alpha feature in this repo:


Also add what release it was added as alpha

msau42 · 2019-07-29T20:00:25Z

keps/sig-storage/20190709-csi-snapshot.md

+* Added Finalizer on the snapshot source PVC to prevent it from being deleted when a snapshot is being created from it.
+* Added check to see whether ListSnapshots is supported by the CSI driver. If it is supported, ListSnapshots will be called to find out the status of a snapshot during static binding; otherwise it is assumed the snapshot ID provided by the admin is valid.
+* If snapshot creation times out, we should allow CSI driver to continue to finish the snapshot creation in the background. Timeout will happen at the foreground so VolumeSnapshot is marked as failed. VolumeSnapshotContent will be created but eventually be marked as failed because there is no VolumeSnapshot requesting for it. This work is on-going.
+* Investigation is on-going to determine whether we should separate common controller logic from other logic that belongs to the sidecar. Can be in the same external-snapshotter repo. The common controller should not be deployed with the driver. It should be deployed by the cluster deployer, or we can provide a way to deploy it as a separate Statefulset, not together with the driver.


I think for sure CRD installation should not be done as part the driver. So we should call that out and who is responsible for managing the CRD

can we classify three groups of people? Cluster admin, storage admin, and users?

jingxu97 · 2019-07-29T20:44:43Z

keps/sig-storage/20190709-csi-snapshot.md

+* Renamed `Ready` to `ReadyToUse` in the `Status` field of `VolumeSnapshot` API object.
+* Changed type of `RestoreSize` in `CSIVolumeSnapshotSource` from `*resource.Quantity`  to `*int64`.
+* Lease based Leader Election support is added.
+* Added `VolumeSnapshotContent` deletion policy.


Deletion policy can be specified in VolumeSnapshotClass, I think

Added clarification that it can be specified in VolumeSnapshotClass. We have it in both Content and Class. What I meant to say is that this is a policy for the content.

jingxu97 · 2019-07-29T20:54:57Z

keps/sig-storage/20190709-csi-snapshot.md

+* Added `VolumeSnapshot` and `VolumeSnapshotContent` in Use Protection using Finalizers.
+* Added Finalizer on the snapshot source PVC to prevent it from being deleted when a snapshot is being created from it.
+* Added check to see whether ListSnapshots is supported by the CSI driver. If it is supported, ListSnapshots will be called to find out the status of a snapshot during static binding; otherwise it is assumed the snapshot ID provided by the admin is valid.
+* If snapshot creation times out, we should allow CSI driver to continue to finish the snapshot creation in the background. Timeout will happen at the foreground so VolumeSnapshot is marked as failed. VolumeSnapshotContent will be created but eventually be marked as failed because there is no VolumeSnapshot requesting for it. This work is on-going.


I think for timeout, VolumeSnapshot status will not be marked as failed so that controller will continue to retry to create until the operation either succeeded or failed.

You are right. Fixed it.

xing-yang · 2019-07-29T21:21:23Z

@msau42 and @jingxu97, addressed your comments. PTAL. Thanks.

msau42 · 2019-07-30T15:39:49Z

/lgtm

jingxu97 · 2019-07-30T18:12:01Z

/assign @saad-ali

saad-ali · 2019-07-30T18:48:40Z

keps/sig-storage/20190709-csi-snapshot.md

+* Snapshot feature is used as a basic building block in other advanced applications. 
+* Feature deployed in production and have gone through at least one K8s upgrade.
+
+## Changes


Can we separate this list between what is already complete and what is in progress for beta so it is easier to understand current state vs what is being done.

saad-ali · 2019-07-30T18:49:19Z

keps/sig-storage/20190709-csi-snapshot.md

+  * Create/delete volume snapshots
+  * Create new volumes from a snapshot
+  * SnapshotContent Deletion/Retain Policy
+  * Snapshot Object in Use Protection


What about Beta API? Secrets? New controller?

xing-yang · 2019-07-30T20:04:04Z

Hi @saad-ali, addressed your comments. PTAL. Thanks!

saad-ali · 2019-07-30T20:51:04Z

keps/sig-storage/20190709-csi-snapshot.md

+### E2E tests
+
+* e2e tests for creating/deleting snapshot.
+* e2e tests for creating volume from snapshot.


Make those P0.

And add e2e tests for:

P1 delete/retain policy

P1 deleting API objects out of order (snapshot protection)

P2 test for secret fields

P2 tests for metrics

yuxiangqian · 2019-07-30T21:10:20Z

keps/sig-storage/20190709-csi-snapshot.md

+* If snapshot creation times out, VolumeSnapshot status will not be marked as failed so that controller will continue to retry to create until the operation either succeeds or fails. It is up to the user or an upper level application that uses the VolumeSnapshot to determine what to do with the snapshot. This work is on-going.
+* Investigation is on-going to determine whether we should separate common controller logic from other logic that belongs to the sidecar. Can be in the same external-snapshotter repo. The common controller should not be deployed with the driver. It should be deployed by the cluster deployer, or we can provide a way to deploy it as a separate Statefulset, not together with the driver. CRD installation should also be done by the cluster deployer.
+* Add secrets field to list-snapshots RPC in the CSI spec. Add “data-source-secret” in create-volume intended for accessing the data source. Implement them in external-snapshotter and external-provisioner.
+* Add metrics support in the snapshot controller.


should we make the list of metrics to-be-added more specific? I am thinking about following for BETA:

operational end to end latency metrics
labels:
a. operation_name, i.e., creation-snapshot, delete-snapshot
b. csi-driver-name

operation error count
labels:
a. operation_name, i.e., creation-snapshot, delete-snapshot
b. csi-driver-name

xing-yang · 2019-07-30T22:14:35Z

Hi @saad-ali and @yuxiangqian, your comments are addressed. PTAL. Thanks!

saad-ali

/lgtm
/approve

k8s-ci-robot · 2019-07-31T00:05:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, saad-ali, xing-yang

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:

~~keps/sig-storage/OWNERS~~ [saad-ali]

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

k8s-ci-robot requested review from childsb and saad-ali July 10, 2019 03:19

xing-yang force-pushed the snapshot_beta branch from 5648724 to f5f04f8 Compare July 10, 2019 03:26

xing-yang mentioned this pull request Jul 10, 2019

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

Closed

msau42 reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot assigned jingxu97 and saad-ali Jul 17, 2019

xing-yang force-pushed the snapshot_beta branch 3 times, most recently from 839f544 to f0139af Compare July 29, 2019 15:43

msau42 reviewed Jul 29, 2019

View reviewed changes

xing-yang force-pushed the snapshot_beta branch from f0139af to eec3201 Compare July 29, 2019 16:59

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2019

msau42 reviewed Jul 29, 2019

View reviewed changes

jingxu97 reviewed Jul 29, 2019

View reviewed changes

xing-yang force-pushed the snapshot_beta branch from eec3201 to 3c30ef5 Compare July 29, 2019 21:18

k8s-ci-robot assigned msau42 Jul 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

saad-ali requested changes Jul 30, 2019

View reviewed changes

xing-yang force-pushed the snapshot_beta branch from 3c30ef5 to df1081f Compare July 30, 2019 20:02

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

saad-ali requested changes Jul 30, 2019

View reviewed changes

yuxiangqian reviewed Jul 30, 2019

View reviewed changes

xing-yang force-pushed the snapshot_beta branch from df1081f to 8166012 Compare July 30, 2019 21:23

Bring Volume Snapshot to Beta

4c59f19

xing-yang force-pushed the snapshot_beta branch from 8166012 to 4c59f19 Compare July 30, 2019 21:27

saad-ali approved these changes Jul 31, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019

k8s-ci-robot merged commit 3e36f06 into kubernetes:master Jul 31, 2019


		The rest of the document includes required information missing from the original design document: test plan and graduation criteria.

		## Test Plan


		## Changes

		Here are the changes since the original design proposal:


		## Implementation History

		Volume snapshot is implemented as alpha feature in this repo:

Bring Volume Snapshot to Beta #1133

Bring Volume Snapshot to Beta #1133

Conversation

xing-yang commented Jul 10, 2019

xing-yang commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jul 17, 2019

kacole2 commented Jul 29, 2019

xing-yang commented Jul 29, 2019

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 commented Jul 29, 2019

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 commented Jul 29, 2019

msau42 commented Jul 30, 2019

jingxu97 commented Jul 30, 2019

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 commented Jul 30, 2019

saad-ali Jul 30, 2019 • edited

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 commented Jul 30, 2019

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 31, 2019

saad-ali Jul 30, 2019 •

edited