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

Wait for the port to be ready before starting #38260

Merged
merged 2 commits into from
Dec 8, 2016
Merged

Wait for the port to be ready before starting #38260

merged 2 commits into from
Dec 8, 2016

Conversation

fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Dec 7, 2016 •
edited by sttts
Loading

Fixes the portforward flakes. See #27673 & #27680

@k8s-ci-robot
Copy link
Contributor

Hi @fraenkel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@fraenkel
Copy link
Contributor Author

fraenkel commented Dec 7, 2016

@sttts Here is one attempt. With two containers we have to see if the container terminated rather than the pod.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Dec 7, 2016
@ixdy ixdy assigned sttts and unassigned ixdy Dec 7, 2016
@sttts sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Dec 7, 2016
@sttts
Copy link
Contributor

sttts commented Dec 7, 2016

@k8s-bot ok to test

@sttts
Copy link
Contributor

sttts commented Dec 7, 2016

Looks good. Can you double check that it does what it should by temporarily adding an time.Sleep(10*time.Second) in the portforward container binary?

@sttts sttts mentioned this pull request Dec 7, 2016
Closed
@sttts
Copy link
Contributor

sttts commented Dec 7, 2016

lgtm

@sttts sttts 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. labels Dec 7, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2016
@fraenkel fraenkel force-pushed the port_forward_readiness branch from ff1f95b to c6594c5 Compare December 7, 2016 15:31
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2016
@fraenkel
Copy link
Contributor Author

fraenkel commented Dec 7, 2016

@sttts The port forwarding tests passed with 50 tries each. The failures above are some logging tests.

@sttts
Copy link
Contributor

sttts commented Dec 7, 2016

@fraenkel can you push the second commit in another PR to check that we can really reproduce the issue with that?

@fraenkel
Copy link
Contributor Author

fraenkel commented Dec 7, 2016

@stts Good news, both GCE e2e and GCE etcd3 e2e fail miserably without the fix.

@fraenkel fraenkel force-pushed the port_forward_readiness branch from 5c0c465 to 6aa8d05 Compare December 7, 2016 18:55
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2016
@fraenkel
Wait for the port to be ready before starting
e4542e2 
- wait until the port is ready
- verify the container terminated
@fraenkel
Re-enable flaky tests
c6d6dcf
@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit 6aa8d05. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 6aa8d05. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 6aa8d05. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 6aa8d05. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit c6d6dcf. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@fraenkel
Copy link
Contributor Author

fraenkel commented Dec 7, 2016

@stts a clean run

@sttts
Copy link
Contributor

sttts commented Dec 8, 2016

@k8s-bot cvm gce e2e test this

@sttts sttts mentioned this pull request Dec 8, 2016
Closed
@sttts
Copy link
Contributor

sttts commented Dec 8, 2016

/cc @kubernetes/sig-testing

@sttts
Copy link
Contributor

sttts commented Dec 8, 2016

Lgtm. This should fix #27673 & #27680 and reenable the three portforwarding tests. If this is not the case, do not hesitate to revert.

@sttts sttts added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/P2 labels Dec 8, 2016
@sttts
Copy link
Contributor

sttts commented Dec 8, 2016

/cc @fejta ^^

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 99c066e into kubernetes:master Dec 8, 2016
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@sttts
Copy link
Contributor

sttts commented Dec 12, 2016

@jessfraz we are seeing flakes (#27680) on the 1.4 branch which would be fixed by 37103 and 38260. Please consider them for cherry-picking.

@sttts
Copy link
Contributor

sttts commented Dec 12, 2016 •
edited by jessfraz
Loading

@saad-ali @david-mcmahon we are seeing flakes (#27680) on the 1.5 branch which would be fixed by #37103 and #38260. Please consider them for cherry-picking.

@sttts sttts added this to the v1.6 milestone Dec 12, 2016
@jessfraz jessfraz modified the milestones: v1.4, v1.6 Dec 14, 2016
@jessfraz jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 14, 2016
@jessfraz jessfraz mentioned this pull request Dec 14, 2016
Merged
@jessfraz
Copy link
Contributor

cherry-picked both in #38782

jessfraz added a commit that referenced this pull request Dec 15, 2016
@jessfraz
Merge pull request #38782 from jessfraz/automated-cherry-pick-of-#37103-
94deaac 
#38260-origin-release-1.4

Automated cherry pick of #37103 #38260
@fraenkel fraenkel deleted the port_forward_readiness branch January 21, 2017 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sttts sttts

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@fraenkel @k8s-ci-robot @k8s-reviewable @sttts @k8s-github-robot @k8s-cherrypick-bot @jessfraz @ixdy