Add e2e test for CSI volume limits #80247

jsafrane · 2019-07-17T09:19:35Z

The test creates as many PVs and pods as the driver reports to support on a single node.

Tested with AWS, it's really slow, but it works. Let's see how GCE works with 128 volumes :-)

/kind feature
/sig storage

Does this PR introduce a user-facing change?:

NONE

@msau42 @davidz627

msau42 · 2019-07-17T14:42:20Z

test/e2e/storage/testsuites/volumelimits.go

+			framework.ExpectNoError(err, "failed waiting for PVs to be deleted")
+		}()
+
+		// Create <limit> nr. of PVCs and pods. With one pod created above, we are 1 pod over the limit.


I think we need more PVCs per pod otherwise we will hit 110 pods per node limit first

Why is it so low???
Anyway, I was not able to even create 129 PVs:

I0717 13:28:50.828] STEP: Waiting for 129 PVCs Bound I0717 13:29:00.642] Jul 17 13:29:00.642: INFO: 30/129 of PVCs are Bound I0717 13:29:07.375] Jul 17 13:29:07.375: INFO: 37/129 of PVCs are Bound I0717 13:29:15.760] Jul 17 13:29:15.759: INFO: 43/129 of PVCs are Bound I0717 13:29:22.415] Jul 17 13:29:22.415: INFO: 52/129 of PVCs are Bound I0717 13:29:30.550] Jul 17 13:29:30.549: INFO: 65/129 of PVCs are Bound I0717 13:29:37.465] Jul 17 13:29:37.465: INFO: 72/129 of PVCs are Bound I0717 13:29:45.482] Jul 17 13:29:45.482: INFO: 80/129 of PVCs are Bound I0717 13:29:53.365] Jul 17 13:29:53.365: INFO: 87/129 of PVCs are Bound ... I0717 13:59:11.115] Jul 17 13:59:11.115: INFO: 105/129 of PVCs are Bound I0717 13:59:11.116] Jul 17 13:59:11.115: INFO: Unexpected error occurred: timed out waiting for the condition

Are you sure it's not because pods couldn't be scheduled? PD driver uses delayed binding

davidz627 · 2019-07-17T20:44:52Z

/cc @davidz627 @hantaowang

k8s-ci-robot · 2019-07-17T20:44:54Z

@davidz627: GitHub didn't allow me to request PR reviews from the following users: hantaowang.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @davidz627 @hantaowang

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.

davidz627 · 2019-07-17T20:47:09Z

@jsafrane need me to run the GCE version or will you handle it? Or the PR is not ready yet?

msau42 · 2019-07-17T20:50:24Z

@davidz627 it's getting run as part of pull-kubernetes-e2e-gce-csi-serial

jsafrane · 2019-07-18T14:04:52Z

I checked late binding both with AWS and mock driver, something is very fishy there. Some scheduler predicate allows only 16 unbound PVCs in a single pod to be scheduled, a pod with 17 unbound PVCs is Pending forever. My theory is that GCE and/or Azure predicate don't check provisioner: and assume the unbound PVCs are theirs. @bertinatto is looking at it.

jsafrane · 2019-08-01T16:37:22Z

@davidz627 @msau42, so I tried with 128 volumes and a pod with 128 volumes failed to run:

[snip] rpc error: code = Internal desc = unknown Attach error: failed when waiting for zonal op: operation operation-1564668114867-58f0eaf2d9379-022fe473-7f224368 failed (LIMIT_EXCEEDED): Exceeded limit 'maximum_persistent_disks' on resource 'e2e-4c57e689bd-930d0-minion-group-00hf'. Limit: 128.0

Is there any off-by-one error here? Or is there any other pod using one slot (logging, metrics, ...) running? It would be great if you debug the test on your end.

davidz627 · 2019-08-01T21:47:40Z

@jsafrane I will look into it

jsafrane · 2019-08-15T14:05:13Z

/retest

davidz627 · 2019-08-15T20:46:31Z

test/e2e/storage/drivers/in_tree.go


-	return testsuites.GetStorageClass(provisioner, parameters, nil, ns, suffix)
+	return testsuites.GetStorageClass(provisioner, parameters, &delayedBinding, ns, suffix)


do we still want to "turn on delayed binding" for these suites in this PR?

No, we don't. Removed

still here :)

davidz627 · 2019-08-15T20:48:07Z

test/e2e/storage/testsuites/volumelimits.go

+	// And one extra pod with a CSI volume should get Pending with a condition
+	// that says it's unschedulable because of volume limit.
+	// BEWARE: the test may create lot of volumes and it's really slow.
+	ginkgo.It("should support volume limits [Slow][Serial]", func() {


would this test also be considered [Disruptive] as it would prevent further scheduling of pods using disks onto this node?

IMO, the test does not break anything (e.g. it does not kill kubelet). It just needs to be alone on a node and that's why we have [Serial].

davidz627 · 2019-08-15T20:48:43Z

test/e2e/storage/testsuites/volumelimits.go

+			ginkgo.Skip(fmt.Sprintf("driver %s does not support volume limits", driverInfo.Name))
+		}
+		var dDriver DynamicPVTestDriver
+		if dDriver = driver.(DynamicPVTestDriver); dDriver == nil {


I thought these checks were all done in some helper function somewhere thats shared between test suites

Technically the error path is not reachable. Retyping driver to DynamicPVTestDriver should always succeed, but I have a safety check just to be sure.

davidz627 · 2019-08-15T20:50:33Z

test/e2e/storage/testsuites/volumelimits.go

+		defer l.resource.cleanupResource()
+
+		// Prepare cleanup
+		defer func() {


maybe split some of this cleanup function into helper functions, it's very long and complicated right now

Moved to separate func.

davidz627 · 2019-08-15T20:51:27Z

test/e2e/storage/testsuites/volumelimits.go

+
+		l.pvNames = sets.NewString()
+		ginkgo.By("Waiting for all PVCs to get Bound")
+		err = wait.Poll(5*time.Second, testSlowMultiplier*framework.PVBindingTimeout, func() (bool, error) {


is there an existing helper function for this?

Not for array of PVCs, only single PVC.

+ I need to collect PV names to make sure they're deleted before the test ends. It takes ages and the test suite could end before the PVs are detached + deleted.

Moved to separate func.

davidz627 · 2019-08-15T20:51:46Z

test/e2e/storage/testsuites/volumelimits.go

+		l.unschedulablePod, err = l.cs.CoreV1().Pods(l.ns.Name).Create(pod)
+
+		ginkgo.By("Waiting for the pod to get unschedulable")
+		err = wait.Poll(5*time.Second, framework.PodStartTimeout, func() (bool, error) {


helper function?

Found WaitForPodCondition, but it's not that shorter.

davidz627

lgtm modulo the extra code change for delayed binding

davidz627 · 2019-08-16T18:07:09Z

test/e2e/storage/drivers/in_tree.go


-	return testsuites.GetStorageClass(provisioner, parameters, nil, ns, suffix)
+	return testsuites.GetStorageClass(provisioner, parameters, &delayedBinding, ns, suffix)


do we still want to turn on delayed binding?

davidz627 · 2019-08-16T18:07:24Z

test/e2e/storage/drivers/in_tree.go


-	return testsuites.GetStorageClass(provisioner, parameters, nil, ns, suffix)
+	return testsuites.GetStorageClass(provisioner, parameters, &delayedBinding, ns, suffix)


still here :)

bertinatto · 2019-08-23T15:33:16Z

/test pull-kubernetes-bazel-build

bertinatto · 2019-08-26T09:26:18Z

@davidz627, @msau42: I addressed the comments, could you take another look?

davidz627

couple small comments, lgtm besides those

davidz627 · 2019-08-26T22:38:36Z

test/e2e/storage/testsuites/volumelimits.go

+
+		// Create <limit> PVCs for one gigantic pod.
+		for i := 0; i < limit; i++ {
+			ginkgo.By(fmt.Sprintf("Creating volume %d/%d", i+1, limit))


this ginkgo.By seems like it's going to be pretty spammy, might not be necessary?

moved above loop

davidz627 · 2019-08-26T22:38:52Z

test/e2e/storage/testsuites/volumelimits.go

+		}
+
+		// Create the gigantic pod with all <limit> PVCs
+		ginkgo.By("Creating pod")


nit:more context on what kind of pod

davidz627 · 2019-08-26T22:40:54Z

test/e2e/storage/testsuites/volumelimits.go

+		err = e2epod.WaitForPodCondition(l.cs, l.ns.Name, l.unschedulablePod.Name, "Unschedulable", framework.PodStartTimeout, func(pod *v1.Pod) (bool, error) {
+			if pod.Status.Phase == v1.PodPending {
+				for _, cond := range pod.Status.Conditions {
+					matched, _ := regexp.MatchString("max.+volume.+count", cond.Message)


What could be between max, volume, and count besides a space? This regex is pretty prescriptive it might as well be a substring match.

This is the string it's trying to match, so it's only a space.

I think @jsafrane wanted to use the same approach used in the mock driver.

davidz627 · 2019-08-26T22:43:44Z

test/e2e/storage/testsuites/volumelimits.go

+		if len(nodeList.Items) != 0 {
+			nodeName = nodeList.Items[0].Name
+		} else {
+			e2elog.Failf("Unable to find ready and schedulable Node")


nit: inconsistent usage of e2elog and ginkgo. above we have a ginkgo.Fail("...") and here we have e2elog.Failf("..."), also logging through e2elog.Logf and ginkgo.By.

Replace one usage of ginkgo.Fail and removed a redundant e2elog.Log.

The rest seems consistent with other files (like provisioning.go), where (AFAIU) ginkgo is used to delimit/document the logic of the test and e2elog for logging additional information.

Looks like someone is trying to clean this up rn, lets be consistent with what they're doing
#81983

bertinatto · 2019-08-27T15:10:25Z

/retest

bertinatto · 2019-08-27T15:43:56Z

/test pull-kubernetes-conformance-kind-ipv6

flaky job

davidz627 · 2019-08-27T18:07:13Z

lgtm please rebase to fewer descriptive commits. Thanks!

The test creates as many PVs and pods as the driver/plugin reports to support on a single node.

bertinatto · 2019-08-28T09:03:53Z

/test pull-kubernetes-bazel-build

bertinatto · 2019-08-28T12:23:26Z

@davidz627, squashed commits and fixed some function calls (e.g., MakeSecPod) that have been moved recently. PTAL

davidz627 · 2019-08-28T18:17:52Z

/lgtm
thanks!

davidz627 · 2019-08-28T18:18:04Z

/priority important-soon

davidz627 · 2019-08-28T18:18:18Z

/hold cancel

davidz627 · 2019-08-28T20:09:37Z

/retest

k8s-ci-robot requested review from copejon and verult July 17, 2019 09:20

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 17, 2019

jsafrane force-pushed the e2e-volume-limits branch 2 times, most recently from 0a8fd7a to d28c254 Compare July 17, 2019 12:25

msau42 reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot requested a review from davidz627 July 17, 2019 20:44

bertinatto mentioned this pull request Jul 19, 2019

Predicates allow only 16 unbound PVCs #80352

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2019

jsafrane force-pushed the e2e-volume-limits branch 2 times, most recently from a5c811f to 2d2c6b4 Compare July 22, 2019 10:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2019

jsafrane force-pushed the e2e-volume-limits branch from 2d2c6b4 to cb5bd05 Compare July 22, 2019 11:52

jsafrane force-pushed the e2e-volume-limits branch 2 times, most recently from dedced8 to bcb9687 Compare August 1, 2019 12:23

davidz627 reviewed Aug 15, 2019

View reviewed changes

jsafrane force-pushed the e2e-volume-limits branch 2 times, most recently from d771ed3 to 29c688d Compare August 16, 2019 13:02

davidz627 reviewed Aug 16, 2019

View reviewed changes

bertinatto force-pushed the e2e-volume-limits branch 2 times, most recently from 6906d21 to 7092875 Compare August 23, 2019 15:00

davidz627 reviewed Aug 26, 2019

View reviewed changes

bertinatto force-pushed the e2e-volume-limits branch 3 times, most recently from e6cafc1 to 61ee449 Compare August 28, 2019 08:20

Add e2e test for volume limits

90d0991

The test creates as many PVs and pods as the driver/plugin reports to support on a single node.

bertinatto force-pushed the e2e-volume-limits branch from 61ee449 to 90d0991 Compare August 28, 2019 08:39

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

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 28, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2019

k8s-ci-robot merged commit c4ccb62 into kubernetes:master Aug 29, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 29, 2019


		return testsuites.GetStorageClass(provisioner, parameters, nil, ns, suffix)
		return testsuites.GetStorageClass(provisioner, parameters, &delayedBinding, ns, suffix)

Add e2e test for CSI volume limits #80247

Add e2e test for CSI volume limits #80247

Conversation

jsafrane commented Jul 17, 2019

Choose a reason for hiding this comment

jsafrane Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

davidz627 commented Jul 17, 2019

msau42 commented Jul 17, 2019

jsafrane commented Jul 18, 2019

jsafrane commented Aug 1, 2019

davidz627 commented Aug 1, 2019

jsafrane commented Aug 15, 2019

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

jsafrane Aug 16, 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

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

bertinatto commented Aug 23, 2019

bertinatto commented Aug 26, 2019

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

bertinatto commented Aug 27, 2019

bertinatto commented Aug 27, 2019

davidz627 commented Aug 27, 2019 • edited Loading

bertinatto commented Aug 28, 2019

bertinatto commented Aug 28, 2019

davidz627 commented Aug 28, 2019

davidz627 commented Aug 28, 2019

davidz627 commented Aug 28, 2019

davidz627 commented Aug 28, 2019

jsafrane Jul 17, 2019 •

edited

Loading

jsafrane Aug 16, 2019 •

edited

Loading

davidz627 commented Aug 27, 2019 •

edited

Loading