Return CommandResult from DockerOperations::executeCommandInNetworkNamespace

7 files changed, 31 insertions, 60 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
index 2a40428cad2..ce4a915f09b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
@@ -8,6 +8,7 @@ import com.yahoo.vespa.hosted.dockerapi.ContainerStats;
 import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
 import java.time.Duration;
 import java.util.List;
@@ -31,7 +32,8 @@ public interface DockerOperations {
 
     ProcessResult executeCommandInContainerAsRoot(NodeAgentContext context, Long timeoutSeconds, String... command);
 
-    ProcessResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command);
+    /** Executes a command in inside containers network namespace, throws on non-zero exit code */
+    CommandResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command);
 
 
     /** Resume node. Resuming a node means that it is ready to take on traffic. */
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
index c96b49976b4..a870fa95e37 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.node.admin.docker;
 
 import com.google.common.net.InetAddresses;
-import com.yahoo.collections.Pair;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.SystemName;
@@ -16,9 +15,10 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeMembers
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddresses;
-import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesImpl;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 
-import java.io.IOException;
+import java.net.Inet4Address;
 import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.nio.file.Path;
@@ -30,7 +30,6 @@ import java.util.List;
 import java.util.Optional;
 import java.util.OptionalLong;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
 /**
  * Class that wraps the Docker class and have some tools related to running programs in docker.
@@ -47,16 +46,12 @@ public class DockerOperationsImpl implements DockerOperations {
     private static final InetAddress IPV4_NPT_PREFIX = InetAddresses.forString("172.17.0.0");
 
     private final Docker docker;
-    private final ProcessExecuter processExecuter;
+    private final Terminal terminal;
     private final IPAddresses ipAddresses;
 
-    public DockerOperationsImpl(Docker docker) {
-        this(docker, new ProcessExecuter(), new IPAddressesImpl());
-    }
-
-    public DockerOperationsImpl(Docker docker, ProcessExecuter processExecuter, IPAddresses ipAddresses) {
+    public DockerOperationsImpl(Docker docker, Terminal terminal, IPAddresses ipAddresses) {
         this.docker = docker;
-        this.processExecuter = processExecuter;
+        this.terminal = terminal;
         this.ipAddresses = ipAddresses;
     }
 
@@ -193,31 +188,17 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
-    public ProcessResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command) {
+    public CommandResult executeCommandInNetworkNamespace(NodeAgentContext context, String... command) {
         int containerPid = docker.getContainer(context.containerName())
                 .filter(container -> container.state.isRunning())
                 .orElseThrow(() -> new RuntimeException(
                         "Found no running container named " + context.containerName().asString()))
                 .pid;
 
-        String[] wrappedCommand = Stream.concat(Stream.of("nsenter",
-                                                          String.format("--net=/proc/%d/ns/net", containerPid),
-                                                          "--"),
-                                                Stream.of(command))
-                                        .toArray(String[]::new);
-
-        try {
-            Pair<Integer, String> result = processExecuter.exec(wrappedCommand);
-            if (result.getFirst() != 0) {
-                throw new RuntimeException(String.format(
-                        "Failed to execute %s in network namespace for %s (PID = %d), exit code: %d, output: %s",
-                        Arrays.toString(wrappedCommand), context.containerName().asString(), containerPid, result.getFirst(), result.getSecond()));
-            }
-            return new ProcessResult(0, result.getSecond(), "");
-        } catch (IOException e) {
-            throw new RuntimeException(String.format("IOException while executing %s in network namespace for %s (PID = %d)",
-                    Arrays.toString(wrappedCommand), context.containerName().asString(), containerPid), e);
-        }
+        return terminal.newCommandLine(context)
+                .add("nsenter", String.format("--net=/proc/%d/ns/net", containerPid), "--")
+                .add(command)
+                .executeSilently();
     }
 
     @Override
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
index 247cf8bffd6..19abe784749 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.acl;
 
 import com.google.common.net.InetAddresses;
 import com.yahoo.log.LogLevel;
-import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.Editor;
@@ -16,12 +15,10 @@ import java.net.InetAddress;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.Arrays;
 import java.util.List;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 
@@ -88,13 +85,9 @@ public class AclMaintainer {
     }
 
     private Supplier<List<String>> listTable(NodeAgentContext context, String table, IPVersion ipVersion) {
-        return () -> {
-            ProcessResult currentRulesResult =
-                    dockerOperations.executeCommandInNetworkNamespace(context, ipVersion.iptablesCmd(), "-S", "-t", table);
-            return Arrays.stream(currentRulesResult.getOutput().split("\n"))
-                    .map(String::trim)
-                    .collect(Collectors.toList());
-        };
+        return () -> dockerOperations
+                .executeCommandInNetworkNamespace(context, ipVersion.iptablesCmd(), "-S", "-t", table)
+                .mapEachLine(String::trim);
     }
 
     private Consumer<List<String>> restoreTable(NodeAgentContext context, String table, IPVersion ipVersion, boolean flush) {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandResult.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandResult.java
index 34cc49770aa..39eccac23c4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandResult.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandResult.java
@@ -20,7 +20,7 @@ public class CommandResult {
     private final int exitCode;
     private final String output;
 
-    CommandResult(CommandLine commandLine, int exitCode, String output) {
+    public CommandResult(CommandLine commandLine, int exitCode, String output) {
         this.commandLine = commandLine;
         this.exitCode = exitCode;
         this.output = output;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
index 48a9e8ca039..c40c25dbcc0 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.node.admin.docker;
 
 import com.google.common.net.InetAddresses;
-import com.yahoo.collections.Pair;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.system.ProcessExecuter;
 import com.yahoo.vespa.hosted.dockerapi.Container;
@@ -14,10 +13,10 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddresses;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesMock;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal;
 import org.junit.Test;
 import org.mockito.InOrder;
 
-import java.io.IOException;
 import java.net.InetAddress;
 import java.nio.file.Paths;
 import java.util.Optional;
@@ -25,7 +24,6 @@ import java.util.OptionalLong;
 
 import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.assertThat;
-import static org.mockito.AdditionalMatchers.aryEq;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.inOrder;
@@ -36,10 +34,10 @@ import static org.mockito.Mockito.when;
 
 public class DockerOperationsImplTest {
     private final Docker docker = mock(Docker.class);
-    private final ProcessExecuter processExecuter = mock(ProcessExecuter.class);
+    private final TestTerminal terminal = new TestTerminal();
     private final IPAddresses ipAddresses = new IPAddressesMock();
     private final DockerOperationsImpl dockerOperations = new DockerOperationsImpl(
-            docker, processExecuter, ipAddresses);
+            docker, terminal, ipAddresses);
 
     @Test
     public void processResultFromNodeProgramWhenSuccess() {
@@ -74,12 +72,11 @@ public class DockerOperationsImplTest {
     }
 
     @Test
-    public void runsCommandInNetworkNamespace() throws IOException {
+    public void runsCommandInNetworkNamespace() {
         NodeAgentContext context = new NodeAgentContextImpl.Builder("container-42.domain.tld").build();
         makeContainer("container-42", Container.State.RUNNING, 42);
 
-        when(processExecuter.exec(aryEq(new String[]{"nsenter", "--net=/proc/42/ns/net", "--", "iptables", "-nvL"})))
-                .thenReturn(new Pair<>(0, ""));
+        terminal.expectCommand("nsenter --net=/proc/42/ns/net -- iptables -nvL 2>&1");
 
         dockerOperations.executeCommandInNetworkNamespace(context, "iptables", "-nvL");
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java
index 8101acb4b3e..b64d3c8051e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java
@@ -1,10 +1,8 @@
 // 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.node.admin.integrationTests;
 
-import com.yahoo.collections.Pair;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeType;
-import com.yahoo.system.ProcessExecuter;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
 import com.yahoo.vespa.hosted.dockerapi.metrics.Metrics;
@@ -21,6 +19,8 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentFactory;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesMock;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TerminalImpl;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TestChildProcess2;
 import com.yahoo.vespa.test.file.TestFileSystem;
 import org.mockito.InOrder;
 import org.mockito.Mockito;
@@ -33,7 +33,6 @@ import java.time.Duration;
 import java.util.Optional;
 import java.util.logging.Logger;
 
-import static com.yahoo.yolean.Exceptions.uncheck;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
@@ -75,8 +74,7 @@ public class DockerTester implements AutoCloseable {
         ipAddresses.addAddress(HOST_HOSTNAME.value(), "f000::");
         for (int i = 1; i < 4; i++) ipAddresses.addAddress("host" + i + ".test.yahoo.com", "f000::" + i);
 
-        ProcessExecuter processExecuter = mock(ProcessExecuter.class);
-        uncheck(() -> when(processExecuter.exec(any(String[].class))).thenReturn(new Pair<>(0, "")));
+        TerminalImpl terminal = new TerminalImpl(command -> new TestChildProcess2(0, ""));
 
         NodeSpec hostSpec = new NodeSpec.Builder()
                 .hostname(HOST_HOSTNAME.value())
@@ -89,7 +87,7 @@ public class DockerTester implements AutoCloseable {
         nodeRepository.updateNodeRepositoryNode(hostSpec);
 
         FileSystem fileSystem = TestFileSystem.create();
-        DockerOperations dockerOperations = new DockerOperationsImpl(docker, processExecuter, ipAddresses);
+        DockerOperations dockerOperations = new DockerOperationsImpl(docker, terminal, ipAddresses);
 
         Metrics metrics = new Metrics();
         NodeAgentFactory nodeAgentFactory = (contextSupplier, nodeContext) -> new NodeAgentImpl(
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
index 8f455a08860..90674f86907 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainerTest.java
@@ -1,7 +1,6 @@
 // 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.node.admin.maintenance.acl;
 
-import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
@@ -9,6 +8,7 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesMock;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 import com.yahoo.vespa.test.file.TestFileSystem;
 import org.junit.Before;
 import org.junit.Test;
@@ -251,13 +251,13 @@ public class AclMaintainerTest {
         doAnswer(invoc -> {
             String path = invoc.getArgument(2);
             writtenFileContents.add(new UnixPath(path).readUtf8File());
-            return new ProcessResult(0, "", "");
+            return new CommandResult(null, 0, "");
         }).when(dockerOperations).executeCommandInNetworkNamespace(any(), endsWith("-restore"), any());
     }
 
-    private void whenListRules(NodeAgentContext context, String table, IPVersion ipVersion, String result) {
+    private void whenListRules(NodeAgentContext context, String table, IPVersion ipVersion, String output) {
         when(dockerOperations.executeCommandInNetworkNamespace(
                 eq(context), eq(ipVersion.iptablesCmd()), eq("-S"), eq("-t"), eq(table)))
-                .thenReturn(new ProcessResult(0, result, ""));
+                .thenReturn(new CommandResult(null, 0, output));
     }
 }