Storage testsuite refactor & cleanup #96573

Jiawei0227 · 2020-11-13T19:57:18Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Right now there are too many different things in the testsuite package which makes it really hard to read and understand.

This PR is to code refactor the test/e2e/storage/testsuite package to extract all the public type definition out and move to a new package called test/e2e/storage/api. The ultimate goal here is to keep the testsuite package as clean and easy to extend as possible. And also isolate the type definition so that we can make the testsuite pkg no different than a third party package. This also makes sure a third party user is easy to use these code and write their own testsuite.

Cleanup all the f.BeforeEach() before new framework to move all the
testskips in the new SkipUnsupportedTests() to make the structure easier.
And provide the standard way of RegisterTests()
Add a InitCustomXXXTestSuite(patterns []patterns) function for each
TestSuite to enable custom test suite definition.
Extract TestSuite, TestDriver, TestPattern, TestConfig
and VolumeResource, SnapshotVolumeResource from testsuite
package and put them into a new package called api.
There are also some utility move around in this PR which is relevant to keep the pkg dependency clean and right.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This is a first step to the testsuite cleanup. We will need to reduce redundant code after this and make sure the utility we write is not building the same wheel again and again.

Does this PR introduce a user-facing change?:

NONE

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

NONE

/sig storage
/sig testing

Jiawei0227 · 2020-11-13T21:22:43Z

/assign @chrishenzie @jingxu97
/cc @msau42

Jiawei0227 · 2020-11-13T21:28:32Z

test/e2e/storage/utils/utils.go

+}
+
+// CreateVolumeSource creates a volume source object
+func CreateVolumeSource(pvcName string, readOnly bool) *v1.VolumeSource {


@chrishenzie Replying to your concern on the CreateVolumeSource function. I think you are absolutely right. Ideally this function should be a private function in the api package. However, since previously it is under testsuite package so it has been abused by provisioning testsuite:

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

Line 225 in c970a46

Volume: *createVolumeSource(claim.Name, false /* readOnly */),

So definitely if we fix provisioning testsuite to not use this, I can put it under storageapi and make it unexported. One thing I noticed when doing this refactor is that a lot of functions have been abused. This PR has been too large to also include those cleanup. So I think it would be good to focus on move the package first. And then we can do clean up for each individual testsuite to make it clean. Does that make sense?

Jiawei0227 · 2020-11-13T21:33:39Z

test/e2e/framework/volume/fixtures.go

+}
+
+// TODO: put this under e2epod once https://github.com/kubernetes/kubernetes/issues/81245
+// is resolved. Otherwise there will be dependency issue.


@chrishenzie Replying to your concern on this function:
There are two possible location I think this function could lie in other than this:

test/e2e/storage/utils/pod.go
The problem with that is this function is being used by "VerifyExecInPodSucceed" and "VerifyExecInPodFail" which are used by "CheckVolumeModeOfPath".
And this "CheckVolumeModeOfPath" is used by e2evolume. So ideally we dont want to put them under storage utils because that will have a situation where parent package import subpackage. This against our principle.

test/e2e/framework/pod
This is a better place for sure. However, currently there is a dependency issue Core e2e test framework should not import sub e2e test frameworks #81245 which prevent us from putting it there. So I add a comment to this. Once that is fixed we can move it over.

Jiawei0227 · 2020-11-13T21:47:01Z

test/e2e/storage/api/testsuite.go

+	}
+
+	// 2. Check if fsType is supported
+	if !dInfo.SupportedFsType.Has(pattern.FsType) {


Replying to @jingxu97 question on #96538 (comment)

When we define an external driver, and e2e framework reads from the config file, it will always add "" so that makes sure it support default fsType

kubernetes/test/e2e/storage/external/external.go

Line 185 in da75c26

driver := &driverDefinition{

Jiawei0227 · 2020-11-13T21:52:58Z

@jingxu97 @chrishenzie Sorry for making so many different PRs. I think there are indeed a lot of things we can improve in the current testsuite package. I want to keep this PR as simple as I can but apparently this is the best I can, lol. So I think it would be good to focus on extracting api things to a new library in this PR. If there is any other changes not related to that, I will keep those in mind or create an issue if necessary and I will submit separate PR for them.

This PR does not have any functional change or code logic change. It is pure code refactor so the code for each testsuite that being executed should be the same. The goal is to make the code looks cleaner. If there is anything related to that you think we can change, please point them out and I will fix them.

Thanks!

jingxu97 · 2020-11-17T18:32:36Z

test/e2e/storage/utils/utils.go

-		}
-	}
-}
-
 // VerifyFSGroupInPod verifies that the passed in filePath contains the expectedFSGroup


how to decide which function goes to utils, and which one goes to e2evolume?

The principles I am using here are:

If this function has only been called by e2e/storage/..., it should go to e2e/storage/utils, because no other component is using it so lets just keep it at the lowest level

If there is any function that has been called outside of storage, it should go to a more general location namely e2evolume or e2epod.

In addition to this, dependency management here is that subpackage can import corepackage, but core cannot import sub.
That being said, previously e2evolume import e2e/storage/utils which is not the right direction. So I move these function up to the e2evolume to clear the dependency.

I think this direction of dependency also align with another effort from sig testing. Does this make sense?

jingxu97 · 2020-11-18T16:39:32Z

test/e2e/framework/volume/fixtures.go

@@ -696,3 +697,71 @@ func GetLinuxLabel() *v1.SELinuxOptions {
 	return &v1.SELinuxOptions{
 		Level: "s0:c0,c1"}
 }
+
+// CheckVolumeModeOfPath check mode of volume
+func CheckVolumeModeOfPath(f *framework.Framework, pod *v1.Pod, volMode v1.PersistentVolumeMode, path string) {


any other tests using this besides storage?

Yep, CheckVolumeModeOfPath is used by testVolumeContent in e2evolume and testVolumeContent is used by TestVolumeClient(), TestVolumeClient is used by e2e/common

https://github.com/kubernetes/kubernetes/search?q=TestVolumeClient

Hmm, maybe I can move everything related to it down and clear the dependency for everything uses TestVolumeClient. I will create a commit for that change to see if that makes the dependency look better

Just tried... It is too much change, almost move everything in e2evolume to storageutils.
The code has reference almost in all the function in e2evolume. I think let's make the dependency change in the next PR. And focus this PR on just restructure the testsuite. The utils definitely need some effort to clean up.

thanks, then I am ok with this PR.

Jiawei0227 · 2020-11-18T18:54:13Z

/retest

jingxu97 · 2020-11-18T23:10:21Z

lgtm,
@xing-yang @msau42 could you help review and approve?

Jiawei0227 · 2020-12-09T17:05:07Z

@msau42 @xing-yang
Can you help to review this PR?

test/e2e/storage/api/BUILD

msau42 · 2020-12-09T19:27:32Z

test/e2e/storage/api/snapshotresource.go

+// CreateSnapshot creates a VolumeSnapshotClass with given SnapshotDeletionPolicy and a VolumeSnapshot
+// from the VolumeSnapshotClass using a dynamic client.
+// Returns the unstructured VolumeSnapshotClass and VolumeSnapshot objects.
+func CreateSnapshot(sDriver SnapshottableTestDriver, config *PerTestConfig, pattern TestPattern, pvcName string, pvcNamespace string, timeouts *framework.TimeoutContext) (*unstructured.Unstructured, *unstructured.Unstructured) {


To me, this seems more of a util compared to the testsuite framework because it's helpers for specific tests. What is your method for distinction?

I think you are right, I need to move this to util

I doubled check on this and find it is not easy to do so because this function as you can tell from the signature:

func CreateSnapshot(sDriver SnapshottableTestDriver, config *PerTestConfig, pattern TestPattern, pvcName string, pvcNamespace string, timeouts *framework.TimeoutContext)

It has a lot of dependency of storage framework(SnapshottableTestDriver, PerTestConfig, TestPattern). That being said, it cannot be moved to storageutils. From my understanding, storageutils should be self-encapsulated. The dependency direction is framework can depend on storageutils while storageutils cannot depend on framework...

I feel like there are a lot of code refactor we can do around the utils for the testsuites. But I think this can be our first step towards that. Let's make this in and then we can babystep the utils refactor and cleanup slowly.

Cleanup all the f.BeforeEach() before new framework to move all the testskips in the new SkipUnsupportedTests() to make the structure easier. And provide the standard way of RegisterTests() Add a InitCustomXXXTestSuite(patterns []patterns) function for each TestSuite to enable custom test suite definition.

Extract TestSuite, TestDriver, TestPattern, TestConfig and VolumeResource, SnapshotVolumeResource from testsuite package and put them into a new package called api. The ultimate goal here is to make the testsuites as clean as possible. And only testsuites in the package.

msau42 · 2020-12-11T17:42:16Z

/lgtm
/approve
/triage accepted

Thanks!

k8s-ci-robot · 2020-12-11T17:42:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Jiawei0227 · 2020-12-11T18:41:24Z

/retest

k8s-ci-robot requested review from copejon and xing-yang November 13, 2020 19:57

Jiawei0227 force-pushed the skiptest branch from cdd6682 to 4224c81 Compare November 13, 2020 21:15

Jiawei0227 changed the title ~~[WIP]Storage testsuite refactor & cleanup~~ Storage testsuite refactor & cleanup Nov 13, 2020

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 Nov 13, 2020

k8s-ci-robot assigned chrishenzie and jingxu97 Nov 13, 2020

k8s-ci-robot requested a review from msau42 November 13, 2020 21:22

Jiawei0227 mentioned this pull request Nov 13, 2020

Testsuite code refactor #96538

Closed

Jiawei0227 commented Nov 13, 2020

View reviewed changes

Jiawei0227 force-pushed the skiptest branch from 4224c81 to 51bfa8b Compare November 13, 2020 21:54

jingxu97 reviewed Nov 17, 2020

View reviewed changes

Jiawei0227 force-pushed the skiptest branch from 51bfa8b to aca6bde Compare November 18, 2020 00:52

jingxu97 reviewed Nov 18, 2020

View reviewed changes

Jiawei0227 force-pushed the skiptest branch 3 times, most recently from 26cf6d2 to 5ffe59c Compare December 9, 2020 07:31

msau42 reviewed Dec 9, 2020

View reviewed changes

Jiawei0227 added 3 commits December 10, 2020 11:12

Add storage framework and address comments

356bea6

Jiawei0227 force-pushed the skiptest branch from 5ffe59c to 356bea6 Compare December 11, 2020 06:54

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 11, 2020

k8s-ci-robot assigned msau42 Dec 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2020

k8s-ci-robot merged commit 49dd01f into kubernetes:master Dec 11, 2020

k8s-ci-robot added this to the v1.21 milestone Dec 11, 2020

thejoycekung mentioned this pull request Dec 14, 2020

[Failing Test] [sig-storage] Several In-tree Volumes tests failing (ci-kubernetes-e2e-gci-gce-serial) #97287

Closed

Jiawei0227 mentioned this pull request Dec 14, 2020

Skip unsupported tests for disruptive testsuite #97291

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage testsuite refactor & cleanup #96573

Storage testsuite refactor & cleanup #96573

Jiawei0227 commented Nov 13, 2020 •

edited by liggitt

Loading

Jiawei0227 commented Nov 13, 2020

Jiawei0227 Nov 13, 2020

Jiawei0227 Nov 13, 2020 •

edited

Loading

Jiawei0227 Nov 13, 2020

Jiawei0227 commented Nov 13, 2020

jingxu97 Nov 17, 2020

Jiawei0227 Nov 17, 2020 •

edited

Loading

jingxu97 Nov 18, 2020

Jiawei0227 Nov 18, 2020

Jiawei0227 Nov 18, 2020

jingxu97 Nov 18, 2020

Jiawei0227 commented Nov 18, 2020

jingxu97 commented Nov 18, 2020

Jiawei0227 commented Dec 9, 2020

msau42 Dec 9, 2020

Jiawei0227 Dec 9, 2020

Jiawei0227 Dec 11, 2020 •

edited

Loading

msau42 commented Dec 11, 2020

k8s-ci-robot commented Dec 11, 2020

Jiawei0227 commented Dec 11, 2020

Storage testsuite refactor & cleanup #96573

Storage testsuite refactor & cleanup #96573

Conversation

Jiawei0227 commented Nov 13, 2020 • edited by liggitt Loading

Jiawei0227 commented Nov 13, 2020

Choose a reason for hiding this comment

Jiawei0227 Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 commented Nov 13, 2020

Choose a reason for hiding this comment

Jiawei0227 Nov 17, 2020 • 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

Choose a reason for hiding this comment

Jiawei0227 commented Nov 18, 2020

jingxu97 commented Nov 18, 2020

Jiawei0227 commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

msau42 commented Dec 11, 2020

k8s-ci-robot commented Dec 11, 2020

Jiawei0227 commented Dec 11, 2020

Jiawei0227 commented Nov 13, 2020 •

edited by liggitt

Loading

Jiawei0227 Nov 13, 2020 •

edited

Loading

Jiawei0227 Nov 17, 2020 •

edited

Loading

Jiawei0227 Dec 11, 2020 •

edited

Loading