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.

Sign up for GitHub

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

Jump to bottom

Check status of framework.CheckPodsRunningReady #25915

Merged
merged 1 commit into from
May 21, 2016
Merged

Check status of framework.CheckPodsRunningReady #25915

merged 1 commit into from
May 21, 2016

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented May 19, 2016 •
edited
Loading

Check status of framework.CheckPodsRunningReady and fail test if it's false, instead of silently
ignoring the failure.

This doesn't fix whatever is causing the pod not to start in #17523 but it does fail the test as soon as it detects the pod didn't start, instead of allowing the testing to proceed.

cc @kubernetes/sig-testing @spxtr @ixdy @kubernetes/rh-cluster-infra

@ncdc ncdc added the release-note-none Denotes a PR that doesn't merit a release note. label May 19, 2016
@ncdc ncdc mentioned this pull request May 19, 2016
Closed
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 19, 2016
@pmorie
Copy link
Member

pmorie commented May 19, 2016

Failing fast always better than failing slow, LGTM.

@yujuhong
Copy link
Contributor

There are several dubious calls of CheckPodsRunningReady in this file (test/e2e/kubectl.go). /cc @kubernetes/kubectl

@yujuhong
Copy link
Contributor

Change LGTM. Thanks! @ncdc, if you're willing to clean up the rest of the calls in the file, that'd be even better.

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2016
@ncdc
Copy link
Member Author

ncdc commented May 20, 2016

I can do it in a bit

On Thursday, May 19, 2016, Yu-Ju Hong notifications@github.com wrote:

Change LGTM. Thanks! @ncdc https://github.com/ncdc, if you're willing
to clean up the rest of the calls in the file, that'd be even better.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25915 (comment)

@yujuhong yujuhong removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@yujuhong
Copy link
Contributor

I can do it in a bit

That's great!

@ncdc
Copy link
Member Author

ncdc commented May 20, 2016

@yujuhong I think I got the rest of them; PTAL

@ncdc ncdc changed the title Fail test if pod isn't running in BeforeEach Check status of framework.CheckPodsRunningReady May 20, 2016
@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@yujuhong
Copy link
Contributor

Thanks!

@pmorie
Copy link
Member

pmorie commented May 20, 2016

@k8s-bot test this issue: #25879

@ncdc
Copy link
Member Author

ncdc commented May 20, 2016

@k8s-bot unit test this issue: #25890

@yujuhong yujuhong assigned yujuhong and unassigned fejta May 20, 2016
@yujuhong
Copy link
Contributor

This PR should help with debugging flakes. Marking it p2 for now.

@yujuhong yujuhong added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 20, 2016
@mikedanese
Copy link
Member

@ncdc Please rebase!

@ncdc
Copy link
Member Author

ncdc commented May 20, 2016

Will do shortly

On Friday, May 20, 2016, Mike Danese notifications@github.com wrote:

@ncdc https://github.com/ncdc Please rebase!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25915 (comment)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2016
Check status of framework.CheckPodsRunningReady
f83a286 
Check status of framework.CheckPodsRunningReady and fail test if it's false, instead of silently
ignoring the failure.
@ncdc
Copy link
Member Author

ncdc commented May 21, 2016

@mikedanese @yujuhong rebased

@yujuhong yujuhong added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2016
@yujuhong
Copy link
Contributor

Thanks!

@k8s-bot
Copy link

k8s-bot commented May 21, 2016

GCE e2e build/test passed for commit f83a286.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 45514f7 into kubernetes:master May 21, 2016
@rootfs rootfs mentioned this pull request Jul 31, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Aug 24, 2016
Merge pull request #29836 from rootfs/azuredd
3544f8a 
Automatic merge from submit-queue

support Azure data disk volume

This is a WIP of supporting azure data disk volume. Will add test and dynamic provisioning support once #29006 is merged

replace #25915
fix #23259

@kubernetes/sig-storage 
@colemickens @brendandburns
@ncdc ncdc deleted the e2e-fix branch February 13, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ncdc @pmorie @yujuhong @mikedanese @k8s-bot @k8s-github-robot @fejta @googlebot