Add Snapshot APIs #6

xing-yang · 2018-07-12T17:58:15Z

This PR adds snapshot APIs as CRD and adds generated files.
The design doc is kubernetes/community#2335

jingxu97 · 2018-07-12T21:19:53Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// VolumeSnapshotConditionUploading means the snapshot is cut and the application
+	// can resume accessing data if ConditionStatus is True. It corresponds
+	// to "Uploading" in GCE PD or "Pending" in AWS and ConditionStatus is True.
+	// This condition type is not applicable in OpenStack Cinder.


maybe make this comment a little more generic. "Uploading" condition is only applicable to the volume plugins which support uploading snapshots to Cloud storage. For example, it corresponds to snapshot "UPLOADING" status in GCE and ...

I didn't see the change?

"VolumeSnapshotConditionUploading means the snapshot is cut" doesn't seem like a correct condition to monitor to determine if creation has completed (means the snapshot is cut and the application can resume accessing data), for a few reasons:

it appears to only apply to some volume plugins... if they don't upload data, this condition would never be true

it is transient... once uploaded, this condition would no longer be true

it is not necessarily mutually exclusive with "Creating"... a plugin could conceivably start streaming an upload while it was still in the process of creating the snapshot

it appears to only apply to some volume plugins... if they don't upload data, this condition would never be true

that's right

it is transient... once uploaded, this condition would no longer be true

right. Snapshot creation goes through a few phases: e.g., in GCE PD, we have CREATING, UPLOADING, and READY. Both creating and uploading are transient. Once that phase passes, the condition is no longer true.

it is not necessarily mutually exclusive with "Creating"... a plugin could conceivably start streaming an upload while it was still in the process of creating the snapshot

I think they are mutually exclusive. After the snapshot is cut, it is no longer in "creating" phase, and either go to "uploading" phase, or directly move to "ready" phase if no uploading is performed.

I read the updated condition guideline again, and have the following thoughts.

Condition should be considered as observations that users cared about instead of state-machine phases. For snapshot, user cares about whether snapshot is created successfully or not first. These can be represented by "ready" and "failed" conditions. The other condition user cares about is when the snapshot is taken (or cut). Because some plugins support uploading which might take a long time. User needs to know when the snapshot is taken (usually in a second) so that application can be resumed. So we could add another condition "snapshotTaken" (or just "Taken") condition. So now we have three conditions "snapshotTaken", "snapshotReady" and "snapshotFailed"

During the snapshot creation, we might see the following conditions at different times.

First when user create VolumeSnapshot object, there is no condition yet. (means controller is working on it, but there is no observed state available yet)

The volume plugin starts to create the snapshot. When the snapshot is taken (cut), controller will get a response and add "SnapshotTaken" condition to true. If snapshot is failed, it will be set to false. If there is no uploading phase, Ready condition will also be true right after snapshot is taken.

If there is an uploading phase, the controller will add "Ready" condition to true after uploading finishes. If uploading failed, the condition is set to false.

Please let me know if you have any feedback/comment about it. Thanks!

jingxu97 · 2018-07-12T21:20:36Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// to "Uploading" in GCE PD or "Pending" in AWS and ConditionStatus is True.
+	// This condition type is not applicable in OpenStack Cinder.
+	VolumeSnapshotConditionUploading VolumeSnapshotConditionType = "Uploading"
+	// VolumeSnapshotConditionReady is added when the snapshot has been successfully created and is ready to be used.


change "is added" to "means"

jingxu97 · 2018-07-12T21:22:24Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	VolumeSnapshotConditionError VolumeSnapshotConditionType = "Error"
+)
+
+// VolumeSnapshotCondition describes the state of a volume snapshot  at a certain point.


remove at a certain point.

jingxu97 · 2018-07-12T21:32:19Z

pkg/apis/volumesnapshot/v1alpha1/types.go

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


I think Spec should be required

PersistentVolumeClaimSpec is optional in PersistentVolumeClaim API object. Should we be consistent with that?

We discussed with @liggitt and we think it should be required also for PersistentVolumeClaimSpec.

