Restore Volume from Snapshot #123

xing-yang · 2018-08-10T04:12:00Z

This PR adds changes to support restoring a volume from a snapshot.

The DataSource addition in core API (kubernetes/kubernetes#67087) and snapshot API/controller changes (kubernetes-csi/external-snapshotter#7) are merged now.

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.

jingxu97 · 2018-08-31T19:43:46Z

pkg/controller/controller.go

+	var checkSnapshotSupport bool
+	if options.PVC.Spec.DataSource != nil {
+		// PVC.Spec.DataSource.Name is the name of the VolumeSnapshot API object
+		if options.PVC.Spec.DataSource.Name == "" {


Since validation code already check this, do we need these checks here?

Actually these can be removed now. I'll remove them.

Maybe we should keep these checks here? When we add support for more Data Source types, the validation check will be changed to allow other Data Sources.

jingxu97 · 2018-08-31T19:45:50Z

pkg/controller/controller.go

+	if checkSnapshotSupport {
+		snapshotObj, err := p.snapshotClient.VolumesnapshotV1alpha1().VolumeSnapshots(options.PVC.Namespace).Get(options.PVC.Spec.DataSource.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, fmt.Errorf("error get snapshot %s from api server: %v", options.PVC.Spec.DataSource.Name, err)


error getting ..?

jingxu97 · 2018-08-31T19:46:54Z

pkg/controller/controller.go

+		}
+		glog.V(5).Infof("VolumeSnapshot %+v", snapshotObj)
+
+		snapContentObj, err := p.snapshotClient.VolumesnapshotV1alpha1().VolumeSnapshotContents().Get(snapshotObj.Spec.SnapshotContentName, metav1.GetOptions{})


Maybe we can check VolumeSnapshot status to see whether it is ready?

lpabon · 2018-08-31T20:14:11Z

pkg/controller/controller.go

@@ -69,13 +71,17 @@ const (
 	backoffSteps    = 10

 	defaultFSType = "ext4"
+
+	snapshotKind     = "VolumeSnapshot"
+	snapshotAPIGroup = "snapshot.storage.k8s.io"


Quick question, shouldn't these values come from external-snapshotter library?

Looks like I can get GroupName from snapshot api package. It is a constant there.

As for "VolumeSnapshot", it is a struct defined in types.go but there is no constant. I think I can get it by instantiating it first and then use "reflect" to get it, but it seems weird to get it that way. I haven't found an easier way.

Changed snapshotAPIGroup to use GroupName from external-snapshotter lib.

lpabon · 2018-08-31T20:17:20Z

pkg/controller/controller.go

@@ -257,6 +292,21 @@ func checkDriverState(grpcClient *grpc.ClientConn, timeout time.Duration) (strin
 		glog.Error("no create/delete volume support detected")
 		return "", fmt.Errorf("no create/delete volume support detected")
 	}
+
+	if checkSnapshotSupport {


Comment here in what situation the code should not check for support.

jingxu97 · 2018-08-31T20:38:50Z

pkg/controller/controller.go

@@ -289,7 +339,21 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 		return nil, fmt.Errorf("claim Selector is not supported")
 	}

-	driverName, err := checkDriverState(p.grpcClient, p.timeout)
+	var checkSnapshotSupport bool


call it needSnapshotSupport?

jingxu97 · 2018-08-31T20:48:44Z

pkg/controller/controller.go

@@ -317,6 +381,46 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 			RequiredBytes: int64(volSizeBytes),
 		},
 	}
+
+	if checkSnapshotSupport {


maybe to put this part into a separate function to get snapshot handler?

jingxu97 · 2018-08-31T20:50:34Z

pkg/controller/controller.go

+		}
+		checkSnapshotSupport = true
+	}
+	driverName, err := checkDriverState(p.grpcClient, p.timeout, checkSnapshotSupport)


Consider we will keep adding more types of data sources, a boolean value added here might not be convenient. But we could change it later.

jingxu97 · 2018-08-31T21:57:43Z

pkg/controller/controller.go

@@ -537,6 +512,51 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 	return pv, nil
 }

+func (p *csiProvisioner) getSnapshotHandle(options controller.VolumeOptions) (*csi.VolumeContentSource, error) {


maybe call it getVolumeContentSource?

davidz627

mostly looks good to me, just a couple nits and questions! Thanks for working on this :)

davidz627 · 2018-09-04T18:02:21Z

Gopkg.toml

@@ -40,14 +44,19 @@
  name = "google.golang.org/grpc"
  version = "1.9.2"

-[[constraint]]
+[[override]]


these probably do not have to be overrides, constraint should be better for most of these as they will behave correct if dependencies are removed/changed. We only need to override if we transitively depend on a certain version of a package, but I believe most of these are direct dependencies so we should just use [[constraint]]

I added "override" because I ran into errors earlier without them. I just removed them and re-ran them. It works now.

davidz627 · 2018-09-04T18:03:54Z

cmd/csi-provisioner/csi-provisioner.go

@@ -86,6 +87,10 @@ func init() {
 	if err != nil {
 		glog.Fatalf("Failed to create client: %v", err)
 	}
+	snapClient, err := snapclientset.NewForConfig(config)


What does NewForConfig mean? is it NewSnapClientFromConfig?

Added a comment for this: "NewForConfig creates a new Clientset for VolumesnapshotV1alpha1Client"

davidz627 · 2018-09-04T18:12:00Z

pkg/controller/controller.go

@@ -289,7 +343,21 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 		return nil, fmt.Errorf("claim Selector is not supported")
 	}

-	driverName, err := checkDriverState(p.grpcClient, p.timeout)
+	var needSnapshotSupport bool


nit: explicitly set needSnapshotSupport := false here

davidz627 · 2018-09-04T18:14:37Z

pkg/controller/controller.go

+		req.VolumeContentSource = volumeContentSource
+	}
+
+	glog.Infof("CreateVolumeRequest %+v", req)


