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

some enhancement to run volume e2e tests #24098

Merged
merged 1 commit into from Apr 18, 2016

Conversation

Projects
None yet
@rootfs
Copy link
Member

rootfs commented Apr 11, 2016

No description provided.

@googlebot googlebot added the cla: yes label Apr 11, 2016

@rootfs rootfs changed the title some enhancement to run volume e2e tests on centos some enhancement to run volume e2e tests Apr 11, 2016

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 11, 2016

GCE e2e build/test passed for commit 5b86f58.

@@ -169,6 +169,9 @@ func volumeTestCleanup(client *client.Client, config VolumeTestConfig) {
expectNoError(err, "Failed to delete client pod: %v", err)
}

By("sleeping a bit so client can stop and unmount")

This comment has been minimized.

@jayunit100

jayunit100 Apr 11, 2016

Member

@rootfs lets see if maybe we can do it directly, using waitForPodNoLongeRunningInNAmespace(f.Client, podName, ns), rather than a magic sleep ? Or else you can even use the lookForStringInLog function. Ping me on slack if you have questions on how to use those, or see test/e2e/examples.go

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

from conversation w/ @rootfs

  • why the new sleep call ? we need another wait to clean up volume mounts . there is an asynchronous wait for kubelet syncing.
  • why the update to hack/local-up-cluster ? we want the kubelet to quickly unmount volumes for local storage testing.
@@ -169,6 +169,9 @@ func volumeTestCleanup(client *client.Client, config VolumeTestConfig) {
expectNoError(err, "Failed to delete client pod: %v", err)
}

By("sleeping a bit so client can stop and unmount")
time.Sleep(20 * time.Second)

This comment has been minimized.

@jayunit100

jayunit100 Apr 11, 2016

Member

@rootfs can you (1) reference the PR that will make this unnecessary (2) add a comment on why we cannot delete the podClient until after this sleep occurs (like you explained to me on the phone :))

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

cc @kubernetes/sig-testing

  • this makes local testing of storage very fluid, adding parity to the shell script testing wrappers.

  • i assume this doesnt slow down local-up-cluster (@rootfs please confirm this)...

  • @rootfs please update the comments on youre addition of the pre client delete sleep(20) ...

    i'd say if the above 3 comments check out, its lgtm to merge....

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 11, 2016

comments added, thanks @jayunit100

// It is possible that the volume server container is stopped before umount kicks in.
// In such case, some volume types (like NFS) is unable to umount the volume.
// To work around this issue, give a pause here.
// Ideally the pause should be longer than the kubelet's sync timeout.

This comment has been minimized.

@jayunit100

jayunit100 Apr 11, 2016

Member

i think we can simplify this with just saying // This sleep prevents a kubelet from failing to unmount a volume, by keeping the container alive for a few seconds, see issue #abcd for details, and remove it once that bug is fixed.

@rootfs rootfs force-pushed the rootfs:e2e-test branch from c318c7e to f63690f Apr 11, 2016

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

ok, lets rebase - and maybe simplify the comment and point to the correct bug in the comment.

@spxtr

This comment has been minimized.

Copy link
Member

spxtr commented Apr 11, 2016

Why do you need to make cluster/test-volume.sh? I think go run hack/e2e.go -v --test --test_args="--ginkgo.focus=Volume" is the preferred way these days.

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

@spxtr there are test-network test-smoke ...

  • if we want parity test-volume is a good wrapper.
  • If we don't want these wrappers, you are right, and maybe @rootfs could submit an "opposite" PR in another issue removing those wrappers :)

I'm open to either way but as the code stands now, test-volume is in line with what already is being done,.... is there an issue to delete these wrappers? If not maybe we should create one.

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 11, 2016

create issue #24100 for mount leak problem

@spxtr

This comment has been minimized.

Copy link
Member

spxtr commented Apr 11, 2016

I should have linked #21843. There's a one-line way of accomplishing the same thing in the docs. It's not really a big deal, though.

@rootfs rootfs force-pushed the rootfs:e2e-test branch from f63690f to 82927a3 Apr 11, 2016

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

@spxtr are we going to delete the other wrappers, if so i agree with you -

