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

Reusable containers #1781

Merged
merged 17 commits into from Oct 26, 2019
Merged

Reusable containers #1781

merged 17 commits into from Oct 26, 2019

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Aug 24, 2019

This PR implements a first step for the reusability of containers (see #781).

Current state

What is implemented:

  • Hashing
  • JDBC URL support
  • Check for overrides like containerIsCreated (we cannot assume reusability if containerIsCreated is overridden)
  • Per-environment enablement
  • Read testcontainers.reuse.enable only from ~/.testcontainers.properties

TODO list:

  • Hashing of copied files
  • Add TC's version to the hash or add "hashing version" property

What is not implemented but planned as step 2:

  • ℹ️ Locking - two parallel tests with the same hash will reuse the same container
  • ℹ️ TTL - containers won't get destroyed after some time (Docker Compose -like behaviour)
  • ⚠️ Cleanup - if the configuration changes, a new container will be started, but the old one will not be destroyed and has to be terminated manually by the user
  • "Started successfully" marker to avoid a race condition where a container was created & hashed but the startup checks were not executed

Challenges ahead

  • Networks
  • Multi-container setups
  • There must be something else 😅

Prerequisites

Add testcontainers.reuse.enable=true to ~/.testcontainers.properties file on your local machine.

Example usage with container objects

KafkaContainer kafka = new KafkaContainer()
    .withNetwork(null)
    .withReuse(true);
kafka.start();

testKafkaFunctionality(kafka.getBootstrapServers());

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:

19:27:07.019 INFO  🐳 [confluentinc/cp-kafka:5.2.1] - Creating container for image: confluentinc/cp-kafka:5.2.1
19:27:07.109 INFO  🐳 [confluentinc/cp-kafka:5.2.1] - Reusing container with ID: 86fb93cd10118c147f419d485781b999c2340b10d3b956ea5bb1bf5e7eaaae2a and hash: e10c95bab8779258336609315f4efa72d191aa9f
19:27:07.115 INFO  🐳 [confluentinc/cp-kafka:5.2.1] - Container confluentinc/cp-kafka:5.2.1 is starting: 86fb93cd10118c147f419d485781b999c2340b10d3b956ea5bb1bf5e7eaaae2a
19:27:07.269 INFO  🐳 [confluentinc/cp-kafka:5.2.1] - Container confluentinc/cp-kafka:5.2.1 started in PT0.295S

Example usage with JDBC URLs

Instant startedAt = Instant.now();
Connection connection = DriverManager.getConnection(
    "jdbc:tc:db2:///?TC_REUSABLE=true"
);

connection.createStatement().execute("SELECT 1  FROM SYSIBM.SYSDUMMY1");

System.out.println("Total test time: " + Duration.between(startedAt, Instant.now()));

Output:

19:10:47.273 INFO  🐳 [ibmcom/db2:11.5.0.0a] - Creating container for image: ibmcom/db2:11.5.0.0a
19:10:47.375 INFO  🐳 [ibmcom/db2:11.5.0.0a] - Reusing container with ID: 7b2da71c8c0c3aa901f5a610bd181e3a398ec4f3c8d22dbd491e23f5e1af47e9 and hash: 848385e00a45f9e1eecc42e90e0bcb7e1f2391bd
19:10:47.381 INFO  🐳 [ibmcom/db2:11.5.0.0a] - Container ibmcom/db2:11.5.0.0a is starting: 7b2da71c8c0c3aa901f5a610bd181e3a398ec4f3c8d22dbd491e23f5e1af47e9
19:10:47.382 INFO  🐳 [ibmcom/db2:11.5.0.0a] - Container ibmcom/db2:11.5.0.0a started in PT0.148S

Total test time: PT2.782S

@bsideup bsideup requested a review from Aug 24, 2019
*/
@Setter(AccessLevel.NONE)
protected DockerClient dockerClient = DockerClientFactory.instance().client();
protected DockerClient dockerClient = LazyDockerClient.INSTANCE;
Copy link
Member Author

@bsideup bsideup Aug 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed this one for unit tests, plus good improvement in general. Was also reported as #1749

Copy link
Contributor

@aguibert aguibert Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks for doing this change @bsideup!


logger().info("Starting container with ID: {}", containerId);
dockerClient.startContainerCmd(containerId).exec();
if (containerInfo == null) {
Copy link
Member Author

@bsideup bsideup Aug 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important information for the reviewers:
these containerInfo == null branches are important to review carefully

@@ -1246,6 +1333,11 @@ public SELF withTmpFs(Map<String, String> mapping) {
return self();
}

public SELF withReuse(boolean reusable) {
Copy link
Member

@rnorth rnorth Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@bsideup bsideup Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. JDBC URL can only have boolean (or lambda reference, but that's tricky)
  2. withReuse(Boolean) is easy to extend later with an overloaded withReuse(ReuseStrategy)
  3. 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.
  4. 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)

Copy link
Member

@rnorth rnorth Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
Copy link
Member

@rnorth rnorth Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good idea.

bsideup added 3 commits Aug 26, 2019
# 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");
Copy link
Contributor

@aguibert aguibert Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
################################################################

Copy link
Member Author

@bsideup bsideup Aug 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@aguibert aguibert Aug 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@dsyer dsyer Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@dsyer dsyer Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@aguibert aguibert Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@bsideup bsideup Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@bsideup bsideup Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we will definitely consider reporting environment-specific configuration set in the classpath file! Good suggestion 👍

bobeal added a commit to stellio-hub/stellio-context-broker that referenced this issue Jan 3, 2022
- remove use of @container annotation since it automatically stops containers after test
- must be manually configured locally to take effect: testcontainers/testcontainers-java#1781
bobeal added a commit to stellio-hub/stellio-context-broker that referenced this issue Jan 3, 2022
#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet