Use Terminal for Yum and SystemCtl

4 files changed, 110 insertions, 144 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtl.java
index c294d9ee806..073978530a5 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtl.java
@@ -2,12 +2,9 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.systemd;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.ChildProcess;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.Command;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.UnexpectedOutputException;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 
 import java.util.Objects;
-import java.util.function.Function;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -27,7 +24,7 @@ public class SystemCtl {
     private static final Pattern UNIT_FILE_STATE_PROPERTY_PATTERN = createPropertyPattern("UnitFileState");
     private static final Pattern ACTIVE_STATE_PROPERTY_PATTERN = createPropertyPattern("ActiveState");
 
-    private final Function<TaskContext, Command> commandSupplier;
+    private final Terminal terminal;
 
     private static Pattern createPropertyPattern(String propertyName) {
         if (!PROPERTY_NAME_PATTERN.matcher(propertyName).matches()) {
@@ -40,8 +37,8 @@ public class SystemCtl {
         return Pattern.compile(regex);
     }
 
-    public SystemCtl() {
-        this(Command::new);
+    public SystemCtl(Terminal terminal) {
+        this.terminal = terminal;
     }
 
     public SystemCtlEnable enable(String unit) { return new SystemCtlEnable(unit); }
@@ -56,8 +53,7 @@ public class SystemCtl {
         }
 
         protected boolean isAlreadyConverged(TaskContext context) {
-            ChildProcess showProcess = systemCtlShow(context);
-            String unitFileState = extractProperty(showProcess, UNIT_FILE_STATE_PROPERTY_PATTERN);
+            String unitFileState = getSystemCtlProperty(context, UNIT_FILE_STATE_PROPERTY_PATTERN);
             return Objects.equals(unitFileState, "enabled");
         }
     }
@@ -68,8 +64,7 @@ public class SystemCtl {
         }
 
         protected boolean isAlreadyConverged(TaskContext context) {
-            ChildProcess showProcess = systemCtlShow(context);
-            String unitFileState = extractProperty(showProcess, UNIT_FILE_STATE_PROPERTY_PATTERN);
+            String unitFileState = getSystemCtlProperty(context, UNIT_FILE_STATE_PROPERTY_PATTERN);
             return Objects.equals(unitFileState, "disabled");
         }
     }
@@ -80,8 +75,7 @@ public class SystemCtl {
         }
 
         protected boolean isAlreadyConverged(TaskContext context) {
-            ChildProcess showProcess = systemCtlShow(context);
-            String activeState = extractProperty(showProcess, ACTIVE_STATE_PROPERTY_PATTERN);
+            String activeState = getSystemCtlProperty(context, ACTIVE_STATE_PROPERTY_PATTERN);
             return Objects.equals(activeState, "active");
         }
     }
@@ -92,8 +86,7 @@ public class SystemCtl {
         }
 
         protected boolean isAlreadyConverged(TaskContext context) {
-            ChildProcess showProcess = systemCtlShow(context);
-            String activeState = extractProperty(showProcess, ACTIVE_STATE_PROPERTY_PATTERN);
+            String activeState = getSystemCtlProperty(context, ACTIVE_STATE_PROPERTY_PATTERN);
             return Objects.equals(activeState, "inactive");
         }
     }
@@ -124,48 +117,40 @@ public class SystemCtl {
                 return false;
             }
 
-            commandSupplier.apply(context)
+            terminal.newCommandLine(context)
                     .add("systemctl", command, unit)
-                    .spawn(logger)
-                    .waitForTermination()
-                    .throwIfFailed();
+                    .execute();
 
             return true;
         }
 
         /**
-         * Find the systemd property value of the property (given by propertyPattern)
-         * matching the 'systemctl show' output (given by showProcess).
+         * @param propertyPattern Pattern to match the output of systemctl show command with
+         *                        exactly 1 group. The matchng group must exist.
+         * @return The matched group from the 'systemctl show' output.
          */
-        protected String extractProperty(ChildProcess showProcess, Pattern propertyPattern) {
-            String output = showProcess.getUtf8Output();
-            Matcher matcher = propertyPattern.matcher(output);
-            if (!matcher.find()) {
-                throw new UnexpectedOutputException(
-                        "Output does not match '" + propertyPattern + "'", showProcess);
-            } else if (matcher.groupCount() != 1) {
-                throw new IllegalArgumentException("Property pattern must have exactly 1 group");
-            }
-
-            return matcher.group(1);
-        }
-
-        protected ChildProcess systemCtlShow(TaskContext context) {
-            ChildProcess process = commandSupplier.apply(context)
+        protected String getSystemCtlProperty(TaskContext context, Pattern propertyPattern) {
+            return terminal.newCommandLine(context)
                     .add("systemctl", "show", unit)
-                    .spawnProgramWithoutSideEffects()
-                    .waitForTermination()
-                    .throwIfFailed();
+                    .executeSilently()
+                    .mapOutput(output -> extractProperty(output, propertyPattern));
+        }
+    }
 
