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

e2e storage: public API for testsuites, support CSIInlineVolume type for generic resource #85540

Merged
merged 3 commits into from Dec 6, 2019

Conversation

@pohly
Copy link
Contributor

pohly commented Nov 22, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Implementing a test suite was impossible outside of the
k8s.io/kubernetes/test/e2e/storage/testsuites package because all
interfaces and structs used by them were private.

Special notes for your reviewer:

As part of revamping the API, genericVolumeTestResource also gets
exported because it is useful for other test suites. Because the
TestResource interface became obsolete a while ago and isn't used
anymore, the new name is just testsuites.VolumeResource.

testpatterns.CSIInlineVolume needs special handling in a few places.
It can now be used in a test pattern for a test suite that uses a
VolumeResource instance.

Does this PR introduce a user-facing change?:

NONE
@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 22, 2019

/sig storage

@k8s-ci-robot k8s-ci-robot added sig/storage and removed needs-sig labels Nov 22, 2019
@k8s-ci-robot k8s-ci-robot requested review from davidz627 and jsafrane Nov 22, 2019
pohly added a commit to pohly/pmem-CSI that referenced this pull request Nov 22, 2019
The original API doesn't support implementing a test suite outside of
Kubernetes. Change submitted upstream in
kubernetes/kubernetes#85540
@pohly pohly force-pushed the pohly:testsuites-api branch from 236d01f to cdae84d Nov 22, 2019
@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 22, 2019

/retest

@davidz627

This comment has been minimized.

Copy link
Contributor

davidz627 commented Nov 22, 2019

I don't have many comments on the code itself but I want us to think very carefully about the implications of exporting all these methods, types, etc. This has bitten us in the butt before where these external interfaces were unable to be changed because of costlyness (#77525 (comment)). I would prefer to keep as much of the surface unexported as possible. A wholesale change of everything to an exported type doesn't seem like the right solution here - lets figure out some other way to tackle it.

/cc @msau42 @misterikkit

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 22, 2019

I would prefer to keep as much of the surface unexported as possible.

So let's consider how much we need to export while still allowing external test suites:

  • The TestSuite interface must be exported, which includes its methods.
  • To implement that interface, TestSuiteInfo must be exported, otherwise an implementation cannot create that struct.
  • VolumeResource is used in almost every existing test suite. It is obviously pretty useful and was what I needed to support different test patterns in a custom test suite, so I think this should be public together with CreateVolumeResource.

That covers the changes in base.go.

The new test patterns are exported just as the existing ones are.

In short, I don't see what names should not be exported.

One way to test that would be to move the base package and individual test suites into different packages.

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 22, 2019

One way to test that the base API is suitable would be to separate it from the code using it. For the sake of minimizing changes it would be best to keep "testsuites" as the API and then move individual testsuites into sub-packages (e.g. test/e2e/storage/testsuites/volumes.go -> test/e2e/storage/testsuites/volumes/volumes.go).

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 22, 2019

One way to test that the base API is suitable would be to separate it from the code using it.

While that's cleaner, it's also some additional churn in the API. Alternatively, we could have a _test.go file in some other package which defines a custom test suite using the testsuites API. It doesn't need to run, just build.

pohly added a commit to pohly/pmem-CSI that referenced this pull request Nov 25, 2019
The original API doesn't support implementing a test suite outside of
Kubernetes. Change submitted upstream in
kubernetes/kubernetes#85540
@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 26, 2019

If we're already going to churn the API, why don't we fix things all in one go?

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 26, 2019

/assign

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 27, 2019

If we're already going to churn the API, why don't we fix things all in one go?

I was just brainstorming about other potential changes. So you want the individual test suites in their own packages?

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 27, 2019

BTW, right now the API just gets extended. Custom test suites don't need to be changed to continue doing what they were doing before. Moving into packages would be a breaking API change. I think it would be cleaner, I'm just not sure how important it is.

pohly added a commit to pohly/pmem-CSI that referenced this pull request Nov 27, 2019
The original API doesn't support implementing a test suite outside of
Kubernetes. Change submitted upstream in
kubernetes/kubernetes#85540
@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 27, 2019

Im not sure we really need to move each testsuite to its own package. I think exporting the structs and methods we need is sufficient

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 27, 2019

Im not sure we really need to move each testsuite to its own package.

Yes, neither am I.

I think exporting the structs and methods we need is sufficient

So the PR is okay as-is, assuming that it meets those goals?

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Nov 28, 2019

I'm asking my teammate to review this PR because we had to do similar work to export various TestSuite interfaces to implement custom test suites.

@pohly pohly force-pushed the pohly:testsuites-api branch from cdae84d to 09cacee Nov 28, 2019
@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Nov 28, 2019

I rebased to resolve a merge conflict in test/e2e/storage/testsuites/volume_expand.go and added the build test that I mentioned in #85540 (comment).

pohly added a commit to pohly/pmem-CSI that referenced this pull request Nov 29, 2019
The original API doesn't support implementing a test suite outside of
Kubernetes. Change submitted upstream in
kubernetes/kubernetes#85540
pohly added a commit to pohly/pmem-CSI that referenced this pull request Dec 3, 2019
The original API doesn't support implementing a test suite outside of
Kubernetes. Change submitted upstream in
kubernetes/kubernetes#85540
@@ -162,6 +186,13 @@ var (
FsType: "ntfs",
FeatureTag: "[sig-windows]",
}
// NtfsEphemeralVolume is TestPattern for "Ephemeral-volume (ntfs)"
NtfsEphemeralVolume = TestPattern{

This comment has been minimized.

Copy link
@msau42

msau42 Dec 5, 2019

Member

csi is not supported on windows yet. This will probably break windows tests?

This comment has been minimized.

Copy link
@pohly

pohly Dec 5, 2019

Author Contributor

I just added it for the sake of completeness. It's not used anywhere yet.

// getTestSuiteInfo returns the TestSuiteInfo for this TestSuite
getTestSuiteInfo() TestSuiteInfo
// GetTestSuiteInfo returns the TestSuiteInfo for this TestSuite
GetTestSuiteInfo() TestSuiteInfo
// defineTest defines tests of the testpattern for the driver.

This comment has been minimized.

Copy link
@msau42

msau42 Dec 5, 2019

Member

comment also needs to be uppercase

This comment has been minimized.

Copy link
@pohly

pohly Dec 5, 2019

Author Contributor

Shall I fix in a follow-up PR?

@@ -0,0 +1,52 @@
/*
Copyright 2018 The Kubernetes Authors.

This comment has been minimized.

Copy link
@msau42

msau42 Dec 5, 2019

Member

2019

This comment has been minimized.

Copy link
@pohly

pohly Dec 5, 2019

Author Contributor

Same here - new PR?

But speaking of the copyright year, is there any process in place for ensuring that this gets updated for existing files? Or is the rule "the first PR to touch an existing file must bump to or add the new year"?

This comment has been minimized.

Copy link
@msau42

msau42 Dec 5, 2019

Member

I believe the year should be when the file was created, not last modified

This comment has been minimized.

Copy link
@msau42

msau42 Dec 5, 2019

Member

Let's just make the changes now. I already approved so final lgtm will be quick

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 5, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

pohly added 3 commits Nov 22, 2019
…for generic resource

Implementing a test suite was impossible outside of the
k8s.io/kubernetes/test/e2e/storage/testsuites package because all
interfaces and structs used by them were private.

As part of revamping the API, genericVolumeTestResource also gets
exported because it is useful for other test suites. Because the
TestResource interface became obsolete a while ago and isn't used
anymore, the new name is just testsuites.VolumeResource.

testpatterns.CSIInlineVolume needs special handling in a few places.
It can now be used in a test pattern for a test suite that uses a
VolumeResource instance.
This will catch accidentally adding a new interface function which
isn't exported. For example, an attempt to implement a new unexported
"foobar()" function then leads to:

test/e2e/storage/testsuites/api_test.go:54:5: cannot use &fakeSuite literal (type *fakeSuite) as type testsuites.TestSuite in assignment:
	*fakeSuite does not implement testsuites.TestSuite (missing testsuites.foobar method)
		have foobar()
		want testsuites.foobar()
FAIL	k8s.io/kubernetes/test/e2e/storage/testsuites [build failed]
@pohly pohly force-pushed the pohly:testsuites-api branch from c536983 to 8227b61 Dec 5, 2019
@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Dec 5, 2019

I fixed both commits with case fix in the comment and year and force-pushed.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 5, 2019

/lgtm
/priority important-soon

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Dec 5, 2019

/retest

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 5, 2019

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

@k8s-ci-robot k8s-ci-robot merged commit d47e136 into kubernetes:master Dec 6, 2019
19 checks passed
19 checks passed
cla/linuxfoundation pohly authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance 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-storage-slow Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-snapshot Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
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
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 6, 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.