Skip to content
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

Allow container reuse opt-in via API #5364

Closed
Sanne opened this issue May 13, 2022 · 5 comments
Closed

Allow container reuse opt-in via API #5364

Sanne opened this issue May 13, 2022 · 5 comments

Comments

@Sanne
Copy link

Sanne commented May 13, 2022

Hello all,

I've enabled support for container reuse in Quarkus dev-services in Quarkus:

It's working great, many thanks for the nice improvement; althought so far I've only enabled it for selected containers, specifically the ones running relational databases people use for testing.

I'm aware - and have documented - that people need to enable this opt-in by setting the relevant property in the .testcontainers.properties configuration file.

This seems like a wise choice since the feature is currently experimental; yet people in our community have been asking to have this working by default on selected, cherry-picked containers, so it would be nice for us to have a way to opt-in for a particular container without people needing to edit the core configuration file.

I understand it's possibly not a good time to do so, but eventually as the feature is considered more mature I'd love to see a way for the API to give a stronger hint to the Testcontainer core that a particular container really should be reused.

Proposal

One approach could be for the withReuse method to accept a three-state enum rather than a boolean, like { OFF|ON|According_to_config }.

Additional consequences

The logic to stop containers in Quarkus also needs to know if the container is allowed to be reused. So I suspect some other methods will need to be adapted to support this as wel, such as isShouldBeReused and possibly others.

Many thanks for the consideration! Reported on suggestion from cc/ @kiview at Devoxx :)

@bsideup
Copy link
Member

bsideup commented May 13, 2022

Hi @Sanne,

Thanks for the extensive feedback!

The reason for it being opt-in is because, if users "hardcode" it, they will end up having it on CIs too, and it leads to dangling/uncontrolled containers. And the withReuse method serves as a flag that "This container can be reused**" (because some logically can't and this is up to the user to decide).

So it isn't about the feature being experimental, but rather CI safety. I understand that devservices are unlikely to be used on CIs, but it is also not a "regular" usage (although an amazing one!)

Would appreciate your ideas of how to address this safely 👍

@Sanne
Copy link
Author

Sanne commented May 24, 2022

Hi @bsideup ! sorry for the delay :)

Ok interesting; in my case I was actually hoping for containers to be "leaked" (intentionally) on our CI run.

If you look at the Quarkus build jobs, you'll see we have > 1,000 Maven modules - and that's just counting the primary repository. A dozen of which will need to run a PostgreSQL container, another dozen will need a DB2 container, Elasticsearch containers, etc.. in a variety of combinations and profiles.

While the PostgreSQL is awesome at quick start, others such as DB2 take north of 10 minutes to boot; validating a PR takes us about 4 hours. Today we're starting containers "imperatively" via specific github action profiles to save time; this is error prone and often gets out of sync as rules need to match the build expectations; more importantly, it implies we're not testing with Testcontainers.

I might have misunderstood some things about Ryuk - what is its purpose? I thought this was a registry to ~control reuse, and terminate lingering containers after some timeout? I actually meant to ask about that.

In short I'd be totally fine to "leak" containers on our CI to allow better reuse, even across builds. Ideally perhaps with Ryuk terminating them in background if there's been no "start request" issued in hours - but that also doesn't feel necessary as most of our CI nodes are spawn brand new for each job.

A pending, separate problem would be to do a little resource management; I doubt our test nodes can keep all those containers "alive" at the same time.

@kiview
Copy link
Member

kiview commented May 24, 2022

Hey @Sanne, thanks for your detailed explanation of your use case 🙂

In the context of TC, Ryuk plays the role of our resource cleanup process. As a default, the lifecycle of resources created by TC is bound to the lifecycle of the JVM process. While it is possible that resources are consciously removed as part of individual test executions, Ryuk acts as a safeguard to ensure no lingering zombies containers (or other resources) are left hanging around after the tests. This is specifically useful on persistent CI runners, but also for local development.

The current implementation of reusable mode will disable this mechanism and is intended for TDD-like workflows on local developer machines. It is not intended for CI usage and you already outlined some of the issues that would appear in such cases:

A pending, separate problem would be to do a little resource management; I doubt our test nodes can keep all those containers "alive" at the same time.

So I fear for the time being, tackling your CI problem challenges with the reusable mode in Testcontainers is not recommended. Of course, there is always potential to improve the mechanisms and make it more flexible and clever, but we currently don't intend to focus on it.

@Sanne
Copy link
Author

Sanne commented May 24, 2022

Fair enough, thanks for all explanations!

I'll close this now, hopefully a better proposal will emerge in time :)

@ailjushkin
Copy link

Hey guys, If I understood correctly, this feature is not unstable already and can be marked as official behaviour.

So I'd remove unstable API in code and added extended javadoc for this, and a paragraph in the docs, that in this way we are trying to make the settings of the re-use of containers explicit to users from the outside.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants