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

podman start --attach, a container created with --rm, does not rm the container #10935

Closed
edsantiago opened this issue Jul 15, 2021 · 5 comments · Fixed by #10942
Closed

podman start --attach, a container created with --rm, does not rm the container #10935

edsantiago opened this issue Jul 15, 2021 · 5 comments · Fixed by #10942
Assignees
Labels
flakes Flakes from Continuous Integration kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@edsantiago
Copy link
Member

edsantiago commented Jul 15, 2021

...at least, not immediately. There's a race condition, and the following test is actually broken:
https://github.com/containers/podman/blob/4c38202bd1f79e0c6dc4d5533e3412a3761192c4/test/e2e/start_test.go#L67-L78

Trivial reproducer:

$ while :;do cid=$(./bin/podman create --rm quay.io/libpod/testimage:20210610 foo);./bin/podman start --attach $cid;./bin/podman container exists $cid && break;done

Will spit out a few iterations of errors, then break (because podman container exists succeeded).

Seems to be root & rootless

@edsantiago edsantiago added flakes Flakes from Continuous Integration kind/bug Categorizes issue or PR as related to a bug. labels Jul 15, 2021
edsantiago added a commit to edsantiago/libpod that referenced this issue Jul 15, 2021
e2e test failures are rife with messages like:

   Expected 1 to equal 0

These make me cry. They're anti-helpful, requiring the reader
to dive into the source code to figure out what those numbers
mean.

Solution: Go tests have a '.Should(Exit(NNN))' mechanism. I
don't know if it spits out a better diagnostic (I have no way
to run e2e tests on my laptop), but I have to fantasize that
it will, and given the state of our flakes I assume that at
least one test will fail and give me the opportunity to see
what the error message looks like.

THIS IS NOT REVIEWABLE CODE. There is no way for a human
to review it. Don't bother. Maybe look at a few random
ones for sanity. If you want to really review, here is
a reproducer of what I did:

   cd test/e2e
   ! positive assertions. The second is the same as the first,
   ! with the addition of (unnecessary) parentheses because
   ! some invocations were written that way. The third is BeZero().
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.To\(Equal\((\d+)\)\)/Expect($1).Should(Exit($2))/' *_test.go
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.To\(\(Equal\((\d+)\)\)\)/Expect($1).Should(Exit($2))/' *_test.go
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.To\(BeZero\(\)\)/Expect($1).Should(Exit(0))/' *_test.go

   ! Same as above, but handles three non-numeric exit codes
   ! in run_exit_test.go
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.To\(Equal\((\S+)\)\)/Expect($1).Should(Exit($2))/' *_test.go

   ! negative assertions. Difference is the spelling of 'To(Not)',
   ! 'ToNot', and 'NotTo'. I assume those are all the same.
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.To\(Not\(Equal\((0)\)\)\)/Expect($1).To(ExitWithError())/' *_test.go
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.ToNot\(Equal\((0)\)\)/Expect($1).To(ExitWithError())/' *_test.go
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.NotTo\(Equal\((0)\)\)/Expect($1).To(ExitWithError())/' *_test.go
   ! negative, old use of BeZero()
   perl -pi -e 's/Expect\((\S+)\.ExitCode\(\)\)\.ToNot\(BeZero\(\)\)/Expect($1).Should(ExitWithError())/' *_test.go

Run those on a clean copy of main branch (at the same branch
point as my PR, of course), then diff against a checked-out
copy of my PR. There should be no differences. Then all you
have to review is that my replacements above are sane.

UPDATE: nope, that's not enough, you also need to add gomega/gexec
to the files that don't have it:

   perl -pi -e '$_ .= "$1/gexec\"\n" if m!^(.*/onsi/gomega)"!' $(grep -L gomega/gexec $(git log -1 --stat | awk '$1 ~ /test\/e2e\// { print $1}'))

UPDATE 2: hand-edit run_volume_test.go

UPDATE 3: sigh, add WaitWithDefaultTimeout() to a couple of places

UPDATE 4: skip a test due to bug containers#10935 (race condition)

Signed-off-by: Ed Santiago <[email protected]>
@vrothberg vrothberg self-assigned this Jul 15, 2021
@vrothberg
Copy link
Member

I am taking a look. Thanks for the reproducer, @edsantiago !

@vrothberg
Copy link
Member

Small fix: s/foo/ls/ in the reproducer. foo will bark immediately as it doesn't exist on the image.

@edsantiago
Copy link
Member Author

That's the point -- I was going with a reproducer of the exact same e2e test

@vrothberg
Copy link
Member

For ls I think the race is natural; only run will wait for the container to be cleaned up.

My gut feeling is that this should be true for foo as well; should podman wait if the command succeeded (if so, for how long)? But I'll have a look at the git history and see what Docker does.

@vrothberg
Copy link
Member

Opened #10942.

vrothberg added a commit to vrothberg/libpod that referenced this issue Jul 16, 2021
Make sure that containers configured for auto removal
(e.g., via `podman create --rm`) are removed in `podman start`
if starting the container failed.

Fixes: containers#10935
Signed-off-by: Valentin Rothberg <[email protected]>
@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flakes Flakes from Continuous Integration kind/bug Categorizes issue or PR as related to a bug. 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 a pull request may close this issue.

2 participants