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

tweak failing-dc e2e test with sleep so docker have time to get the hook container logs #17746

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Dec 13, 2017

This tweaks the fixture for e2e test where the hook pod is executing really fast and Docker daemon seems to not have enough time to gather the container logs. This is NOT fixing the issue with Docker, but unblocking the queue as this is flaking hard... We should still continue to investigate the issue in Docker.

@tnozicka @deads2k

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 13, 2017
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2017
@tnozicka tnozicka self-requested a review December 13, 2017 15:46
Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2017
@tnozicka
Copy link
Contributor

the logs issue is tracked in #17747

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@@ -405,7 +405,10 @@ os::cmd::try_until_text 'oc get pods -l openshift.io/deployer-pod.type=hook-post
os::cmd::expect_success 'oc create -f test/testdata/failing-dc.yaml'
os::cmd::try_until_success 'oc get rc/failing-dc-1'
os::cmd::expect_failure 'oc rollout status dc/failing-dc'
os::cmd::expect_success_and_text 'oc logs dc/failing-dc' 'test pre hook executed'
# FIXME: getting the hook logs seems to be flaking, re-enable when we find the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do it like this :( we need the test, we just need to slightly modify it so that docker has a chance to get that last line. Add sleep 1 like here in test/testdata/failing-dc.yamlrather than disabling this test.

/hold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't do sleep 1 that just makes the flake less common, not fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @tnozicka adding sleep 1 will just hide this problem and we will never get to fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We (openshift team) don't own docker and we won't fix it, we can't block our team just to know about this bug. There should be an open issue to remove those sleeps but for now we need to limit the # of flakes.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2017
@mfojtik
Copy link
Contributor Author

mfojtik commented Dec 14, 2017

/retest

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 14, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2017
@mfojtik mfojtik changed the title tweak failing-dc e2e test to not depend on hook logs tweak failing-dc e2e test with sleep so docker have time to get the hook container logs Dec 14, 2017
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2017
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, soltysh, tnozicka

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@soltysh
Copy link
Contributor

soltysh commented Dec 14, 2017

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2017
@mfojtik
Copy link
Contributor Author

mfojtik commented Dec 14, 2017

@runcom I wonder if something similar like this is happening also with CRI-O

@mfojtik
Copy link
Contributor Author

mfojtik commented Dec 14, 2017

flake: #13068 (and others..)

/retest

@runcom
Copy link
Member

runcom commented Dec 14, 2017

We haven't seen failures on the cri-o side actually :)

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@mfojtik
Copy link
Contributor Author

mfojtik commented Dec 18, 2017

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 19, 2017

@mfojtik: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_gce 6ace95d link /test extended_conformance_gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 17572, 17746).

@openshift-merge-robot openshift-merge-robot merged commit 0f89555 into openshift:master Dec 19, 2017
@mfojtik mfojtik deleted the tweak-dc-e2e-test branch September 5, 2018 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants