-
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 all 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 |
---|---|---|
@@ -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.