Reusable containers #1781
Reusable containers #1781
Conversation
# Conflicts: # modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
| */ | ||
| @Setter(AccessLevel.NONE) | ||
| protected DockerClient dockerClient = DockerClientFactory.instance().client(); | ||
| protected DockerClient dockerClient = LazyDockerClient.INSTANCE; |
|
|
||
| logger().info("Starting container with ID: {}", containerId); | ||
| dockerClient.startContainerCmd(containerId).exec(); | ||
| if (containerInfo == null) { |
There was a problem hiding this comment.
Important information for the reviewers:
these containerInfo == null branches are important to review carefully
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java
Outdated
Show resolved
Hide resolved
modules/jdbc/src/main/java/org/testcontainers/jdbc/ContainerDatabaseDriver.java
Show resolved
Hide resolved
…th_properties' into reusable_containers
core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java
Outdated
Show resolved
Hide resolved
| @@ -1246,6 +1333,11 @@ public SELF withTmpFs(Map<String, String> mapping) { | |||
| return self(); | |||
| } | |||
|
|
|||
| public SELF withReuse(boolean reusable) { | |||
There was a problem hiding this comment.
Re this bit of the API, I'm afraid I still really would like this to not be a boolean, so that we have room for expansion. e.g. to have an enum or interface to allow (pseudo-code-ish):
.withReuse( Reuse.reuseContainers() )
but also allow, for the sake of #1713 (and the hilariously old draft implementation):
.withReuse ( Reuse.reuseImagesAfterInitialization() ) // definitely needs a better name
I think we could get the second style of reuse done very quickly for JDBC-based containers.
With the reduced scope of container reuse in this PR, do you think this buys us enough cognitive space to fit this flexibility in?
There was a problem hiding this comment.
While I understand why we would want to add it, here my 2c why I think we should not do it as part of the first step:
- JDBC URL can only have boolean (or lambda reference, but that's tricky)
withReuse(Boolean)is easy to extend later with an overloadedwithReuse(ReuseStrategy)- The image caching is another great optimization, but I don't feel that it is the same as the container reusing - we will need to start a new container every time (which is also CI friendly, unlike this PR), which kinda makes it a sugar for the image + capturing an image after the start of container.
- The reuse is currently has to be enabled with the property. Having different strategies may make it harder because I assume it will be per-strategy enablement
The implementation details of the reuse also make it not easy to have strategies for it, and we may never have more than one (given №3)
There was a problem hiding this comment.
OK, I'm convinced 😄
I'll add a 5th reason: while I'd love to do it, we may never get around to image reuse. To be pragmatic, I don't want to hold up this almost feature for a someday-maybe feature.
| try { | ||
| Method method = type.getDeclaredMethod("containerIsCreated", String.class); | ||
| if (method.getDeclaringClass() != GenericContainer.class) { | ||
| logger().warn("{} can't be reused because it overrides {}", getClass(), method.getName()); |
# Conflicts: # core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java # core/src/main/java/org/testcontainers/utility/TestcontainersConfiguration.java
| createCommand.getLabels().put(HASH_LABEL, hash); | ||
| } | ||
| } else { | ||
| logger().info("Reuse was requested but the environment does not support the reuse of containers"); |
There was a problem hiding this comment.
IMO we should make this a big/obvious INFO message that includes instructions on how to enable reusable containers. This way users that don't closely follow the new TC features will still hear about it and know how to enable it.
I'm thinking something like:
################################################################
Reuse was requested but the environment does not support the reuse of containers.
To enable container reuse, add the property 'testcontainers.reuse.enable=true'
to a file at ~/.testcontainers.properties (you may need to create it).
################################################################
There was a problem hiding this comment.
This warning will only appear if they use withReuse(true), which means that they are aware of the feature already :)
Also, we expect this feature to be pretty well discoverable by anyone who cares about the speed of the tests (will also make sure that the docs explain it well), and I don't feel that we need the banner
There was a problem hiding this comment.
I think for projects with multiple developers it may be less obvious. One developer on a team may know about the feature and add withReuse(true) but other developers may not
There was a problem hiding this comment.
I just tried this feature for the first time, and I have to say it's completely non-obvious. I see the INFO log, but there is no hint about what I need to do to fix it.
There was a problem hiding this comment.
BTW the file name in the banner hint above appears to be wrong as well. I think it needs to be ~/.testcontainers.properties. Windows users are probably going to be confused by that as well. Is it the only way to set that property?
There was a problem hiding this comment.
Was just trying to debug this feature... it is very difficult to discover.
Additionally, it's setup such that it forces you to use ~/.testcontainers.properties to configure it... why not allow it to also be set from the classpath testcontainers.properties?
By forcing it to be under $HOME, you're forcing end-users to setup all build servers, every developer workstation, etc to configure this file. I don't see why you can't let the end-user opt-in to this feature with the classpath config file, this makes the most sense for enterprise development where consistency is king
There was a problem hiding this comment.
The intent of this feature (reusing containers across multiple test runs) was to only have it apply for developer workstations. @bsideup specifically did not want it to be used for CI/build servers because container instances could pile up and bog down remote servers and easily go un-noticed.
That being said, I do think a more prominent INFO/WARN message for this situation would help users figure out how to turn this on in their local dev workstations.
There was a problem hiding this comment.
Thanks @aguibert for providing a good answer :)
So yeah, what Andy said :)
I do think a more prominent INFO/WARN message for this situation would help users figure out how to turn this on in their local dev workstations.
Yes, next iteration we will definitely improve the UX around it 👍
There was a problem hiding this comment.
What about if a user does check this in to the file on their classpath, to ignore it (as you are already doing), but also print out the explanation that @aguibert mentioned and that the setting needs to be set elsewhere.
(Perhaps that is what you meant by improve the UX)
There was a problem hiding this comment.
yes, we will definitely consider reporting environment-specific configuration set in the classpath file! Good suggestion 👍
- remove use of @container annotation since it automatically stops containers after test - must be manually configured locally to take effect: testcontainers/testcontainers-java#1781
#554) - remove use of @container annotation since it automatically stops containers after test - must be manually configured locally to take effect: testcontainers/testcontainers-java#1781
|
Hi everyone, I like this reuse feature ! But I am really interested in the step 2 of this page notably about the locking feature. In fact I really need to be able to launch my tests in parallel with maven. If I do so, it could happen that I will end up with 2 containers instead of 1 that will be reused. Implementing the step 2 with a locking mechanism will definitely fix this issue. Do we know if we plan to do it ? Thanks Loïc |
|
Hey @loic-seguin, we currently have no fixed plan on how and when to proceed, but we agree that reusable mode is a very interesting and valuable feature 👍 |
…s (#6843)
Added the new `withReuse(true)` flag from this new testcontainers
feature: testcontainers/testcontainers-java#1781
This allows not only reusing a container between different tests in the
same run, but also keeping the container alive for subsequent runs to
decrease container startup time.
This PR implements a first step for the reusability of containers (see #781).
Current state
What is implemented:
containerIsCreated(we cannot assume reusability ifcontainerIsCreatedis overridden)testcontainers.reuse.enableonly from~/.testcontainers.propertiesTODO list:
What is not implemented but planned as step 2:
Challenges ahead
Prerequisites
Add
testcontainers.reuse.enable=trueto~/.testcontainers.propertiesfile on your local machine.Example usage with container objects
Here we unset the network that was implicitly created by
KafkaContainer(but not used in this case), because otherwise the network id will always be random and affect the hash.Kafka is an exceptional case (to be fixed, left for backward compatibility) and most of other containers do not set the network implicitly.
Output:
Example usage with JDBC URLs
Output: