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

docker_container: wait for removal if removal is in process #65854

Merged
merged 4 commits into from Dec 29, 2019
Merged

docker_container: wait for removal if removal is in process #65854

merged 4 commits into from Dec 29, 2019

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Fixes #65811.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_container

@ansibot
Copy link
Contributor

ansibot commented Dec 15, 2019

cc @DBendit @WojciechowskiPiotr @akshay196 @chouseknecht @danihodovic @dariko @dusdanig @joshuaconner @jwitko @kassiansun @softzilla @tbouvet @zfil
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. cloud docker module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Dec 15, 2019
@felixfontein felixfontein mentioned this pull request Dec 15, 2019
Closed
@darkdragon-001
Copy link

Tested and working fine for me!

@felixfontein felixfontein changed the title [WIP] docker_container: wait for removal if removal is in process docker_container: wait for removal if removal is in process Dec 16, 2019
@felixfontein
Copy link
Contributor Author

Great! In that case, it's no longer a WIP ;)

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Dec 16, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 24, 2019
Andersson007
Andersson007 reviewed Dec 27, 2019
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@felixfontein , can we reproduce the situation with CI tests?

@felixfontein
Copy link
Contributor Author

@felixfontein , can we reproduce the situation with CI tests?

I knew you would be asking this ;-) The problem with this is that I didn't even manage to reproduce it locally, so I have no clue on how to reproduce it in CI. Also, I'd guess that reproducing this would induce a high strain on the CI system, potentially making it quite expensive (also in money terms). So I guess it is best to make sure that this doesn't hurt existing use-cases (which CI should do), and hope that @darkdragon-001's successful tests indicate that it actually works in situations where the wait is required.

@Andersson007
Copy link
Contributor

Andersson007 commented Dec 27, 2019 •
edited

Ok, clear, if @darkdragon-001 tested it manually that’s ok (in this case, i said feeling pain a bit:)
I haven’t looked at the code in detail, hope to have time on Monday

@darkdragon-001
Copy link

It's working fine for me. If I interpret the changes correctly, it just makes sure that the container is deleted before a new one is created.

If you want to reproduce this in CI, the "correct" way would be to create a dummy docker storage provider which gives gives information like deleted when set with CI. But I guess it's not worth the work... Alternatively emulate very slow storage. But again, I don't know if it's worth to regularly put that much work on the CI. So I would say that manual tests with manual code review should be fine here...

@felixfontein felixfontein mentioned this pull request Dec 29, 2019
Merged
@WojciechowskiPiotr
Copy link
Contributor

shipit

@ansibot ansibot removed community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. labels Dec 29, 2019
@ansibot ansibot added shipit This PR is ready to be merged by Core and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 29, 2019
@felixfontein felixfontein merged commit 4df5bdb into ansible:devel Dec 29, 2019
@felixfontein felixfontein deleted the docker_container-wait-for-removal branch December 29, 2019 22:16
@felixfontein
Copy link
Contributor Author

@darkdragon-001 thanks a lot for testing this fix!
@Andersson007 @WojciechowskiPiotr thanks a lot for reviewing!

felixfontein added a commit to felixfontein/ansible that referenced this pull request Dec 29, 2019
@felixfontein
docker_container: wait for removal if removal is in process (ansible#…
dbc76b4 
…65854)

* Allow to inspect containers directly.

* Wait for containers to be removed before recreating them.

* Also wait for containers to be removed before creating them.

* Add changelog.

(cherry picked from commit 4df5bdb)
felixfontein added a commit to felixfontein/ansible that referenced this pull request Dec 29, 2019
@felixfontein
docker_container: wait for removal if removal is in process (ansible#…
d4d6b4f 
…65854)

* Allow to inspect containers directly.

* Wait for containers to be removed before recreating them.

* Also wait for containers to be removed before creating them.

* Add changelog.

(cherry picked from commit 4df5bdb)
This was referenced Dec 29, 2019
Merged
Merged
@Andersson007
Copy link
Contributor

@felixfontein , thanks!
ps, people are waiting for the new option added in 2.10:)

This was referenced Dec 31, 2019
Merged
Merged
@felixfontein
Copy link
Contributor Author

I've created a PR for that option here: #66144

I also found a bug (I think), namely the module shouldn't wait during check mode. I've fixed it here: #66145

countless-integers pushed a commit to countless-integers/ansible that referenced this pull request Jan 6, 2020
@felixfontein @countless-integers
docker_container: wait for removal if removal is in process (ansible#…
ca3dac0 
…65854)

* Allow to inspect containers directly.

* Wait for containers to be removed before recreating them.

* Also wait for containers to be removed before creating them.

* Add changelog.
mattclay pushed a commit that referenced this pull request Jan 10, 2020
@felixfontein @mattclay
[2.8] docker_container: wait for removal if removal is in process (#6…
facf938 
…6118)

* docker_container: wait for removal if removal is in process (#65854)

* Allow to inspect containers directly.

* Wait for containers to be removed before recreating them.

* Also wait for containers to be removed before creating them.

* Add changelog.

(cherry picked from commit 4df5bdb)

* Don't wait for removal during check mode. (#66145)

(cherry picked from commit 14e32c8)
mattclay pushed a commit that referenced this pull request Jan 10, 2020
@felixfontein @mattclay
[2.9] docker_container: wait for removal if removal is in process (#6…
cd348c1 
…6117)

* docker_container: wait for removal if removal is in process (#65854)

* Allow to inspect containers directly.

* Wait for containers to be removed before recreating them.

* Also wait for containers to be removed before creating them.

* Add changelog.

(cherry picked from commit 4df5bdb)

* Don't wait for removal during check mode. (#66145)

(cherry picked from commit 14e32c8)
@ansible ansible locked and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Andersson007 Andersson007 Andersson007 left review comments

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. cloud docker has_issue module This issue/PR relates to a module. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docker_container with auto_remove:yes fails
5 participants
@felixfontein @ansibot @darkdragon-001 @Andersson007 @WojciechowskiPiotr