-            // Make sure we're able to parse UTF-8 output.
-            process.getUtf8Output();
 
-            return process;
+    /**
+     * Find the systemd property value of the property (given by propertyPattern)
+     * matching the 'systemctl show' output (given by showProcess).
+     */
+    private static String extractProperty(String showOutput, Pattern propertyPattern) {
+        Matcher matcher = propertyPattern.matcher(showOutput);
+        if (!matcher.find()) {
+            throw new IllegalArgumentException("Pattern '" + propertyPattern +
+                    "' didn't match output");
+        } else if (matcher.groupCount() != 1) {
+            throw new IllegalArgumentException("Property pattern must have exactly 1 group");
         }
-    }
 
-    // For testing
-    SystemCtl(Function<TaskContext, Command> commandSupplier) {
-        this.commandSupplier = commandSupplier;
+        return matcher.group(1);
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java
index dbb4c909a4b..26a88e39e14 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java
@@ -2,13 +2,12 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.yum;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.ChildProcess;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.Command;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandLine;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
-import java.util.function.Supplier;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 
@@ -22,11 +21,11 @@ public class Yum {
     private static final Pattern REMOVE_NOOP_PATTERN = Pattern.compile("(?dm)^No Packages marked for removal$");
 
     private final TaskContext taskContext;
-    private final Supplier<Command> commandSupplier;
+    private final Terminal terminal;
 
-    public Yum(TaskContext taskContext) {
+    public Yum(TaskContext taskContext, Terminal terminal) {
         this.taskContext = taskContext;
-        this.commandSupplier = () -> new Command(taskContext);
+        this.terminal = terminal;
     }
 
     /**
@@ -49,7 +48,7 @@ public class Yum {
                                             Pattern noopPattern) {
         return new GenericYumCommand(
                 taskContext,
-                commandSupplier,
+                terminal,
                 yumCommand,
                 Arrays.asList(packages),
                 noopPattern);
@@ -59,19 +58,19 @@ public class Yum {
         private static Logger logger = Logger.getLogger(GenericYumCommand.class.getName());
 
         private final TaskContext taskContext;
-        private final Supplier<Command> commandSupplier;
+        private final Terminal terminal;
         private final String yumCommand;
         private final List<String> packages;
         private final Pattern commandOutputNoopPattern;
         private Optional<String> enabledRepo = Optional.empty();
 
         private GenericYumCommand(TaskContext taskContext,
-                                  Supplier<Command> commandSupplier,
+                                  Terminal terminal,
                                   String yumCommand,
                                   List<String> packages,
                                   Pattern commandOutputNoopPattern) {
             this.taskContext = taskContext;
-            this.commandSupplier = commandSupplier;
+            this.terminal = terminal;
             this.yumCommand = yumCommand;
             this.packages = packages;
             this.commandOutputNoopPattern = commandOutputNoopPattern;
@@ -88,30 +87,22 @@ public class Yum {
         }
 
         public boolean converge() {
-            Command command = commandSupplier.get();
-            command.add("yum", yumCommand, "--assumeyes");
-            enabledRepo.ifPresent(repo -> command.add("--enablerepo=" + repo));
-            command.add(packages);
-            ChildProcess childProcess = command
-                    .spawnProgramWithoutSideEffects()
-                    .waitForTermination()
-                    .throwIfFailed();
+            CommandLine commandLine = terminal.newCommandLine(taskContext);
+            commandLine.add("yum", yumCommand, "--assumeyes");
+            enabledRepo.ifPresent(repo -> commandLine.add("--enablerepo=" + repo));
+            commandLine.add(packages);
 
             // There's no way to figure out whether a yum command would have been a no-op.
             // Therefore, run the command and parse the output to decide.
-            String output = childProcess.getUtf8Output();
-            if (commandOutputNoopPattern.matcher(output).find()) {
-                return false;
-            } else {
-                childProcess.logAsModifyingSystemAfterAll(logger);
-                return true;
+            boolean modifiedSystem = commandLine
+                    .executeSilently()
+                    .mapOutput(output -> !commandOutputNoopPattern.matcher(output).find());
+
+            if (modifiedSystem) {
+                commandLine.recordSilentExecutionAsSystemModification();
             }
-        }
-    }
 
-    // For testing
-    Yum(TaskContext taskContext, Supplier<Command> commandSupplier) {
-        this.taskContext = taskContext;
-        this.commandSupplier = commandSupplier;
+            return modifiedSystem;
+        }
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtlTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtlTest.java
index 53aafe6b47c..085402410b3 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtlTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/systemd/SystemCtlTest.java
@@ -2,13 +2,10 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.systemd;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.Command;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandException;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.TestCommandSupplier;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.ChildProcessFailureException;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal;
 import org.junit.Test;
 
-import java.util.function.Function;
-
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -19,48 +16,45 @@ import static org.mockito.Mockito.mock;
  */
 public class SystemCtlTest {
     private final TaskContext taskContext = mock(TaskContext.class);
-    private final TestCommandSupplier testCommandSupplier = new TestCommandSupplier(taskContext);
-    private final Function<TaskContext, Command> commandSupplier = context -> testCommandSupplier.get();
+    private final TestTerminal terminal = new TestTerminal();
 
     @Test
     public void enable() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                "systemctl show docker",
+        terminal.expectCommand(
+                "systemctl show docker 2>&1",
                 0,
                 "a=b\n" +
                         "UnitFileState=disabled\n" +
                         "bar=zoo\n")
-                .expectCommand("systemctl enable docker", 0, "");
+                .expectCommand("systemctl enable docker 2>&1", 0, "");
 
-        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(commandSupplier).enable("docker");
+        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(terminal).enable("docker");
         assertTrue(enableDockerService.converge(taskContext));
     }
 
     @Test
     public void enableIsNoop() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                        "systemctl show docker",
+        terminal.expectCommand(
+                        "systemctl show docker 2>&1",
                         0,
                         "a=b\n" +
                                 "UnitFileState=enabled\n" +
                                 "bar=zoo\n")
-                .expectCommand("systemctl enable docker", 0, "");
+                .expectCommand("systemctl enable docker 2>&1", 0, "");
 
-        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(commandSupplier).enable("docker");
+        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(terminal).enable("docker");
         assertFalse(enableDockerService.converge(taskContext));
     }
 
 
     @Test
     public void enableCommandFailre() throws Exception {
-        testCommandSupplier.expectCommand("systemctl show docker", 1, "error");
-        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(commandSupplier).enable("docker");
+        terminal.expectCommand("systemctl show docker 2>&1", 1, "error");
+        SystemCtl.SystemCtlEnable enableDockerService = new SystemCtl(terminal).enable("docker");
         try {
             enableDockerService.converge(taskContext);
             fail();
-        } catch (CommandException e) {
+        } catch (ChildProcessFailureException e) {
             // success
         }
     }
@@ -68,43 +62,41 @@ public class SystemCtlTest {
 
     @Test
     public void start() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                        "systemctl show docker",
+        terminal.expectCommand(
+                        "systemctl show docker 2>&1",
                         0,
                         "a=b\n" +
                                 "ActiveState=failed\n" +
                                 "bar=zoo\n")
-                .expectCommand("systemctl start docker", 0, "");
+                .expectCommand("systemctl start docker 2>&1", 0, "");
 
-        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(commandSupplier).start("docker");
+        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(terminal).start("docker");
         assertTrue(startDockerService.converge(taskContext));
     }
 
     @Test
     public void startIsNoop() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                        "systemctl show docker",
+        terminal.expectCommand(
+                        "systemctl show docker 2>&1",
                         0,
                         "a=b\n" +
                                 "ActiveState=active\n" +
                                 "bar=zoo\n")
-                .expectCommand("systemctl start docker", 0, "");
+                .expectCommand("systemctl start docker 2>&1", 0, "");
 
-        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(commandSupplier).start("docker");
+        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(terminal).start("docker");
         assertFalse(startDockerService.converge(taskContext));
     }
 
 
     @Test
     public void startCommandFailre() throws Exception {
-        testCommandSupplier.expectCommand("systemctl show docker", 1, "error");
-        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(commandSupplier).start("docker");
+        terminal.expectCommand("systemctl show docker 2>&1", 1, "error");
+        SystemCtl.SystemCtlStart startDockerService = new SystemCtl(terminal).start("docker");
         try {
             startDockerService.converge(taskContext);
             fail();
-        } catch (CommandException e) {
+        } catch (ChildProcessFailureException e) {
             // success
         }
     }
@@ -112,35 +104,33 @@ public class SystemCtlTest {
 
     @Test
     public void disable() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                        "systemctl show docker",
+        terminal.expectCommand(
+                        "systemctl show docker 2>&1",
                         0,
                         "a=b\n" +
                                 "UnitFileState=enabled\n" +
                                 "bar=zoo\n")
-                .expectCommand("systemctl disable docker", 0, "");
+                .expectCommand("systemctl disable docker 2>&1", 0, "");
 
-        assertTrue(new SystemCtl(commandSupplier).disable("docker").converge(taskContext));
+        assertTrue(new SystemCtl(terminal).disable("docker").converge(taskContext));
     }
 
     @Test
     public void stop() throws Exception {
-        testCommandSupplier
-                .expectCommand(
-                        "systemctl show docker",
+        terminal.expectCommand(
+                        "systemctl show docker 2>&1",
                         0,
                         "a=b\n" +
                                 "ActiveState=active\n" +
                                 "bar=zoo\n")
-                .expectCommand("systemctl stop docker", 0, "");
+                .expectCommand("systemctl stop docker 2>&1", 0, "");
 
-        assertTrue(new SystemCtl(commandSupplier).stop("docker").converge(taskContext));
+        assertTrue(new SystemCtl(terminal).stop("docker").converge(taskContext));
     }
 
     @Test
     public void restart() throws Exception {
-        testCommandSupplier.expectCommand("systemctl restart docker", 0, "");
-        assertTrue(new SystemCtl(commandSupplier).restart("docker").converge(taskContext));
+        terminal.expectCommand("systemctl restart docker 2>&1", 0, "");
+        assertTrue(new SystemCtl(terminal).restart("docker").converge(taskContext));
     }
 }
\ No newline at end of file
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java
index c893c709950..f010ea07d99 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java
@@ -2,8 +2,8 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.yum;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandException;
-import com.yahoo.vespa.hosted.node.admin.task.util.process.TestCommandSupplier;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.ChildProcessFailureException;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -14,21 +14,21 @@ import static org.mockito.Mockito.mock;
 
 public class YumTest {
     TaskContext taskContext = mock(TaskContext.class);
-    TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext);
+    TestTerminal terminal = new TestTerminal();
 
     @Before
     public void tearDown() {
-        commandSupplier.verifyInvocations();
+        terminal.verifyAllCommandsExecuted();
     }
 
     @Test
     public void testAlreadyInstalled() {
-        commandSupplier.expectCommand(
-                "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
+        terminal.expectCommand(
+                "yum install --assumeyes --enablerepo=repo-name package-1 package-2 2>&1",
                 0,
                 "foobar\nNothing to do\n");
 
-        Yum yum = new Yum(taskContext, commandSupplier);
+        Yum yum = new Yum(taskContext, terminal);
         assertFalse(yum
                 .install("package-1", "package-2")
                 .enableRepo("repo-name")
@@ -37,36 +37,36 @@ public class YumTest {
 
     @Test
     public void testAlreadyUpgraded() {
-        commandSupplier.expectCommand(
-                "yum upgrade --assumeyes package-1 package-2",
+        terminal.expectCommand(
+                "yum upgrade --assumeyes package-1 package-2 2>&1",
                 0,
                 "foobar\nNo packages marked for update\n");
 
-        assertFalse(new Yum(taskContext, commandSupplier)
+        assertFalse(new Yum(taskContext, terminal)
                 .upgrade("package-1", "package-2")
                 .converge());
     }
 
     @Test
     public void testAlreadyRemoved() {
-        commandSupplier.expectCommand(
-                "yum remove --assumeyes package-1 package-2",
+        terminal.expectCommand(
+                "yum remove --assumeyes package-1 package-2 2>&1",
                 0,
                 "foobar\nNo Packages marked for removal\n");
 
-        assertFalse(new Yum(taskContext, commandSupplier)
+        assertFalse(new Yum(taskContext, terminal)
                 .remove("package-1", "package-2")
                 .converge());
     }
 
     @Test
     public void testInstall() {
-        commandSupplier.expectCommand(
-                "yum install --assumeyes package-1 package-2",
+        terminal.expectCommand(
+                "yum install --assumeyes package-1 package-2 2>&1",
                 0,
                 "installing, installing");
 
-        Yum yum = new Yum(taskContext, commandSupplier);
+        Yum yum = new Yum(taskContext, terminal);
         assertTrue(yum
                 .install("package-1", "package-2")
                 .converge());
@@ -74,26 +74,26 @@ public class YumTest {
 
     @Test
     public void testInstallWithEnablerepo() {
-        commandSupplier.expectCommand(
-                "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
+        terminal.expectCommand(
+                "yum install --assumeyes --enablerepo=repo-name package-1 package-2 2>&1",
                 0,
                 "installing, installing");
 
-        Yum yum = new Yum(taskContext, commandSupplier);
+        Yum yum = new Yum(taskContext, terminal);
         assertTrue(yum
                 .install("package-1", "package-2")
                 .enableRepo("repo-name")
                 .converge());
     }
 
-    @Test(expected = CommandException.class)
+    @Test(expected = ChildProcessFailureException.class)
     public void testFailedInstall() {
-        commandSupplier.expectCommand(
-                "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
+        terminal.expectCommand(
+                "yum install --assumeyes --enablerepo=repo-name package-1 package-2 2>&1",
                 1,
                 "error");
 
-        Yum yum = new Yum(taskContext, commandSupplier);
+        Yum yum = new Yum(taskContext, terminal);
         yum.install("package-1", "package-2")
                 .enableRepo("repo-name")
                 .converge();