Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip preprovisioned and inline volume tests if driver supports dynamic provisioning #81375

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@msau42
Copy link
Member

commented Aug 13, 2019

What type of PR is this?
/kind failing-test

What this PR does / why we need it:
Most volume features don't care if it's an inline, preprovisioned PV or dynamic PV. If the driver supports dynamic provisioning, then we don't need to run the same test against preprovisioned or inline volume types for the same driver. I've added this behavior to all storage test suites except for:

  • volumes.go: basic test suite one which tests a simple read/write + persistence, and we want to test all permutations here
  • volumemode.go: failure modes are different depending on preprovisioned vs dynamic pv
  • other test suites that only support one VolType pattern

This will help reduce the number of e2e tests.

Which issue(s) this PR fixes:

Fixes #
Helps with #81197

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@davidz627

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

For Migration I think we want to test Pre-Provisioned + Inline explicitly. Can we keep them on with a flag or something? we might be able to reuse -storage.migratedPlugins

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

The basic volumes test suite will still run with preprovisioned and inline patterns. Is that sufficient for migration? (+ the test cases not in the framework)

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Also please help review if any of the tested functionality would actually behave differently between inline, preprovisioned or dynamic cases

@msau42 msau42 changed the title Skip preprovisioned and inline volume tests if driver supports dynamic provisioning WIP: Skip preprovisioned and inline volume tests if driver supports dynamic provisioning Aug 13, 2019

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch from 5224b81 to 61ece14 Aug 13, 2019

@mkimuram

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

I agree the basic idea of skipping duplicated tests between volume types.
However, I hope that at least one of the drivers should be tested for all tests with inline and pre-provisioned. (Currently, in-tree gluster driver doesn't setup dynamic provisioner in e2e, so it is expected to be tested for all tests with inline and pre-provisioned, as I expect.)

Also please help review if any of the tested functionality would actually behave differently between inline, pre-provisioned or dynamic cases

Block volume tests for block unsupported driver will behave differently between pre-provisioned and dynamic provisioning, because they fail in different phases. For other places, I don't think that we added similar checks for different behavior.

(I will check the codes in details later, but most test codes that leverage createGenericVolumeTestResource should handle volumeSource without caring volume types.)

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch 3 times, most recently from b3a3061 to 8808954 Aug 13, 2019

@msau42 msau42 changed the title WIP: Skip preprovisioned and inline volume tests if driver supports dynamic provisioning Skip preprovisioned and inline volume tests if driver supports dynamic provisioning Aug 14, 2019

test/e2e/storage/testsuites/base.go Outdated Show resolved Hide resolved
test/e2e/storage/testsuites/disruptive.go Outdated Show resolved Hide resolved

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch 2 times, most recently from 10819de to ca60e50 Aug 14, 2019

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

/retest

@jsafrane
Copy link
Member

left a comment

LGTM-ish, I have just one nit

test/e2e/storage/testsuites/disruptive.go Outdated Show resolved Hide resolved

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch 2 times, most recently from 4550f06 to abb0cff Aug 15, 2019

@@ -47,11 +47,14 @@ func InitDisruptiveTestSuite() TestSuite {
},

This comment has been minimized.

Copy link
@msau42

msau42 Aug 15, 2019

Author Member

@mkimuram @jsafrane what do you think of removing the disruptive tests for the FS volume mode? It should already be covered by subpath disruptive tests.

This comment has been minimized.

Copy link
@msau42

msau42 Aug 15, 2019

Author Member

Pushed new change removing them

This comment has been minimized.

Copy link
@mkimuram

mkimuram Aug 15, 2019

Contributor

In my understanding, (D-2) is subset of (S-1) and (D-3) is subset of (S-2). Differences between them are whether they check subpath or not.
So, (D-2) and (D-3) will be covered by subpath tests, but (D-1) won't.
To test (D-1), it would be worth keeping disruptive tests for filesystem.
(Instead, we will be able to modify runTestFile in disruptiveTestTable to skip (D-2) and (D-3), if needed.)

[Subpath test]
  (S-1) "should unmount if pod is gracefully deleted while kubelet is down [Disruptive][Slow][LinuxOnly]"
  (S-2) "should unmount if pod is force deleted while kubelet is down [Disruptive][Slow][LinuxOnly]"

[Disruptive test]
  (D-1) "Should test that pv written before kubelet restart is readable after restart."
  (D-2) "Should test that pv used in a pod that is deleted while the kubelet is down cleans up when the kubelet returns."
  (D-3) "Should test that pv used in a pod that is force deleted while the kubelet is down cleans up when the kubelet returns."

This comment has been minimized.

Copy link
@msau42

msau42 Aug 15, 2019

Author Member

Good catch, pushed a new commit that disables the last 2 tests for file mode.

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch from abb0cff to 393be0d Aug 15, 2019

msau42 added some commits Aug 13, 2019

Skip preprovisioned and inline volume tests if driver supports dynami…
…c provisioning.

Also remove FS VolMode disruptive tests because they are already covered
through the subpath disruptive test cases.

Change-Id: Ice4b30b0d8fdcb1f7fd61e54d27f53557de9f13a
Disable kubelet restart tests for file volmode
Change-Id: I016cba4aba11d6e8574d4bc2d1f26284a068e562

@msau42 msau42 force-pushed the msau42:reduce-storage-e2es branch from 393be0d to 62f0b7d Aug 15, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Aug 15, 2019

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

/retest

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

/priority important-soon

@msau42

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

/retest

@jsafrane

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

LGTM

@mkimuram

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 16, 2019

@k8s-ci-robot k8s-ci-robot merged commit 8f0d626 into kubernetes:master Aug 16, 2019

21 of 23 checks passed

pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation msau42 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.