Death Note :) #545

bsideup · 2018-01-15T20:46:16Z

Start a helper "Ryuk" container to make sure that we clean the containers after the execution even if the JVM was "kill -9"ed.

Based on https://github.com/bsideup/moby-ryuk and uses my public automated build for now:
https://hub.docker.com/r/bsideup/moby-ryuk/builds/bhcmbnqyhtnvcqg8ijvnh6n/

Will move to TC org & hub once we get a name

bsideup · 2018-01-15T20:48:17Z

core/src/main/java/org/testcontainers/DockerClientFactory.java

@@ -178,22 +174,6 @@ private void checkMountableFile(DockerClient dockerClient, String id) {
        }
    }

-    private void checkExposedPort(String hostIpAddress, DockerClient dockerClient, String id) {


this check is not needed anymore, Ryuk demands a connectivity hence implicitly checks for the exposed port

bsideup · 2018-01-15T20:49:24Z

core/src/main/java/org/testcontainers/DockerClientFactory.java

@@ -95,7 +102,7 @@ public DockerClient client() {
        log.info("Docker host IP address is {}", hostIpAddress);
        DockerClient client = strategy.getClient();

-        if (!preconditionsChecked) {


had to change the name because it's not just preconditionsChecked anymore

bsideup · 2018-01-15T20:51:32Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -93,6 +93,7 @@ public DockerComposeContainer(String identifier, List<File> composeFiles) {

        // Use a unique identifier so that containers created for this compose environment can be identified
        this.identifier = identifier;
+        project = identifier + Base58.randomString(6).toLowerCase();


without this change, compose.start(); compose.stop(); compose.start() would use the same randomIndentifier and might lead to some unpredictable side effects

Would it be possible to move this code into the starting() method and remove the duplicate line (245) in finished()? Else we should wrap this line in a private method generateProjectName(String identifier)

bsideup · 2018-01-15T20:52:50Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -165,38 +166,15 @@ private void applyScaling() {
    }

    private void registerContainersForShutdown() {
-        // Ensure that all service containers that were launched by compose will be killed at shutdown


Awesome 🗡️

✂️ 😄

bsideup · 2018-01-15T20:53:55Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+
+        String ryukContainerId = client.createContainerCmd(ryukImage)
+                .withHostConfig(new HostConfig() {
+                    @JsonProperty("AutoRemove")


docker-java is behind, but this flag will remove the container if it stops (and it will stop when the cleanup happen)

Aha, I was wondering what would remove this container 😄

kiview · 2018-01-16T08:16:33Z

core/src/main/java/org/testcontainers/DockerClientFactory.java

    private static final String TINY_IMAGE = TestcontainersConfiguration.getInstance().getTinyImage();
    private static DockerClientFactory instance;

    // Cached client configuration
    private DockerClientProviderStrategy strategy;
-    private boolean preconditionsChecked = false;
+    private boolean initialized = false;


This boolean is of course false by default, or did you want to make it explicitly?

I just renamed the variable :) But yes, explicit false helps to understand the logic

kiview · 2018-01-16T08:21:17Z

core/src/main/java/org/testcontainers/DockerClientFactory.java

@@ -95,7 +102,7 @@ public DockerClient client() {
        log.info("Docker host IP address is {}", hostIpAddress);
        DockerClient client = strategy.getClient();

-        if (!preconditionsChecked) {
+        if (!initialized) {


I think we can encapsulate the following block like:

if (!initialized) { initialize(); }

Let's not do it in this PR.

No problem, but why not? 😉

because it's out of scope, and if somebody submits another PR where they change DockerClientFactory, but we didn't merge "Death Note" PR yet, it will create more conflicts.
I'm happy to extract method as a follow up, or before, or in parallel, but don't want to increase the scope of this change :)

Also think about reverts in case we discover some major bug :)

Yes okay, makes sense 🙂

kiview · 2018-01-16T08:30:40Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -165,38 +166,15 @@ private void applyScaling() {
    }

    private void registerContainersForShutdown() {
-        // Ensure that all service containers that were launched by compose will be killed at shutdown


Awesome 🗡️

kiview · 2018-01-16T08:33:48Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -93,6 +93,7 @@ public DockerComposeContainer(String identifier, List<File> composeFiles) {

        // Use a unique identifier so that containers created for this compose environment can be identified
        this.identifier = identifier;
+        project = identifier + Base58.randomString(6).toLowerCase();


Would it be possible to move this code into the starting() method and remove the duplicate line (245) in finished()? Else we should wrap this line in a private method generateProjectName(String identifier)

kiview · 2018-01-16T08:42:11Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -464,7 +462,12 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
            createCommand.withPrivileged(privilegedMode);
        }

-        createCommand.withLabels(Collections.singletonMap("org.testcontainers", "true"));
+        createContainerCmdModifiers.forEach(hook -> hook.accept(createCommand));


Why do you have to do this now?

just moved the call from another place because we must add our labels after user sets his own

kiview · 2018-01-16T08:45:04Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

 public final class ResourceReaper {

    private static final Logger LOGGER = LoggerFactory.getLogger(ResourceReaper.class);
+
+    private static final List<List<Map.Entry<String, String>>> REGISTERED_FILTERS = new ArrayList<>();


Nitpicking, but I would not write this variable in capital letters, since the collection is mutable.

kiview · 2018-01-16T08:51:37Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+                .withName("tc-ryuk-" + DockerClientFactory.SESSION_ID)
+                .withLabels(Collections.singletonMap(DockerClientFactory.TESTCONTAINERS_LABEL, "true"))
+                .withBinds(
+                        new Bind("/var/run/docker.sock", new Volume("/var/run/docker.sock")),


I think for Docker for Windows, we need to add a leading slash here 😞 (that's what we also did in ContainerisedDockerCompose). Also see this SO answer (without real explanation) 😆 https://stackoverflow.com/questions/36765138/bind-to-docker-socket-on-windows/41005007#41005007

weird, but nice catch :) Will do

kiview · 2018-01-16T09:07:29Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+            REGISTERED_FILTERS.notifyAll();
+        }
+    }
+
    /**
     * Register a container to be cleaned up, either on explicit call to stopAndRemoveContainer, or at JVM shutdown.


Shall we remove this method now?

No, it's public. We can not remove it now

rnorth

Nice! Just a few queries/requests then I think it's good to merge!

rnorth · 2018-01-21T21:57:25Z

core/src/main/java/org/testcontainers/DockerClientFactory.java

-
-                MountableFile mountableFile = MountableFile.forClasspathResource(this.getClass().getName().replace(".", "/") + ".class");
+            String ryukContainerId = ResourceReaper.start(hostIpAddress, client);
+            log.info("Ryuk started");


Super-trivial, but please could we change the message a bit for people who don't/won't grok the name? Something like "Ryuk started - will monitor and terminate Testcontainers containers on JVM exit" perhaps?

It's a bit wordy, but less mysterious.

rnorth · 2018-01-21T21:58:34Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -165,38 +166,15 @@ private void applyScaling() {
    }

    private void registerContainersForShutdown() {
-        // Ensure that all service containers that were launched by compose will be killed at shutdown


✂️ 😄

rnorth · 2018-01-21T22:07:18Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+                })
+                .withExposedPorts(new ExposedPort(8080))
+                .withPublishAllPorts(true)
+                .withName("tc-ryuk-" + DockerClientFactory.SESSION_ID)


Nice to name the container 😄
Please could we just expand tc to testcontainers though? It's just a thing that people can google more easily if they're wondering what this container is.

rnorth · 2018-01-21T22:08:02Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+
+        String ryukContainerId = client.createContainerCmd(ryukImage)
+                .withHostConfig(new HostConfig() {
+                    @JsonProperty("AutoRemove")


Aha, I was wondering what would remove this container 😄

rnorth · 2018-01-21T22:09:41Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

+                        }
+                    }
+                },
+                "tc-ryuk"


Same as above, please could you expand tc to testcontainers for googlability?

bsideup · 2018-01-22T07:40:54Z

@rnorth done 👍

…ners after the execution even if the JVM was "kill -9"ed

… update Ryuk image to 0.2.2 with timeout fix

…me REGISTERED_FILTERS -> DEATH_NOTE for more fun :)

Relates to #545 Relates to testcontainers#2998

…e on macOS (#3159) Relates to #545 Relates to #2998

…e on macOS (testcontainers#3159) Relates to testcontainers#545 Relates to testcontainers#2998

bsideup added the enhancement label Jan 15, 2018

bsideup requested review from rnorth and kiview January 15, 2018 20:46

bsideup commented Jan 15, 2018

View reviewed changes

bsideup added this to the 1.6.0 milestone Jan 15, 2018

kiview requested changes Jan 16, 2018

View reviewed changes

kiview approved these changes Jan 16, 2018

View reviewed changes

rnorth requested changes Jan 21, 2018

View reviewed changes

bsideup mentioned this pull request Jan 23, 2018

Clean up automatically when container stopped #552

Closed

bsideup added 8 commits January 27, 2018 11:58

Start a helper "Ryuk" container to make sure that we clean the contai…

40d51fe

…ners after the execution even if the JVM was "kill -9"ed

fix DockerComposeContainer.java

69dcb17

always encode filters, fix binds

23c0840

use checkAndPullImage, add synchronized to the initial filter adding,…

8df3a48

… update Ryuk image to 0.2.2 with timeout fix

CR fixes, fix DockerClientFactoryTest, set shutdown hook lazily, rena…

29de66b

…me REGISTERED_FILTERS -> DEATH_NOTE for more fun :)

CHANGELOG.md

dbfc5bb

expand "tc" to "testcontainers", log the purpose of Ryuk

cf60eba

fix after merge

0ad2797

bsideup force-pushed the death_note branch from d06e85a to 0ad2797 Compare January 27, 2018 11:06

Merge branch 'master' into death_note

73206a1

rnorth approved these changes Jan 27, 2018

View reviewed changes

bsideup merged commit 2dc109f into master Jan 27, 2018

bsideup deleted the death_note branch January 27, 2018 16:27

bsideup added the type/feature label Jan 28, 2018

bsideup mentioned this pull request Apr 4, 2018

Feature Request: write container id to .tmp file #634

Closed

bsideup mentioned this pull request Apr 11, 2018

DockerComposeContainer.registerContainersForShutdown() not finding spawned containers #194

Closed

gesellix referenced this pull request in gesellix/testcontainers-java Aug 26, 2020

Fix bind mounts for ResourceReaper/ryuk and ContainerisedDockerCompose

8e94b84

Relates to #545 Relates to testcontainers#2998

gesellix mentioned this pull request Aug 26, 2020

Fix bind mounts for ResourceReaper/ryuk and ContainerisedDockerCompose on macOS #3159

Merged

gesellix referenced this pull request in gesellix/testcontainers-java Aug 28, 2020

Fix bind mounts for ResourceReaper/ryuk and ContainerisedDockerCompose

3797957

Relates to #545 Relates to testcontainers#2998

rnorth pushed a commit that referenced this pull request Aug 29, 2020

Fix bind mounts for ResourceReaper/ryuk and ContainerisedDockerCompos…

8bc3bf8

…e on macOS (#3159) Relates to #545 Relates to #2998

raviagarwal7 pushed a commit to raviagarwal7/testcontainers-java that referenced this pull request Nov 25, 2020

Fix bind mounts for ResourceReaper/ryuk and ContainerisedDockerCompos…

b2950ae

…e on macOS (testcontainers#3159) Relates to testcontainers#545 Relates to testcontainers#2998

fredericbirke mentioned this pull request Feb 15, 2021

Automatically clean up containers after sigkill during testrun SwissLife-OSS/squadron#83

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Death Note :) #545

Death Note :) #545

bsideup commented Jan 15, 2018 •

edited

Loading

bsideup Jan 15, 2018

bsideup Jan 15, 2018

bsideup Jan 15, 2018

kiview Jan 16, 2018

bsideup Jan 15, 2018

kiview Jan 16, 2018

rnorth Jan 21, 2018

bsideup Jan 15, 2018

rnorth Jan 21, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

bsideup Jan 16, 2018

kiview Jan 16, 2018

kiview Jan 16, 2018

kiview Jan 16, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

kiview Jan 16, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

kiview Jan 16, 2018

bsideup Jan 16, 2018

rnorth left a comment

rnorth Jan 21, 2018

rnorth Jan 21, 2018

rnorth Jan 21, 2018

rnorth Jan 21, 2018

rnorth Jan 21, 2018

bsideup commented Jan 22, 2018

Death Note :) #545

Death Note :) #545

Conversation

bsideup commented Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup commented Jan 22, 2018

bsideup commented Jan 15, 2018 •

edited

Loading