Merge pull request #4799 from vespa-engine/hakonhall/support-yum-upgrade-and-remove

Support yum upgrade and remove

6 files changed, 167 insertions, 52 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
index 00bcca71970..71a4c7c109b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
@@ -2,16 +2,27 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.process;
 
 import java.nio.file.Path;
+import java.util.logging.Logger;
 
 /**
  * @author hakonhall
  */
 public interface ChildProcess extends AutoCloseable {
+    String commandLine();
     ChildProcess waitForTermination();
     int exitValue();
     ChildProcess throwIfFailed();
     String getUtf8Output();
 
+    /**
+     * Only call this if process was spawned under the assumption the program had no side
+     * effects (see Command::spawnProgramWithoutSideEffects).  If it is determined later
+     * that the program did in fact have side effects (modified system), this method can
+     * be used to log that fact. Alternatively, call TaskContext::logSystemModification
+     * directly.
+     */
+    void logAsModifyingSystemAfterAll(Logger logger);
+
     @Override
     void close();
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
index 367688f0bb4..31ccbe90fda 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
@@ -1,9 +1,11 @@
 // 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.node.admin.task.util.process;
 
+import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
 
 import java.nio.file.Path;
+import java.util.logging.Logger;
 
 /**
  * Represents a forked child process that still exists or has terminated.
@@ -11,17 +13,28 @@ import java.nio.file.Path;
  * @author hakonhall
  */
 public class ChildProcessImpl implements ChildProcess {
+    private final TaskContext taskContext;
     private final Process process;
     private final Path processOutputPath;
     private final String commandLine;
 
-    ChildProcessImpl(Process process, Path processOutputPath, String commandLine) {
+    ChildProcessImpl(TaskContext taskContext,
+                     Process process,
+                     Path processOutputPath,
+                     String commandLine) {
+        this.taskContext = taskContext;
         this.process = process;
         this.processOutputPath = processOutputPath;
         this.commandLine = commandLine;
     }
 
+    @Override
+    public String commandLine() {
+        return commandLine;
+    }
+
     public String getUtf8Output() {
+        waitForTermination();
         return new UnixPath(processOutputPath).readUtf8File();
     }
 
@@ -39,10 +52,12 @@ public class ChildProcessImpl implements ChildProcess {
     }
 
     public int exitValue() {
+        waitForTermination();
         return process.exitValue();
     }
 
     public ChildProcess throwIfFailed() {
+        waitForTermination();
         if (process.exitValue() != 0) {
             throw new CommandException("Execution of program [" + commandLine +
                     "] failed, stdout/stderr was: <" + suffixOfOutputForLog() + ">");
@@ -65,6 +80,11 @@ public class ChildProcessImpl implements ChildProcess {
     }
 
     @Override
+    public void logAsModifyingSystemAfterAll(Logger logger) {
+        taskContext.logSystemModification(logger, "Executed command: " + commandLine);
+    }
+
+    @Override
     public void close() {
         if (process.isAlive()) {
             process.destroyForcibly();
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
index 049490f2705..51acb1d2bca 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
@@ -56,7 +56,7 @@ public class Command {
                 .redirectOutput(temporaryFile.toFile());
         Process process = IOExceptionUtil.uncheck(builder::start);
 
-        return new ChildProcessImpl(process, temporaryFile, commandLine);
+        return new ChildProcessImpl(context, process, temporaryFile, commandLine);
     }
 
     String commandLine() {
@@ -89,7 +89,15 @@ public class Command {
         return "\"" + doubleQuoteEscaped + "\"";
     }
 
-    public ChildProcess spawnWithoutLoggingCommand() {
+    /**
+     * Spawns a process that do not modify the system.
+     *
+     * This method can also be used to spawn a process that MAY have side effects
+     * to be determined at some later time. The caller is then responsible for calling
+     * TaskContext::logSystemModification afterwards. The caller is encouraged to
+     * call ChildProcess::logAsModifyingSystemAfterAll to do this.
+     */
+    public ChildProcess spawnProgramWithoutSideEffects() {
         return spawn(null);
     }
 }
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 c1514f1056b..35729b5a428 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
@@ -5,22 +5,26 @@ 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 java.util.ArrayList;
 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;
 
 /**
  * @author hakonhall
  */
 public class Yum {
-    private static Logger logger = Logger.getLogger(Yum.class.getName());
+    private static final Pattern INSTALL_NOOP_PATTERN =
+            Pattern.compile("\nNothing to do\n$");
+    private static final Pattern UPGRADE_NOOP_PATTERN =
+            Pattern.compile("\nNo packages marked for update\n$");
+    private static final Pattern REMOVE_NOOP_PATTERN =
+            Pattern.compile("\nNo Packages marked for removal\n$");
 
     private final TaskContext taskContext;
     private final Supplier<Command> commandSupplier;
-    private List<String> packages = new ArrayList<>();
 
     public Yum(TaskContext taskContext) {
         this.taskContext = taskContext;
@@ -30,57 +34,86 @@ public class Yum {
     /**
      * @param packages A list of packages, each package being of the form name-1.2.3-1.el7.noarch
      */
-    public Install install(String... packages) {
-        return new Install(taskContext, Arrays.asList(packages));
+    public GenericYumCommand install(String... packages) {
+        return newYumCommand("install", packages, INSTALL_NOOP_PATTERN);
     }
 
-    public class Install {
+    public GenericYumCommand upgrade(String... packages) {
+        return newYumCommand("upgrade", packages, UPGRADE_NOOP_PATTERN);
+    }
+
+    public GenericYumCommand remove(String... packages) {
+        return newYumCommand("remove", packages, REMOVE_NOOP_PATTERN);
+    }
+
+    private GenericYumCommand newYumCommand(String yumCommand,
+                                            String[] packages,
+                                            Pattern noopPattern) {
+        return new GenericYumCommand(
+                taskContext,
+                commandSupplier,
+                yumCommand,
+                Arrays.asList(packages),
+                noopPattern);
+    }
+
+    public static class GenericYumCommand {
+        private static Logger logger = Logger.getLogger(Yum.class.getName());
+
         private final TaskContext taskContext;
+        private final Supplier<Command> commandSupplier;
+        private final String yumCommand;
         private final List<String> packages;
+        private final Pattern commandOutputNoopPattern;
         private Optional<String> enabledRepo = Optional.empty();
 
-        public Install(TaskContext taskContext, List<String> packages) {
+        private GenericYumCommand(TaskContext taskContext,
+                                  Supplier<Command> commandSupplier,
+                                  String yumCommand,
+                                  List<String> packages,
+                                  Pattern commandOutputNoopPattern) {
             this.taskContext = taskContext;
+            this.commandSupplier = commandSupplier;
+            this.yumCommand = yumCommand;
             this.packages = packages;
+            this.commandOutputNoopPattern = commandOutputNoopPattern;
 
             if (packages.isEmpty()) {
                 throw new IllegalArgumentException("No packages specified");
             }
         }
 
-        public Install enableRepo(String repo) {
+        @SuppressWarnings("unchecked")
+        public GenericYumCommand enableRepo(String repo) {
             enabledRepo = Optional.of(repo);
             return this;
         }
 
         public boolean converge() {
-            if (packages.stream().allMatch(Yum.this::isInstalled)) {
-                return false;
-            }
-
-            execute();
-            return true;
-        }
-
-        private void execute() {
             Command command = commandSupplier.get();
-            command.add("yum", "install", "--assumeyes");
+            command.add("yum", yumCommand, "--assumeyes");
             enabledRepo.ifPresent(repo -> command.add("--enablerepo=" + repo));
             command.add(packages);
-            command.spawn(logger).waitForTermination().throwIfFailed();
+            ChildProcess childProcess = command
+                    .spawnProgramWithoutSideEffects()
+                    .waitForTermination()
+                    .throwIfFailed();
+
+            // 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).matches()) {
+                return false;
+            } else {
+                childProcess.logAsModifyingSystemAfterAll(logger);
+                return true;
+            }
         }
     }
 
+    // For testing
     Yum(TaskContext taskContext, Supplier<Command> commandSupplier) {
         this.taskContext = taskContext;
         this.commandSupplier = commandSupplier;
     }
-
-    private boolean isInstalled(String package_) {
-        ChildProcess childProcess = commandSupplier.get()
-                .add("yum", "list", "installed", package_)
-                .spawnWithoutLoggingCommand();
-        childProcess.waitForTermination();
-        return childProcess.exitValue() == 0;
-    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java
index 59c853f949d..edd7aa13627 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java
@@ -50,6 +50,14 @@ public class TestCommand extends Command {
 
             @Override
             public Path getProcessOutputPath() { return null; }
+
+            @Override
+            public void logAsModifyingSystemAfterAll(Logger logger) { }
+
+            @Override
+            public String commandLine() {
+                return "program";
+            }
         };
     }
 
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 d852be26229..4a3c1061ea6 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
@@ -4,55 +4,90 @@ 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 org.junit.Before;
 import org.junit.Test;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 public class YumTest {
+    TaskContext taskContext = mock(TaskContext.class);
+    TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext);
+
+    @Before
+    public void tearDown() {
+        commandSupplier.verifyInvocations();
+    }
+
     @Test
     public void testAlreadyInstalled() {
-        TaskContext taskContext = mock(TaskContext.class);
-        TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext);
-
-        commandSupplier.expectCommand("yum list installed package-1", 0, "");
-        commandSupplier.expectCommand("yum list installed package-2", 0, "");
+        commandSupplier.expectCommand(
+                "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
+                0,
+                "\nNothing to do\n");
 
         Yum yum = new Yum(taskContext, commandSupplier);
-        yum.install("package-1", "package-2")
+        assertFalse(yum
+                .install("package-1", "package-2")
                 .enableRepo("repo-name")
-                .converge();
+                .converge());
+    }
 
-        commandSupplier.verifyInvocations();
+    @Test
+    public void testAlreadyUpgraded() {
+        commandSupplier.expectCommand(
+                "yum upgrade --assumeyes package-1 package-2",
+                0,
+                "\nNo packages marked for update\n");
+
+        assertFalse(new Yum(taskContext, commandSupplier)
+                .upgrade("package-1", "package-2")
+                .converge());
+    }
+
+    @Test
+    public void testAlreadyRemoved() {
+        commandSupplier.expectCommand(
+                "yum remove --assumeyes package-1 package-2",
+                0,
+                "\nNo Packages marked for removal\n");
+
+        assertFalse(new Yum(taskContext, commandSupplier)
+                .remove("package-1", "package-2")
+                .converge());
     }
 
     @Test
     public void testInstall() {
-        TaskContext taskContext = mock(TaskContext.class);
-        TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext);
+        commandSupplier.expectCommand(
+                "yum install --assumeyes package-1 package-2",
+                0,
+                "installing, installing");
+
+        Yum yum = new Yum(taskContext, commandSupplier);
+        assertTrue(yum
+                .install("package-1", "package-2")
+                .converge());
+    }
 
-        commandSupplier.expectCommand("yum list installed package-1", 0, "");
-        commandSupplier.expectCommand("yum list installed package-2", 1, "");
+    @Test
+    public void testInstallWithEnablerepo() {
         commandSupplier.expectCommand(
                 "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
                 0,
-                "");
+                "installing, installing");
 
         Yum yum = new Yum(taskContext, commandSupplier);
-        yum.install("package-1", "package-2")
+        assertTrue(yum
+                .install("package-1", "package-2")
                 .enableRepo("repo-name")
-                .converge();
-
-        commandSupplier.verifyInvocations();
+                .converge());
     }
 
     @Test(expected = CommandException.class)
     public void testFailedInstall() {
-        TaskContext taskContext = mock(TaskContext.class);
-        TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext);
-
-        commandSupplier.expectCommand("yum list installed package-1", 0, "");
-        commandSupplier.expectCommand("yum list installed package-2", 1, "");
         commandSupplier.expectCommand(
                 "yum install --assumeyes --enablerepo=repo-name package-1 package-2",
                 1,