Rework multi-volume test to use StatefulSet #66925

msau42 · 2018-08-02T18:09:17Z

What this PR does / why we need it:
The e2e test that got added as part of #66832 fails in a multi-zone environment because the volumes get provisioned in random zones. This PR reworks the test to use StatefulSet instead, which handles provisioning multiple PVCs in the same zone.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

msau42 · 2018-08-02T18:09:38Z

/assign @davidz627

msau42 · 2018-08-02T20:56:00Z

/retest

davidz627

mostly looks good. just some questions and readability things.

davidz627 · 2018-08-02T22:54:18Z

test/e2e/storage/persistent_volumes.go

-				for _, pvc := range pvcs {
-					framework.Logf("Created PVC %q", pvc.Name)
-					framework.ExpectNoError(framework.WaitForPersistentVolumeClaimPhase(v1.ClaimBound, c, ns, pvc.Name, framework.Poll, framework.ClaimProvisionTimeout))
+				writeCmd += "&& sleep 10000"


why are we sleeping for so long?

I just want to simulate a long running process

davidz627 · 2018-08-02T22:55:31Z

test/e2e/storage/persistent_volumes.go

-				By("Recreating the pod and validating the data")
-				framework.ExpectNoError(framework.DeletePodWithWait(f, c, pod))
+				By("Deleting the StatefulSet but not the volumes")
+				ss, err = ssTester.Scale(ss, 0)


do we have to scale down before deleting? If so, can you add a comment for why

mainly because i wanted to use the statefulset testing library which doesn't have a method to wait for deletion, and this is what they do.

davidz627 · 2018-08-02T23:00:23Z

test/e2e/storage/persistent_volumes.go

-				framework.ExpectNoError(framework.WaitForPodSuccessInNamespace(c, pod.Name, ns))
+				ssTester.WaitForRunningAndReady(1, ss)
+
+				By("Deleting the pod and revalidating the data")


Is it necessary to double validate here? We're already deleting the statefulset and bringing a new one up.

not strictly. I can remove it

davidz627 · 2018-08-02T23:02:25Z

test/e2e/storage/persistent_volumes.go

+
+func getVolumeFile(i int) string {
+	// mountPath is /mnt/vol<i+1>
+	return fmt.Sprintf("/mnt/vol%v/data%v", i+1, i+1)


just use i.

davidz627 · 2018-08-02T23:02:36Z

test/e2e/storage/persistent_volumes.go

+	for i := 0; i < pvcCount; i++ {
+		pvc := framework.MakePersistentVolumeClaim(framework.PersistentVolumeClaimConfig{}, ns)
+		pvc.GenerateName = ""
+		pvc.Name = fmt.Sprintf("vol%v", i+1)


just use i

davidz627 · 2018-08-02T23:04:14Z

test/e2e/storage/persistent_volumes.go

+				},
+			},
+			VolumeClaimTemplates: claims,
+			ServiceName:          "test-service",


do we need this? there is no corresponding service being created

davidz627 · 2018-08-02T23:07:31Z

test/e2e/storage/persistent_volumes.go

-				}
-				pod := framework.MakePod(ns, nil, pvcs, false, writeCmd)
-				pod, err = c.CoreV1().Pods(ns).Create(pod)
+				spec := makeStatefulSetWithPVCs(ns, writeCmd, numVols, probe)


from the top level of this test it is not clear where the volume mount names are coming from, it would be better if you could somehow extract the volume mount paths out to this top level and pass it into this function so that the test is slightly more readable.
#testingOnTheToiletEp529

msau42 · 2018-08-03T00:40:21Z

Updated

davidz627

small comment then lgtm

davidz627 · 2018-08-03T00:44:26Z

test/e2e/storage/persistent_volumes.go

 				Expect(err).NotTo(HaveOccurred())
-				framework.ExpectNoError(framework.WaitForPodSuccessInNamespace(c, pod.Name, ns))
+				ssTester.WaitForRunningAndReady(1, ss)


clean up the statefulset after test?

davidz627 · 2018-08-03T00:46:16Z

test/e2e/storage/persistent_volumes.go

+				claims := []v1.PersistentVolumeClaim{}
+				for i := 0; i < numVols; i++ {
+					pvc := framework.MakePersistentVolumeClaim(framework.PersistentVolumeClaimConfig{}, ns)
+					pvc.GenerateName = ""


do we need to setting this explicitly to ""

nope i've removed

neolit123 · 2018-08-03T00:49:33Z

/sig storage

msau42 · 2018-08-03T01:37:14Z

updated

msau42 · 2018-08-03T02:20:49Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

davidz627 · 2018-08-03T17:16:42Z

/lgtm

k8s-ci-robot · 2018-08-03T17:17:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, msau42

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:

~~test/e2e/storage/OWNERS~~ [msau42]

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

k8s-github-robot · 2018-08-03T21:36:01Z

Automatic merge from submit-queue (batch tested with PRs 66933, 66925). If you want to cherry-pick this change to another branch, please follow the instructions here.

…-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #66925: Rework multi-volume test to use StatefulSet Cherry pick of #66925 on release-1.10. #66925: Rework multi-volume test to use StatefulSet

…-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #66925: Rework multi-volume test to use StatefulSet Cherry pick of #66925 on release-1.11. #66925: Rework multi-volume test to use StatefulSet

…-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #66832: Detect if GCE PD udev link is wrong and try to correct it Cherry pick of #66832 on release-1.9. #66832: Detect if GCE PD udev link is wrong and try to correct it #66925: Rework multi-volume test to use StatefulSet

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2018

k8s-ci-robot requested review from jeffvance and rootfs August 2, 2018 18:09

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

k8s-ci-robot assigned davidz627 Aug 2, 2018

davidz627 reviewed Aug 2, 2018

View reviewed changes

msau42 force-pushed the udev branch from ba4751f to eb8f218 Compare August 3, 2018 00:39

davidz627 reviewed Aug 3, 2018

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Aug 3, 2018

Rework multi-volume test to use StatefulSet

4fbf25f

msau42 force-pushed the udev branch from eb8f218 to 4fbf25f Compare August 3, 2018 01:36

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

k8s-github-robot merged commit babdbff into kubernetes:master Aug 3, 2018

This was referenced Aug 3, 2018

Automated cherry pick of #66925: Rework multi-volume test to use StatefulSet #66976

Merged

Automated cherry pick of #66925: Rework multi-volume test to use StatefulSet #66977

Merged

dims mentioned this pull request Aug 3, 2018

Update the nginx image with docker.io/nginx image #66055

Merged

msau42 mentioned this pull request Aug 4, 2018

Automated cherry pick of #66832: Detect if GCE PD udev link is wrong and try to correct it #66880

Merged

msau42 deleted the udev branch November 17, 2018 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework multi-volume test to use StatefulSet #66925

Rework multi-volume test to use StatefulSet #66925

msau42 commented Aug 2, 2018

msau42 commented Aug 2, 2018

msau42 commented Aug 2, 2018

davidz627 left a comment

davidz627 Aug 2, 2018

msau42 Aug 2, 2018

davidz627 Aug 2, 2018

msau42 Aug 2, 2018

davidz627 Aug 2, 2018

msau42 Aug 2, 2018

davidz627 Aug 2, 2018

davidz627 Aug 2, 2018

davidz627 Aug 2, 2018

davidz627 Aug 2, 2018

msau42 Aug 2, 2018

msau42 commented Aug 3, 2018

davidz627 left a comment

davidz627 Aug 3, 2018

davidz627 Aug 3, 2018

msau42 Aug 3, 2018

neolit123 commented Aug 3, 2018

msau42 commented Aug 3, 2018

msau42 commented Aug 3, 2018

davidz627 commented Aug 3, 2018

k8s-ci-robot commented Aug 3, 2018

k8s-github-robot commented Aug 3, 2018

Rework multi-volume test to use StatefulSet #66925

Rework multi-volume test to use StatefulSet #66925

Conversation

msau42 commented Aug 2, 2018

msau42 commented Aug 2, 2018

msau42 commented Aug 2, 2018

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

msau42 commented Aug 3, 2018

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

neolit123 commented Aug 3, 2018

msau42 commented Aug 3, 2018

msau42 commented Aug 3, 2018

davidz627 commented Aug 3, 2018

k8s-ci-robot commented Aug 3, 2018

k8s-github-robot commented Aug 3, 2018