jingxu97 · 2018-07-12T21:40:50Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+type VolumeSnapshotSpec struct {
+	// PersistentVolumeClaimName is the name of the PVC being snapshotted
+	// +optional
+	PersistentVolumeClaimName string `json:"persistentVolumeClaimName" protobuf:"bytes,1,opt,name=persistentVolumeClaimName"`


this is also a required field

I think by leaving this one optional, we can support static binding?

oh, I think you are right

jingxu97 · 2018-07-12T21:44:29Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
+
+	// Spec represents the desired state of the snapshot
+	// +optional


spec should be required?

I see that PersistentVolumeSpec is an optional field in PersistentVolume. So we should probably be consistent with that?

jingxu97 · 2018-07-12T21:45:25Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// +optional
+	metav1.ListMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
+
+	Items []VolumeSnapshotData `json:"items" protobuf:"bytes,2,rep,name=items"`


better to add comments to all fields?

jingxu97 · 2018-07-12T21:48:03Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// VolumeSnapshotRef is part of bi-directional binding between VolumeSnapshot
+	// and VolumeSnapshotData
+	// +optional
+	VolumeSnapshotRef *core_v1.ObjectReference `json:"volumeSnapshotRef" protobuf:"bytes,2,opt,name=volumeSnapshotRef"`


also required field?

I see that ClaimRef is an optional field in PersistentVolumeSpec. It becomes non-nil when bound. I'll add a comment here.

jingxu97 · 2018-07-12T21:53:19Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// PersistentVolumeRef represents the PersistentVolume that the snapshot has been
+	// taken from
+	// +optional
+	PersistentVolumeRef *core_v1.ObjectReference `json:"persistentVolumeRef" protobuf:"bytes,3,opt,name=persistentVolumeRef"`


required field?

Similarly, this becomes non-nil when bound.

jingxu97 · 2018-07-12T21:58:24Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	// Required.
+	SnapshotHandle string `json:"snapshotHandle"`
+
+	// Timestamp when the point-in-time snapshot is taken on the storage


it is not very clear that which time is used here.

This timestamp will be generated by the CSI driver after the snapshot is cut. I'll add this to the comment.

jingxu97 · 2018-07-12T22:04:52Z

cc @liggitt

jingxu97 · 2018-07-12T22:05:57Z

cc @saad-ali

liggitt · 2018-07-13T20:24:42Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+
+const (
+	// VolumeSnapshotDataResourcePlural is "volumesnapshotdatas"
+	VolumeSnapshotDataResourcePlural = "volumesnapshotdatas"


"datas" is not a correct plural

Do you have a suggestion for this? "data" is a plural itself. How do we differentiate between singular and plural if they are the same?

Looks like "volumesnapshotdatas" will be generated automatically for clientset when generating code for the CRD. So unless if we rename VolumeSnapshotData to something else, it will be "volumesnapshotdatas".

Looks like "volumesnapshotdatas" will be generated automatically for clientset when generating code for the CRD. So unless if we rename VolumeSnapshotData to something else, it will be "volumesnapshotdatas".

That's correct, and is a gap in the client generator. This has also been reported by others (see https://kubernetes.slack.com/archives/C0EG7JC6T/p1528416072000218). However, we shouldn't accept an incorrect API because of that... it would be better to participate in improving the generator to allow specifying the plural (or rename the VolumeSnapshotData type)

liggitt · 2018-07-13T20:30:34Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+// VolumeSnapshotConditionType is the type of VolumeSnapshot conditions
+type VolumeSnapshotConditionType string
+
+// These are valid conditions of a volume snapshot.


would recommend reviewing the updated condition guidelines in https://github.com/kubernetes/community/pull/2350/files

Changed VolumeSnapshotConditionType to "Created", "Available", and "Error" following today's discussion during the snapshot design meeting.

liggitt · 2018-07-13T21:14:28Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// VolumeSnapshotData represents the actual "on-disk" snapshot object
+type VolumeSnapshotData struct {


is the relationship between VolumeSnapshot and VolumeSnapshotData 1:1 or 1:*?

Renamed VolumeSnapshotData to VolumeSnapshotContent.

jingxu97 · 2018-07-13T22:33:11Z

cc @thockin

msau42 · 2018-07-18T17:59:53Z

pkg/apis/volumesnapshot/v1alpha1/register.go

+// addKnownTypes adds the set of types defined in this package to the supplied scheme.
+func addKnownTypes(scheme *runtime.Scheme) error {
+	scheme.AddKnownTypes(SchemeGroupVersion,
+		&SnapshotClass{},


To be consistent with all the objects, I think they should all start with either "Snapshot" or "VolumeSnapshot", but not a mix.

Ok, will change to VolumeSnapshotClass.

kangarlou · 2018-07-18T20:21:17Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// VolumeSnapshotContent represents the actual "on-disk" snapshot object
+type VolumeSnapshotContent struct {


I know we had some discussion on this in the last call. Wouldn't VolumeSnapshotClaim or VolumeSnapshotRequest as the user-facing object and VolumeSnapshot as the storage backend representation be better names? These names follow the PersistentVolumeClaim-PersistentVolume convention and can be pluralized.

Unfortunately, PersistentVolumeClaim does not follow the name pattern as all other user-facing objects. For example, we don't use PodClaim to represent user's pod. We think we should have named PVC and PV as PersistentVolume (user) and PersistentVolumeContent (system admin) at the first place. So instead of following the wrong pattern, we decided to use VolumeSnapshot to represent the user's snapshot.

PVCs don't follow other user-facing objects because other objects don't follow the dual-object model where we have a request and a response or a user-facing representation and an internal representation, but I agree with you that it's better not to follow the wrong pattern and have the more straightforward name (i.e., VolumeSnapshot) exposed to the users.

OK I'm late to to conversation so don't really have much right to input. I would prefer sticking to the PVC/PV pattern even if you belive it is wrong as it exists now. I personally don't belive it is incorrect as @kangarlou says nothing else follows the request response pattern. It will make users familiar with the PVC/PV pattern pick it up easier. All through reading it I was trying to map it to PVC/PV model.

sorry to reply late. Yes, naming is hard. But since we have been using these names for a while, we decided to continue use these names. The relationship between snapshot and its content is very similar to pvc/pv.

wackxu · 2018-07-19T09:18:33Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	CreatedAt int64 `json:"createdAt,omitempty" protobuf:"varint,3,opt,name=createdAt"`
+}
+
+// GetObjectKind is required to satisfy Object interface


the following is not need, so can remove these lines.

Sure. I'll take a look. Thanks.

xing-yang · 2018-08-02T22:47:49Z

pkg/apis/volumesnapshot/v1alpha1/types.go

-	// +optional
-	AvailableAt *metav1.Time `json:"availableAt" protobuf:"bytes,2,opt,name=availableAt"`
+	// Bound is set to true only if the snapshot is ready to use (e.g., finish uploading if
+	// there is an uplaoding phase) and also VolumeSnapshot andn its VolumeSnapshotContent


s/uplaoding/uploading

s/andn/and

xing-yang · 2018-08-02T22:49:26Z

pkg/apis/volumesnapshot/v1alpha1/types.go

@@ -93,15 +88,17 @@ type VolumeSnapshotList struct {

 // VolumeSnapshotSpec is the desired state of the volume snapshot
 type VolumeSnapshotSpec struct {
-	// PersistentVolumeClaimName is the name of the PVC being snapshotted
+	// PersistentVolumeClaimName is the name of the PVC being snapshotted.
+	// If not specified, user can create SnapshotContent and bind it with VolumeSnapshot manually.


s/SnapshotContent/VolumeSnapshotContent

xing-yang · 2018-08-03T00:14:20Z

pkg/apis/volumesnapshot/v1alpha1/types.go

-	// +optional
-	AvailableAt *metav1.Time `json:"availableAt" protobuf:"bytes,2,opt,name=availableAt"`
+	// Bound is set to true only if the snapshot is ready to use (e.g., finish uploading if
+	// there is an uploading phase) and also VolumeSnapshot andn its VolumeSnapshotContent


This PR adds generated files under pkg/client and vendor folder.

This PR adds snapshot APIs as CRD.

lpabon

lgtm!

saad-ali

/lgtm
/approve

saad-ali · 2018-08-14T00:02:15Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+// Represents the source from CSI volume snapshot
+type CSIVolumeSnapshotSource struct {
+	// Driver is the name of the driver to use for this snapshot.
+	// Required.


Please add:

// This MUST be the same name returned by the CSI GetPluginName() call for // that driver.

saad-ali · 2018-08-14T00:15:32Z

pkg/apis/volumesnapshot/v1alpha1/types.go

+	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
+
+	// Spec represents the desired state of the snapshot data
+	Spec VolumeSnapshotContentSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`


No status in VolumeSnapshotContent? No way to know if the object is bound or not?

We had status in VolumeSnapshotContent earlier on but decided to drop it because it is exactly the same as status in VolumeSnapshot. Now we just use "Ready" in VolumsSnapshotStatus to let user know it is bound and snapshot is ready for use.

k8s-ci-robot · 2018-08-14T00:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [saad-ali,xing-yang]

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

xing-yang changed the title ~~Snapshot apis~~ Add Snapshot APIs Jul 12, 2018

jingxu97 reviewed Jul 12, 2018

View reviewed changes

liggitt reviewed Jul 13, 2018

View reviewed changes

xing-yang force-pushed the snapshot_apis branch 4 times, most recently from 3c74892 to c14d975 Compare July 16, 2018 19:30

msau42 reviewed Jul 18, 2018

View reviewed changes

kangarlou reviewed Jul 18, 2018

View reviewed changes

wackxu reviewed Jul 19, 2018

View reviewed changes

jdumars mentioned this pull request Jul 20, 2018

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

Closed

xing-yang mentioned this pull request Jul 23, 2018

CSI snapshot design kubernetes/community#2335

Merged

xing-yang force-pushed the snapshot_apis branch 2 times, most recently from 0bc5a94 to a12ed3e Compare August 1, 2018 03:34

xing-yang mentioned this pull request Aug 1, 2018

Add Snapshot Controller #7

Merged

xing-yang commented Aug 2, 2018

View reviewed changes

jingxu97 force-pushed the snapshot_apis branch from 142a462 to d5d06d5 Compare August 3, 2018 00:07

xing-yang commented Aug 3, 2018

View reviewed changes

jingxu97 force-pushed the snapshot_apis branch from d5d06d5 to d298be4 Compare August 3, 2018 17:35

xing-yang added 2 commits August 10, 2018 06:20

Add generated file

e213d18

This PR adds generated files under pkg/client and vendor folder.

Add Snapshot APIs

3832c9d

This PR adds snapshot APIs as CRD.

xing-yang force-pushed the snapshot_apis branch from 8c7dcf1 to 3832c9d Compare August 10, 2018 13:23

lpabon approved these changes Aug 10, 2018

View reviewed changes

saad-ali approved these changes Aug 14, 2018

View reviewed changes

k8s-ci-robot assigned saad-ali Aug 14, 2018

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

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

saad-ali added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2018

k8s-ci-robot merged commit cc19fd3 into kubernetes-csi:master Aug 14, 2018

xing-yang mentioned this pull request Jan 21, 2019

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

Closed

6 tasks

xing-yang deleted the snapshot_apis branch April 8, 2019 02:06

Add Snapshot APIs #6

Add Snapshot APIs #6

Conversation

xing-yang commented Jul 12, 2018 • edited by jingxu97 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jul 13, 2018 • edited by jingxu97 Loading

Choose a reason for hiding this comment

jingxu97 Jul 13, 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

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 commented Jul 12, 2018

jingxu97 commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jul 13, 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

jingxu97 commented Jul 13, 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

garthy Jul 23, 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

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

lpabon left a comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 14, 2018

xing-yang commented Jul 12, 2018 •

edited by jingxu97

Loading

liggitt Jul 13, 2018 •

edited by jingxu97

Loading

jingxu97 Jul 13, 2018 •

edited

Loading

liggitt Jul 13, 2018 •

edited

Loading

garthy Jul 23, 2018 •

edited

Loading

xing-yang Aug 2, 2018 •

edited

Loading