Skip to content
This repository was archived by the owner on May 9, 2022. It is now read-only.

Docker#delete_container: rescue on "device or resource busy" error#7

Merged
spraints merged 2 commits into
masterfrom
rescue-from-bad-deletions
Jan 25, 2016
Merged

Docker#delete_container: rescue on "device or resource busy" error#7
spraints merged 2 commits into
masterfrom
rescue-from-bad-deletions

Conversation

@parkr
Copy link
Copy Markdown
Contributor

@parkr parkr commented Jan 22, 2016

@spraints Not really sure the best way to go about testing this change, but this should help us handle moby/moby#9665.

Hoosegow does automatically attempt to delete the container once the process ends. Would a better option be to sleep 1 and try to delete again? Seems crazy but maybe that’d quell the “resource busy” errors we see.

Comment thread lib/hoosegow/docker.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The begin and end aren't necessary, since this method is so short.

Do you know which other errors delete might raise? I think it'd be fine to swallow any Docker::Error::ServerError. It might be nice to log them.

@spraints
Copy link
Copy Markdown
Contributor

Would a better option be to sleep 1 and try to delete again?

The containers I looked at were gone from docker, but they were still on disk. So sleep 1 and try again probably wouldn't help, you'd just get a 404 on the retry. In moby/moby#9665, it looks like the files get pretty thoroughly wedged, and need a reboot sometimes to be able to be removed.

@parkr parkr force-pushed the rescue-from-bad-deletions branch from 66904c7 to 9be5319 Compare January 23, 2016 21:02
@parkr parkr force-pushed the rescue-from-bad-deletions branch from 9be5319 to b863f08 Compare January 23, 2016 21:18
@parkr
Copy link
Copy Markdown
Contributor Author

parkr commented Jan 23, 2016

@spraints Added a (kind of gross) test and updated the method. Let me know what you think!

@parkr parkr added the bug label Jan 23, 2016
@spraints
Copy link
Copy Markdown
Contributor

This looks ok. 👍 The test doesn't seem to do all that much, since it's all mock interactions. Do you think it's worth keeping the test?

@parkr
Copy link
Copy Markdown
Contributor Author

parkr commented Jan 24, 2016

It kept me from forgetting the ::Docker. Not a high-value test but felt bad
submitting a patch without a test.

Matt Burke notifications@github.com schrieb am Sa., 23. Jan. 2016 um
14:45:

This looks ok. [image: 👍] The test doesn't seem to do all that much,
since it's all mock interactions. Do you think it's worth keeping the test?


Reply to this email directly or view it on GitHub
#7 (comment).

Parker

@spraints
Copy link
Copy Markdown
Contributor

It kept me from forgetting the ::Docker. Not a high-value test but felt bad
submitting a patch without a test.

Yeah, that makes sense. Tests that mostly exercise stubs feel weird to me. The ::Docker thing is a good reason to keep the test around. I'll ✂️ a new version 🔜.

spraints added a commit that referenced this pull request Jan 25, 2016
Docker#delete_container: rescue on "device or resource busy" error
@spraints spraints merged commit 87b2f19 into master Jan 25, 2016
@spraints spraints deleted the rescue-from-bad-deletions branch January 25, 2016 12:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants