Add multi PVC versions of e2e tests for WaitForFirstConsumer #70362

ddebroy · 2018-10-29T09:40:17Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR adds e2e tests that exercise creation of pods with multiple PVCs associated with storage classes configured with volume binding mode WaitForFirstConsumer . The tests ensure all PVs get provisioned in the same zone as the pod due to delayed binding.

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 #68517

Special notes for your reviewer:
RePD versions of these tests will be added to this PR. This is currently read to review for PD and EBS.

Does this PR introduce a user-facing change?:
"NONE"

NONE

/sig storage

ddebroy · 2018-10-29T09:40:58Z

/assign @msau42

ddebroy · 2018-10-29T09:41:26Z

/assign @marun

msau42 · 2018-10-29T22:26:45Z

/assign @verult

ddebroy · 2018-10-31T17:22:15Z

Multiple PVC tests for regional PDs with delayed binding is now in. Tests for multi-PVCs with EBS and GCE PD are passing. So the multi PVC tests and utils is ready to be reviewed.

The remaining portion is the tests with node selectors and taints which will build on this.

verult · 2018-10-31T20:48:19Z

test/e2e/framework/util.go

+	return WaitForPersistentVolumeClaimsPhase(phase, c, ns, []string{pvcName}, Poll, timeout, true)
+}
+
+// WaitForPersistentVolumeClaimPhase waits for any or all of the PersistentVolumeClaims to be in a specific phase or until timeout occurs, whichever comes first.


nit: "waits for any (if matchAny is true) or all (if matchAny is false)..."

verult · 2018-10-31T20:51:42Z

test/e2e/framework/util.go

 			}
 		}
+		if phaseFoundInAllClaims && len(pvcNames) > 0 {


Should we have a separate error for len(pvcNames) == 0?

verult · 2018-10-31T20:54:40Z

test/e2e/storage/volume_provisioning.go

 )

 func testBindingWaitForFirstConsumer(client clientset.Interface, claim *v1.PersistentVolumeClaim, class *storage.StorageClass) (*v1.PersistentVolume, *v1.Node) {
+	pvs, node := testBindingWaitForFirstConsumerMultiPVC(client, []*v1.PersistentVolumeClaim{claim}, class)
+	return pvs[0], node


Expect(len(pvs)).ToNot(Equal(0))

verult · 2018-10-31T20:56:13Z

test/e2e/storage/volume_provisioning.go

-	Expect(err).NotTo(HaveOccurred())
+	By("creating claims")
+	var createdClaims []*v1.PersistentVolumeClaim
+	var claimNames []string


nit: combine the two slices into a map

The functions used below like WaitForPersistentVolumeClaimsPhase, CreatePod need access to the slices createdClaims and claimNames. So putting them into a map will require extracting the keys and values as slices multiple times down below. To avoid doing that and given we don't need to look up claim through names, I am keeping it as two separate slices that can be directly passed to the functions used below.

verult · 2018-10-31T20:58:02Z

test/e2e/storage/volume_provisioning.go

 	defer func() {
-		framework.ExpectNoError(framework.DeletePersistentVolumeClaim(client, claim.Name, claim.Namespace), "Failed to delete PVC ", claim.Name)
+		for _, claim := range createdClaims {
+			framework.ExpectNoError(framework.DeletePersistentVolumeClaim(client, claim.Name, claim.Namespace), "Failed to delete PVC ", claim.Name)


Would a single deletion error stop other deletions? Should we issue all deletes first and check errors later?

verult · 2018-10-31T21:15:54Z

test/e2e/storage/volume_provisioning.go

 	Expect(err).To(HaveOccurred())
+	for _, claim := range createdClaims {


could this be simplified to framework.WaitForPersistentVolumeClaimsPhase(v1.ClaimPending) and a very short timeout? If there's another function that just checks the instantaneous phase that'd be even better

Waiting and checking that the claim did not bind for a while is necessary to verify that the WaitForFirstConsumer is indeed taking effect. If we wait for an extremely short period (through something like framework.WaitForPersistentVolumeClaimsPhase(v1.ClaimPending)), the verification that WaitForFirstConsumer is indeed taking effect will not be effective as a PVC with Immediate binding mode also stay Pending for a little while (as the PV is created, attached, mounted etc.) before switching to bound.

verult · 2018-10-31T21:23:15Z

test/e2e/storage/volume_provisioning.go


-	return pv, node
+		zone, ok := node.Labels[kubeletapis.LabelZoneFailureDomain]


This makes the function unusable for CSI drivers. IMO we should leave label/affinity checking outside the function for now. Maybe in the future we could have each volume plugin under test implement a callback check for this.

In volume_provisioning.go, the zone checks have been moved out of testBindingWaitForFirstConsumerMultiPVC. So I think this should be good already.

verult · 2018-10-31T21:24:27Z

test/e2e/storage/volume_provisioning.go

 	node, err := client.CoreV1().Nodes().Get(pod.Spec.NodeName, metav1.GetOptions{})
 	Expect(err).NotTo(HaveOccurred())

-	pv, err := client.CoreV1().PersistentVolumes().Get(claim.Spec.VolumeName, metav1.GetOptions{})
-	Expect(err).NotTo(HaveOccurred())
+	By("re-checking the claims to see they binded")


If the label and node affinity check is left here, update the By() message

Not necessary as the node affinity/label checks have been moved out.

verult · 2018-10-31T21:26:15Z

test/e2e/storage/volume_provisioning.go

@@ -885,6 +911,52 @@ var _ = utils.SIGDescribe("Dynamic Provisioning", func() {
 			}
 		})
 	})
+	Describe("DynamicProvisioner delayed binding with multiple PVCs [Slow]", func() {
+		It("should create multiple persistent volumes in the same zone as node after a pod mounting the claims is started", func() {
+			tests := []testsuites.StorageClassTest{


Could this test be pulled into a common function instead?

verult · 2018-10-31T21:27:08Z

test/e2e/storage/volume_provisioning.go

@@ -965,7 +1037,57 @@ var _ = utils.SIGDescribe("Dynamic Provisioning", func() {
 			}
 		})
 	})
-
+	Describe("DynamicProvisioner delayed binding with allowedTopologies with multiple PVCs [Slow]", func() {
+		It("should create multiple persistent volumes in the same zone as specified in allowedTopologies after a pod mounting the claims is started", func() {


ditto (common function)

verult · 2018-10-31T21:28:44Z

test/e2e/storage/regional_pd.go

@@ -74,15 +74,17 @@ var _ = utils.SIGDescribe("Regional PD", func() {
 		})

 		It("should provision storage with delayed binding [Slow]", func() {
-			testRegionalDelayedBinding(c, ns)
+			testRegionalDelayedBinding(c, ns, 1)


nit: 1 /* pvcCount */

ddebroy · 2018-11-02T08:51:35Z

Thanks for reviewing @verult. Code review comments so far addressed.

ddebroy · 2018-11-03T06:32:46Z

/retest

verult · 2018-11-05T22:36:55Z

test/e2e/storage/volume_provisioning.go

@@ -251,6 +261,101 @@ func checkGCEPD(volume *v1.PersistentVolume, volumeType string) error {
 	return nil
 }

+func testZonalDelayedBinding(c clientset.Interface, ns string, pvcCount int) {
+	tests := []testsuites.StorageClassTest{


testZonalDelayedBinding() and testZonalDelayedBindingWithAllowedTopology() share a lot of code, and their StorageClassTests are the same too. I was thinking we could combine these two functions

verult · 2018-11-05T22:37:23Z

mostly LGTM, some minor comments!

ddebroy · 2018-11-08T06:43:01Z

@verult latest CR comments addressed.

verult

/lgtm

just a minor comment

verult · 2018-11-09T19:24:42Z

test/e2e/storage/volume_provisioning.go

@@ -261,17 +261,22 @@ func checkGCEPD(volume *v1.PersistentVolume, volumeType string) error {
 	return nil
 }

-func testZonalDelayedBinding(c clientset.Interface, ns string, pvcCount int) {
+func testZonalDelayedBinding(c clientset.Interface, ns string, specifyAllowedTopology bool, pvcCount int) {
+	storageClassNameFmt := "Delayed binding %s storage class test %s"


storageClassTestNameFmt

Signed-off-by: Deep Debroy <ddebroy@docker.com>

ddebroy · 2018-11-09T20:19:59Z

Thanks for reviewing @verult . Addressed the last review comment above and squashed all commits so far. PTAL and add back the LGTM if all looks good.

verult · 2018-11-09T22:01:18Z

/lgtm

saad-ali · 2018-11-10T03:09:06Z

/approve

k8s-ci-robot · 2018-11-10T03:09:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, saad-ali

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/OWNERS~~ [saad-ali]

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

AishSundar · 2018-11-12T02:07:02Z

/milestone v1.13
/priority important-soon

fejta-bot · 2018-11-12T07:35:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot requested review from rmmh and rootfs October 29, 2018 09:40

k8s-ci-robot assigned msau42 Oct 29, 2018

k8s-ci-robot assigned marun Oct 29, 2018

k8s-ci-robot assigned verult Oct 29, 2018

ddebroy changed the title ~~[WIP] Add multi PVC versions of e2e tests for WaitForFirstConsumer~~ Add multi PVC versions of e2e tests for WaitForFirstConsumer Oct 31, 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 Oct 31, 2018

verult reviewed Oct 31, 2018

View reviewed changes

verult reviewed Nov 1, 2018

View reviewed changes

msau42 mentioned this pull request Nov 5, 2018

Topology Aware Volume Scheduling kubernetes/enhancements#490

Closed

verult reviewed Nov 5, 2018

View reviewed changes

verult reviewed Nov 9, 2018

View reviewed changes

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

Add multi PVC versions of tests for delayed binding

923df46

Signed-off-by: Deep Debroy <ddebroy@docker.com>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2018

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

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

k8s-ci-robot added this to the v1.13 milestone Nov 12, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 12, 2018

k8s-ci-robot merged commit 0ca67bd into kubernetes:master Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi PVC versions of e2e tests for WaitForFirstConsumer #70362

Add multi PVC versions of e2e tests for WaitForFirstConsumer #70362

ddebroy commented Oct 29, 2018

ddebroy commented Oct 29, 2018

ddebroy commented Oct 29, 2018

msau42 commented Oct 29, 2018

ddebroy commented Oct 31, 2018 •

edited

verult Oct 31, 2018

verult Oct 31, 2018

verult Oct 31, 2018

verult Oct 31, 2018

ddebroy Nov 2, 2018

verult Oct 31, 2018

verult Oct 31, 2018

ddebroy Nov 2, 2018 •

edited

verult Oct 31, 2018

ddebroy Nov 2, 2018 •

edited

verult Oct 31, 2018

ddebroy Nov 2, 2018

verult Oct 31, 2018

verult Oct 31, 2018

verult Oct 31, 2018

ddebroy commented Nov 2, 2018

ddebroy commented Nov 3, 2018

verult Nov 5, 2018

verult commented Nov 5, 2018

ddebroy commented Nov 8, 2018

verult left a comment

verult Nov 9, 2018

ddebroy commented Nov 9, 2018

verult commented Nov 9, 2018

saad-ali commented Nov 10, 2018

k8s-ci-robot commented Nov 10, 2018

AishSundar commented Nov 12, 2018

fejta-bot commented Nov 12, 2018

		Expect(err).To(HaveOccurred())
		for _, claim := range createdClaims {


		return pv, node
		zone, ok := node.Labels[kubeletapis.LabelZoneFailureDomain]

Add multi PVC versions of e2e tests for WaitForFirstConsumer #70362

Add multi PVC versions of e2e tests for WaitForFirstConsumer #70362

Conversation

ddebroy commented Oct 29, 2018

ddebroy commented Oct 29, 2018

ddebroy commented Oct 29, 2018

msau42 commented Oct 29, 2018

ddebroy commented Oct 31, 2018 • edited

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

ddebroy Nov 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Nov 2, 2018 • edited

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

ddebroy commented Nov 2, 2018

ddebroy commented Nov 3, 2018

Choose a reason for hiding this comment

verult commented Nov 5, 2018

ddebroy commented Nov 8, 2018

verult left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Nov 9, 2018

verult commented Nov 9, 2018

saad-ali commented Nov 10, 2018

k8s-ci-robot commented Nov 10, 2018

AishSundar commented Nov 12, 2018

fejta-bot commented Nov 12, 2018

ddebroy commented Oct 31, 2018 •

edited

ddebroy Nov 2, 2018 •

edited

ddebroy Nov 2, 2018 •

edited