diff options
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, |