@spxtr

This comment has been minimized.

Copy link
Member

spxtr commented Apr 11, 2016

Yes, we are going to delete the other wrappers eventually.

It's tough because we're not really sure who is actually using them, which is why we want to make sure that there is only one supported way of running e2e, which is supposed to be hack/e2e.go.

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 11, 2016

@spxtr ok, i am going to revert test-volume.sh addition.

@jayunit100

This comment has been minimized.

Copy link
Member

jayunit100 commented Apr 11, 2016

ok, @rootfs then lets not add a new wrapper

  • point to the issues in the comment
  • delete the wrapper script
  • rebase
    then i think its good to merg

@rootfs rootfs force-pushed the rootfs:e2e-test branch from 82927a3 to 779fa49 Apr 11, 2016

@spxtr

This comment has been minimized.

Copy link
Member

spxtr commented Apr 11, 2016

@rootfs @jayunit100 Thanks :)

@k8s-github-robot k8s-github-robot added size/S and removed size/M labels Apr 11, 2016

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 11, 2016

reverted test-volum.sh and squashed

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 11, 2016

GCE e2e build/test passed for commit c318c7e.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 11, 2016

GCE e2e build/test passed for commit f63690f.

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 13, 2016

@zmerlynn PTAL thanks!

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Apr 15, 2016

LGTM

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test failed for commit 08c8300.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@rootfs rootfs force-pushed the rootfs:e2e-test branch from 08c8300 to 5470e6e Apr 15, 2016

@k8s-github-robot k8s-github-robot removed the lgtm label Apr 15, 2016

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test passed for commit 5470e6e.

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 15, 2016

@timothysc can you take another look? the e2e framework just gets updated and new commit just corrects that. thanks!

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Apr 15, 2016

@rootfs please squash your commits.


BeforeEach(func() {
c = f.Client
namespace = f.Namespace
fw = f

This comment has been minimized.

@ixdy

ixdy Apr 15, 2016

Member

why not just use f everywhere?

This comment has been minimized.

@rootfs

rootfs Apr 15, 2016

Author Member

thanks!

@rootfs rootfs force-pushed the rootfs:e2e-test branch from 5470e6e to 7e552ec Apr 15, 2016

enhancements to run volume e2e test:
in e2e/volumes.go: give time to allow pod cleanup and volume unmount happen before volume server exit;
skip cinder volume test if not running with openstack provider

comment on why pause before containerized server is stopped in volume e2e tests, fix #24100

updates NFS server image to 0.6, per #22529

fix persistent_volume e2e test: test cleanup doesn't expect client pod; delete PV after test

Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:e2e-test branch from 7e552ec to ee9ed4d Apr 15, 2016

@rootfs

This comment has been minimized.

Copy link
Member Author

rootfs commented Apr 15, 2016

@timothysc squashed

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test passed for commit 7e552ec.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test passed for commit ee9ed4d.

@k8s-teamcity-mesosphere

This comment has been minimized.

Copy link

k8s-teamcity-mesosphere commented on ee9ed4d Apr 15, 2016

TeamCity OSS :: Kubernetes Mesos :: 4 - Smoke Tests Build 21572 outcome was FAILURE
Summary: Exit code 1 Build time: 00:42:29

This comment has been minimized.

Copy link

k8s-teamcity-mesosphere replied Apr 15, 2016

TeamCity OSS :: Kubernetes Mesos :: 4 - Smoke Tests Build 21575 outcome was FAILURE
Summary: Exit code 1 Build time: 00:27:18

@timothysc timothysc added the lgtm label Apr 16, 2016

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit ee9ed4d.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit ee9ed4d.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 18, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Apr 18, 2016

GCE e2e build/test passed for commit ee9ed4d.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 18, 2016

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b731647 into kubernetes:master Apr 18, 2016

5 of 6 checks passed

Submit Queue Github CI tests are not green.
Details
Jenkins GCE Node e2e Build finished.
Details
Jenkins GCE e2e 277 tests run, 112 skipped, 0 failed.
Details
Jenkins unit/integration 5918 tests run, 18 skipped, 0 failed.
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.