From 624f8771690aebe969a9ecbec61121907f21d680 Mon Sep 17 00:00:00 2001 From: David Gebler Date: Fri, 3 Jul 2015 14:30:50 +0100 Subject: [PATCH 1/4] add forceable container removal if needed --- flocker/node/_docker.py | 4 ++-- flocker/node/functional/test_docker.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/flocker/node/_docker.py b/flocker/node/_docker.py index 594148a87f..c952131bb7 100644 --- a/flocker/node/_docker.py +++ b/flocker/node/_docker.py @@ -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): container_name = self._to_container_name(unit_name) def _remove(): @@ -608,7 +608,7 @@ def _remove(): message_type="flocker:docker:container_remove", container=container_name ).write() - self._client.remove_container(container_name) + self._client.remove_container(container_name, force=force) Message.new( message_type="flocker:docker:container_removed", container=container_name diff --git a/flocker/node/functional/test_docker.py b/flocker/node/functional/test_docker.py index b0b2121790..79268b6f3d 100644 --- a/flocker/node/functional/test_docker.py +++ b/flocker/node/functional/test_docker.py @@ -99,7 +99,7 @@ def start_container(self, unit_name, environment=None, volumes=(), mem_limit=None, cpu_shares=None, restart_policy=RestartNever(), - command_line=None): + command_line=None, force=False): """ Start a unit and wait until it reaches the `active` state or the supplied `expected_state`. @@ -114,6 +114,8 @@ def start_container(self, unit_name, :param cpu_shares: See ``IDockerClient.add``. :param restart_policy: See ``IDockerClient.add``. :param command_line: See ``IDockerClient.add``. + :param bool force: Force container removal via SIGKILL + during cleanup. :return: ``Deferred`` that fires with the ``DockerClient`` when the unit reaches the expected state. @@ -130,7 +132,7 @@ def start_container(self, unit_name, restart_policy=restart_policy, command_line=command_line, ) - self.addCleanup(client.remove, unit_name) + self.addCleanup(client.remove, unit_name, force) d.addCallback(lambda _: wait_for_unit_state(client, unit_name, expected_states)) @@ -356,6 +358,7 @@ def image_built(image_name): unit_name=unit_name, image_name=image_name, environment=Environment(variables=expected_variables), + force=True, ) d.addCallback(image_built) From 5caa9203b5856482bb7276571a825a45a5fb3b06 Mon Sep 17 00:00:00 2001 From: David Gebler Date: Fri, 3 Jul 2015 15:03:17 +0100 Subject: [PATCH 2/4] test force removal --- flocker/node/functional/test_docker.py | 40 ++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/flocker/node/functional/test_docker.py b/flocker/node/functional/test_docker.py index 79268b6f3d..f6ed0fd01a 100644 --- a/flocker/node/functional/test_docker.py +++ b/flocker/node/functional/test_docker.py @@ -140,6 +140,46 @@ def start_container(self, unit_name, return d + def test_force_remove(self): + """ + ``DockerClient.remove`` can forcefully remove a running container + when the ``force`` parameter is set to True. + """ + client = DockerClient() + unit_name = u'test-force-remove' + expected_states = (u'active',) + d = client.add( + unit_name=unit_name, + image_name=u'clusterhq/flask' + ) + d.addCallback( + lambda _: wait_for_unit_state( + client, unit_name, expected_states + ) + ) + + def check_is_listed(_): + listed = client.list() + + def check_unit(results): + self.assertTrue(unit_name in [unit.name for unit in results]) + listed.addCallback(check_unit) + return listed + + d.addCallback(check_is_listed) + d.addCallback(lambda _: client.remove(unit_name, force=True)) + + def check_is_removed(_): + listed = client.list() + + def check_unit(results): + self.assertFalse(unit_name in [unit.name for unit in results]) + listed.addCallback(check_unit) + return listed + + d.addCallback(check_is_removed) + return d + def test_default_base_url(self): """ ``DockerClient`` instantiated with a default base URL for a socket From d60b09ad105df2abab88d9a6c5fc4f6b6faadc60 Mon Sep 17 00:00:00 2001 From: David Gebler Date: Tue, 7 Jul 2015 14:36:28 +0100 Subject: [PATCH 3/4] address some review comments --- flocker/node/_docker.py | 5 +++-- flocker/node/functional/test_docker.py | 6 +++--- flocker/node/test/test_docker.py | 25 +++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/flocker/node/_docker.py b/flocker/node/_docker.py index c952131bb7..9ad2c01a4c 100644 --- a/flocker/node/_docker.py +++ b/flocker/node/_docker.py @@ -208,13 +208,14 @@ def exists(unit_name): otherwise ``False``. """ - def remove(unit_name): + def remove(unit_name, force): """ Stop and delete the given unit. This can be done multiple times in a row for the same unit. :param unicode unit_name: The name of the unit to stop. + :param bool force: Force removal of the unit. :return: ``Deferred`` that fires once the unit has been stopped and removed. @@ -271,7 +272,7 @@ def add(self, unit_name, image_name, ports=frozenset(), environment=None, def exists(self, unit_name): return succeed(unit_name in self._units) - def remove(self, unit_name): + def remove(self, unit_name, force=False): if unit_name in self._units: del self._units[unit_name] return succeed(None) diff --git a/flocker/node/functional/test_docker.py b/flocker/node/functional/test_docker.py index f6ed0fd01a..bd29b49df7 100644 --- a/flocker/node/functional/test_docker.py +++ b/flocker/node/functional/test_docker.py @@ -150,7 +150,7 @@ def test_force_remove(self): expected_states = (u'active',) d = client.add( unit_name=unit_name, - image_name=u'clusterhq/flask' + image_name=u"openshift/busybox-http-app" ) d.addCallback( lambda _: wait_for_unit_state( @@ -162,7 +162,7 @@ def check_is_listed(_): listed = client.list() def check_unit(results): - self.assertTrue(unit_name in [unit.name for unit in results]) + self.assertIn(unit_name, [unit.name for unit in results]) listed.addCallback(check_unit) return listed @@ -173,7 +173,7 @@ def check_is_removed(_): listed = client.list() def check_unit(results): - self.assertFalse(unit_name in [unit.name for unit in results]) + self.assertNotIn(unit_name, [unit.name for unit in results]) listed.addCallback(check_unit) return listed diff --git a/flocker/node/test/test_docker.py b/flocker/node/test/test_docker.py index c3a8b9b232..cf3227ab16 100644 --- a/flocker/node/test/test_docker.py +++ b/flocker/node/test/test_docker.py @@ -117,6 +117,31 @@ def test_removed_does_not_exist(self): d.addCallback(self.assertFalse) return d + def test_forced_removed_does_not_exist(self): + """ + A container removed with the ``force`` switch does not exist. + """ + client = fixture(self) + name = random_name(self) + image = u"openshift/busybox-http-app" + d = client.add(name, image) + d.addCallback(lambda _: client.list()) + expected = Unit( + name=name, container_name=name, activation_state=u"active", + container_image=image, + ) + + def got_units(units): + result = units.pop() + result = result.set("container_name", name) + self.assertEqual(expected, result) + + d.addCallback(got_units) + d.addCallback(lambda _: client.remove(name, force=True)) + d.addCallback(lambda _: client.exists(name)) + d.addCallback(self.assertFalse) + return d + def test_added_is_listed(self): """ An added container is included in the output of ``list()``. From 5d7c1a72ceea21bc5097a62391f3c3fbb7d9dea5 Mon Sep 17 00:00:00 2001 From: David Gebler Date: Wed, 8 Jul 2015 10:41:46 +0100 Subject: [PATCH 4/4] add reference to Docker problem in docstring --- flocker/node/functional/test_docker.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/flocker/node/functional/test_docker.py b/flocker/node/functional/test_docker.py index bd29b49df7..036d8e1025 100644 --- a/flocker/node/functional/test_docker.py +++ b/flocker/node/functional/test_docker.py @@ -144,6 +144,19 @@ def test_force_remove(self): """ ``DockerClient.remove`` can forcefully remove a running container when the ``force`` parameter is set to True. + + This is intended to help avoid an intermittent error in other + tests caused by a Docker behaviour where an "device busy" error + occurs when attempting to remove a container. + + In many cases, it is reported that forced removal of the container + via the Docker API or "-f" CLI switch is a work-around that prevents + this error, though is not a solution to the underlying problem. + + See https://github.com/docker/docker/issues/9665 + See http://blog.hashbangbash.com/2014/11/ + docker-devicemapper-fix-for-device-or-resource-busy-ebusy/ + See FLOC-1194. """ client = DockerClient() unit_name = u'test-force-remove'