enhancement: add reaper option - #949
Conversation
✅ Deploy Preview for testcontainers-rust ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
9f4f50b to
00cb21d
Compare
a1a05e8 to
e0769a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0769a251e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn reaper(&self) -> bool; | ||
|
|
||
| fn reaper_ttl_seconds(&self) -> u32; |
There was a problem hiding this comment.
Add defaults for the new Image methods
Adding these as required methods breaks every existing downstream Image implementation, and it also leaves the repo's own ExecBeforeReady test image under the http_wait_plain feature without implementations, so all-features builds fail. Since the behavior already has obvious defaults (false and the default Ryuk timeout), make these default trait methods instead of requiring every image to add boilerplate.
Useful? React with 👍 / 👎.
|
|
||
| let host_port = reaper_container.get_host_port_ipv4(8080).await?; | ||
|
|
||
| let mut stream = std::net::TcpStream::connect(format!("127.0.0.1:{}", host_port))?; |
There was a problem hiding this comment.
Use the resolved Docker host for Ryuk
When the Docker daemon is remote, configured via tc.host/DOCKER_HOST=tcp://..., or the tests run inside a container, mapped ports are not necessarily reachable on the test process's 127.0.0.1; the rest of the crate handles this through get_host()/docker_hostname(). With this hard-coded address, with_reaper(true) fails to connect to a successfully-started Ryuk container in those supported environments, so use the reaper container's resolved host with the mapped port instead.
Useful? React with 👍 / 👎.
| .with_mount(Mount::bind_mount( | ||
| "/var/run/docker.sock", | ||
| "/var/run/docker.sock", |
There was a problem hiding this comment.
Mount the configured Docker socket into Ryuk
This hard-codes /var/run/docker.sock, but the client explicitly supports rootless and other Unix socket locations via Docker host resolution. In those supported setups, the Ryuk container either gets a missing/wrong socket or fails to start, so any reaper-enabled container cannot be registered; derive the bind source from the configured Unix Docker host instead of assuming the default path.
Useful? React with 👍 / 👎.
| .with_env_var("RYUK_CONNECTION_TIMEOUT", seconds_formatted.clone()) | ||
| .with_env_var("RYUK_RECONNECTION_TIMEOUT", seconds_formatted.clone()); | ||
|
|
||
| let mut reaper_container = reaper_image.start().await.unwrap(); |
There was a problem hiding this comment.
Propagate Ryuk startup failures instead of panicking
When users opt into the reaper and Ryuk cannot be pulled or started (for example because the socket bind is rejected or the image is unavailable), this unwrap() panics inside start() even though the public API returns Result. That prevents callers from handling the startup failure consistently with other container setup errors; propagate the error with ? instead.
Useful? React with 👍 / 👎.
e2acb33 to
716b69a
Compare
* add testcontainers/ryuk as a reaper option * have this OFF by default, can be set with <image>.with_reaper(true) * add a timeout for the reaper container that represents when a reaper container should stop and stop others * add no_drop option for not stopping a container when Drop is called, this is used to ensure the reaper container isn't stopped when it leaves the start() scope fixes testcontainers#577
716b69a to
793b348
Compare
|
Thanks for the contribution! I wonder if it would be a good idea to have this as a feature (non-default). |
Happy to help! The way I wrote it, it is off by default. If you run a container the reaper will not run unless you explicitly tell it to. It's not a "feature" turned off/on at compile time; it's a setting in the image instance at runtime. What do you think about that? edit: tagging @mervyn-mccreight |
|
Hey :) To me this is semantically roughly the same feature as the existing "watchdog" feature, but with another implementation. It tries to shut down containers that somehow life after everything is finished. I'm asking myself: Is there a use-case I can image that I want ryuk just for certain containers and not having it being the Fail-Safe for everything when I actively want to activate it? Or is it more like I either want it to be active in general or I want it to be not active in general? At the moment I view it more like someone I want to either be active in general or not active in general, but I may be missing thoughts. What do you think @DDtKey ? |
fixes #577