Reuse containers

[tisonkun]

Like https://java.testcontainers.org/features/reuse/

I currently use bollard directly to reuse containers between tests, while noticed that testcontainers encapsulation is valuable and ergonomic, like all the code to support get_host_port_ipv4.

So if testcontainers can support reuse containers, users like me can benefit from both features.

[tisonkun]

My current code in gist - https://gist.github.com/tisonkun/c2ac3154bd6bea003c6097b195a9e32d

[DDtKey]

Hi 👋
Thank you for raising the issue!

Yes, it makes sense to support and shouldn't be hard

[gurinderu]

How do you want to make a reuse ID? Should it be calculated automatically from the source code & call site meta or provided by hand?

[gurinderu]

Also, it is not clear to understand when to stop the container, lets say we run nextest with the same reuse id in a serial way. Nextest use process-per-test strategy and we don't have ryuk

[DDtKey]

Reusable containers are not expected to be cleaned.

Documentation from dotnet impl:

Enabling reuse will disable the resource reaper, meaning the resource will not be cleaned up

and also from Java:

Reusable containers are not suited for CI usage and as an experimental feature not all Testcontainers features are fully working (e.g., resource cleanup or networking).

You can check java and dotnet docs, because it describes how it works. And we can rely on their impls of identifiers to a certain extent.

Btw, ryuk will be supported soon I think, just need to find spare time 🙂

[tisonkun]

Reusable containers are not suited for CI usage

Just a comment to this statement: We reuse containers in the CI environment, and the runner will destroy everything once the job is finished. So it works well for our CI usage :D

[DDtKey]

Yes, actually it should work if your runners are properly configured and don't store the state between different jobs. E.g DnD approach will work

[the-wondersmith]

@DDtKey I've actually been working on implementing support for this. Would you accept a PR for the feature?

[DDtKey]

@the-wondersmith I will be happy to review the PR.

Feel free to open 🙂

[the-wondersmith]

@the-wondersmith I will be happy to review the PR.

Feel free to open 🙂

Will do, as soon as I'm able 😁

[the-wondersmith]

@DDtKey Looked over what I had and realized it would probably be better to break it up into two PRs. The implementation I've been working on uses labels on the testcontainers-managed containers to figure out if a container should be reaped at drop or at exit, so support for applying the labels to begin with is a prerequisite but also a pretty useful feature all on its own.

tl;dr I just opened a precursor PR that adds support for labels 😅, once we're able to get that merged I'll follow up with the full reusable feature

[DDtKey]

