Re-introduce exec in docker container with timeout method

4 files changed, 102 insertions, 13 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
index b628bd77d91..d2cffda329f 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
@@ -72,13 +72,36 @@ public interface Docker {
     void deleteUnusedDockerImages();
 
     /**
-     * Execute a command in docker container as "yahoo" user
-     * TODO: Make this function interruptible
+     * Execute a command in docker container as "yahoo". Will block until the command is finished.
      *
-     * @param args          Program arguments. args[0] must be the program filename.
-     * @throws RuntimeException  (or some subclass thereof) on failure, including docker failure, command failure
+     * @param containerName The name of the container
+     * @param command The command with arguments to run.
+     *
+     * @return exitcodes, stdout and stderr in the ProcessResult
+     */
+    ProcessResult executeInContainer(ContainerName containerName, String... command);
+
+    /**
+     * Execute a command in docker container as "root". Will block until the command is finished.
+     *
+     * @param containerName The name of the container
+     * @param command The command with arguments to run.
+     *
+     * @return exitcodes, stdout and stderr in the ProcessResult
      */
-    ProcessResult executeInContainer(ContainerName containerName, String... args);
+    ProcessResult executeInContainerAsRoot(ContainerName containerName, String... command);
+
+    /**
+     * Execute a command in docker container as "root"
+     * The timeout will not kill the process spawned.
+     *
+     * @param containerName The name of the container
+     * @param timeoutSeconds Timeout for the process to finish in seconds or without timeout if empty
+     * @param command The command with arguments to run.
+     *
+     * @return exitcodes, stdout and stderr in the ProcessResult
+     */
+    ProcessResult executeInContainerAsRoot(ContainerName containerName, Long timeoutSeconds, String... command);
+
 
-    ProcessResult executeInContainerAsRoot(ContainerName containerName, String... args);
 }
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerExecTimeoutException.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerExecTimeoutException.java
new file mode 100644
index 00000000000..153fc0257a3
--- /dev/null
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerExecTimeoutException.java
@@ -0,0 +1,16 @@
+package com.yahoo.vespa.hosted.dockerapi;
+
+/**
+ * Runtime exception to be thrown when the exec commands did not finish in time.
+ *
+ * The underlying process has not been killed. If you need the process to be
+ * killed you need to wrap it into a commands that times out.
+ *
+ * @author smorgrav
+ */
+@SuppressWarnings("serial")
+public class DockerExecTimeoutException extends RuntimeException {
+    public DockerExecTimeoutException(String msg) {
+        super(msg);
+    }
+}
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
index 6b1d63dd12f..66cd50ddb20 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
@@ -225,22 +225,26 @@ public class DockerImpl implements Docker {
 
     @Override
     public ProcessResult executeInContainer(ContainerName containerName, String... args) {
-        return executeInContainerAsUser(containerName, "yahoo", args);
+        return executeInContainerAsUser(containerName, "yahoo", Optional.empty(), args);
     }
 
     @Override
     public ProcessResult executeInContainerAsRoot(ContainerName containerName, String... args) {
-        return executeInContainerAsUser(containerName, "root", args);
+        return executeInContainerAsUser(containerName, "root", Optional.empty(), args);
+    }
+
+    @Override
+    public ProcessResult executeInContainerAsRoot(ContainerName containerName, Long timeoutSeconds, String... args) {
+        return executeInContainerAsUser(containerName, "root", Optional.of(timeoutSeconds), args);
     }
 
     /**
      * Execute command in container as user, "user" can be "username", "username:group", "uid" or "uid:gid"
      */
-    private ProcessResult executeInContainerAsUser(ContainerName containerName, String user, String... args) {
-        assert args.length >= 1;
+    private ProcessResult executeInContainerAsUser(ContainerName containerName, String user, Optional<Long> timeoutSeconds, String... command) {
         try {
             final ExecCreateCmdResponse response = dockerClient.execCreateCmd(containerName.asString())
-                    .withCmd(args)
+                    .withCmd(command)
                     .withAttachStdout(true)
                     .withAttachStderr(true)
                     .withUser(user)
@@ -249,7 +253,16 @@ public class DockerImpl implements Docker {
             ByteArrayOutputStream output = new ByteArrayOutputStream();
             ByteArrayOutputStream errors = new ByteArrayOutputStream();
             ExecStartCmd execStartCmd = dockerClient.execStartCmd(response.getId());
-            execStartCmd.exec(new ExecStartResultCallback(output, errors)).awaitCompletion();
+            ExecStartResultCallback callback = execStartCmd.exec(new ExecStartResultCallback(output, errors));
+
+            if (timeoutSeconds.isPresent()) {
+                if (!callback.awaitCompletion(timeoutSeconds.get(), TimeUnit.SECONDS)) {
+                    throw new DockerExecTimeoutException(String.format("Command '%s' did not finish within %s seconds.", command[0], timeoutSeconds));
+                }
+            } else {
+                // Wait for completion no timeout
+                callback.awaitCompletion();
+            }
 
             final InspectExecResponse state = dockerClient.inspectExecCmd(execStartCmd.getExecId()).exec();
             assert !state.isRunning();
@@ -260,7 +273,7 @@ public class DockerImpl implements Docker {
         } catch (DockerException | InterruptedException e) {
             numberOfDockerDaemonFails.add();
             throw new RuntimeException("Container '" + containerName.asString()
-                    + "' failed to execute " + Arrays.toString(args), e);
+                    + "' failed to execute " + Arrays.toString(command), e);
         }
     }
 
diff --git a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
index 2c28eac5e4f..f91fccb2cc7 100644
--- a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
+++ b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
@@ -115,6 +115,43 @@ public class DockerTest {
         assertThat(docker.getAllContainersManagedBy(MANAGER_NAME).isEmpty(), is(true));
     }
 
+    /**
+     * Test the expected behavior for exec when it times out - it should throw an exception when it times out,
+     * and before the process completes.
+     *
+     * The test timeout value is set quite high to avoid noise if screwdriver is slow but lower than the process time.
+     */
+    @Test(expected = DockerExecTimeoutException.class, timeout = 2000)
+    public void testContainerExecHounorsTimeout() throws IOException, InterruptedException, ExecutionException {
+        final ContainerName containerName = new ContainerName("docker-test-foo");
+        final String containerHostname = "hostName1";
+
+        docker.createContainerCommand(dockerImage, containerName, containerHostname).withManagedBy(MANAGER_NAME).create();
+        docker.startContainer(containerName);
+        docker.executeInContainerAsRoot(containerName, 1L, "sh", "-c", "sleep 5");
+    }
+
+    /**
+     * Test the expected behavior for exec that completes before specified timeout - it should return when the process finishes and not
+     * wait for the timeout. Some previous tests indicated that this was not behaving correctly.
+     *
+     * No timeout implies infinite timeout.
+     *
+     * The test timeout value is set quite high to avoid noise if screwdriver is slow
+     */
+    @Test(timeout = 4000)
+    public void testContainerExecDoesNotBlockUntilTimeoutWhenCommandFinishesBeforeTimeout() throws IOException, InterruptedException, ExecutionException {
+        final ContainerName containerName = new ContainerName("docker-test-foo");
+        final String containerHostname = "hostName1";
+
+        docker.createContainerCommand(dockerImage, containerName, containerHostname).withManagedBy(MANAGER_NAME).create();
+        docker.startContainer(containerName);
+        docker.executeInContainerAsRoot(containerName, 2L, "sh", "-c", "echo hei");
+
+        // Also test that this is the behavoir when not specifying timeout
+        docker.executeInContainerAsRoot(containerName,"sh", "-c", "echo hei");
+    }
+
     @Test
     public void testDockerNetworking() throws InterruptedException, ExecutionException, IOException {
         String hostName1 = "docker10.test.yahoo.com";