New volume snapshot e2e tests #235

dkoshkin · 2019-03-06T19:36:00Z

Is this a bug fix or adding new feature?
Fixes #228

What is this PR about? / Why do we need it?
Add a new e2e test to provision a pod, write to a volume, take a snapshot, provision a new pod with a volume source as the snapshot, and then validate the data is as expected.

• [SLOW TEST:175.757 seconds]
[ebs-csi-e2e] [single-az] Dynamic Provisioning
/Users/dkoshkin/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/dynamic_provisioning.go:39
  should create a pod, write and read to it, take a volume snapshot, and create another pod from the snapshot
  /Users/dkoshkin/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/dynamic_provisioning.go:411
------------------------------
SSSSSSSSSSSS
Ran 1 of 33 Specs in 175.758 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 32 Skipped
PASS

Ginkgo ran 1 suite in 3m4.633563865s
Test Suite Passed

What testing is done?
Locally ran the e2e test about 10 times.

dkoshkin · 2019-03-06T20:10:09Z

• Failure in Spec Setup (BeforeEach) [22.792 seconds]
[ebs-csi-e2e] [single-az] Pre-Provisioned
/home/prow/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/pre_provsioning.go:66
[env] should use a pre-provisioned volume and delete PV with reclaimPolicy "Delete" [BeforeEach]
/home/prow/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/pre_provsioning.go:191
could not provision a volume: failed to get an available volume in EC2: RequestLimitExceeded: Request limit exceeded.
status code: 503, request id: 1a89d9af-a527-440a-a147-756c43307116

dkoshkin · 2019-03-06T20:18:48Z

/retest

dkoshkin · 2019-03-06T22:09:56Z

/retest

coveralls · 2019-03-07T00:26:39Z

Pull Request Test Coverage Report for Build 514

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 66.689%

Totals
Change from base Build 512:	0.0%
Covered Lines:	997
Relevant Lines:	1495

💛 - Coveralls

leakingtapan · 2019-03-08T18:52:08Z

tests/e2e/testsuites/testsuites.go

+	}
+}
+
+func (t *TestPersistentVolumeClaim) Snapshot() {


Not needed?

leakingtapan · 2019-03-08T18:53:37Z

tests/e2e/testsuites/specs.go

@@ -154,3 +181,14 @@ func (volume *VolumeDetails) SetupPreProvisionedPersistentVolumeClaim(client cli

 	return tpvc, cleanupFuncs
 }
+
+func CreateVolumeSnapshotClass(client restclientset.Interface, namespace *v1.Namespace, csiDriver driver.VolumeSnapshotTestDriver) (*TestVolumeSnapshotClass, []func()) {


Why is CreateVolumeSnapshotClass returning a slice of clean up function? The only resource needs to be cleaned up is the CreateVolumeSnapshotClass itself?

leakingtapan · 2019-03-08T18:58:14Z

tests/e2e/testsuites/dynamically_provisioned_volume_snapshot_tester.go

+// DynamicallyProvisionedVolumeSnapshotTest will provision required StorageClass(es),VolumeSnapshotClass(es), PVC(s) and Pod(s)
+// Waiting for the PV provisioner to create a new PV
+// Testing if the Pod(s) can write and read to mounted volumes
+// Create a snapshot, validate the data is still on the disk, and then write and read to it again


Should mention this is testing delete snapshot as well

leakingtapan · 2019-03-08T19:05:23Z

tests/e2e/testsuites/testsuites.go

+
+func (t *TestVolumeSnapshotClass) Cleanup() {
+	framework.Logf("deleting VolumeSnapshotClass %s", t.volumeSnapshotClass.Name)
+	err := snapshotclientset.New(t.client).VolumesnapshotV1alpha1().VolumeSnapshotClasses().Delete(t.volumeSnapshotClass.Name, nil)


I feel we have too many clients here, and it sometimes is confusing. Could we create the snapshotclientset at the very beginning inside BeforeEach instead of creating restclient there and passing it all the way down here? How do you think?

After putting it in its own Describe it feels cleaner since all of these tests would require the client.

leakingtapan · 2019-03-08T19:09:50Z

tests/e2e/dynamic_provisioning.go

@@ -30,19 +31,28 @@ import (

 	awscloud "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud"
 	ebscsidriver "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
 )

 var _ = Describe("[ebs-csi-e2e] [single-az] Dynamic Provisioning", func() {


Could you create the snapshot tests in a separate Describe like Describe("[ebs-csi-e2e] [single-az] Snapshot"). So that we could add more snapshot related tests there (eg. snapshot with pre-provisioned, nvme, block volume) if needed without mixing too much in in existing tests.

leakingtapan · 2019-03-11T17:37:00Z

/lgtm
/approve

k8s-ci-robot · 2019-03-11T17:37:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkoshkin, leakingtapan

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~~ [leakingtapan]

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

…ency-openshift-4.15-ose-aws-ebs-csi-driver OCPBUGS-19101: Updating ose-aws-ebs-csi-driver images to be consistent with ART

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 6, 2019

k8s-ci-robot requested review from ddebroy and leakingtapan March 6, 2019 19:36

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 6, 2019

dkoshkin force-pushed the dkoshkin/snapshot-e2e branch from f60cc5f to 7668516 Compare March 6, 2019 21:12

dkoshkin force-pushed the dkoshkin/snapshot-e2e branch from f9ee18e to 4c2e6ec Compare March 7, 2019 01:16

leakingtapan reviewed Mar 8, 2019

View reviewed changes

dkoshkin force-pushed the dkoshkin/snapshot-e2e branch from 4c2e6ec to 9fc59ec Compare March 10, 2019 19:02

New volume snapshot e2e tests

c4fe59e

dkoshkin force-pushed the dkoshkin/snapshot-e2e branch from 9fc59ec to c4fe59e Compare March 10, 2019 20:10

k8s-ci-robot assigned leakingtapan Mar 11, 2019

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

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

k8s-ci-robot merged commit 65941b6 into kubernetes-sigs:master Mar 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New volume snapshot e2e tests #235

New volume snapshot e2e tests #235

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

coveralls commented Mar 7, 2019 •

edited

Loading

leakingtapan Mar 8, 2019

dkoshkin Mar 11, 2019

leakingtapan Mar 8, 2019

dkoshkin Mar 11, 2019 •

edited

Loading

leakingtapan Mar 8, 2019

dkoshkin Mar 11, 2019

leakingtapan Mar 8, 2019

dkoshkin Mar 10, 2019

leakingtapan Mar 8, 2019

dkoshkin Mar 11, 2019

leakingtapan commented Mar 11, 2019

k8s-ci-robot commented Mar 11, 2019

New volume snapshot e2e tests #235

New volume snapshot e2e tests #235

Conversation

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

dkoshkin commented Mar 6, 2019

coveralls commented Mar 7, 2019 • edited Loading

Pull Request Test Coverage Report for Build 514

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkoshkin Mar 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leakingtapan commented Mar 11, 2019

k8s-ci-robot commented Mar 11, 2019

coveralls commented Mar 7, 2019 •

edited

Loading

dkoshkin Mar 11, 2019 •

edited

Loading