Actually I'd like to see how exactly the labels are going to be used, before merging this part. For now it's more like black box without justification (don't get me wrong)

I mean this PR has references(in documentation) to some internal usage which does not exist yet and it's harder to review without full context

Also I can provide some guidance or highlight something while having the whole context

We still can merge them as 2 separate PRs if agreed, but you can open second one as soon as you are ready.

Thank you!

[the-wondersmith]

... this PR has references(in documentation) to some internal usage which does not exist yet

Ah! Apologies for the confusion. The note in the docs is an attempt to make it as clear as possible that the com.testcontainers namespace should be treated as reserved for internal use by the testcontainers crate, and therefore users shouldn't try to use labels like com.testcontainers.my-label. Nothing is actually using anything in the com.testcontainers namespace, the idea is just to establish the "standard" or pattern, so to speak, that nothing in the testcontainers crate itself should / will use any label outside of the com.testcontainers namespace with the exception of the managed-by label.

If you look at the code, you'll see that the managed-by: testcontainers label is unconditionally applied to all containers started by testcontainers, and it's explicitly done in a way that it cannot be overridden by user code. The idea is that future features or even expansions / refactors of existing ones will be able to use labels to determine appropriate behavior by doing simple checks like:

// ...

if labels.get("managed-by").is_some_and(|value| value == "testcontainers")
   && labels.get("com.testcontainers.some-feature").is_some_and(|value| value == "true") {
  // feature code
}

// ...

Its uh... similar to Kubernetes' well-known labels concept 😅.

... I'd like to see how exactly the labels are going to be used, before merging this part.

That's totally fair.

We still can merge them as 2 separate PRs if agreed, ...

Of course, naturally happy to do it in whatever way works best 😁. Thank you for taking the time to look it over! I'll have the next PR up as soon as time allows

[DDtKey]

Yeah, thank you for the detailed answer!
I understand possible benefits of labels, just wanted to see the whole picture.
The labels feature itself is most likely will be merged, but it's much better to see how they are coupled with each other first.

This way we can decide what the convention should be used and if it's aligned with other implementations of testcontainers (in other languages, or at least doesn't contradict with them). Labels also can be helpful for introduction of ryuk (#577)

[the-wondersmith]

@DDtKey Apologies for the delay (life and all 😅). The follow up PR for a working implementation of reusable containers is up.

So far as I'm able to tell, I've maintained behavioral parity with the implementation(s) in other languages. Originally, I'd included global reaping behavior as well that used ctor to clean up containers at the end of a test session. I stripped that out though, as I kinda realized it muddied things up too much for a solid single-feature PR review.

Anyway, eager for your thoughts, whenever you have time 😁

[DDtKey]

@the-wondersmith thank you! I'll review as soon as I can.

There is no a goal to clean reusable containers, at least for now. Because it's pretty complicated, some users may want them to be alive for a long time and reuse once per hour for example.

Also, what I thinking about is to allow controlling this behavior via ENV variable, it's common case that I don't want to change code, but run them in reusable mode only on my machine.

[the-wondersmith]

@the-wondersmith thank you! I'll review as soon as I can.

There is no a goal to clean reusable containers, at least for now. Because it's pretty complicated, some users may want them to be alive for a long time and reuse once per hour for example.

Also, what I thinking about is to allow controlling this behavior via ENV variable, it's common case that I don't want to change code, but run them in reusable mode only on my machine.

Yeah, that's kinda what I figured as well. I hadn't considered that that part though. Adding env var based control shouldn't be hard at all - just pull the value from env instead of hard-coding false where it's set by default 🤔.

I'll add it shortly, if you like 😁

[the-wondersmith]

@DDtKey looked at adding the env control for reusability behavior, and it looks like it might be more complicated than I originally thought? The relatively simple way I outlined above absolutely will work, but it's kinda "quick and dirty" by comparison with the existing paradigm. I did get it working properly such that it uses Config as the source of the default reuse flag value, but it ended up involving a kinda ugly async -> sync -> async section that I really don't love at all.

IMO, the best way forward would be to get the labels feature reviewed and merged, then the current implementation of reusable reviewed and merged, but the review process for reusable should also include identification of followup work that needs doing (for which I'll volunteer by default).

Thoughts?

[DDtKey]

Yes, that totally make sense to implement ENV variable control separately in follow up PR

[the-wondersmith]

@DDtKey Don't want to accidentally come off as pushy, but I do want to push forward on the reusable containers feature if possible. I know Thanksgiving in the U.S. completely wrecked my working week last week personally, and I assume I'm not the only one (especially with the other end-of-year holidays fast approaching), so no progress here as-of-yet is completely understandable 😅.

tl;dr without overstepping or accidentally becoming abrasive, is there anything that needs doing / that I can do to help push forward on this? FTR, "just wait" is totally acceptable, I just figured I'd check is all 🙂

[DDtKey]

I'll review the PRs this week, sorry for the delay 🙏

[the-wondersmith]

I'll review the PRs this week, sorry for the delay 🙏

Please don't apologize! We've all got lives, and PR review is hardly at the top of anyone's list 😅. Thank you for doing it at all 😁

[the-wondersmith]

@tisonkun reusable containers was officially merged this morning 😁. If you can, please give it a whirl and report back with any thoughts?

For the sake of transparency, there is still planned follow-up work on the feature (e.g. env var control). Still though, feedback would be great 🙂

[tisonkun]

@the-wondersmith Cool! Thanks everyone helps in this thread.

I'll test this feature in this week.

[the-wondersmith]

@DDtKey where did we end up on the remaining / follow-up work? i.e. what's left to do, what can I (potentially) start on?

[DDtKey]

Thanks one more time @the-wondersmith for implementing this feature! 🙏

I think follow-ups were mentioned here:

#757 (review)

• env-variable control
• proper reaper for "current session" (ryuk)

[the-wondersmith]

Thanks one more time @the-wondersmith for implementing this feature! 🙏

It was absolutely my pleasure 😁

I think follow-ups were mentioned here:

#757 (review)

• env-variable control
• proper reaper for "current session" (ryuk)

Where's the best place to discuss these? Should we continue here, or maybe open a separate tracking issue?

[DDtKey]

I think it's better to create (follow-up/sub)-issues and discuss them separately

Because basic implementation has been provided already

[the-wondersmith]

I think it's better to create (follow-up/sub)-issues and discuss them separately

Because basic implementation has been provided already

Agreed. Spun up an issue here. On re-read though, maybe I should have made 2? Look at the new one and let me know - I'll spin the ryuk discussion to its own issue if that'd be easier / better.

[tisonkun]

I tried at tisonkun/morax#19

@the-wondersmith it seems not work. See the PR for reproduce and fail logs.

[tisonkun]

Also, there is a lack of documents about ReuseDirective. I don't know the different between CurrentSession and Always clearly.