-
Notifications
You must be signed in to change notification settings - Fork 324
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
7c09ccc
Add .with_ryuk method to enable container cleanup with ryuk
santi 957b863
Add tests for ryuk cleanup
santi fc30fc1
Merge branch 'master' into feat/ryuk-the-reaper
santi e523dba
Change param name for clarity
santi 543ce3e
Remove unused imports
santi a487dc4
Add x-tc-sid header to all Docker APi requests from testcontainers
santi 38a9d5a
Combine imports
santi a214089
Fix circular import by importing from correct script
santi fdaee73
Fix lint
santi d4ddcba
Add testcontainer language implementation to default container labels
santi 3711fcf
Merge branch 'main' into feat/ryuk-the-reaper
santi 89113a3
Rename with_ryuk API to with_auto_remove
santi 101fd11
Remove unused imports and function
santi 8802f39
Update Ryuk tests to not rely on __del__ dunder for cleanup
santi 7449d14
Update if-statement for better readability
santi 6c1650d
Use __future__ annotations to avoid quotation marks
santi 3e2c66c
Move Ryuk container image setting into config.py
santi c401df7
Move all Ryuk config-from-env logic to config.py. Rename to RYUK_ prefix
santi 581e38b
Fix lint
santi 5b1988e
Fix bug where language label was not added when provided custom labels
santi 6c24543
Bump Ryuk container version
santi 77251bb
Merge branch 'main' into feat/ryuk-the-reaper
santi 4c37960
Add env variables to Docs. Add env variable for disabling Ryuk
santi dc66658
Replace programmatic .with_auto_remove() API with env variable for di…
santi dd18c01
Downgrade PyYAML to 5.3.1 to fix Cython build problem
santi 1e2f7a1
Add dependency restriction to testcontainers-compose to mitigate Cyth…
santi a4827f2
Allow any single digit of containers to be killed. Some may be dangli…
santi 8396907
Merge branch 'main' into feat/ryuk-the-reaper
santi 07d4849
Remove deprecated setup file
santi c239d52
Use absolute imports
santi 22e03eb
Fix flake8 lint error
santi 6dce010
Merge remote-tracking branch 'origin/main' into feat/ryuk-the-reaper
santi fed8d49
Lint
santi a510983
Add comment on why quotes are used for type annotation
santi 51ccbc4
Shorter comment do make linter happy
santi a0d8487
Disable ruff rule for prefering X | Y over Optional[X] to support Pyt…
santi e9974b3
Use absolute imports
santi 904ae1c
Add env variable docs to README
santi 74c82af
Move Reaper class into container.py to avoid circular dependency
santi ed47f57
Add missing newline to README
santi 5772dca
Remove noqa lint rule exceptions
santi da6f3d8
fix: linting with ruff, keep runtime typging
totallyzen 81cb09e
Replace future import with quoted type hints
santi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
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 = { | ||
"org.testcontainers.lang": "python", | ||
} | ||
|
||
if image == REAPER_IMAGE: | ||
return labels | ||
|
||
labels[LABEL_SESSION_ID] = SESSION_ID | ||
return labels |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Uh oh!
There was an error while loading. Please reload this page.
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.