e2e: support long CSI driver names #86000

timoreimann · 2019-12-06T13:29:10Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The storage e2e test suite uses given CSI driver names as pod names. For pod names that also get enriched by a prefix and suffix, it is very easy to exceed the 63 character limit that pod names are subject to, thereby causing tests to fail. This is already the case for moderately sized CSI driver names like dobs.csi.digitalocean.com that we employ for DigitalOcean's CSI driver: about ~15 tests were failing on us due to this problem.

This change fixes the described problem by omitting the driver name from the pod name suffix.

It also allows us to drop VolumeResource.VolType.

This change runs CSI driver names through a shortening function that reduces each domain name label of the driver name to its first letter. For all practical purposes, this should prevent pod name character length violations. (Original approach that was changed along the review.)

Special notes for your reviewer:

I discussed the matter with @msau42 a couple of days ago on Slack. We concluded that truncating or short name usage would make sense.

I did not implement either of the two directly because of the following reasons:

Truncating means removing/shortening either the prefix or the (randomly generated) suffix of a pod name, with the implications that pods are harder to associate to tests or could cause collisions with other pod names, respectively. Neither seemed ideal.
The short name is currently not part of the interface that is passed to individual tests. Unless we were willing to do some rather nasty type assertions, we would have to extend the interface used to retrieve test driver details from. This seemed like a rather invasive change.
Finally, the proposed solution of shortening by domain name labels is effective while still allowing to identify involved CSI drivers (with a bit of a squint).

Does this PR introduce a user-facing change?:

-->

NONE

k8s-ci-robot · 2019-12-06T13:29:19Z

Hi @timoreimann. Thanks for your PR.

I'm waiting for a kubernetes 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.

timoreimann · 2019-12-06T13:30:31Z

/sig storage

timoreimann · 2019-12-06T13:44:06Z

/assign @msau42

msau42 · 2019-12-06T20:30:34Z

/ok-to-test

timoreimann · 2019-12-07T14:21:22Z

/retest

msau42 · 2019-12-10T02:25:32Z

test/e2e/storage/testsuites/base.go

