Skip to content

"streaming output" logs test: fix flake#6640

Merged
openshift-merge-robot merged 1 commit into
containers:masterfrom
edsantiago:fix_flaky_logs_test
Jun 17, 2020
Merged

"streaming output" logs test: fix flake#6640
openshift-merge-robot merged 1 commit into
containers:masterfrom
edsantiago:fix_flaky_logs_test

Conversation

@edsantiago
Copy link
Copy Markdown
Member

Test has been flaking excessively. A quick look shows that
the test itself is broken, making a bad assumption.

'podman logs -f' is guaranteed to exit when a container
terminates. This does not (and should not) mean that the
container has been cleaned up. It is undefined and unsafe
to run 'podman run -n same-name-as-terminated-container'
immediately after 'podman logs' exits.

Solution: instead of 'podman run', do 'podman inspect'.
This, too, is unsafe, but we can expect to see one of
two possible conditions:

  1. command succeeds, in which case we require that
    container State.Status be "exited"; or
  2. command fails, in which case we expect "no such
    container" in error output

For full coverage we should add a small delay-check test
to (1) to ensure that the container is cleaned up after
a short amount of time. Leaving that as a TODO because
it's more than my Go skills can handle, and I want to
get this checked in ASAP to get rid of the flake hassle.

Signed-off-by: Ed Santiago santiago@redhat.com

Copy link
Copy Markdown
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, let's get this in 👍

Thanks, Ed!

Test has been flaking excessively. A quick look shows that
the test itself is broken, making a bad assumption.

'podman logs -f' is guaranteed to exit when a container
terminates. This does not (and should not) mean that the
container has been cleaned up. It is undefined and unsafe
to run 'podman run -n same-name-as-terminated-container'
immediately after 'podman logs' exits.

Solution: instead of 'podman run', do 'podman inspect'.
This, too, is unsafe, but we can expect to see one of
two possible conditions:

  1) command succeeds, in which case we require that
     container State.Status be "exited"; or
  2) command fails, in which case we expect "no such
     container" in error output

For full coverage we should add a small delay-check test
to (1) to ensure that the container is cleaned up after
a short amount of time. Leaving that as a TODO because
it's more than my Go skills can handle, and I want to
get this checked in ASAP to get rid of the flake hassle.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the fix_flaky_logs_test branch from ce94d0e to 6d5a432 Compare June 17, 2020 13:04
@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 17, 2020

/lgtm
/hold

@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 Jun 17, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 17, 2020

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, mheon

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2020
@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Jun 17, 2020

LGTM

1 similar comment
@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM

@jgallucci32
Copy link
Copy Markdown
Contributor

@edsantiago Will this test break if we revert the logs -f logic back to its previous state where it would hang indefinitely? PR #6644 reverts back to the original state where logs would hang when following a stopped container. It should still pass a test where the container is removed because that should stop following the logs.

@edsantiago
Copy link
Copy Markdown
Member Author

@jgallucci32 the podman logs part of this test is untouched since June 2019, so I expect that nothing should change.

@edsantiago
Copy link
Copy Markdown
Member Author

/hold cancel
Whew! Flakes were mostly the "logs -f" race condition, but also one weird "rmi" one.

test fedora-32 fedora-32

2020-06-17T07:50:36 integration_test

test ubuntu-20 ubuntu-20

2020-06-17T07:45:51 system_test

2020-06-17T08:23:23 system_test

@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 Jun 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 1acd2ad into containers:master Jun 17, 2020
@edsantiago edsantiago deleted the fix_flaky_logs_test branch June 17, 2020 15:06
@vrothberg
Copy link
Copy Markdown
Member

Great work, Ed!

I can take a look at the rmi ones (bookmarked) tomorrow. It is popping up frequently but I couldn't yet reproduce.

@jgallucci32
Copy link
Copy Markdown
Contributor

@edsantiago I just rebased #6644 to make sure this doesn't break backing out of the follow logs code added this week.

@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants