Skip to content

Commit 047b419

Browse files
Improve remove docker container function to take into account stopped containers (#12979)
1 parent ac189fe commit 047b419

File tree

4 files changed

+108
-12
lines changed

4 files changed

+108
-12
lines changed

localstack-core/localstack/utils/container_utils/container_client.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -706,9 +706,11 @@ def list_containers(self, filter: Union[list[str], str, None] = None, all=True)
706706

707707
def get_running_container_names(self) -> list[str]:
708708
"""Returns a list of the names of all running containers"""
709-
result = self.list_containers(all=False)
710-
result = [container["name"] for container in result]
711-
return result
709+
return self.__get_container_names(return_all=False)
710+
711+
def get_all_container_names(self) -> list[str]:
712+
"""Returns a list of the names of all containers including stopped ones"""
713+
return self.__get_container_names(return_all=True)
712714

713715
def is_container_running(self, container_name: str) -> bool:
714716
"""Checks whether a container with a given name is currently running"""
@@ -1133,6 +1135,11 @@ def login(self, username: str, password: str, registry: Optional[str] = None) ->
11331135
:param registry: Registry url
11341136
"""
11351137

1138+
def __get_container_names(self, return_all: bool) -> list[str]:
1139+
result = self.list_containers(all=return_all)
1140+
result = [container["name"] for container in result]
1141+
return result
1142+
11361143

11371144
class Util:
11381145
MAX_ENV_ARGS_LENGTH = 20000

localstack-core/localstack/utils/container_utils/docker_cmd_client.py

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -266,17 +266,21 @@ def commit(
266266
) from e
267267

268268
def remove_container(self, container_name: str, force=True, check_existence=False) -> None:
269-
if check_existence and container_name not in self.get_running_container_names():
269+
if check_existence and container_name not in self.get_all_container_names():
270270
return
271271
cmd = self._docker_cmd() + ["rm"]
272272
if force:
273273
cmd.append("-f")
274274
cmd.append(container_name)
275275
LOG.debug("Removing container with cmd %s", cmd)
276276
try:
277-
run(cmd)
277+
output = run(cmd)
278+
# When the container does not exist, the output could have the error message without any exception
279+
if isinstance(output, str) and not force:
280+
self._check_output_and_raise_no_such_container_error(container_name, output=output)
278281
except subprocess.CalledProcessError as e:
279-
self._check_and_raise_no_such_container_error(container_name, error=e)
282+
if not force:
283+
self._check_and_raise_no_such_container_error(container_name, error=e)
280284
raise ContainerException(
281285
"Docker process returned with errorcode %s" % e.returncode, e.stdout, e.stderr
282286
) from e
@@ -915,12 +919,20 @@ def _check_and_raise_no_such_container_error(
915919
Check the given client invocation error and raise a `NoSuchContainer` exception if it
916920
represents a `no such container` exception from Docker or Podman.
917921
"""
922+
self._check_output_and_raise_no_such_container_error(
923+
container_name_or_id, str(error.stdout), error=str(error.stderr)
924+
)
918925

919-
# consider different error messages for Docker/Podman
920-
error_messages = ("No such container", "no container with name or ID")
921-
process_stdout_lower = to_str(error.stdout).lower()
922-
if any(msg.lower() in process_stdout_lower for msg in error_messages):
923-
raise NoSuchContainer(container_name_or_id, stdout=error.stdout, stderr=error.stderr)
926+
def _check_output_and_raise_no_such_container_error(
927+
self, container_name_or_id: str, output: str, error: Optional[str] = None
928+
):
929+
"""
930+
Check the given client invocation output and raise a `NoSuchContainer` exception if it
931+
represents a `no such container` exception from Docker or Podman.
932+
"""
933+
possible_not_found_messages = ("No such container", "no container with name or ID")
934+
if any(msg.lower() in output.lower() for msg in possible_not_found_messages):
935+
raise NoSuchContainer(container_name_or_id, stdout=output, stderr=error)
924936

925937
def _transform_container_labels(self, labels: Union[str, dict[str, str]]) -> dict[str, str]:
926938
"""

localstack-core/localstack/utils/container_utils/docker_sdk_client.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ def unpause_container(self, container_name: str) -> None:
266266

267267
def remove_container(self, container_name: str, force=True, check_existence=False) -> None:
268268
LOG.debug("Removing container: %s", container_name)
269-
if check_existence and container_name not in self.get_running_container_names():
269+
if check_existence and container_name not in self.get_all_container_names():
270270
LOG.debug("Aborting removing due to check_existence check")
271271
return
272272
try:

tests/integration/docker_utils/test_docker.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -826,6 +826,83 @@ def test_list_containers_filter_illegal_filter(self, docker_client: ContainerCli
826826
with pytest.raises(ContainerException):
827827
docker_client.list_containers(filter="illegalfilter=foobar")
828828

829+
def test_get_running_container_names_should_ignore_stopped_containers(
830+
self, docker_client: ContainerClient, create_container
831+
):
832+
cn1 = _random_container_name()
833+
cn2 = _random_container_name()
834+
cn3 = _random_container_name()
835+
836+
create_container("alpine", name=cn1, command=["sleep", "30"])
837+
create_container("alpine", name=cn2, command=["sleep", "30"])
838+
create_container("alpine", name=cn3, command=["sleep", "30"])
839+
840+
docker_client.start_container(cn1)
841+
docker_client.start_container(cn2)
842+
843+
container_names = docker_client.get_running_container_names()
844+
# We dont do equal comparison here to avoid brittle tests since there could be other containers.
845+
assert 2 <= len(container_names)
846+
assert all(x in container_names for x in [cn1, cn2])
847+
assert cn3 not in container_names
848+
849+
def test_get_all_container_names_should_include_even_stopped_containers(
850+
self, docker_client: ContainerClient, create_container
851+
):
852+
cn1 = _random_container_name()
853+
cn2 = _random_container_name()
854+
cn3 = _random_container_name()
855+
856+
create_container("alpine", name=cn1, command=["sleep", "30"])
857+
create_container("alpine", name=cn2, command=["sleep", "30"])
858+
create_container("alpine", name=cn3, command=["sleep", "30"])
859+
860+
docker_client.start_container(cn1)
861+
docker_client.start_container(cn2)
862+
863+
container_names = docker_client.get_all_container_names()
864+
# We dont do equal comparison here to avoid brittle tests since there could be other containers.
865+
assert 3 <= len(container_names)
866+
assert all(x in container_names for x in [cn1, cn2, cn3])
867+
868+
def test_remove_container_should_work_when_container_is_running_and_checking_container_existence(
869+
self, docker_client: ContainerClient, create_container
870+
):
871+
cn1 = _random_container_name()
872+
create_container("alpine", name=cn1, command=["sleep", "30"])
873+
874+
docker_client.start_container(cn1)
875+
876+
docker_client.remove_container(cn1, check_existence=True)
877+
878+
container_names = docker_client.get_all_container_names()
879+
assert cn1 not in container_names
880+
881+
def test_remove_container_should_work_when_container_is_stopped_and_checking_container_existence(
882+
self, docker_client: ContainerClient, create_container
883+
):
884+
cn1 = _random_container_name()
885+
create_container("alpine", name=cn1, command=["sleep", "30"])
886+
docker_client.remove_container(cn1, check_existence=True)
887+
888+
container_names = docker_client.get_all_container_names()
889+
assert cn1 not in container_names
890+
891+
def test_remove_container_should_throw_exception_when_container_doesnt_exist_and_not_forcing(
892+
self, docker_client: ContainerClient
893+
):
894+
with pytest.raises(NoSuchContainer):
895+
docker_client.remove_container(
896+
"mygiganonexistingcontainer", check_existence=False, force=False
897+
)
898+
899+
def test_remove_container_should_work_even_when_container_doesnt_exist_because_of_forcing(
900+
self, docker_client: ContainerClient
901+
):
902+
docker_client.remove_container(
903+
"mygiganonexistingcontainer", check_existence=False, force=True
904+
)
905+
829906
def test_list_containers_filter(self, docker_client: ContainerClient, create_container):
830907
name_prefix = "filter_tests_"
831908
cn1 = name_prefix + _random_container_name()

0 commit comments

Comments
 (0)