Add first few e2e test cases #151

dkoshkin · 2018-12-18T16:12:10Z

Is this a bug fix or adding new feature?
New Feature

What is this PR about? / Why do we need it?
This PR adds the first few scenarios to test the driver provisioning EBS volumes, attaching them to a pod and asserting data can be written and read on those volumes.

Closes #139
This is a follow up to #139 and as a result that PR would be closed if this one is approved and merged.

Some important distinctions between the PRs:

This PR does not rely on yaml spec files, instead builds the specs in Go code
Provides helper functions for some common test setup that includes creating/cleanup of StorageClasses, PVCs, test Pods
Reuses the upstream k8s.io/kubernetes/test/e2e/framework package - which provides many utilities and helper functions for e2e tests
- Bootstrap/cleanup of namespaces for each test
- Deleting and Waiting for resources to get into desired state

The intention of the above is to provide a framework for writing additional tests. However, as more scenarios are added it is expected that this code will need to be extended to support those.

Additional tests that we intend to write (not part of this PR):

It("should create a PV object, bind to a PVC and be deleted with Delete reclaimPolicy")
It("should create a PV object, bind to a PVC and be retained with Retain reclaimPolicy")
It("should create multiple PV objects, bind to PVCs and attach each to different pods on the same node")
It("should reattach a volume after a pod is deleted and data can be read")
It("should reattach a volume after a pod is rescheduled from a drained node and data can be read")
It("should allow for a pod writing and reading data, expanding the PersistentVolume and writing and reading data")
It("should allow for volume provisioning with encryption enabled")
It("should allow for topology aware volume provisioning")
It("should allow for statically creating a PV and writing and reading data")

IMPORTANT one of the new dependencies k8s.io/kubernetes/test/e2e/framework pulls in bitbucket.org/ww/goautoneg which is a Mercurial repo hosted in Bitbucket. Unfortunately Travis does not support Mercurial.
This is not an issue with Go 1.10 or when Go mods aren't enabled, so in this PR I've also set GO111MODULE=off. Another justification is that prow still does not use Go mods either so our build and test frameworks will at least look the same.
Once either that dependency is dropped or there is some other fix we can re-enable it but k8s.io/kubernetes/test/e2e/framework provides some pretty crucial code and I don't want to just remove it at this point.

What are everyone else's thoughts on this?

What testing is done?
Run e2e tests using images built with the latest code from master.

k8s-ci-robot · 2018-12-18T16:12:24Z

Hi @dkoshkin. Thanks for your PR.

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

coveralls · 2018-12-18T16:14:55Z

Pull Request Test Coverage Report for Build 268

