Add Snapshot Controller #7

xing-yang · 2018-07-12T18:04:16Z

This PR adds external snapshot controller and main package under cmd.

Which issue(s) this PR fixes:
Fixes #
kubernetes/enhancements#177

jingxu97 · 2018-07-12T22:13:31Z

pkg/connection/connection.go

+	}
+
+	// Create VolumeSnapshotData in the database
+	snapshotData := &crdv1.VolumeSnapshotData{


I feel it is better to create VolumeSnapshotData object in the controller, not in connection code.

jingxu97 · 2018-07-12T23:28:00Z

pkg/controller/util.go

+	return true, nil
+}
+
+func GenSnapshotStatus(snapshotCon *crdv1.VolumeSnapshotCondition) *crdv1.VolumeSnapshotCondition {


what is the purpose of this function?

This makes a copy of of the Condition. Probably not needed. We can use DeepCopy.

jingxu97 · 2018-07-12T23:33:57Z

cmd/csi-snapshotter/main.go

+		},
+	}
+
+	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(vs)


remove this part?

jingxu97 · 2018-07-12T23:35:48Z

cmd/csi-snapshotter/main.go

+	// wait until CRD gets processed
+	err = WaitForSnapshotResource(crdClient)
+	if err != nil {
+		panic(err)


why use panic instead of os.Exist like below?

Will change it.

jingxu97 · 2018-07-13T18:25:28Z

pkg/connection/connection.go

+			Name: snapDataName,
+		},
+		Spec: crdv1.VolumeSnapshotDataSpec{
+			VolumeSnapshotRef: &v1.ObjectReference{


this volumesnapshotRef should be the existing one, instead of creating a new one?

Sure. I'll check it here.

jingxu97 · 2018-07-25T19:57:11Z

pkg/connection/connection.go

+
+	req := csi.CreateSnapshotRequest{
+		SourceVolumeId:        volume.Spec.CSI.VolumeHandle,
+		Name:                  snapshot.Name,


should use snapshot.UID? Different namespaces might have the same snapshot name which will cause problem here.

Good catch. Yes, it should be snapshot.UID. Will fix it.

jingxu97 · 2018-07-25T20:21:57Z

pkg/connection/connection.go

 	}

-	return rsp.Entries[0].Snapshot.Status, nil
+	return rsp.Entries[0].Snapshot.Status, rsp.Entries[0].Snapshot.CreatedAt, nil


I thought this rsp.Entries[0].Snapshot.CreatedAt is when snapshot is cut, not when it is ready or something else.

Yes. In ConvertSnapshotStatus in controller/util.go, this timestamp is used to set CreatedAt (when snapshot is cut). For AvailableAt and timestamp in VolumeError, the current time is used.

sngchlko · 2018-08-09T06:45:45Z

pkg/connection/connection.go

+		SourceVolumeId:        volume.Spec.CSI.VolumeHandle,
+		Name:                  snapshotName,
+		Parameters:            parameters,
+		CreateSnapshotSecrets: nil,


As you know, some storage providers need secrets to create the snapshot.
Do you have any plan to implement this?

@sngchlko Thanks for the feedback! Yes, this should be fixed.

@sngchlko It's fixed here: xing-yang@dd820b5

This PR adds changes to support restoring a volume from a snapshot. Note: Mark it WIP until the DataSource addition in core API (kubernetes/kubernetes#67087) and snapshot API/controller changes (kubernetes-csi/external-snapshotter#7) are merged.

lpabon

lgtm

lpabon · 2018-08-10T18:46:29Z

cmd/csi-snapshotter/main.go

+		os.Exit(1)
+	}
+
+	glog.V(2).Infof("Start NewCSISnapshotController with snapshotter %s", *snapshotter)


Can be added after, but it would be nice to log the options.

lpabon · 2018-08-10T18:47:39Z

pkg/controller/csi_handler.go

+
+func (handler *csiHandler) CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume *v1.PersistentVolume, parameters map[string]string) (string, string, int64, *csi.SnapshotStatus, error) {
+
+	ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)


wackxu · 2018-08-22T12:29:55Z

pkg/controller/snapshot_controller.go

+			content.Spec.VolumeSnapshotRef.Name == snapshot.Name &&
+			content.Spec.VolumeSnapshotRef.Namespace == snapshot.Namespace &&
+			content.Spec.VolumeSnapshotRef.UID == snapshot.UID {
+			found = true


do we need check whether the snapshotClass is same?

wackxu · 2018-08-22T12:30:46Z

pkg/controller/snapshot_controller.go

+				Namespace:  snapshot.Namespace,
+				Name:       snapshot.Name,
+				UID:        snapshot.UID,
+				APIVersion: "v1alpha1",


it should be snapshot.storage.k8s.io/v1alpha1

wackxu · 2018-08-22T13:28:41Z

pkg/controller/snapshot_controller.go

+		glog.V(4).Infof("error is already set in snapshot, do not retry to create: %s", snapshot.Status.Error.Message)
+		return snapshot, nil
+	}
+	class, err := ctrl.GetClassFromVolumeSnapshot(snapshot)


We may update snapshot when the snapshot do not set VolumeSnapshotClassName field and use the default snapshot class , in this case, the follow update will always be version conflict, so we`d better return the snapshot from the function.

Will fix it. Thanks

good point. I am thinking maybe we should use two functions here instead of one to make it more clear.
if len(className) > 0 {
// call GetClassFromVolumeSnapshot()
} else {
// call SetDefaultClass()
}

This commit splits GetClassFromVolumeSnapshot to two functions, GetSnapshotClass and SetDefaultSnapshotClass.

Change VolumeSnapshotClassName to a pointer to string

jingxu97 · 2018-08-23T20:59:01Z

pkg/controller/snapshot_controller_base.go

+		if err == nil {
+			// The volume snapshot still exists in informer cache, the event must have
+			// been add/update/sync
+			if ctrl.shouldProcessSnapshot(snapshot) {


here shouldProcessSnapshot might update the snapshot, then this function should return the new version of the snapshot so that the following updateSnapshot can use the new version

Also rename shouldProcessSnapshot to checkAndUpdateSnapshotClass.

xing-yang · 2018-08-24T14:10:25Z

Addressed all review comments.

jingxu97 · 2018-08-24T23:37:59Z

pkg/controller/snapshot_controller.go

+		return nil
+	}
+	snapshotClone := snapshot.DeepCopy()
+	if snapshot.Status.Error == nil {


remove this line to allow new discovered error to show up.

jsafrane · 2018-08-27T08:50:38Z

lgtm-ish, I went through the code and tests and it looks usable. IMO, it's good for alpha, it needs serious testing.

saad-ali

/lgtm
/approve

jsafrane · 2018-08-29T17:07:44Z

/test all

This PR adds changes to support restoring a volume from a snapshot. Note: The DataSource addition in core API (kubernetes/kubernetes#67087) and snapshot API/controller changes (kubernetes-csi/external-snapshotter#7) are merged.

xing-yang changed the title ~~Add Snapshot Controller~~ WIP Add Snapshot Controller Jul 12, 2018

jingxu97 reviewed Jul 12, 2018

View reviewed changes

jingxu97 reviewed Jul 13, 2018

View reviewed changes

xing-yang force-pushed the snapshot_controller branch 2 times, most recently from 0020694 to 9b48753 Compare July 14, 2018 18:53

xing-yang force-pushed the snapshot_controller branch 3 times, most recently from e3c46ae to fdfad5e Compare July 23, 2018 20:34

xing-yang changed the title ~~WIP Add Snapshot Controller~~ Add Snapshot Controller Jul 25, 2018

xing-yang mentioned this pull request Jul 25, 2018

Move Snapshot APIs in-tree kubernetes/kubernetes#64663

Closed

jingxu97 reviewed Jul 25, 2018

View reviewed changes

xing-yang force-pushed the snapshot_controller branch 2 times, most recently from 40c9790 to d3b1d87 Compare August 1, 2018 03:54

xing-yang force-pushed the snapshot_controller branch 3 times, most recently from e87f318 to 0573211 Compare August 6, 2018 20:35

xing-yang mentioned this pull request Aug 7, 2018

WIP: Restore Volume from Snapshot kubernetes-csi/external-provisioner#119

Closed

xing-yang force-pushed the snapshot_controller branch 2 times, most recently from bc02d60 to 3841a99 Compare August 8, 2018 03:18

xing-yang mentioned this pull request Aug 9, 2018

Support snapshot in rbdplugin ceph/ceph-csi#61

Merged

xing-yang force-pushed the snapshot_controller branch from 3e881ed to 325fda7 Compare August 9, 2018 03:30

sngchlko reviewed Aug 9, 2018

View reviewed changes

xing-yang mentioned this pull request Aug 10, 2018

Restore Volume from Snapshot kubernetes-csi/external-provisioner#123

Merged

xing-yang force-pushed the snapshot_controller branch from 325fda7 to 7fa276f Compare August 10, 2018 14:01

lpabon approved these changes Aug 10, 2018

View reviewed changes

xing-yang added 3 commits August 21, 2018 19:33

Rename Size to RestoreSize

233b717

Re-generate deepcopy file

66b982d

Change Size to RestoreSize in snapshot controller

61c67ae

wackxu mentioned this pull request Aug 22, 2018

Add UT for Snapshot controller #12

Merged

wackxu reviewed Aug 22, 2018

View reviewed changes

xing-yang added 7 commits August 22, 2018 13:41

Split GetClassFromVolumeSnapshot to two functions

d95ff46

This commit splits GetClassFromVolumeSnapshot to two functions, GetSnapshotClass and SetDefaultSnapshotClass.

Change VolumeSnapshotClassName to pointer to a string

7baa5bf

Re-generate deepcopy file after VolumeSnapshotClass change

1893402

Change controller to use VolumeSnapshotClassName as pointer

7140b77

Fix a typo

de25b16

Address review comments

a9dd5c8

Merge pull request #11 from xing-yang/fixes

4f5aec4

Change VolumeSnapshotClassName to a pointer to string

jingxu97 reviewed Aug 23, 2018

View reviewed changes

shouldProcessSnapshot should return *VolumeSnapshot

25be5fd

Also rename shouldProcessSnapshot to checkAndUpdateSnapshotClass.

Fix error when checking and updating snapshotclass

9f3146b

jingxu97 reviewed Aug 24, 2018

View reviewed changes

Allow new discovered error to show up

17c7e1b

saad-ali approved these changes Aug 29, 2018

View reviewed changes

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

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

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

k8s-ci-robot merged commit 234c01b into kubernetes-csi:master Aug 30, 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_controller branch April 8, 2019 02:06


		func (handler csiHandler) CreateSnapshot(snapshot crdv1.VolumeSnapshot, volume v1.PersistentVolume, parameters map[string]string) (string, string, int64, csi.SnapshotStatus, error) {

		ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)

Add Snapshot Controller #7

Add Snapshot Controller #7

Conversation

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

lpabon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Aug 27, 2018

saad-ali left a comment

Choose a reason for hiding this comment

jsafrane commented Aug 29, 2018

xing-yang commented Jul 12, 2018 •

edited

Loading