Merge pull request #6998 from vespa-engine/freva/clean-up-docker-api

Clean up docker-api

9 files changed, 89 insertions, 78 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
index 4c538d6a194..346223d0e7e 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
@@ -7,8 +7,8 @@ package com.yahoo.vespa.hosted.dockerapi;
 public class ContainerResources {
     public static final ContainerResources UNLIMITED = ContainerResources.from(0, 0);
 
-    public final int cpuShares;
-    public final long memoryBytes;
+    private final int cpuShares;
+    private final long memoryBytes;
 
     ContainerResources(int cpuShares, long memoryBytes) {
         this.cpuShares = cpuShares;
@@ -21,6 +21,14 @@ public class ContainerResources {
                 (long) ((1L << 30) * memoryGb));
     }
 
+    public int cpuShares() {
+        return cpuShares;
+    }
+
+    public long memoryBytes() {
+        return memoryBytes;
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerStatsImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerStats.java
index a56c1e41a51..738a65bc08b 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerStatsImpl.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerStats.java
@@ -10,14 +10,14 @@ import java.util.Optional;
  *
  * @author freva
  */
-public class ContainerStatsImpl implements Docker.ContainerStats {
+public class ContainerStats {
     private final Map<String, Object> networks;
     private final Map<String, Object> cpuStats;
     private final Map<String, Object> memoryStats;
     private final Map<String, Object> blkioStats;
 
-    public ContainerStatsImpl(Map<String, Object> networks, Map<String, Object> cpuStats,
-                              Map<String, Object> memoryStats, Map<String, Object> blkioStats) {
+    public ContainerStats(Map<String, Object> networks, Map<String, Object> cpuStats,
+                          Map<String, Object> memoryStats, Map<String, Object> blkioStats) {
         // Network stats are null when container uses host network
         this.networks = Optional.ofNullable(networks).orElse(Collections.emptyMap());
         this.cpuStats = cpuStats;
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/CreateContainerCommandImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/CreateContainerCommandImpl.java
index d95f7b7b8e1..cf168df4634 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/CreateContainerCommandImpl.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/CreateContainerCommandImpl.java
@@ -6,6 +6,7 @@ import com.github.dockerjava.api.command.CreateContainerCmd;
 import com.github.dockerjava.api.model.Bind;
 import com.github.dockerjava.api.model.Capability;
 import com.github.dockerjava.api.model.Ulimit;
+import com.yahoo.vespa.hosted.dockerapi.exception.DockerException;
 
 import java.net.Inet6Address;
 import java.net.InetAddress;
@@ -24,6 +25,7 @@ import java.util.stream.IntStream;
 import java.util.stream.Stream;
 
 class CreateContainerCommandImpl implements Docker.CreateContainerCommand {
+
     private final DockerClient docker;
     private final DockerImage dockerImage;
     private final ContainerResources containerResources;
@@ -148,8 +150,8 @@ class CreateContainerCommandImpl implements Docker.CreateContainerCommand {
 
         final CreateContainerCmd containerCmd = docker
                 .createContainerCmd(dockerImage.asString())
-                .withCpuShares(containerResources.cpuShares)
-                .withMemory(containerResources.memoryBytes)
+                .withCpuShares(containerResources.cpuShares())
+                .withMemory(containerResources.memoryBytes())
                 .withName(containerName.asString())
                 .withHostName(hostName)
                 .withLabels(labels)
@@ -202,11 +204,11 @@ class CreateContainerCommandImpl implements Docker.CreateContainerCommand {
                 .skip(1)
                 .collect(Collectors.joining(" "));
 
-        return String.join(" ",
+        return Stream.of(
                 "--name " + containerName.asString(),
                 "--hostname " + hostName,
-                "--cpu-shares " + containerResources.cpuShares,
-                "--memory " + containerResources.memoryBytes,
+                "--cpu-shares " + containerResources.cpuShares(),
+                "--memory " + containerResources.memoryBytes(),
                 toRepeatedOption("--label", labelList),
                 toRepeatedOption("--ulimit", ulimitList),
                 toRepeatedOption("--env", environmentAssignments),
@@ -219,7 +221,9 @@ class CreateContainerCommandImpl implements Docker.CreateContainerCommand {
                 toOptionalOption("--entrypoint", entrypointExecuteable),
                 toFlagOption("--privileged", privileged),
                 dockerImage.asString(),
-                entrypointArgs);
+                entrypointArgs)
+                .filter(s -> !s.isEmpty())
+                .collect(Collectors.joining(" "));
     }
 
     /**
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 5e8a0feb099..966b5da9def 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
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.dockerapi;
 
 import java.net.InetAddress;
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 
 /**
@@ -61,21 +60,8 @@ public interface Docker {
             ContainerName containerName,
             String hostName);
 
-    interface ContainerStats {
-        Map<String, Object> getNetworks();
-        Map<String, Object> getCpuStats();
-        Map<String, Object> getMemoryStats();
-        Map<String, Object> getBlkioStats();
-    }
-
-    default boolean networkNATed() {
-        return false;
-    }
-
     Optional<ContainerStats> getContainerStats(ContainerName containerName);
 
-    void createContainer(CreateContainerCommand createContainerCommand);
-
     void startContainer(ContainerName containerName);
 
     void stopContainer(ContainerName containerName);
@@ -97,8 +83,6 @@ public interface Docker {
      */
     boolean pullImageAsyncIfNeeded(DockerImage image);
 
-    void deleteImage(DockerImage dockerImage);
-
     /**
      * Deletes the local images that are currently not in use by any container and not recently used.
      */
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 c5c4547f796..00753f59461 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
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.dockerapi;
 
 import com.github.dockerjava.api.DockerClient;
 import com.github.dockerjava.api.command.ExecCreateCmdResponse;
-import com.github.dockerjava.api.command.ExecStartCmd;
 import com.github.dockerjava.api.command.InspectContainerResponse;
 import com.github.dockerjava.api.command.InspectExecResponse;
 import com.github.dockerjava.api.command.InspectImageResponse;
@@ -22,6 +21,9 @@ import com.github.dockerjava.core.command.PullImageResultCallback;
 import com.github.dockerjava.jaxrs.JerseyDockerCmdExecFactory;
 import com.google.inject.Inject;
 import com.yahoo.log.LogLevel;
+import com.yahoo.vespa.hosted.dockerapi.exception.ContainerNotFoundException;
+import com.yahoo.vespa.hosted.dockerapi.exception.DockerException;
+import com.yahoo.vespa.hosted.dockerapi.exception.DockerExecTimeoutException;
 import com.yahoo.vespa.hosted.dockerapi.metrics.CounterWrapper;
 import com.yahoo.vespa.hosted.dockerapi.metrics.Dimensions;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
@@ -51,7 +53,7 @@ import static com.yahoo.vespa.hosted.dockerapi.DockerNetworkCreator.NetworkAddre
 public class DockerImpl implements Docker {
     private static final Logger logger = Logger.getLogger(DockerImpl.class.getName());
 
-    public static final String DOCKER_CUSTOM_MACVLAN_NETWORK_NAME = "vespa-macvlan";
+    private static final String DOCKER_CUSTOM_MACVLAN_NETWORK_NAME = "vespa-macvlan";
     static final String LABEL_NAME_MANAGEDBY = "com.yahoo.vespa.managedby";
     private static final String FRAMEWORK_CONTAINER_PREFIX = "/";
 
@@ -83,7 +85,7 @@ public class DockerImpl implements Docker {
     }
 
     // For testing
-    DockerImpl(final DockerClient dockerClient) {
+    DockerImpl(DockerClient dockerClient) {
         this(null, null);
         this.dockerClient = dockerClient;
     }
@@ -106,11 +108,6 @@ public class DockerImpl implements Docker {
         }
     }
 
-    @Override
-    public boolean networkNATed() {
-        return config.networkNATed();
-    }
-
     private void setupDockerNetworkIfNeeded() throws IOException {
         if (!dockerClient.listNetworksCmd().withNameFilter(DOCKER_CUSTOM_MACVLAN_NETWORK_NAME).exec().isEmpty()) return;
 
@@ -140,7 +137,7 @@ public class DockerImpl implements Docker {
     }
 
     @Override
-    public boolean pullImageAsyncIfNeeded(final DockerImage image) {
+    public boolean pullImageAsyncIfNeeded(DockerImage image) {
         try {
             synchronized (monitor) {
                 if (scheduledPulls.contains(image)) return true;
@@ -159,7 +156,7 @@ public class DockerImpl implements Docker {
         }
     }
 
-    private void removeScheduledPoll(final DockerImage image) {
+    private void removeScheduledPoll(DockerImage image) {
         synchronized (monitor) {
             scheduledPulls.remove(image);
         }
@@ -168,7 +165,7 @@ public class DockerImpl implements Docker {
     /**
      * Check if a given image is already in the local registry
      */
-    boolean imageIsDownloaded(final DockerImage dockerImage) {
+    boolean imageIsDownloaded(DockerImage dockerImage) {
         return inspectImage(dockerImage).isPresent();
     }
 
@@ -195,6 +192,8 @@ public class DockerImpl implements Docker {
             dockerClient.connectToNetworkCmd()
                     .withContainerId(containerName.asString())
                     .withNetworkId(networkName).exec();
+        } catch (NotFoundException e) {
+            throw new ContainerNotFoundException(containerName);
         } catch (RuntimeException e) {
             numberOfDockerDaemonFails.add();
             throw new DockerException("Failed to connect container '" + containerName.asString() +
@@ -222,33 +221,27 @@ public class DockerImpl implements Docker {
      */
     private ProcessResult executeInContainerAsUser(ContainerName containerName, String user, Optional<Long> timeoutSeconds, String... command) {
         try {
-            final ExecCreateCmdResponse response = dockerClient.execCreateCmd(containerName.asString())
-                    .withCmd(command)
-                    .withAttachStdout(true)
-                    .withAttachStderr(true)
-                    .withUser(user)
-                    .exec();
+            ExecCreateCmdResponse response = execCreateCmd(containerName, user, command);
 
             ByteArrayOutputStream output = new ByteArrayOutputStream();
             ByteArrayOutputStream errors = new ByteArrayOutputStream();
-            ExecStartCmd execStartCmd = dockerClient.execStartCmd(response.getId());
-            ExecStartResultCallback callback = execStartCmd.exec(new ExecStartResultCallback(output, errors));
+            ExecStartResultCallback callback = dockerClient.execStartCmd(response.getId())
+                    .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));
-                }
+                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();
-            Integer exitCode = state.getExitCode();
-            assert exitCode != null;
+            InspectExecResponse state = dockerClient.inspectExecCmd(response.getId()).exec();
+            if (state.isRunning())
+                throw new DockerException("Command '%s' did not finish within %s seconds.");
 
-            return new ProcessResult(exitCode, new String(output.toByteArray()), new String(errors.toByteArray()));
+            return new ProcessResult(state.getExitCode(), new String(output.toByteArray()), new String(errors.toByteArray()));
         } catch (RuntimeException | InterruptedException e) {
             numberOfDockerDaemonFails.add();
             throw new DockerException("Container '" + containerName.asString()
@@ -256,6 +249,19 @@ public class DockerImpl implements Docker {
         }
     }
 
+    private ExecCreateCmdResponse execCreateCmd(ContainerName containerName, String user, String... command) {
+        try {
+            return dockerClient.execCreateCmd(containerName.asString())
+                    .withCmd(command)
+                    .withAttachStdout(true)
+                    .withAttachStderr(true)
+                    .withUser(user)
+                    .exec();
+        } catch (NotFoundException e) {
+            throw new ContainerNotFoundException(containerName);
+        }
+    }
+
     private Optional<InspectContainerResponse> inspectContainerCmd(String container) {
         try {
             return Optional.of(dockerClient.inspectContainerCmd(container).exec());
@@ -273,7 +279,7 @@ public class DockerImpl implements Docker {
             DockerStatsCallback statsCallback = dockerClient.statsCmd(containerName.asString()).exec(new DockerStatsCallback());
             statsCallback.awaitCompletion(5, TimeUnit.SECONDS);
 
-            return statsCallback.stats.map(stats -> new ContainerStatsImpl(
+            return statsCallback.stats.map(stats -> new ContainerStats(
                     stats.getNetworks(), stats.getCpuStats(), stats.getMemoryStats(), stats.getBlkioStats()));
         } catch (NotFoundException ignored) {
             return Optional.empty();
@@ -284,21 +290,11 @@ public class DockerImpl implements Docker {
     }
 
     @Override
-    public void createContainer(CreateContainerCommand createContainerCommand) {
-        try {
-            dockerClient.execCreateCmd(createContainerCommand.toString());
-        } catch (NotModifiedException ignored) {
-            // If is already created, ignore
-        } catch (RuntimeException e) {
-            numberOfDockerDaemonFails.add();
-            throw new DockerException("Failed to create container '" + createContainerCommand.toString() + "'", e);
-        }
-    }
-
-    @Override
     public void startContainer(ContainerName containerName) {
         try {
             dockerClient.startContainerCmd(containerName.asString()).exec();
+        } catch (NotFoundException e) {
+            throw new ContainerNotFoundException(containerName);
         } catch (NotModifiedException ignored) {
             // If is already started, ignore
         } catch (RuntimeException e) {
@@ -308,9 +304,11 @@ public class DockerImpl implements Docker {
     }
 
     @Override
-    public void stopContainer(final ContainerName containerName) {
+    public void stopContainer(ContainerName containerName) {
         try {
             dockerClient.stopContainerCmd(containerName.asString()).withTimeout(secondsToWaitBeforeKilling).exec();
+        } catch (NotFoundException e) {
+            throw new ContainerNotFoundException(containerName);
         } catch (NotModifiedException ignored) {
             // If is already stopped, ignore
         } catch (RuntimeException e) {
@@ -328,8 +326,8 @@ public class DockerImpl implements Docker {
             });
 
             dockerClient.removeContainerCmd(containerName.asString()).exec();
-        } catch (NotFoundException ignored) {
-            // If container doesn't exist ignore
+        } catch (NotFoundException e) {
+            throw new ContainerNotFoundException(containerName);
         } catch (RuntimeException e) {
             numberOfDockerDaemonFails.add();
             throw new DockerException("Failed to delete container '" + containerName.asString() + "'", e);
@@ -366,7 +364,7 @@ public class DockerImpl implements Docker {
                 .orElse(Stream.empty());
     }
 
-    private boolean isManagedBy(final com.github.dockerjava.api.model.Container container, String manager) {
+    private boolean isManagedBy(com.github.dockerjava.api.model.Container container, String manager) {
         final Map<String, String> labels = container.getLabels();
         return labels != null && manager.equals(labels.get(LABEL_NAME_MANAGEDBY));
     }
@@ -393,8 +391,7 @@ public class DockerImpl implements Docker {
         }
     }
 
-    @Override
-    public void deleteImage(final DockerImage dockerImage) {
+    private void deleteImage(final DockerImage dockerImage) {
         try {
             dockerClient.removeImageCmd(dockerImage.asString()).exec();
         } catch (NotFoundException ignored) {
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/ContainerNotFoundException.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/ContainerNotFoundException.java
new file mode 100644
index 00000000000..b237228ee8e
--- /dev/null
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/ContainerNotFoundException.java
@@ -0,0 +1,13 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.dockerapi.exception;
+
+import com.yahoo.vespa.hosted.dockerapi.ContainerName;
+
+/**
+ * @author freva
+ */
+public class ContainerNotFoundException extends DockerException {
+    public ContainerNotFoundException(ContainerName containerName) {
+        super("No such container: " + containerName.asString());
+    }
+}
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerException.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/DockerException.java
index b5b622977fb..df6bb702bf7 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerException.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/DockerException.java
@@ -1,5 +1,5 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.hosted.dockerapi;
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.dockerapi.exception;
 
 /**
  * This exception wraps any exception thrown by docker-java
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/exception/DockerExecTimeoutException.java
index a315bc09d0b..39813db5c1e 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerExecTimeoutException.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/DockerExecTimeoutException.java
@@ -1,5 +1,5 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.hosted.dockerapi;
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.dockerapi.exception;
 
 /**
  * Runtime exception to be thrown when the exec commands did not finish in time.
@@ -10,7 +10,7 @@ package com.yahoo.vespa.hosted.dockerapi;
  * @author smorgrav
  */
 @SuppressWarnings("serial")
-public class DockerExecTimeoutException extends RuntimeException {
+public class DockerExecTimeoutException extends DockerException {
     public DockerExecTimeoutException(String msg) {
         super(msg);
     }
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/package-info.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/package-info.java
new file mode 100644
index 00000000000..a5ec5f6c235
--- /dev/null
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/exception/package-info.java
@@ -0,0 +1,5 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+@ExportPackage
+package com.yahoo.vespa.hosted.dockerapi.exception;
+
+import com.yahoo.osgi.annotation.ExportPackage;