-
Notifications
You must be signed in to change notification settings - Fork 327
feat(reliability): integrate the ryuk container for better container cleanup #314
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
Changes from 42 commits
7c09ccc
957b863
fc30fc1
e523dba
543ce3e
a487dc4
38a9d5a
a214089
fdaee73
d4ddcba
3711fcf
89113a3
101fd11
8802f39
7449d14
6c1650d
3e2c66c
c401df7
581e38b
5b1988e
6c24543
77251bb
4c37960
dc66658
dd18c01
1e2f7a1
a4827f2
8396907
07d4849
c239d52
22e03eb
6dce010
fed8d49
a510983
51ccbc4
a0d8487
e9974b3
904ae1c
74c82af
ed47f57
5772dca
da6f3d8
81cb09e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,8 @@ | |
MAX_TRIES = int(environ.get("TC_MAX_TRIES", 120)) | ||
SLEEP_TIME = int(environ.get("TC_POOLING_INTERVAL", 1)) | ||
TIMEOUT = MAX_TRIES * SLEEP_TIME | ||
|
||
RYUK_IMAGE: str = environ.get("RYUK_CONTAINER_IMAGE", "testcontainers/ryuk:0.5.1") | ||
RYUK_PRIVILEGED: bool = environ.get("TESTCONTAINERS_RYUK_PRIVILEGED", "false") == "true" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets evaluated on import and can't be manupulated afterwards better make it a method for better evaluation. Currently in order for this to work I need:
This setup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @teodor-t-tenev - #532 solves this, and its included since 4.3.2 - i think it should work if you have regular imports at the top of your file and later do from testcontainers.core.config import testcontainers_config as config
config.ryuk_disabled = True There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the reply. That did the work for 4.3.2. |
||
RYUK_DISABLED: bool = environ.get("TESTCONTAINERS_RYUK_DISABLED", "false") == "true" | ||
RYUK_DOCKER_SOCKET: str = environ.get("TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE", "/var/run/docker.sock") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,18 @@ | ||
import contextlib | ||
from platform import system | ||
from typing import Optional | ||
from __future__ import annotations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we sure this is what we want to do here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either we need to put almost everything in return types in quotes and switch to old syntax for unions, or we can have this import which postpones the evaluation of type hints by basically making them evaluate as strings. Both approaches are fully supported by linters such as Ruff, Pylance and Mypy. When we drop support for 3.9 we can more easily use modern syntax without breaking backwards compatibility (Even though we have to wait until 3.11 before we can use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the record, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced the |
||
|
||
from docker.models.containers import Container | ||
from platform import system | ||
from socket import socket | ||
from typing import TYPE_CHECKING, Optional | ||
|
||
from testcontainers.core.config import RYUK_DISABLED, RYUK_DOCKER_SOCKET, RYUK_IMAGE, RYUK_PRIVILEGED | ||
from testcontainers.core.docker_client import DockerClient | ||
from testcontainers.core.exceptions import ContainerStartException | ||
from testcontainers.core.labels import LABEL_SESSION_ID, SESSION_ID | ||
from testcontainers.core.utils import inside_container, is_arm, setup_logger | ||
from testcontainers.core.waiting_utils import wait_container_is_ready | ||
from testcontainers.core.waiting_utils import wait_container_is_ready, wait_for_logs | ||
|
||
if TYPE_CHECKING: | ||
from docker.models.containers import Container | ||
|
||
logger = setup_logger(__name__) | ||
|
||
|
@@ -25,7 +30,12 @@ class DockerContainer: | |
... delay = wait_for_logs(container, "Hello from Docker!") | ||
""" | ||
|
||
def __init__(self, image: str, docker_client_kw: Optional[dict] = None, **kwargs) -> None: | ||
def __init__( | ||
self, | ||
image: str, | ||
docker_client_kw: Optional[dict] = None, | ||
**kwargs, | ||
) -> None: | ||
self.env = {} | ||
self.ports = {} | ||
self.volumes = {} | ||
|
@@ -36,29 +46,32 @@ def __init__(self, image: str, docker_client_kw: Optional[dict] = None, **kwargs | |
self._name = None | ||
self._kwargs = kwargs | ||
|
||
def with_env(self, key: str, value: str) -> "DockerContainer": | ||
def with_env(self, key: str, value: str) -> DockerContainer: | ||
self.env[key] = value | ||
return self | ||
|
||
def with_bind_ports(self, container: int, host: Optional[int] = None) -> "DockerContainer": | ||
def with_bind_ports(self, container: int, host: Optional[int] = None) -> DockerContainer: | ||
self.ports[container] = host | ||
return self | ||
|
||
def with_exposed_ports(self, *ports: int) -> "DockerContainer": | ||
def with_exposed_ports(self, *ports: int) -> DockerContainer: | ||
for port in ports: | ||
self.ports[port] = None | ||
return self | ||
|
||
def with_kwargs(self, **kwargs) -> "DockerContainer": | ||
def with_kwargs(self, **kwargs) -> DockerContainer: | ||
self._kwargs = kwargs | ||
return self | ||
|
||
def maybe_emulate_amd64(self) -> "DockerContainer": | ||
def maybe_emulate_amd64(self) -> DockerContainer: | ||
if is_arm(): | ||
return self.with_kwargs(platform="linux/amd64") | ||
return self | ||
|
||
def start(self) -> "DockerContainer": | ||
def start(self) -> DockerContainer: | ||
if not RYUK_DISABLED and self.image != RYUK_IMAGE: | ||
logger.debug("Creating Ryuk container") | ||
Reaper.get_instance() | ||
totallyzen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logger.info("Pulling image %s", self.image) | ||
docker_client = self.get_docker_client() | ||
self._container = docker_client.run( | ||
|
@@ -69,7 +82,7 @@ def start(self) -> "DockerContainer": | |
ports=self.ports, | ||
name=self._name, | ||
volumes=self.volumes, | ||
**self._kwargs | ||
**self._kwargs, | ||
) | ||
logger.info("Container started: %s", self._container.short_id) | ||
return self | ||
|
@@ -78,21 +91,12 @@ def stop(self, force=True, delete_volume=True) -> None: | |
self._container.remove(force=force, v=delete_volume) | ||
self.get_docker_client().client.close() | ||
|
||
def __enter__(self) -> "DockerContainer": | ||
def __enter__(self) -> DockerContainer: | ||
return self.start() | ||
|
||
def __exit__(self, exc_type, exc_val, exc_tb) -> None: | ||
self.stop() | ||
|
||
def __del__(self) -> None: | ||
""" | ||
__del__ runs when Python attempts to garbage collect the object. | ||
In case of leaky test design, we still attempt to clean up the container. | ||
""" | ||
with contextlib.suppress(Exception): | ||
if self._container is not None: | ||
self.stop() | ||
|
||
def get_container_host_ip(self) -> str: | ||
# infer from docker host | ||
host = self.get_docker_client().host() | ||
|
@@ -127,15 +131,15 @@ def get_exposed_port(self, port: int) -> str: | |
return port | ||
return mapped_port | ||
|
||
def with_command(self, command: str) -> "DockerContainer": | ||
def with_command(self, command: str) -> DockerContainer: | ||
self._command = command | ||
return self | ||
|
||
def with_name(self, name: str) -> "DockerContainer": | ||
def with_name(self, name: str) -> DockerContainer: | ||
self._name = name | ||
return self | ||
|
||
def with_volume_mapping(self, host: str, container: str, mode: str = "ro") -> "DockerContainer": | ||
def with_volume_mapping(self, host: str, container: str, mode: str = "ro") -> DockerContainer: | ||
mapping = {"bind": container, "mode": mode} | ||
self.volumes[host] = mapping | ||
return self | ||
|
@@ -155,3 +159,54 @@ def exec(self, command) -> tuple[int, str]: | |
if not self._container: | ||
raise ContainerStartException("Container should be started before executing a command") | ||
return self._container.exec_run(command) | ||
|
||
|
||
class Reaper: | ||
_instance: Optional[Reaper] = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. presumably if this is here we can actually reuse ryuk on multiple instances but then the actual code for telling ryuk about it is not in a regular instance public method - so not sure but i think this part could be more coherent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really sure if I understand what you are pointing to here - The |
||
_container: Optional[DockerContainer] = None | ||
_socket: Optional[socket] = None | ||
|
||
@classmethod | ||
def get_instance(cls) -> Reaper: | ||
if not Reaper._instance: | ||
Reaper._instance = Reaper._create_instance() | ||
|
||
return Reaper._instance | ||
|
||
@classmethod | ||
def delete_instance(cls) -> None: | ||
if Reaper._socket is not None: | ||
Reaper._socket.close() | ||
Reaper._socket = None | ||
|
||
if Reaper._container is not None: | ||
Reaper._container.stop() | ||
Reaper._container = None | ||
|
||
if Reaper._instance is not None: | ||
Reaper._instance = None | ||
|
||
@classmethod | ||
def _create_instance(cls) -> Reaper: | ||
logger.debug(f"Creating new Reaper for session: {SESSION_ID}") | ||
|
||
Reaper._container = ( | ||
DockerContainer(RYUK_IMAGE) | ||
.with_name(f"testcontainers-ryuk-{SESSION_ID}") | ||
.with_exposed_ports(8080) | ||
.with_volume_mapping(RYUK_DOCKER_SOCKET, "/var/run/docker.sock", "rw") | ||
.with_kwargs(privileged=RYUK_PRIVILEGED) | ||
.start() | ||
) | ||
wait_for_logs(Reaper._container, r".* Started!") | ||
|
||
container_host = Reaper._container.get_container_host_ip() | ||
container_port = int(Reaper._container.get_exposed_port(8080)) | ||
|
||
Reaper._socket = socket() | ||
Reaper._socket.connect((container_host, container_port)) | ||
Reaper._socket.send(f"label={LABEL_SESSION_ID}={SESSION_ID}\r\n".encode()) | ||
|
||
Reaper._instance = Reaper() | ||
|
||
return Reaper._instance |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
from typing import Optional | ||
from uuid import uuid4 | ||
|
||
from testcontainers.core.config import RYUK_IMAGE | ||
|
||
SESSION_ID: str = str(uuid4()) | ||
LABEL_SESSION_ID = "org.testcontainers.session-id" | ||
LABEL_LANG = "org.testcontainers.lang" | ||
|
||
|
||
def create_labels(image: str, labels: Optional[dict[str, str]]) -> dict[str, str]: | ||
if labels is None: | ||
labels = {} | ||
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you like you can add the following labels by default too:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK, for the other languages we just set the core version too. At least for Java and .NET, there are no individual versions. |
||
labels[LABEL_LANG] = "python" | ||
|
||
if image == RYUK_IMAGE: | ||
return labels | ||
|
||
labels[LABEL_SESSION_ID] = SESSION_ID | ||
return labels |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
from testcontainers.core import container | ||
from testcontainers.core.container import Reaper | ||
from testcontainers.core.container import DockerContainer | ||
from testcontainers.core.waiting_utils import wait_for_logs | ||
|
||
|
||
def test_wait_for_reaper(): | ||
container = DockerContainer("hello-world").start() | ||
wait_for_logs(container, "Hello from Docker!") | ||
|
||
assert Reaper._socket is not None | ||
Reaper._socket.close() | ||
|
||
assert Reaper._container is not None | ||
wait_for_logs(Reaper._container, r".* Removed \d .*", timeout=30) | ||
|
||
Reaper.delete_instance() | ||
|
||
|
||
def test_container_without_ryuk(monkeypatch): | ||
monkeypatch.setattr(container, "RYUK_DISABLED", True) | ||
with DockerContainer("hello-world") as cont: | ||
wait_for_logs(cont, "Hello from Docker!") | ||
assert Reaper._instance is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change this default to true to try to preserve backwards compatibility - like in principle we should make it really easy for people to move to the new thing and only really bother everyone else much later on.
like in jvm langs you can do
echo ryuk.disabled=true >> src/test/resources/testcontainers.properties
- im not sure of the python equivalent off the bat - maybe we have to stick to some sort of env var stuffThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about setting it to
true
by default and release 4.1 (or 4.2, whichever this one lands in) and then changing it tofalse
and release 5.0 straight away? That way we don't force people into using Ryuk on their 4.x builds, but I really think this should be enabled by default, as it is the expected behavior as seen in other implementations.By releasing it in both versions almost simultaneously we give them time to upgrade, and they have the possibility to test out the functionality without major bumping anything.