@@ -199,7 +199,7 @@ func CreateVolumeResource(driver TestDriver, config *PerTestConfig, pattern test
 		framework.Logf("Creating resource for inline volume")
 		if iDriver, ok := driver.(InlineVolumeTestDriver); ok {
 			r.VolSource = iDriver.GetVolumeSource(false, pattern.FsType, r.Volume)
-			r.VolType = dInfo.Name
+			r.VolType = shortenCSIDriverName(dInfo.Name)


My understanding is that dInfo.Name comes from the user-provided external config file, and this doesn't actually have to equal the full csi driver name, it just needs to be a descriptive name. Are you seeing otherwise?

If it doesn't actually have to match the driver name, can we just document the name length limit?

My understanding is that it has to be the full CSI driver name: The name is first declared as the provisioner and then fed into the StorageClass value we create.

I also just ran a quick test and selected a different name, but that makes tests hang/fail with

waiting for a volume to be created, either by external provisioner "foobar-driver" or manually created by system administrator

I think the tests need some way to reference the provisioner, and at least with the name-based approach the name has to match that.

Please let me know if I misunderstood.

Looking at the external CSI hostpath driver, it seems that it also defines the full name and injects it into the YAML configuration file accordingly.

I also see the documentation getting updated to use what looks like the full name.

Ok thanks for pointing that out. I'm actually trying to search for which tests are using r.VolType, and I only found the one here:

kubernetes/test/e2e/storage/testsuites/volumes.go

Line 211 in a6f41a4

suffix := generateSuffixForPodName(volumeType)

I'm actually wondering if we can remove this r.VolType altogether, and use a different naming scheme for that test.

There's another occurrence in

kubernetes/test/e2e/storage/testsuites/subpath.go

Line 503 in eef4c00

suffix = generateSuffixForPodName(volumeType)

However, it's not just one or two tests that are affected due to the dynamic nature of how storage tests are generated. Below is an example output of our CSI tests running without the patch this PR proposes, showing 16 failures seemingly all of them failing due to the length problem:

Summarizing 16 Failures: [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should fail if subpath file is outside the volume [Slow][LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support creating multiple subpath from same volumes [Slow] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support restarting containers using directory as subpath [Slow] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should fail if subpath directory is outside the volume [Slow] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support file as subpath [LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support existing single file [LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support existing directories when readOnly specified in the volumeSource [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support readOnly file specified in the volumeMount [LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support restarting containers using file as subpath [Slow][LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should be able to unmount after the subpath directory is deleted [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support non-existent path [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support existing directory [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should fail if subpath with backstepping is outside the volume [Slow] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should support readOnly directory specified in the volumeMount [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should fail if non-existent subpath is outside the volume [Slow][LinuxOnly] [Fail] External Storage [Driver: dobs.csi.digitalocean.com-dev] [Testpattern: Dynamic PV (default fs)] subPath [It] should verify container cannot write to subpath readonly volumes [Slow] Ran 27 of 5062 Specs in 394.036 seconds FAIL! -- 11 Passed | 16 Failed | 0 Pending | 5035 Skipped

This is only to show the extent of the tests that are affected; it's not to say we possibly can't address this problem by fixing the two occurrences of the generateSuffixForPodName function call (or its implementation).

I tracked git history down where the volume type was first introduced for the pod name suffix: it looks like the addition of the e2e subpath tests brought it in.

@jsafrane I see you were the author back then. Maybe you have context around whether the volume type is strictly necessary to be part of the suffix name.

To ease the discussion, I just pushed a commit that omits the volume type from the pod name suffix. The tests still ran successfully for my CSI driver with that change.

Maybe you have context around whether the volume type is strictly necessary to be part of the suffix name.

No, I just wanted something that's easy to read in e2e logs. The pod names can be virtually anything.

Yeah I like the 2nd commit better :-) Let's also just remove r.VolType altogether?

Removed r.VolType and made some necessary adjustments. (Parts of subpath.go were still referencing it.)

I tried to reconcile both desires to have pod names readable in logs and long driver names not break tests by only including the actual volume type (i.e., excluding the driver name) in pod name suffices. I feel that's a good compromise between the two goals.

Please let me know what you think.

timoreimann · 2019-12-11T12:56:03Z

/retest

msau42

lgtm, just one minor comment. Thanks for looking into this!
/approve

msau42 · 2019-12-13T20:56:23Z

test/e2e/storage/testsuites/subpath.go

@@ -348,9 +350,9 @@ func (s *subPathTestSuite) DefineTests(driver TestDriver, pattern testpatterns.T
 		init()
 		defer cleanup()

-		if strings.HasPrefix(l.resource.VolType, "hostPath") || strings.HasPrefix(l.resource.VolType, "csi-hostpath") {
+		if strings.HasPrefix(driverName, "hostPath") || strings.HasPrefix(driverName, "csi-hostpath") {


the "csi-hostpath" check can be removed

msau42 · 2019-12-13T21:06:04Z

Also, can you squash your commits?

/priority important-soon

k8s-ci-robot · 2019-12-13T21:07:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The storage e2e test suite uses given CSI driver names as pod names. For pod names that also get enriched by a prefix and suffix, it is very easy to exceed the 63 character limit that pod names are subject to, thereby causing tests to fail. This change fixes the described problem by omitting the driver name from the pod name suffix. It also allows us to drop VolumeResource.VolType.

timoreimann · 2019-12-13T21:57:43Z

Commits squashed.

Thanks for the review, @msau42.

msau42 · 2019-12-13T23:05:17Z

/lgtm

timoreimann · 2019-12-13T23:51:55Z

/retest

fejta-bot · 2019-12-14T05:02:22Z

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

Review the full test history for this PR.