feat(e2e): refactor hard-coded provision size #78306

hoyho · 2019-05-24T15:23:00Z

test/e2e/storage/testsuites creates volumes dynamically. Initially, the size of those volumes was
hard-coded in the test, which prevented using the tests with storage backends that couldn't support
that hard-coded size

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
some e2e test will fail due to the hard coding volume size 5Gi and some
diver not support.

previously, there's a discussion and PR to solve this. But after half year,
we need to redo it base on PR because some code have changed and not easy to rebase or test

see #72080

Which issue(s) this PR fixes:

Fixes #72080

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

k8s-ci-robot · 2019-05-24T15:23:02Z

Welcome @hoyho!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-05-24T15:23:02Z

@hoyho: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2019-05-24T15:23:09Z

Hi @hoyho. 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.

neolit123 · 2019-05-27T17:22:16Z

/sig storage

neolit123 · 2019-05-27T17:23:18Z

Merge branch 'master' into feat/e2e-storage-dynamic-size

there shouldn't be such merge commits. please squash the change into one commit.
also, if you want reviewers to take a look at this try pinging #sig-storage on slack.

neolit123 · 2019-05-27T17:23:24Z

/release-note-none

jsafrane · 2019-05-28T11:18:25Z

/ok-to-test

rootfs · 2019-05-29T12:27:41Z

/assign @jsafrane

hoyho · 2019-09-30T18:40:10Z

ping @hoyho any updates?
/remove-area e2e-test-framework
/remove-sig testing
/priority important-soon
/milestone v1.17

WIP

hoyho · 2019-10-02T22:05:07Z

/test pull-kubernetes-e2e-gce-storage-slow

msau42 · 2019-10-04T22:17:58Z

/retest

hoyho · 2019-10-05T11:09:01Z

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-conformance-kind-ipv6

pohly

Don't we have some test which actually writes quite a bit of data? volume IO or something? That test then may need a larger minimum volume size.

I can't think of a reason why a test should have a maximum volume size.

pohly · 2019-10-07T13:38:38Z

test/e2e/storage/drivers/csi.go

@@ -80,6 +81,9 @@ func initHostPathCSIDriver(name string, capabilities map[testsuites.Capability]b
 			SupportedFsType: sets.NewString(
 				"", // Default fsType
 			),
+			SupportedSizeRange: volume.SizeRange{
+				Min: "5Gi",
+			},


This can be removed. The hostpath driver is one of those drivers where we can be sure that it has no minimum size.

This can be removed. The hostpath driver is one of those drivers where we can be sure that it has no minimum size.

I am afraid the hostpath driver should have a minimum size 1Mb?
From source code here https://github.com/kubernetes-csi/csi-driver-host-path/blob/3488dc7f994e33485629b86b69a6f34ebb7ef2d9/pkg/hostpath/hostpath.go#L181 If the volume access type is 'block', then the driver will call fallocate with size unit M .
Too small size will be invalid.

Right, I was thinking of filesystem mode. For block mode I am not sure whether the driver should round up internally - this sounds like a bug in the driver.

Anyway, let's then keep the minimum size.

pohly · 2019-10-07T13:41:00Z

test/e2e/storage/external/external.go

-	// provisioned volumes. Default is "5GiB".
-	ClaimSize string
+	// provisioned volumes.
+	SupportedSizeRange volume.SizeRange


ClaimSize must be replaced with SupportedSizeRange to match the field name.

pohly · 2019-10-07T13:43:47Z

test/e2e/storage/testsuites/base.go

@@ -181,7 +186,7 @@ type genericVolumeTestResource struct {

 var _ TestResource = &genericVolumeTestResource{}

-func createGenericVolumeTestResource(driver TestDriver, config *PerTestConfig, pattern testpatterns.TestPattern) *genericVolumeTestResource {
+func createGenericVolumeTestResource(driver TestDriver, config *PerTestConfig, pattern testpatterns.TestPattern, tsSupportedSizeRange volume.SizeRange) *genericVolumeTestResource {


The ts prefix is non-obvious here. What does it stand for? "Test"?

What about testVolumeSizeRange as variable name?

pohly · 2019-10-07T13:45:13Z

test/e2e/storage/testsuites/base.go

@@ -214,7 +219,11 @@ func createGenericVolumeTestResource(driver TestDriver, config *PerTestConfig, p
 	case testpatterns.DynamicPV:
 		framework.Logf("Creating resource for dynamic PV")
 		if dDriver, ok := driver.(DynamicPVTestDriver); ok {
-			claimSize := dDriver.GetClaimSize()
+			var err error
+			dSupportedSizeRange := dDriver.GetDriverInfo().SupportedSizeRange


Same here - does "d" stand for "driver" or "dynamic"? Better spell it out.

For consistency with the proposed "testVolumeSizeRange", how about "driverVolumeSizeRange"?

pohly · 2019-10-07T13:46:31Z

test/e2e/storage/testsuites/multivolume.go

@@ -49,6 +50,10 @@ func InitMultiVolumeTestSuite() TestSuite {
 				testpatterns.BlockVolModePreprovisionedPV,
 				testpatterns.BlockVolModeDynamicPV,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


Why does the test have a max size? I don't think it needs one.

pohly · 2019-10-07T13:47:23Z

test/e2e/storage/testsuites/subpath.go

@@ -65,6 +65,10 @@ func InitSubPathTestSuite() TestSuite {
 				testpatterns.DefaultFsDynamicPV,
 				testpatterns.NtfsDynamicPV,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


pohly · 2019-10-07T14:15:20Z

test/e2e/storage/testsuites/volume_expand.go

@@ -57,6 +58,10 @@ func InitVolumeExpandTestSuite() TestSuite {
 				testpatterns.DefaultFsDynamicPVAllowExpansion,
 				testpatterns.BlockVolModeDynamicPVAllowExpansion,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


pohly · 2019-10-07T14:15:31Z

test/e2e/storage/testsuites/volume_io.go

@@ -68,6 +68,10 @@ func InitVolumeIOTestSuite() TestSuite {
 				testpatterns.DefaultFsPreprovisionedPV,
 				testpatterns.DefaultFsDynamicPV,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


pohly · 2019-10-07T14:15:46Z

test/e2e/storage/testsuites/volumemode.go

@@ -60,6 +61,10 @@ func InitVolumeModeTestSuite() TestSuite {
 				testpatterns.BlockVolModePreprovisionedPV,
 				testpatterns.BlockVolModeDynamicPV,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


pohly · 2019-10-07T14:15:58Z

test/e2e/storage/testsuites/volumes.go

@@ -71,6 +71,10 @@ func InitVolumesTestSuite() TestSuite {
 				testpatterns.BlockVolModePreprovisionedPV,
 				testpatterns.BlockVolModeDynamicPV,
 			},
+			supportedSizeRange: volume.SizeRange{
+				Min: "1Mi",
+				Max: "10Gi",


test/e2e/storage/testsuites creates volumes dynamically. Initially, the size of those volumes was hard-coded in the test, which prevented using the tests with storage backends that couldn't support that hard-coded size

hoyho · 2019-10-08T00:33:25Z

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kind

pohly · 2019-10-08T06:05:57Z

/lgtm

msau42 · 2019-10-08T21:29:48Z

/approve

k8s-ci-robot · 2019-10-08T21:30:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hoyho, 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/framework/volume/OWNERS~~ [msau42]
~~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

…ic-size feat(e2e): refactor hard-coded provision size

k8s-ci-robot requested review from rootfs and saad-ali May 24, 2019 15:23

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label May 27, 2019

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 27, 2019

hoyho force-pushed the feat/e2e-storage-dynamic-size branch from 55d845c to 3bc62cb Compare May 28, 2019 03:36

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 May 28, 2019

k8s-ci-robot assigned jsafrane May 29, 2019

k8s-ci-robot removed sig/testing Categorizes an issue or PR as relevant to SIG Testing. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 29, 2019

hoyho force-pushed the feat/e2e-storage-dynamic-size branch from 075da07 to ef57183 Compare October 2, 2019 08:09

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 2, 2019

hoyho force-pushed the feat/e2e-storage-dynamic-size branch from ef57183 to 37e56a8 Compare October 2, 2019 14:18

hoyho force-pushed the feat/e2e-storage-dynamic-size branch from 37e56a8 to 0017048 Compare October 4, 2019 16:08

hoyho requested review from pohly, oomichi and msau42 October 5, 2019 12:30

pohly suggested changes Oct 7, 2019

View reviewed changes

hoyho added 2 commits October 8, 2019 01:33

feat(e2e): refactor hard-coded provision size

23320c9

test/e2e/storage/testsuites creates volumes dynamically. Initially, the size of those volumes was hard-coded in the test, which prevented using the tests with storage backends that couldn't support that hard-coded size

Add unit test for function getSizeRangesIntersection

b5c1432

hoyho force-pushed the feat/e2e-storage-dynamic-size branch from 0017048 to b5c1432 Compare October 7, 2019 17:34

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2019

k8s-ci-robot merged commit 05474a6 into kubernetes:master Oct 9, 2019

This was referenced Oct 9, 2019

Storage DriverInfo has redundant fields MaxSize and SupportedSizeRange #83689

Closed

Add Supported size range to driver info config kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#396

Merged

pohly mentioned this pull request Oct 14, 2019

Direct Mode: NodeStageVolume fails when created with 1MB size intel/pmem-csi#429

Closed

ohsewon pushed a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019

Merge pull request kubernetes#78306 from hoyho/feat/e2e-storage-dynam…

7080ba2

…ic-size feat(e2e): refactor hard-coded provision size

hoyho mentioned this pull request Oct 18, 2019

cleanup duplicated MaxFileSize in e2e storage test #84079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(e2e): refactor hard-coded provision size #78306

feat(e2e): refactor hard-coded provision size #78306

hoyho commented May 24, 2019

k8s-ci-robot commented May 24, 2019

k8s-ci-robot commented May 24, 2019

k8s-ci-robot commented May 24, 2019

neolit123 commented May 27, 2019

neolit123 commented May 27, 2019

neolit123 commented May 27, 2019

jsafrane commented May 28, 2019

rootfs commented May 29, 2019

hoyho commented Sep 30, 2019

hoyho commented Oct 2, 2019

msau42 commented Oct 4, 2019

hoyho commented Oct 5, 2019

pohly left a comment

pohly Oct 7, 2019

hoyho Oct 7, 2019

pohly Oct 8, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

pohly Oct 7, 2019

hoyho commented Oct 8, 2019

pohly commented Oct 8, 2019

msau42 commented Oct 8, 2019

k8s-ci-robot commented Oct 8, 2019

feat(e2e): refactor hard-coded provision size #78306

feat(e2e): refactor hard-coded provision size #78306

Conversation

hoyho commented May 24, 2019

k8s-ci-robot commented May 24, 2019

k8s-ci-robot commented May 24, 2019

k8s-ci-robot commented May 24, 2019

neolit123 commented May 27, 2019

neolit123 commented May 27, 2019

neolit123 commented May 27, 2019

jsafrane commented May 28, 2019

rootfs commented May 29, 2019

hoyho commented Sep 30, 2019

hoyho commented Oct 2, 2019

msau42 commented Oct 4, 2019

hoyho commented Oct 5, 2019

pohly 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

hoyho commented Oct 8, 2019

pohly commented Oct 8, 2019

msau42 commented Oct 8, 2019

k8s-ci-robot commented Oct 8, 2019