remove debug logs or decrease verbosity

Changed to glog.V(5).Infof

davidz627 · 2018-09-04T18:19:41Z

pkg/controller/controller.go

+
+	snapshotSource := csi.VolumeContentSource_Snapshot{
+		Snapshot: &csi.VolumeContentSource_SnapshotSource{
+			Id: snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle,


nit: nil check on this SnapshotHandle?

davidz627 · 2018-09-04T18:21:38Z

pkg/controller/controller.go

+	glog.V(5).Infof("VolumeContentSource_Snapshot %+v", snapshotSource)
+
+	if snapshotObj.Status.RestoreSize != nil {
+		capacity := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]


nit: exists check

davidz627 · 2018-09-04T18:35:37Z

pkg/controller/controller_test.go

+		volOpts              controller.VolumeOptions
+		restoredVolSizeSmall bool
+		wrongDataSource      bool
+		validSnapshotStatus  bool


nit: rename to snapshotStatusReady

davidz627 · 2018-09-04T18:36:47Z

pkg/controller/controller_test.go

@@ -511,6 +620,42 @@ func provisionMockServerSetupExpectations(identityServer *driver.MockIdentitySer
 	}, nil).Times(1)
 }

+func provisionFromSnapshotMockServerSetupExpectations(identityServer *driver.MockIdentityServer, controllerServer *driver.MockControllerServer) {


Its not incredibly clear what expectations we have here, could you add a comment explaining what expectations these are and what we use them for?

Added comments

davidz627 · 2018-09-04T18:37:04Z

pkg/controller/controller_test.go

+		}
+
+		// Setup mock call expectations.
+		if tc.wrongDataSource == false {


its not clear to me what this means and why we do this

Added the following comments:
// If tc.wrongDataSource is false, DataSource is valid
// and the controller will proceed to check whether the plugin supports snapshot.
// So in this case, we need the plugin to report snapshot support capabilities;
// Otherwise, the controller will fail the operation so it won't check the capabilities.

davidz627 · 2018-09-04T18:37:54Z

pkg/controller/controller_test.go

+		if tc.wrongDataSource == false {
+			provisionFromSnapshotMockServerSetupExpectations(identityServer, controllerServer)
+		}
+		if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.validSnapshotStatus {


what happens if any of these are true? Specifically if restoredVolSizeSmall is true it doesn't look like we do anything special

Added the following comments:
// If tc.restoredVolSizeSmall is true, or tc.wrongDataSource is true, or
// tc.snapshotStatusReady is false, create volume from snapshot operation will fail
// early and therefore CreateVolume is not expected to be called.
// When the if condition is met, it is a valid create volume from snapshot
// operation and CreateVolume is expected to be called.

jingxu97 · 2018-09-04T20:14:03Z

pkg/controller/controller.go

@@ -529,6 +529,10 @@ func (p *csiProvisioner) getVolumeContentSource(options controller.VolumeOptions
 	}
 	glog.V(5).Infof("VolumeSnapshotContent %+v", snapContentObj)

+	if snapContentObj.Spec.VolumeSnapshotSource.CSI == nil || len(snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle) == 0 {


Just to confirm we don't need to check Spec.VolumeSnapshotSource because it is required field?

Yes, Spec.VolumeSnapshotSource is required but Spec.VolumeSnapshotSource.CSI could be nil. If CSI is specified, SnapshotHandle is required so that check can be removed.

So I removed the check for SnapshotHandle but kept the check for CSI is nil. Let me know if this is okay.

xing-yang · 2018-09-04T20:50:19Z

@jingxu97 @davidz627 @lpabon Review comments are addressed. Can you please review again? Thanks!

davidz627 · 2018-09-04T21:06:03Z

/lgtm

lpabon · 2018-09-05T03:20:22Z

/lgtm
/approve

k8s-ci-robot · 2018-09-05T03:20:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lpabon, 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~~ [lpabon]

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

Use Connection() from util package

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 19, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2018

xing-yang changed the title ~~WIP: Restore Volume from Snapshot~~ Restore Volume from Snapshot Aug 30, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2018

xing-yang added 2 commits August 30, 2018 08:45

Restore Volume from Snapshot

8f47a48

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.

Update vendor files

e2c9284

xing-yang force-pushed the datasource2 branch from a400a73 to e2c9284 Compare August 30, 2018 15:55

jingxu97 reviewed Aug 31, 2018

View reviewed changes

lpabon reviewed Aug 31, 2018

View reviewed changes

jingxu97 reviewed Aug 31, 2018

View reviewed changes

Address review comments

dde6249

jingxu97 reviewed Aug 31, 2018

View reviewed changes

Rename getSnapshotHandle to getVolumeContentSource

a496622

xing-yang force-pushed the datasource2 branch from f28996a to a496622 Compare September 2, 2018 01:47

davidz627 reviewed Sep 4, 2018

View reviewed changes

Modify Gopkg.toml to replace override with constraint

8361874

jingxu97 reviewed Sep 4, 2018

View reviewed changes

Addressed review comments

c736242

xing-yang force-pushed the datasource2 branch from 9a36e5e to c736242 Compare September 4, 2018 20:24

k8s-ci-robot assigned davidz627 Sep 4, 2018

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

k8s-ci-robot assigned lpabon Sep 5, 2018

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

k8s-ci-robot merged commit 265fecd into kubernetes-csi:master Sep 5, 2018

xing-yang mentioned this pull request Jan 21, 2019

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

Closed

6 tasks

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#123 from jsafrane/use-lib-connection

0e6f5f2

Use Connection() from util package

Restore Volume from Snapshot #123

Restore Volume from Snapshot #123

Conversation

xing-yang commented Aug 10, 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

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

davidz627 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

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 Sep 4, 2018

davidz627 commented Sep 4, 2018

lpabon commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

xing-yang commented Aug 10, 2018 •

edited

Loading

jingxu97 Aug 31, 2018 •

edited

Loading