-
Notifications
You must be signed in to change notification settings - Fork 297
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
feat(reliability): integrate the ryuk container for better container cleanup #314
Changes from 9 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 |
---|---|---|
@@ -0,0 +1,4 @@ | ||
from os import environ | ||
|
||
|
||
REAPER_IMAGE = environ.get("RYUK_CONTAINER_IMAGE", "testcontainers/ryuk:0.3.4") | ||
santi marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from uuid import uuid4 | ||
from typing import Optional | ||
|
||
from .images import REAPER_IMAGE | ||
|
||
|
||
SESSION_ID: str = str(uuid4()) | ||
LABEL_SESSION_ID = "org.testcontainers.session-id" | ||
|
||
|
||
def create_labels(image: str, labels: Optional[dict]) -> dict: | ||
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. |
||
|
||
if image == REAPER_IMAGE: | ||
return labels | ||
|
||
labels[LABEL_SESSION_ID] = SESSION_ID | ||
return labels |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
from os import environ | ||
from socket import socket | ||
from typing import TYPE_CHECKING, Optional | ||
|
||
|
||
from .utils import setup_logger | ||
from .images import REAPER_IMAGE | ||
from .waiting_utils import wait_for_logs | ||
from .labels import LABEL_SESSION_ID, SESSION_ID | ||
|
||
if TYPE_CHECKING: | ||
from .container import DockerContainer | ||
|
||
|
||
logger = setup_logger(__name__) | ||
|
||
|
||
class Reaper: | ||
santi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_instance: "Optional[Reaper]" = None | ||
_container: "Optional[DockerContainer]" = None | ||
_socket: Optional[socket] = None | ||
|
||
@classmethod | ||
def get_instance(cls) -> "Reaper": | ||
santi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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": | ||
from .container import DockerContainer | ||
|
||
docker_socket = environ.get( | ||
"TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE", "/var/run/docker.sock" | ||
) | ||
logger.debug(f"Creating new Reaper for session: {SESSION_ID}") | ||
|
||
Reaper._container = ( | ||
DockerContainer(REAPER_IMAGE) | ||
santi marked this conversation as resolved.
Show resolved
Hide resolved
HofmeisterAn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.with_ryuk(True) | ||
.with_name(f"testcontainers-ryuk-{SESSION_ID}") | ||
.with_exposed_ports(8080) | ||
.with_volume_mapping(docker_socket, "/var/run/docker.sock", "rw") | ||
.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,21 @@ | ||
from testcontainers.core.reaper 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").with_ryuk(True).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 1 .*", timeout=15) | ||
|
||
Reaper.delete_instance() | ||
|
||
|
||
def test_container_without_ryuk(): | ||
container = DockerContainer("hello-world").start() | ||
wait_for_logs(container, "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.
I think Ryuk should be enabled by default and consider the
TESTCONTAINERS_RYUK_DISABLED
env variable.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.
If that is okay with you, I would love that! I think however we should do a major version bump for that, as it is quite a breaking change for all package users.
I fully support it though, brings the behaviour closer to the rest of the implementations
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.
Yeah, good point. Usually, it should not affact existing environments. Although aligning with the other implementations would be great, I am fine with either of both. @tillahoffmann any preferences?
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.
@tillahoffmann Quite a bit of discussion about enabling Ryuk by default in this PR. I think we should make it on by default, but keep the current common usage pattern by using a context if we want automatic cleanup control. See this comment for a more detailed answer: #314 (comment)
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.
Now changed the implementation to enable Ryuk by default, with the possibility to turn it off for specfic use cases with
.with_auto_remove(False)
. As per the discussion in other comments, sometestcontainers
implementations doesn't allow Ryuk to be disabled at all, should I still add the env variable to override the programatic approach (or let the programatic approach override the env variable)?Personally I vote no because of the ambiguity in what should take presendence, then at the very least we need to be very clear about the order of config loading, and apply it for all other env variables as well.
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.
Just looked through the
testcontainers-node
implementation, and there the API does not allow disabling Ryuk programmatically at all, only through theTESTCONTAINERS_RYUK_DISABLED
env variable. I am slightly leaning towards removing.with_auto_remove
and instead rely solely on the env variable in order to keep implementations in sync.Thoughts?
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.
👍 This is how most TC language implementations do it.