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

[FLOC-1194] Add forceable container removal if needed #1753

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dwgebler
Copy link
Contributor

@dwgebler dwgebler commented Jul 3, 2015

Probably fixes FLOC-1194 by allowing the affected test to force container removal during cleanup.

Review on Reviewable

@@ -546,7 +546,7 @@ def _blocking_container_runs(self, container_name):
).write()
return result['State']['Running']

def remove(self, unit_name):
def remove(self, unit_name, force=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is an implementation of an interface method, IDockerClient.remove.

  • The interface needs to be updated similarly.
  • The other implementation needs to be updated similarly.
  • The interface-based test suite factory, make_idockerclient_tests, needs to have tests for the new functionality added.

@exarkun
Copy link
Contributor

exarkun commented Jul 7, 2015

Thanks. Please address and re-submit.

@@ -208,13 +208,14 @@ def exists(unit_name):
otherwise ``False``.
"""

def remove(unit_name):
def remove(unit_name, force):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have thought zope.verify would have caught this, but the signature in the interface should match the signature in the implementation, so the interface ought to have force=False.

@wallrj
Copy link
Contributor

wallrj commented Jul 10, 2015

http://blog.hashbangbash.com/2014/11/docker-devicemapper-fix-for-device-or-resource-busy-ebusy/ seems to describe the problem and the fix.

Do we know if any of our supported Distros implement the suggested fix in their systemd / upstart units?

@binocarlos tells me that Docker on Ubuntu uses AUFs by default if the kernel module is detected; exactly because of this device mapper issue. Although judging by the @dwgebler 's other link (moby/moby#9665) the problem also occurs with AUFs.
Does anyone know if AUFs is less flaky than devicemapper? If so, can we add AUFS to our list of package dependencies and document that we recommend using docker with AUFS instead of devicemapper.

I'm not convinced about the force=True fix.
Our DockerClientAPI.remove already attempts to stop the container before removal and waits for it to stop.
And I'm sure @jongiddy and others have considered adding the force option in the past for other reasons or perhaps for this reason.

I also notice that docker API 1.19 has a v option which sounds related:

I don't really know what to say. I think I'll reject this so that we can discuss it further.

@wallrj
Copy link
Contributor

wallrj commented Jul 10, 2015

Has anyone tested this:

...work around?

Start another container for the same image then remove the old container.

@dwgebler
Copy link
Contributor Author

Start another container for the same image then remove the old container.

I didn't try that one - force does definitely seem flaky; there was a suggestion on the Docker issue that it worked for some people in resolving the problem of removing the container, but I'm not sure why that would be the case, given that the only documented behaviour of force I can find is that it first terminates the process in the container via SIGKILL, which should be the same as stopping the container first - which we already do.

As per the discussion we were just having in the office, supporting a more up-to-date Docker, i.e. 1.7 and 1.19 API might also be a good thing to start thinking about at this time.

@ci-jenkins-clusterhq
Copy link

Can someone verify this PR?

@jml
Copy link
Contributor

jml commented Feb 9, 2016

This PR has been around for a while. Maybe we should close it for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants