add import snapshot e2e tests #674

AndyXiangLi · 2020-12-24T01:17:56Z

Is this a bug fix or adding new feature?
Adding e2e test for import snapshot scenario

What is this PR about? / Why do we need it?

What testing is done?
#317

k8s-ci-robot · 2020-12-24T01:18:04Z

Hi @AndyXiangLi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

coveralls · 2020-12-24T01:21:23Z

Pull Request Test Coverage Report for Build 1415

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 81.713%

Totals
Change from base Build 1406:	0.0%
Covered Lines:	1622
Relevant Lines:	1985

💛 - Coveralls

fejta-bot · 2020-12-24T03:32:59Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

ayberk

Great job! A couple comments. Also can you add how you tested these changes? Did you run them?

/ok-to-test

ayberk · 2020-12-24T19:47:31Z

tests/e2e/pre_provsioning.go

@@ -38,7 +39,8 @@ const (

 	awsAvailabilityZonesEnv = "AWS_AVAILABILITY_ZONES"

-	dummyVolumeName = "pre-provisioned"
+	dummyVolumeName   = "pre-provisioned"
+	dummySnapshotName = "pre-provisioned snapshot"


pre-provisioned-snapshot

ayberk · 2020-12-24T20:08:27Z

tests/e2e/testsuites/pre_provisioned_snapshot_volume_tester.go

+	defer tvsc.DeleteVolumeSnapshotContent(tvolumeSnapshotContent)
+	defer tvsc.DeleteSnapshot(tvs)
+
+	t.Pod.Volumes[0].DataSource = &DataSource{Name: tvs.Name}


We should check add a check and fail early here: if len(t.Pod.Volumes) < 1

ayberk · 2020-12-24T20:09:38Z

tests/e2e/testsuites/pre_provisioned_snapshot_volume_tester.go

+	binding := storagev1.VolumeBindingWaitForFirstConsumer
+	t.Pod.Volumes[0].VolumeBindingMode = &binding
+	tPod := NewTestPod(client, namespace, t.Pod.Cmd)
+	volume := t.Pod.Volumes[0]


You can move this above line 50 and use it throughout.

ayberk · 2020-12-24T20:11:11Z

tests/e2e/testsuites/pre_provisioned_snapshot_volume_tester.go

+	By("deploying a second pod with a volume restored from the snapshot")
+	tPod.Create()
+	defer tPod.Cleanup()
+	By("checking that the pods command exits with no error")


How are we verifying that we restored the correct snapshot(ie data)?

ayberk · 2020-12-24T20:13:42Z

tests/e2e/testsuites/testsuites.go

+func (t *TestVolumeSnapshotClass) DeleteVolumeSnapshotContent(vsc *v1beta1.VolumeSnapshotContent) {
+	By("deleting a VolumeSnapshotContent " + vsc.Name)
+	err := snapshotclientset.New(t.client).SnapshotV1beta1().VolumeSnapshotContents().Delete(vsc.Name, &metav1.DeleteOptions{})
+	//framework.ExpectNoError(err)


Delete or uncomment please :)

ayberk · 2020-12-24T20:17:27Z

tests/e2e/testsuites/testsuites.go

@@ -334,7 +423,7 @@ func (t *TestPersistentVolumeClaim) Cleanup() {
 	// attempts may fail, as the volume is still attached to a node because
 	// kubelet is slowly cleaning up the previous pod, however it should succeed
 	// in a couple of minutes.
-	if t.persistentVolume.Spec.PersistentVolumeReclaimPolicy == v1.PersistentVolumeReclaimDelete {
+	if t.persistentVolume != nil && t.persistentVolume.Spec.PersistentVolumeReclaimPolicy == v1.PersistentVolumeReclaimDelete {


Hmm curious, did you get a panic because of the missing check here?

Yes, PV is nil in this case

Hmm I hope we're not hiding a "should not happen" case here. I'll take another look later. Should be fine for now.

ayberk · 2020-12-24T20:19:23Z

examples/kubernetes/snapshot/specs/snapshot-import/claim.yaml

@@ -0,0 +1,16 @@
+apiVersion: v1


Can you also update the aws-ebs-csi-driver/examples/kubernetes/snapshot/README?

Sure, as the import snapshot is a different test case, should I put them in one README or separate it?

I think we had different use cases on that README. Up to you.

ayberk · 2020-12-24T20:19:35Z

examples/kubernetes/snapshot/specs/snapshot-import/volume-snapshot-content.yaml

@@ -8,6 +8,7 @@ spec:
    name: static-snapshot-demo
    namespace: default 
  source:
-    snapshotHandle: snap-0fba4d7649d765c50
+    snapshotHandle: snap-0644009a49add11ac


Why do we need this?

This is not necessary, It is just for my testing.. I will change it back, good catch

ayberk · 2020-12-24T20:21:33Z

tests/e2e/pre_provsioning.go

@@ -17,6 +17,7 @@ package e2e
 import (
 	"context"
 	"fmt"
+	restclientset "k8s.io/client-go/rest"


I'd use a more descriptive name. Maybe k8srestclient?

ayberk · 2020-12-24T20:21:58Z

tests/e2e/testsuites/pre_provisioned_snapshot_volume_tester.go

+	v1 "k8s.io/api/core/v1"
+	storagev1 "k8s.io/api/storage/v1"
+	clientset "k8s.io/client-go/kubernetes"
+	restclientset "k8s.io/client-go/rest"


Simiarly here too :)

k8s-ci-robot · 2021-01-04T19:08:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndyXiangLi
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 24, 2020

k8s-ci-robot requested review from bertinatto and gnufied December 24, 2020 01:18

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 24, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 24, 2020

ayberk suggested changes Dec 24, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 24, 2020

AndyXiangLi force-pushed the master branch 3 times, most recently from 04bb6ff to ea88ab9 Compare December 28, 2020 23:10

AndyXiangLi closed this Jan 4, 2021

AndyXiangLi force-pushed the master branch from ea88ab9 to ddaa68b Compare January 4, 2021 19:07

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2021

AndyXiangLi mentioned this pull request Jan 4, 2021

add import snapshot e2e test #678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add import snapshot e2e tests #674

add import snapshot e2e tests #674

AndyXiangLi commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

coveralls commented Dec 24, 2020 •

edited

Loading

fejta-bot commented Dec 24, 2020

ayberk left a comment

ayberk Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

AndyXiangLi Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

AndyXiangLi Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

AndyXiangLi Dec 24, 2020

ayberk Dec 24, 2020

ayberk Dec 24, 2020

k8s-ci-robot commented Jan 4, 2021

add import snapshot e2e tests #674

add import snapshot e2e tests #674

Conversation

AndyXiangLi commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

coveralls commented Dec 24, 2020 • edited Loading

Pull Request Test Coverage Report for Build 1415

💛 - Coveralls

fejta-bot commented Dec 24, 2020

ayberk 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

k8s-ci-robot commented Jan 4, 2021

coveralls commented Dec 24, 2020 •

edited

Loading