Wait robustness (#504)

Refactor port awaits for improved robustness: * We now allow a list of multiple startup liveness check ports to be defined for a container. This helps, e.g. for the browser containers, and lets us wait for both Selenium and VNC ports to be listening. I think this will help eliminate random flapping tests in this area and should fix #466 * For exposed/bound ports, we now check that the port is accepting connections both (1) from within the container, and (2) from the testcontainers host. Previously, the internal check was done for Docker on Mac/Win and the external check for Linux. Now both are used for all environments, which simplifies the logic and gives a solid guarantee that the port truly is listening. (For reference, the internal check was done due to issues with the Docker networking stack opening a listening socket before the containerized service itself was listening). * Also, we now have a WaitAllStrategy that lets more than one wait strategy be used. For browser containers, we now wait for (a) a log message, and (b) the listening ports to be available. * Broken out some aspects of the wait strategies/port detection into separate classes and used this to help improve test coverage. * Refactored/tidied some code in network link configuration that was redundant, using Docker API filters instead of streamed filtering of containers
testcontainers · Dec 10, 2017 · f1ce2b1 · f1ce2b1
1 parent 6620d23
commit f1ce2b1
Show file tree

Hide file tree

Showing 16 changed files with 434 additions and 110 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,13 +9,17 @@ All notable changes to this project will be documented in this file.
 - Allowing `addExposedPort` to be used after ports have been specified with `withExposedPorts` (#453)
 - Stopping creation of temporary directory prior to creating temporary file (#443)
 - Ensure that temp files are created in a temp directory (#423)
+- Added `WaitAllStrategy` as a mechanism for composing multiple startup `WaitStrategy` objects together
+- Changed `BrowserWebDriverContainer` to use improved wait strategies, to eliminate race conditions when starting VNC recording containers. This should lead to far fewer 'error' messages logged when starting up selenium containers, and less exposure to race related bugs (fixes #466). 
 
 ### Changed
 - Make Network instances reusable (i.e. work with `@ClassRule`) ([\#469](https://github.com/testcontainers/testcontainers-java/issues/469))
 - Added support for explicitly setting file mode when copying file into container ([\#446](https://github.com/testcontainers/testcontainers-java/issues/446), [\#467](https://github.com/testcontainers/testcontainers-java/issues/467)) 
 - Use Visible Assertions 2.1.0 for pre-flight test output (eliminating Jansi/JNR-POSIX dependencies for lower likelihood of conflict. JNA is now used internally by Visible Assertions instead).
 - Mark all links functionality as deprecated. This is pending removal in a later release. Please see [\#465](https://github.com/testcontainers/testcontainers-java/issues/465). {@link Network} features should be used instead.
 - Added support for copying files to/from running containers ([\#378](https://github.com/testcontainers/testcontainers-java/issues/378))
+- Add `getLivenessCheckPorts` as an eventual replacement for `getLivenessCheckPort`; this allows multiple ports to be included in post-startup wait strategies.
+- Refactor wait strategy port checking and improve test coverage.
 - Added support for customising the recording file name ([\#500](https://github.com/testcontainers/testcontainers-java/issues/500))
 
 ## [1.4.3] - 2017-10-14

diff --git a/core/src/main/java/org/testcontainers/containers/GenericContainer.java b/core/src/main/java/org/testcontainers/containers/GenericContainer.java
@@ -9,8 +9,9 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import lombok.*;
-import org.apache.commons.compress.utils.IOUtils;
 import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
+import org.apache.commons.compress.utils.IOUtils;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.junit.runner.Description;
 import org.rnorth.ducttape.ratelimits.RateLimiter;
@@ -44,6 +45,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static org.testcontainers.containers.output.OutputFrame.OutputType.STDERR;
@@ -96,7 +98,6 @@ public class GenericContainer<SELF extends GenericContainer<SELF>>
     @NonNull
     private List<Bind> binds = new ArrayList<>();
 
-    @NonNull
     private boolean privilegedMode;
 
     @NonNull
@@ -326,8 +327,11 @@ protected void containerIsStarted(InspectContainerResponse containerInfo) {
 
     /**
      * @return the port on which to check if the container is ready
+     * @deprecated see {@link GenericContainer#getLivenessCheckPorts()} for replacement
      */
+    @Deprecated
     protected Integer getLivenessCheckPort() {
+        // legacy implementation for backwards compatibility
         if (exposedPorts.size() > 0) {
             return getMappedPort(exposedPorts.get(0));
         } else if (portBindings.size() > 0) {
@@ -337,6 +341,39 @@ protected Integer getLivenessCheckPort() {
         }
     }
 
+    /**
+     * @return the ports on which to check if the container is ready
+     */
+    @NotNull
+    @NonNull
+    protected Set<Integer> getLivenessCheckPorts() {
+        final Set<Integer> result = new HashSet<>();
+        result.addAll(getExposedPortNumbers());
+        result.addAll(getBoundPortNumbers());
+
+        // for backwards compatibility
+        if (this.getLivenessCheckPort() != null) {
+            result.add(this.getLivenessCheckPort());
+        }
+
+        return result;
+    }
+
+    private List<Integer> getExposedPortNumbers() {
+        return exposedPorts.stream()
+                .map(this::getMappedPort)
+                .collect(Collectors.toList());
+    }
+
+    private List<Integer> getBoundPortNumbers() {
+        return portBindings.stream()
+                .map(PortBinding::parse)
+                .map(PortBinding::getBinding)
+                .map(Ports.Binding::getHostPortSpec)
+                .map(Integer::valueOf)
+                .collect(Collectors.toList());
+    }
+
     private void applyConfiguration(CreateContainerCmd createCommand) {
 
         // Set up exposed ports (where there are no host port bindings defined)
@@ -376,31 +413,16 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
             String alias = linkEntries.getKey();
             LinkableContainer linkableContainer = linkEntries.getValue();
 
-            Set<Link> links = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .map(container -> new Link(container.getNames()[0], alias))
-                    .collect(Collectors.toSet());
+            Set<Link> links = findLinksFromThisContainer(alias, linkableContainer);
             allLinks.addAll(links);
 
-            boolean linkableContainerIsRunning = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .map(com.github.dockerjava.api.model.Container::getId)
-                    .map(id -> dockerClient.inspectContainerCmd(id).exec())
-                    .anyMatch(linkableContainerInspectResponse -> linkableContainerInspectResponse.getState().getRunning());
-
-            if (!linkableContainerIsRunning) {
+            if (allLinks.size() == 0) {
                 throw new ContainerLaunchException("Aborting attempt to link to container " +
                         linkableContainer.getContainerName() +
                         " as it is not running");
             }
 
-            Set<String> linkedContainerNetworks = dockerClient.listContainersCmd().exec().stream()
-                    .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
-                    .filter(container -> container.getNetworkSettings() != null &&
-                            container.getNetworkSettings().getNetworks() != null)
-                    .flatMap(container -> container.getNetworkSettings().getNetworks().keySet().stream())
-                    .distinct()
-                    .collect(Collectors.toSet());
+            Set<String> linkedContainerNetworks = findAllNetworksForLinkedContainers(linkableContainer);
             allLinkedContainerNetworks.addAll(linkedContainerNetworks);
         }
 
@@ -443,6 +465,26 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
         createCommand.withLabels(Collections.singletonMap("org.testcontainers", "true"));
     }
 
+    private Set<Link> findLinksFromThisContainer(String alias, LinkableContainer linkableContainer) {
+        return dockerClient.listContainersCmd()
+                .withStatusFilter("running")
+                .exec().stream()
+                .flatMap(container -> Stream.of(container.getNames()))
+                .filter(name -> name.endsWith(linkableContainer.getContainerName()))
+                .map(name -> new Link(name, alias))
+                .collect(Collectors.toSet());
+    }
+
+    private Set<String> findAllNetworksForLinkedContainers(LinkableContainer linkableContainer) {
+        return dockerClient.listContainersCmd().exec().stream()
+                .filter(container -> container.getNames()[0].endsWith(linkableContainer.getContainerName()))
+                .filter(container -> container.getNetworkSettings() != null &&
+                        container.getNetworkSettings().getNetworks() != null)
+                .flatMap(container -> container.getNetworkSettings().getNetworks().keySet().stream())
+                .distinct()
+                .collect(Collectors.toSet());
+    }
+
     /**
      * {@inheritDoc}
      */
@@ -1015,11 +1057,20 @@ protected Logger logger() {
 
         /**
          * @return the port on which to check if the container is ready
+         * @deprecated see {@link AbstractWaitStrategy#getLivenessCheckPorts()}
          */
+        @Deprecated
         protected Integer getLivenessCheckPort() {
             return container.getLivenessCheckPort();
         }
 
+        /**
+         * @return the ports on which to check if the container is ready
+         */
+        protected Set<Integer> getLivenessCheckPorts() {
+            return container.getLivenessCheckPorts();
+        }
+
         /**
          * @return the rate limiter to use
          */

diff --git a/core/src/main/java/org/testcontainers/containers/VncRecordingSidekickContainer.java b/core/src/main/java/org/testcontainers/containers/VncRecordingSidekickContainer.java
@@ -1,6 +1,7 @@
 package org.testcontainers.containers;
 
 import com.github.dockerjava.api.command.InspectContainerResponse;
+import org.jetbrains.annotations.NotNull;
 import org.testcontainers.containers.traits.LinkableContainer;
 import org.testcontainers.containers.traits.VncService;
 import org.testcontainers.utility.TestcontainersConfiguration;
@@ -10,6 +11,9 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.util.Set;
+
+import static java.util.Collections.emptySet;
 
 /**
  * 'Sidekick container' with the sole purpose of recording the VNC screen output from another container.
@@ -46,10 +50,11 @@ protected void containerIsStarting(InspectContainerResponse containerInfo) {
         // do nothing
     }
 
+    @NotNull
     @Override
-    protected Integer getLivenessCheckPort() {
+    protected Set<Integer> getLivenessCheckPorts() {
         // no liveness check needed
-        return null;
+        return emptySet();
     }
 
     @Override

diff --git a/core/src/main/java/org/testcontainers/containers/wait/HostPortWaitStrategy.java b/core/src/main/java/org/testcontainers/containers/wait/HostPortWaitStrategy.java
@@ -1,19 +1,18 @@
 package org.testcontainers.containers.wait;
 
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang.SystemUtils;
 import org.rnorth.ducttape.TimeoutException;
 import org.rnorth.ducttape.unreliables.Unreliables;
-import org.testcontainers.DockerClientFactory;
 import org.testcontainers.containers.ContainerLaunchException;
 import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.dockerclient.DockerMachineClientProviderStrategy;
-import org.testcontainers.dockerclient.WindowsClientProviderStrategy;
+import org.testcontainers.containers.wait.internal.ExternalPortListeningCheck;
+import org.testcontainers.containers.wait.internal.InternalCommandPortListeningCheck;
 
-import java.net.Socket;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 
 /**
  * Waits until a socket connection can be established on a port exposed or mapped by the container.
@@ -23,87 +22,40 @@
 @Slf4j
 public class HostPortWaitStrategy extends GenericContainer.AbstractWaitStrategy {
 
-    private static final String SUCCESS_MARKER = "TESTCONTAINERS_SUCCESS";
     @Override
     protected void waitUntilReady() {
-        final Integer port = getLivenessCheckPort();
-        if (null == port) {
-            log.debug("Liveness check port of {} is empty. Not waiting.", container.getContainerName());
+        final Set<Integer> externalLivenessCheckPorts = getLivenessCheckPorts();
+        if (externalLivenessCheckPorts.isEmpty()) {
+            log.debug("Liveness check ports of {} is empty. Not waiting.", container.getContainerName());
             return;
         }
 
-        Callable<Boolean> checkStrategy;
+        @SuppressWarnings("unchecked")
+        List<Integer> exposedPorts = container.getExposedPorts();
 
-        if (shouldCheckWithCommand()) {
-            List<Integer> exposedPorts = container.getExposedPorts();
+        final Set<Integer> internalPorts = getInternalPorts(externalLivenessCheckPorts, exposedPorts);
 
-            Integer exposedPort = exposedPorts.stream()
-                    .filter(it -> port.equals(container.getMappedPort(it)))
-                    .findFirst()
-                    .orElse(null);
+        Callable<Boolean> internalCheck = new InternalCommandPortListeningCheck(container, internalPorts);
 
-            if (null == exposedPort) {
-                log.warn("Liveness check port of {} is set to {}, but it's not listed in exposed ports.",
-                        container.getContainerName(), port);
-                return;
-            }
-
-            String[][] commands = {
-                     { "/bin/sh", "-c", "nc -vz -w 1 localhost " + exposedPort + " && echo " + SUCCESS_MARKER },
-                     { "/bin/bash", "-c", "</dev/tcp/localhost/" + exposedPort + " && echo " + SUCCESS_MARKER }
-            };
-
-            checkStrategy = () -> {
-                for (String[] command : commands) {
-                    try {
-                        if (container.execInContainer(command).getStdout().contains(SUCCESS_MARKER)) {
-                            return true;
-                        }
-                    } catch (InterruptedException e) {
-                        throw new RuntimeException(e);
-                    } catch (Exception e) {
-                        continue;
-                    }
-                }
-
-                return false;
-            };
-        } else {
-            checkStrategy = () -> {
-                new Socket(container.getContainerIpAddress(), port).close();
-                return true;
-            };
-        }
+        Callable<Boolean> externalCheck = new ExternalPortListeningCheck(container, externalLivenessCheckPorts);
 
         try {
             Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> {
-                return getRateLimiter().getWhenReady(() -> {
-                    try {
-                        return checkStrategy.call();
-                    } catch (Exception e) {
-                        throw new RuntimeException(e);
-                    }
-                });
+                return getRateLimiter().getWhenReady(() -> internalCheck.call() && externalCheck.call());
             });
 
         } catch (TimeoutException e) {
             throw new ContainerLaunchException("Timed out waiting for container port to open (" +
-                    container.getContainerIpAddress() + ":" + port + " should be listening)");
+                    container.getContainerIpAddress() +
+                    " ports: " +
+                    externalLivenessCheckPorts +
+                    " should be listening)");
         }
     }
 
-    private boolean shouldCheckWithCommand() {
-        // Special case for Docker for Mac, see #160
-        if (! DockerClientFactory.instance().isUsing(DockerMachineClientProviderStrategy.class) &&
-                SystemUtils.IS_OS_MAC_OSX) {
-            return true;
-        }
-
-        // Special case for Docker for Windows, see #160
-        if (DockerClientFactory.instance().isUsing(WindowsClientProviderStrategy.class)) {
-            return true;
-        }
-
-        return false;
+    private Set<Integer> getInternalPorts(Set<Integer> externalLivenessCheckPorts, List<Integer> exposedPorts) {
+        return exposedPorts.stream()
+                .filter(it -> externalLivenessCheckPorts.contains(container.getMappedPort(it)))
+                .collect(Collectors.toSet());
     }
 }
diff --git a/core/src/main/java/org/testcontainers/containers/wait/WaitAllStrategy.java b/core/src/main/java/org/testcontainers/containers/wait/WaitAllStrategy.java
@@ -0,0 +1,38 @@
+package org.testcontainers.containers.wait;
+
+import org.rnorth.ducttape.timeouts.Timeouts;
+import org.testcontainers.containers.GenericContainer;
+
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Wait strategy that waits for a number of other strategies to pass in series.
+ */
+public class WaitAllStrategy implements WaitStrategy {
+
+    private final List<WaitStrategy> strategies = new ArrayList<>();
+    private Duration timeout = Duration.ofSeconds(30);
+
+    @Override
+    public void waitUntilReady(GenericContainer container) {
+        Timeouts.doWithTimeout((int) timeout.toMillis(), TimeUnit.MILLISECONDS, () -> {
+            for (WaitStrategy strategy : strategies) {
+                strategy.waitUntilReady(container);
+            }
+        });
+    }
+
+    public WaitAllStrategy withStrategy(WaitStrategy strategy) {
+        this.strategies.add(strategy);
+        return this;
+    }
+
+    @Override
+    public WaitStrategy withStartupTimeout(Duration startupTimeout) {
+        this.timeout = startupTimeout;
+        return this;
+    }
+}