Add snapshots support #131

tsmetana · 2018-11-30T09:21:54Z

This is the first version of volume snapshotting support.

It implements:

CREATE_DELETE_SNAPSHOT capability support (i.e. no snapshot listing so far).
create volume from snapshot

I did only basic testing of the feature so far, but it looks to be working fine. I will add some examples and documentation of the feature.

Ref Issue #25

coveralls · 2018-11-30T09:24:49Z

Pull Request Test Coverage Report for Build 424

136 of 235 (57.87%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.6%) to 63.814%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/driver/controller.go	43	67	64.18%
pkg/cloud/cloud.go	93	130	71.54%
pkg/cloud/fakes.go	0	38	0.0%

Totals
Change from base Build 417:	-0.6%
Covered Lines:	917
Relevant Lines:	1437

💛 - Coveralls

bertinatto

I took a look at the initial implementation and the direction we're heading to looks good to me.

I just skimmed through the tests, but I think it'd be a good idea to also add an integration test while the e2e tests infra is being worked out.

bertinatto · 2018-12-03T09:44:18Z

pkg/cloud/cloud.go

+		Description:       aws.String(descriptions),
+	}
+
+	res, err := c.ec2.CreateSnapshotWithContext(ctx, request)


Just something to keep in mind:

I wonder if creating a snapshot of a big volume can take a lot of time. If so, ctx would time out and the operation cancelled.

The standard timeout that comes from the external-snapshotter is 1 minute [1]. If a snapshot creation in AWS might take more than 1 minute, we may have to set a higher value in the external-snapshotter manifest file.

[1] https://github.com/kubernetes-csi/external-snapshotter/blob/master/cmd/csi-snapshotter/main.go#L54

BTW, can you push the manifest for the external-snapshotter in deploy/kubernetes as well?

You're right... I've never seen an AWS disk where the snapshot would take that long. The change in the external controller that simplified the phase handling is quite new (the actual snapshotting is usually quick but then some backends take a long time to move the snapshot however the original disk is OK to be used). I'll figure this out.

bertinatto · 2018-12-03T09:47:37Z

pkg/cloud/cloud.go

+		err := c.waitForCreate(ctx, volumeID)
+		if err != nil {
+			return nil, fmt.Errorf("failed to restore snapshot %s: %v", diskOptions.SnapshotID, err)
+		}
 	}


Hopefully this will no longer be necessary once #126 is merged.

#126 is merged, could you rebase?

pkg/cloud/cloud.go

pkg/driver/controller.go

leakingtapan

@tsmetana Thanks for the PR. And sorry for getting back on this after a while. Could you rebase this on top of latest change?

pkg/cloud/cloud.go

leakingtapan · 2019-01-10T06:45:13Z

pkg/cloud/cloud.go

+		err := c.waitForCreate(ctx, volumeID)
+		if err != nil {
+			return nil, fmt.Errorf("failed to restore snapshot %s: %v", diskOptions.SnapshotID, err)
+		}
 	}


#126 is merged, could you rebase?

leakingtapan · 2019-01-10T06:47:36Z

pkg/cloud/cloud.go

+	}
+	request := &ec2.CreateSnapshotInput{
+		VolumeId:          aws.String(volumeID),
+		DryRun:            aws.Bool(false),


NIT: I think this is default to false, maybe we can leave it out

pkg/cloud/cloud.go

k8s-ci-robot · 2019-01-15T13:15:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tsmetana
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jsafrane

If they are not already assigned, you can assign the PR to them by writing /assign @jsafrane in a comment when ready.

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

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

tsmetana · 2019-01-15T13:16:43Z

Rebased with some changes to the backoff logic.

leakingtapan

@tsmetana sorry for the delay on this.

Could you add the external snapshotter in controller manifest file?

leakingtapan · 2019-02-01T05:59:33Z

pkg/cloud/cloud.go

+	// Truncated exponential backoff: if the exponential backoff times-out, just keep polling using the longest interval
+	err := wait.ExponentialBackoff(backoff, conditionFunc)
+	if err == wait.ErrWaitTimeout {
+		timeout := time.Duration(backoff.Duration.Seconds() * math.Pow(backoff.Factor, float64(backoff.Steps)))


Could you explain more on this code path? Like when does this happen? Does this mean when we spent 1 min at ExponentialBackoff then it times out, we will spend another min on PollInfinite? Not sure what's the benefit of this

Kuberentes API is supposed to be "intent based" so as long as there exists a VolumeSnapshot object the API should try to create it (no timeout). The idea here is not to extend the polling period too much so in case the operation succeeds/fail we are able to inform the user. Since it's unknown how long may the operation take I only used the exponential backoff for the first few poll iterations and then stopped to prolong the interval and make it constant.

leakingtapan · 2019-02-01T06:04:31Z

pkg/driver/controller.go

@@ -124,6 +126,20 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		Encrypted:        isEncrypted,
 		KmsKeyID:         kmsKeyId,
 	}
+
+	// Shall we restore a snapshot?


Do we still need this comment? If the snapshot ID is provide, the create volume API call will create the volume from snapshot, right?

Yes. Will remove it with reabse.

leakingtapan · 2019-02-01T06:07:36Z

pkg/driver/controller.go

+		return nil, status.Errorf(codes.Internal, "Could not create snapshot %q: %v", snapshotName, err)
+	}
+	csiSnapshot, err := newCreateSnapshotResponse(snapshot)
+	return csiSnapshot, err


