Prevent duplicate attempts to start Ryuk container (#2245)

rnorth · bsideup · bsideup · commit fb869c06db06 · 2020-01-19T14:04:13.000+01:00
Prevent duplicate attempts to start Ryuk container
If for example, checks fail once but the Docker client is otherwise initialized, an error would occur due to an attempt to start a Ryuk container twice with the same name.
This changed is aimed at avoiding that situation.

Additionally, if checks fail once they should fail on every subsequent attempt to start a client. Therefore, we cache the failure and rethrow it.


Co-authored-by: Sergei Egorov &lt;bsideup@gmail.com&gt;
diff --git a/core/src/main/java/org/testcontainers/DockerClientFactory.java b/core/src/main/java/org/testcontainers/DockerClientFactory.java
@@ -17,9 +17,6 @@
 import lombok.SneakyThrows;
 import lombok.Synchronized;
 import lombok.extern.slf4j.Slf4j;
-import org.hamcrest.BaseMatcher;
-import org.hamcrest.Description;
-import org.rnorth.visibleassertions.VisibleAssertions;
 import org.testcontainers.dockerclient.DockerClientProviderStrategy;
 import org.testcontainers.dockerclient.DockerMachineClientProviderStrategy;
 import org.testcontainers.images.TimeLimitedLoggedPullImageResultCallback;
@@ -68,12 +65,16 @@ public class DockerClientFactory {
     @VisibleForTesting
     DockerClient dockerClient;
 
+    @VisibleForTesting
+    RuntimeException cachedChecksFailure;
+
     private String activeApiVersion;
     private String activeExecutionDriver;
 
     @Getter(lazy = true)
     private final boolean fileMountingSupported = checkMountableFile();
 
+
     static {
         System.setProperty("org.testcontainers.shaded.io.netty.packagePrefix", "org.testcontainers.shaded.");
     }
@@ -124,6 +125,12 @@ public DockerClient client() {
             return dockerClient;
         }
 
+        // fail-fast if checks have failed previously
+        if (cachedChecksFailure != null) {
+            log.debug("There is a cached checks failure - throwing", cachedChecksFailure);
+            throw cachedChecksFailure;
+        }
+
         final DockerClientProviderStrategy strategy = getOrInitializeStrategy();
 
         String hostIpAddress = strategy.getDockerHostIpAddress();
@@ -140,51 +147,56 @@ public DockerClient client() {
                 "  Operating System: " + dockerInfo.getOperatingSystem() + "\n" +
                 "  Total Memory: " + dockerInfo.getMemTotal() / (1024 * 1024) + " MB");
 
-        String ryukContainerId = null;
+        final String ryukContainerId;
+
         boolean useRyuk = !Boolean.parseBoolean(System.getenv("TESTCONTAINERS_RYUK_DISABLED"));
         if (useRyuk) {
+            log.debug("Ryuk is enabled");
             ryukContainerId = ResourceReaper.start(hostIpAddress, client);
             log.info("Ryuk started - will monitor and terminate Testcontainers containers on JVM exit");
+        } else {
+            log.debug("Ryuk is disabled");
+            ryukContainerId = null;
         }
 
         boolean checksEnabled = !TestcontainersConfiguration.getInstance().isDisableChecks();
         if (checksEnabled) {
-            VisibleAssertions.info("Checking the system...");
-            checkDockerVersion(version.getVersion());
-            if (ryukContainerId != null) {
-                checkDiskSpace(client, ryukContainerId);
-            } else {
-                runInsideDocker(
-                    client,
-                    createContainerCmd -> {
-                        createContainerCmd.withName("testcontainers-checks-" + SESSION_ID);
-                        createContainerCmd.getHostConfig().withAutoRemove(true);
-                        createContainerCmd.withCmd("tail", "-f", "/dev/null");
-                    },
-                    (__, containerId) -> {
-                        checkDiskSpace(client, containerId);
-                        return "";
-                    }
-                );
+            log.debug("Checks are enabled");
+
+            try {
+                log.info("Checking the system...");
+                checkDockerVersion(version.getVersion());
+                if (ryukContainerId != null) {
+                    checkDiskSpace(client, ryukContainerId);
+                } else {
+                    runInsideDocker(
+                        client,
+                        createContainerCmd -> {
+                            createContainerCmd.withName("testcontainers-checks-" + SESSION_ID);
+                            createContainerCmd.getHostConfig().withAutoRemove(true);
+                            createContainerCmd.withCmd("tail", "-f", "/dev/null");
+                        },
+                        (__, containerId) -> {
+                            checkDiskSpace(client, containerId);
+                            return "";
+                        }
+                    );
+                }
+            } catch (RuntimeException e) {
+                cachedChecksFailure = e;
+                throw e;
             }
+        } else {
+            log.debug("Checks are disabled");
         }
 
         dockerClient = client;
         return dockerClient;
     }
 
     private void checkDockerVersion(String dockerVersion) {
-        VisibleAssertions.assertThat("Docker version", dockerVersion, new BaseMatcher<String>() {
-            @Override
-            public boolean matches(Object o) {
-                return new ComparableVersion(o.toString()).compareTo(new ComparableVersion("1.6.0")) >= 0;
-            }
-
-            @Override
-            public void describeTo(Description description) {
-                description.appendText("should be at least 1.6.0");
-            }
-        });
+        boolean versionIsSufficient = new ComparableVersion(dockerVersion).compareTo(new ComparableVersion("1.6.0")) >= 0;
+        check("Docker server version should be at least 1.6.0", versionIsSufficient);
     }
 
     private void checkDiskSpace(DockerClient dockerClient, String id) {
@@ -201,12 +213,21 @@ private void checkDiskSpace(DockerClient dockerClient, String id) {
 
         DiskSpaceUsage df = parseAvailableDiskSpace(outputStream.toString());
 
-        VisibleAssertions.assertTrue(
+        check(
                 "Docker environment should have more than 2GB free disk space",
                 df.availableMB.map(it -> it >= 2048).orElse(true)
         );
     }
 
+    private void check(String message, boolean isSuccessful) {
+        if (isSuccessful) {
+            log.info("✔︎ {}", message);
+        } else {
+            log.error("❌ {}", message);
+            throw new IllegalStateException("Check failed: " + message);
+        }
+    }
+
     private boolean checkMountableFile() {
         DockerClient dockerClient = client();
 
@@ -267,8 +288,8 @@ private <T> T runInsideDocker(DockerClient client, Consumer<CreateContainerCmd>
         } finally {
             try {
                 client.removeContainerCmd(id).withRemoveVolumes(true).withForce(true).exec();
-            } catch (NotFoundException | InternalServerErrorException ignored) {
-                log.debug("", ignored);
+            } catch (NotFoundException | InternalServerErrorException e) {
+                log.debug("Swallowed exception while removing container", e);
             }
         }
     }
@@ -286,7 +307,7 @@ DiskSpaceUsage parseAvailableDiskSpace(String dfOutput) {
         for (String line : lines) {
             String[] fields = line.split("\\s+");
             if (fields.length > 5 && fields[5].equals("/")) {
-                long availableKB = Long.valueOf(fields[3]);
+                long availableKB = Long.parseLong(fields[3]);
                 df.availableMB = Optional.of(availableKB / 1024L);
                 df.usedPercent = Optional.of(Integer.valueOf(fields[4].replace("%", "")));
                 break;
@@ -318,10 +339,4 @@ public String getActiveExecutionDriver() {
     public boolean isUsing(Class<? extends DockerClientProviderStrategy> providerStrategyClass) {
         return strategy != null && providerStrategyClass.isAssignableFrom(this.strategy.getClass());
     }
-
-    private static class NotEnoughDiskSpaceException extends RuntimeException {
-        NotEnoughDiskSpaceException(String message) {
-            super(message);
-        }
-    }
 }
diff --git a/core/src/test/java/org/testcontainers/DockerClientFactoryTest.java b/core/src/test/java/org/testcontainers/DockerClientFactoryTest.java
@@ -1,19 +1,28 @@
 package org.testcontainers;
 
+import com.github.dockerjava.api.DockerClient;
+import com.github.dockerjava.api.exception.DockerException;
 import com.github.dockerjava.api.exception.NotFoundException;
+import org.junit.Rule;
 import org.junit.Test;
+import org.mockito.Mockito;
 import org.rnorth.visibleassertions.VisibleAssertions;
 import org.testcontainers.DockerClientFactory.DiskSpaceUsage;
 import org.testcontainers.dockerclient.LogToStringContainerCallback;
+import org.testcontainers.utility.MockTestcontainersConfigurationRule;
 import org.testcontainers.utility.TestcontainersConfiguration;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 /**
  * Test for {@link DockerClientFactory}.
  */
 public class DockerClientFactoryTest {
 
+    @Rule
+    public MockTestcontainersConfigurationRule configurationMock = new MockTestcontainersConfigurationRule();
+
     @Test
     public void runCommandInsideDockerShouldNotFailIfImageDoesNotExistsLocally() {
 
@@ -52,4 +61,31 @@ public void dockerHostIpAddress() {
         instance.strategy = null;
         assertThat(instance.dockerHostIpAddress()).isNotNull();
     }
+
+    @Test
+    public void failedChecksFailFast() {
+        Mockito.doReturn(false).when(TestcontainersConfiguration.getInstance()).isDisableChecks();
+
+        // Make sure that Ryuk is started
+        assertThat(DockerClientFactory.instance().client()).isNotNull();
+
+        DockerClientFactory instance = new DockerClientFactory();
+        DockerClient dockerClient = instance.dockerClient;
+        assertThat(instance.cachedChecksFailure).isNull();
+        try {
+            // Remove cached client to force the initialization logic
+            instance.dockerClient = null;
+
+            // Ryuk should fail to start twice due to the name conflict (equal to the session id)
+            assertThatThrownBy(instance::client).isInstanceOf(DockerException.class);
+
+            RuntimeException failure = new IllegalStateException("Boom!");
+            instance.cachedChecksFailure = failure;
+            // Fail fast
+            assertThatThrownBy(instance::client).isEqualTo(failure);
+        } finally {
+            instance.dockerClient = dockerClient;
+            instance.cachedChecksFailure = null;
+        }
+    }
 }
diff --git a/core/src/test/java/org/testcontainers/utility/MockTestcontainersConfigurationRule.java b/core/src/test/java/org/testcontainers/utility/MockTestcontainersConfigurationRule.java
@@ -23,6 +23,9 @@ public Statement apply(@NotNull Statement base, @NotNull Description description
             @Override
             public void evaluate() throws Throwable {
                 TestcontainersConfiguration previous = REF.get();
+                if (previous == null) {
+                    previous = TestcontainersConfiguration.getInstance();
+                }
                 REF.set(Mockito.spy(previous));
 
                 try {