7 of 37 (18.92%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 50.352%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/cloud/cloud.go	3	4	75.0%
pkg/cloud/devicemanager/manager.go	2	3	66.67%
pkg/driver/driver.go	0	4	0.0%
pkg/driver/identity.go	0	4	0.0%
pkg/driver/controller.go	2	11	18.18%
pkg/driver/node.go	0	11	0.0%

Totals
Change from base Build 267:	0.0%
Covered Lines:	572
Relevant Lines:	1136

💛 - Coveralls

leakingtapan · 2018-12-19T18:20:13Z

/ok-to-test

leakingtapan · 2018-12-20T00:48:05Z

/ok-to-test

dkoshkin · 2018-12-20T14:47:22Z

@leakingtapan looks like the sanity test failed because I changed the way that test is run and it now depends on having the ginkgo binary.
I can change the that test back but for the e2e test the ginkgo binary would be required to run the test suite in parallel.

leakingtapan

I aggregated all test cases (both mentioned or implemented in this PR and in #118) at #118 lets track them there as top level issue.

Could you add some README about how to run e2e test locally and what is required to run it at tests/e2e?

docs/README.md

leakingtapan · 2018-12-20T18:36:31Z

docs/README.md

@@ -105,21 +105,21 @@ Please go through [CSI Spec](https://github.com/container-storage-interface/spec

 ### Requirements
 * Golang 1.11.2+
-* [Ginkgo](https://github.com/onsi/ginkgo) for integration and end-to-end testing
+* [Ginkgo](https://github.com/onsi/ginkgo) in your PATH for end-to-end testing


I think ginkgo is still needed for integration test?

From the user's perspective no, its a dependency that will be downloaded for them.

Are we requiring user to download themselves or we download it for them? I don't see we are doing it, did I miss something

To run e2e, the user will need to download it themselves and make sure ginkgo binary is in their PATH.

For other tests its a dependency in go.mod and present in vendor dir so the user won't need to do anything special here besides running go mod download

tests/e2e/driver/ebs_csi_driver.go

tests/e2e/driver/driver.go

tests/e2e/dynamic_provisioning.go

tests/e2e/testsuites/pod_dynamic_volume_writer_reader.go

leakingtapan

Could you also squash the commit with messages once all the changes are done？This will help a lot when generating release note during release.

leakingtapan · 2018-12-20T22:06:39Z

one of the new dependencies k8s.io/kubernetes/test/e2e/framework pulls in bitbucket.org/ww/goautoneg which is a Mercurial repo hosted in Bitbucket. Unfortunately Travis does not support Mercurial.

But I should be able to do go mod download locally, right?

leakingtapan

And how are we reporting test results? This doesn't have to be done in this PR, but in the end, we will need to report test result in testgrid. Here is the dashboard for EBS CSI driver. And we will need to add a new tab "e2e" with test result reported similar to GCE PD as an example

hack/run-e2e-test

leakingtapan · 2018-12-20T22:54:16Z

tests/e2e/dynamic_provisioning.go

+	"github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/e2e/testsuites"
+)
+
+var _ = Describe("[ebs-csi] Dynamic Provisioning", func() {


I think this test requires all the worker nodes are in the same az? Otherwise it will randomly failure due to az mismatch without volume scheduling.

Thats correct, maybe I can add a tag so users can skip when running it?
There will also be tests that test the topology aware scheduling feature.

leakingtapan · 2018-12-20T23:16:07Z

tests/e2e/dynamic_provisioning.go

+		volumeType2 := "io1"
+		pods := []testsuites.PodDetails{
+			{
+				Cmd: "echo 'hello world' > /mnt/test-1/data && echo 'hello world' > /mnt/test-2/data && grep 'hello world' /mnt/test-1/data  && grep 'hello world' /mnt/test-2/data",


This is not a big deal for now. Seems the mount path in Cmd is depending on how tpod.SetupVolume works on generating the mount path and volume name. This might cause some issue when generation logic is changed but Cmd is not updated to reflect the change.

Yeah I didnt like that either :(, as the tests evolve and more are written I'll try to find a way to improve that.

dkoshkin · 2018-12-21T16:50:56Z

But I should be able to do go mod download locally, right?

Yes as long as you have hg installed it will work.

dkoshkin · 2018-12-21T18:09:12Z

And how are we reporting test results?

Yeah IMO until these tests are running automatically adding it in the testgrid doesn't really provide much value.

dkoshkin · 2018-12-21T18:14:12Z

One of the commits migrates from glog to klog

The kubernetes community migrated to that package in v1.13.
When I made a change to reuse values the e2e package from the actual codebase, the tests failed with:

 e2e timed out. path: ./tests/e2e
[1] /var/folders/t2/t_mn_7kj01n062f32ds9gqc00000gn/T/ginkgo570950472/e2e.test flag redefined: log_dir
[1] panic: /var/folders/t2/t_mn_7kj01n062f32ds9gqc00000gn/T/ginkgo570950472/e2e.test flag redefined: log_dir

There isn't a real fix for that due to the way Go's vendoring works (this is one of the reasons why kubernetes migrated). Also the glog package hasn't been updated in years.

leakingtapan · 2018-12-21T22:48:07Z

Yeah IMO until these tests are running automatically adding it in the testgrid doesn't really provide much value.

Agree. @gyuho will help us setting up periodic prow job at the mean time when presubmit job is not ready. And when periodic job is ready, we can start adding into testgrid.

leakingtapan · 2018-12-21T22:49:00Z

@dkoshkin For klog change, could you squash the 5 commits into 2 commits? One is for adding e2e tests and once is for klog update.

Some other changes required * Update Go and vendored Kubernetes versions * Update vendor directory * Disable go mod in Travis CI

* The Kubernetes has moved over to using k8s.io/klog * Using glog will cause "flag redefined: log_dir" when running e2e that import values from github.com/kubernetes-sigs/aws-ebs-csi-driver/ * Similar to issue kubernetes/kubernetes#35096

dkoshkin · 2018-12-21T23:12:07Z

@leakingtapan squashed

leakingtapan · 2018-12-22T00:42:55Z

/lgtm
/approve

k8s-ci-robot · 2018-12-22T00:42:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkoshkin, leakingtapan

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:

~~OWNERS~~ [leakingtapan]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 18, 2018

k8s-ci-robot requested review from d-nishi and leakingtapan December 18, 2018 16:12

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 18, 2018

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 18, 2018

dkoshkin force-pushed the e2e branch from 17bcdd4 to 387da15 Compare December 19, 2018 00:16

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 Dec 20, 2018

dkoshkin force-pushed the e2e branch from 6b660d8 to acd272d Compare December 20, 2018 15:23

leakingtapan reviewed Dec 20, 2018

View reviewed changes

dkoshkin force-pushed the e2e branch 2 times, most recently from 3868aee to 9fb6f27 Compare December 21, 2018 18:06

dkoshkin force-pushed the e2e branch from 9fb6f27 to 000cfd5 Compare December 21, 2018 18:16

dkoshkin added 2 commits December 21, 2018 18:10

Add simple e2e test cases

716434b

Some other changes required * Update Go and vendored Kubernetes versions * Update vendor directory * Disable go mod in Travis CI

dkoshkin force-pushed the e2e branch from 000cfd5 to 0c34e16 Compare December 21, 2018 23:11

k8s-ci-robot assigned leakingtapan Dec 22, 2018

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

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

k8s-ci-robot merged commit e014aa9 into kubernetes-sigs:master Dec 22, 2018

leakingtapan mentioned this pull request Dec 22, 2018

Add end to end test cases #118

Closed

13 tasks

pires deleted the e2e branch December 22, 2018 08:24

dkoshkin mentioned this pull request Dec 26, 2018

Fix klog flag init #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add first few e2e test cases #151

Add first few e2e test cases #151

dkoshkin commented Dec 18, 2018 •

edited

Loading

k8s-ci-robot commented Dec 18, 2018

coveralls commented Dec 18, 2018 •

edited

Loading

leakingtapan commented Dec 19, 2018

leakingtapan commented Dec 20, 2018

dkoshkin commented Dec 20, 2018

leakingtapan left a comment

leakingtapan Dec 20, 2018

dkoshkin Dec 21, 2018

leakingtapan Dec 21, 2018

dkoshkin Dec 21, 2018

leakingtapan left a comment

leakingtapan commented Dec 20, 2018

leakingtapan left a comment

leakingtapan Dec 20, 2018

dkoshkin Dec 21, 2018

leakingtapan Dec 20, 2018

dkoshkin Dec 21, 2018

dkoshkin commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

leakingtapan commented Dec 21, 2018

leakingtapan commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

leakingtapan commented Dec 22, 2018

k8s-ci-robot commented Dec 22, 2018

Add first few e2e test cases #151

Add first few e2e test cases #151

Conversation

dkoshkin commented Dec 18, 2018 • edited Loading

k8s-ci-robot commented Dec 18, 2018

coveralls commented Dec 18, 2018 • edited Loading

Pull Request Test Coverage Report for Build 268

💛 - Coveralls

leakingtapan commented Dec 19, 2018

leakingtapan commented Dec 20, 2018

dkoshkin commented Dec 20, 2018

leakingtapan 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

leakingtapan left a comment

Choose a reason for hiding this comment

leakingtapan commented Dec 20, 2018

leakingtapan 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

dkoshkin commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

leakingtapan commented Dec 21, 2018

leakingtapan commented Dec 21, 2018

dkoshkin commented Dec 21, 2018

leakingtapan commented Dec 22, 2018

k8s-ci-robot commented Dec 22, 2018

dkoshkin commented Dec 18, 2018 •

edited

Loading

coveralls commented Dec 18, 2018 •

edited

Loading