NIT:

Suggested change

return csiSnapshot, err

return newCreateSnapshotResponse(snapshot)

OK. Will change this.

leakingtapan · 2019-02-04T19:16:35Z

/retest

leakingtapan · 2019-02-08T18:59:48Z

Thx @tsmetana for rebasing the change. We are merging this to snapshot branch to bake it a bit more and fix some timeout issue. After that, we will merge the final change into master

@dkoshkin

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 30, 2018

k8s-ci-robot requested review from bertinatto and d-nishi November 30, 2018 09:21

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 30, 2018

bertinatto reviewed Dec 3, 2018

View reviewed changes

leakingtapan changed the base branch from next to master December 3, 2018 20:13

kubernetes-sigs deleted a comment from suphanatnack Dec 26, 2018

leakingtapan reviewed Jan 10, 2019

View reviewed changes

tsmetana force-pushed the snapshots-next branch from 38721a1 to 37a3169 Compare January 15, 2019 13:15

tsmetana force-pushed the snapshots-next branch from 37a3169 to ae92ad4 Compare January 15, 2019 13:45

leakingtapan reviewed Feb 1, 2019

View reviewed changes

Add snapshots support

8e5d99f

tsmetana force-pushed the snapshots-next branch from ae92ad4 to 8e5d99f Compare February 8, 2019 17:07

leakingtapan changed the base branch from master to snapshot February 8, 2019 17:11

leakingtapan merged commit 39fc9bd into kubernetes-sigs:snapshot Feb 8, 2019

This was referenced Feb 18, 2019

Merge Snapshot to Master Branch #217

Closed

Merge snapshot to master branch #218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshots support #131

Add snapshots support #131

tsmetana commented Nov 30, 2018 •

edited by leakingtapan

Loading

coveralls commented Nov 30, 2018 •

edited

Loading

bertinatto left a comment

bertinatto Dec 3, 2018

bertinatto Dec 3, 2018

tsmetana Dec 4, 2018 •

edited

Loading

bertinatto Dec 3, 2018

leakingtapan Jan 10, 2019

leakingtapan left a comment

leakingtapan Jan 10, 2019

leakingtapan Jan 10, 2019

k8s-ci-robot commented Jan 15, 2019

tsmetana commented Jan 15, 2019

leakingtapan left a comment

leakingtapan Feb 1, 2019

tsmetana Feb 4, 2019 •

edited

Loading

leakingtapan Feb 1, 2019

tsmetana Feb 4, 2019

leakingtapan Feb 1, 2019

tsmetana Feb 4, 2019

leakingtapan commented Feb 4, 2019

leakingtapan commented Feb 8, 2019

	return csiSnapshot, err
	return newCreateSnapshotResponse(snapshot)

Add snapshots support #131

Add snapshots support #131

Conversation

tsmetana commented Nov 30, 2018 • edited by leakingtapan Loading

coveralls commented Nov 30, 2018 • edited Loading

Pull Request Test Coverage Report for Build 424

💛 - Coveralls

bertinatto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmetana Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leakingtapan left a 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 Jan 15, 2019

tsmetana commented Jan 15, 2019

leakingtapan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmetana Feb 4, 2019 • 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

leakingtapan commented Feb 4, 2019

leakingtapan commented Feb 8, 2019

tsmetana commented Nov 30, 2018 •

edited by leakingtapan

Loading

coveralls commented Nov 30, 2018 •

edited

Loading

tsmetana Dec 4, 2018 •

edited

Loading

tsmetana Feb 4, 2019 •

edited

Loading