Skip invoking yum on install/remove if package(s) are present/missing in rpm

4 files changed, 126 insertions, 68 deletions

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 482f324a9b5..d8a131f5ed1 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,16 +2,10 @@
 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.CommandResult;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 
 import java.util.List;
 import java.util.Optional;
-import java.util.function.Function;
-import java.util.regex.Pattern;
-import java.util.stream.Collectors;
-import java.util.stream.IntStream;
-import java.util.stream.Stream;
 
 import static com.yahoo.vespa.hosted.node.admin.task.util.yum.YumCommand.GenericYumCommand;
 import static com.yahoo.vespa.hosted.node.admin.task.util.yum.YumCommand.InstallFixedYumCommand;
@@ -21,20 +15,6 @@ import static com.yahoo.vespa.hosted.node.admin.task.util.yum.YumCommand.Install
  */
 public class Yum {
 
-    // Note: "(?dm)" makes newline be \n (only), and enables multiline mode where ^$ match lines with find()
-    public static final Pattern INSTALL_NOOP_PATTERN = Pattern.compile("(?dm)^Nothing to do\\.?$");
-    public static final Pattern UPGRADE_NOOP_PATTERN = Pattern.compile("(?dm)^No packages marked for update$");
-    public static final Pattern REMOVE_NOOP_PATTERN = Pattern.compile("(?dm)^No [pP]ackages marked for removal\\.?$");
-
-    // WARNING: These must be in the same order as the supplier below
-    private static final String RPM_QUERYFORMAT = Stream.of("NAME", "EPOCH", "VERSION", "RELEASE", "ARCH")
-            .map(formatter -> "%{" + formatter + "}")
-            .collect(Collectors.joining("\\n"));
-    private static final Function<YumPackageName.Builder, List<Function<String, YumPackageName.Builder>>>
-            PACKAGE_NAME_BUILDERS_GENERATOR = builder -> List.of(
-                builder::setName, builder::setEpoch, builder::setVersion, builder::setRelease, builder::setArchitecture);
-
-
     private final Terminal terminal;
 
     public Yum(Terminal terminal) {
@@ -42,21 +22,7 @@ public class Yum {
     }
 
     public Optional<YumPackageName> queryInstalled(TaskContext context, String packageName) {
-        CommandResult commandResult = terminal.newCommandLine(context)
-                .add("rpm", "-q", packageName, "--queryformat", RPM_QUERYFORMAT)
-                .ignoreExitCode()
-                .executeSilently();
-
-        if (commandResult.getExitCode() != 0) return Optional.empty();
-
-        YumPackageName.Builder builder = new YumPackageName.Builder();
-        List<Function<String, YumPackageName.Builder>> builders = PACKAGE_NAME_BUILDERS_GENERATOR.apply(builder);
-        List<Optional<String>> lines = commandResult.mapEachLine(line -> Optional.of(line).filter(s -> !"(none)".equals(s)));
-        if (lines.size() != builders.size()) throw new IllegalStateException(String.format(
-                "Unexpected response from rpm, expected %d lines, got %s", builders.size(), commandResult.getOutput()));
-
-        IntStream.range(0, builders.size()).forEach(i -> lines.get(i).ifPresent(builders.get(i)::apply));
-        return Optional.of(builder.build());
+        return YumCommand.queryInstalled(terminal, context, packageName);
     }
 
     /** Lock and install, or if necessary downgrade, a package to a given version. */
@@ -65,7 +31,7 @@ public class Yum {
     }
 
     public GenericYumCommand install(YumPackageName... packages) {
-        return new GenericYumCommand(terminal, "install", List.of(packages), INSTALL_NOOP_PATTERN);
+        return new GenericYumCommand(terminal, GenericYumCommand.CommandType.install, List.of(packages));
     }
 
     public GenericYumCommand install(String package1, String... packages) {
@@ -78,7 +44,7 @@ public class Yum {
 
 
     public GenericYumCommand upgrade(YumPackageName... packages) {
-        return new GenericYumCommand(terminal, "upgrade", List.of(packages), INSTALL_NOOP_PATTERN, UPGRADE_NOOP_PATTERN);
+        return new GenericYumCommand(terminal, GenericYumCommand.CommandType.upgrade, List.of(packages));
     }
 
     public GenericYumCommand upgrade(String package1, String... packages) {
@@ -91,7 +57,7 @@ public class Yum {
 
 
     public GenericYumCommand remove(YumPackageName... packages) {
-        return new GenericYumCommand(terminal, "remove", List.of(packages), REMOVE_NOOP_PATTERN);
+        return new GenericYumCommand(terminal, GenericYumCommand.CommandType.remove, List.of(packages));
     }
 
     public GenericYumCommand remove(String package1, String... packages) {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumCommand.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumCommand.java
index f7ca453256f..2a01a5ebcb4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumCommand.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumCommand.java
@@ -4,20 +4,37 @@ package com.yahoo.vespa.hosted.node.admin.task.util.yum;
 import com.yahoo.component.Version;
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandLine;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
 
 /**
  * @author freva
  */
 public abstract class YumCommand<T extends YumCommand<T>> {
 
+    // Note: "(?dm)" makes newline be \n (only), and enables multiline mode where ^$ match lines with find()
+    public static final Pattern INSTALL_NOOP_PATTERN = Pattern.compile("(?dm)^Nothing to do\\.?$");
+    public static final Pattern UPGRADE_NOOP_PATTERN = Pattern.compile("(?dm)^No packages marked for update$");
+    public static final Pattern REMOVE_NOOP_PATTERN = Pattern.compile("(?dm)^No [pP]ackages marked for removal\\.?$");
+
+    // WARNING: These must be in the same order as the supplier below
+    private static final String RPM_QUERYFORMAT = Stream.of("NAME", "EPOCH", "VERSION", "RELEASE", "ARCH")
+            .map(formatter -> "%{" + formatter + "}")
+            .collect(Collectors.joining("\\n"));
+    private static final Function<YumPackageName.Builder, List<Function<String, YumPackageName.Builder>>>
+            PACKAGE_NAME_BUILDERS_GENERATOR = builder -> List.of(
+            builder::setName, builder::setEpoch, builder::setVersion, builder::setRelease, builder::setArchitecture);
+
     private List<String> enabledRepos = List.of();
     private final Terminal terminal;
 
@@ -54,34 +71,31 @@ public abstract class YumCommand<T extends YumCommand<T>> {
         private static final Pattern UNKNOWN_PACKAGE_PATTERN = Pattern.compile("(?dm)^No package ([^ ]+) available\\.$");
 
         private final Terminal terminal;
-        private final String yumCommand;
-        private final List<Pattern> outputNoopPatterns;
+        private final CommandType yumCommand;
         private final List<YumPackageName> packages;
         private final List<String> options = new ArrayList<>();
 
-        GenericYumCommand(Terminal terminal, String yumCommand, List<YumPackageName> packages, Pattern... outputNoopPatterns) {
+        GenericYumCommand(Terminal terminal, CommandType yumCommand, List<YumPackageName> packages) {
             super(terminal);
             this.terminal = terminal;
             this.yumCommand = yumCommand;
             this.packages = packages;
-            this.outputNoopPatterns = List.of(outputNoopPatterns);
 
             switch (yumCommand) {
-                case "install": {
+                case install: {
                     if (packages.size() > 1) options.add("skip_missing_names_on_install=False");
                     break;
                 }
-                case "upgrade": {
+                case upgrade: {
                     if (packages.size() > 1) options.add("skip_missing_names_on_update=False");
                     break;
                 }
-                case "remove": break;
+                case remove: break;
                 default: throw new IllegalArgumentException("Unknown yum command: " + yumCommand);
             }
 
-            if (packages.isEmpty() && ! "upgrade".equals(yumCommand)) {
+            if (packages.isEmpty() && yumCommand != CommandType.upgrade)
                 throw new IllegalArgumentException("No packages specified");
-            }
         }
 
         @Override
@@ -92,13 +106,14 @@ public abstract class YumCommand<T extends YumCommand<T>> {
 
         @Override
         public boolean converge(TaskContext context) {
-            if (packages.isEmpty() && ! "upgrade".equals(yumCommand)) {
-                throw new IllegalArgumentException("No packages specified");
-            }
+            if (yumCommand == CommandType.install)
+                if (packages.stream().allMatch(pkg -> isInstalled(context, pkg))) return false;
+            if (yumCommand == CommandType.remove)
+                if (packages.stream().noneMatch(pkg -> isInstalled(context, pkg))) return false;
 
             Version yumVersion = version(context);
             CommandLine commandLine = terminal.newCommandLine(context);
-            commandLine.add("yum", yumCommand);
+            commandLine.add("yum", yumCommand.name());
             addParametersToCommandLine(commandLine);
             commandLine.add(packages.stream().map(pkg -> pkg.toName(yumVersion)).collect(Collectors.toList()));
 
@@ -121,10 +136,19 @@ public abstract class YumCommand<T extends YumCommand<T>> {
                 throw new IllegalArgumentException("Unknown package: " + unknownPackageMatcher.group(1));
             }
 
-            return outputNoopPatterns.stream().noneMatch(pattern -> pattern.matcher(output).find());
+            return yumCommand.outputNoopPatterns.stream().noneMatch(pattern -> pattern.matcher(output).find());
         }
 
         protected GenericYumCommand getThis() { return this; }
+
+        enum CommandType {
+            install(INSTALL_NOOP_PATTERN), remove(REMOVE_NOOP_PATTERN), upgrade(INSTALL_NOOP_PATTERN, UPGRADE_NOOP_PATTERN);
+
+            private final List<Pattern> outputNoopPatterns;
+            CommandType(Pattern... outputNoopPatterns) {
+                this.outputNoopPatterns = List.of(outputNoopPatterns);
+            }
+        }
     }
 
 
@@ -211,7 +235,7 @@ public abstract class YumCommand<T extends YumCommand<T>> {
 
             String output = installCommand.executeSilently().getUntrimmedOutput();
 
-            if (Yum.INSTALL_NOOP_PATTERN.matcher(output).find()) {
+            if (INSTALL_NOOP_PATTERN.matcher(output).find()) {
                 if (CHECKING_FOR_UPDATE_PATTERN.matcher(output).find()) {
                     // case 3.
                     var upgradeCommand = terminal.newCommandLine(context).add("yum", "downgrade");
@@ -233,4 +257,25 @@ public abstract class YumCommand<T extends YumCommand<T>> {
         protected InstallFixedYumCommand getThis() { return this; }
     }
 
+    protected boolean isInstalled(TaskContext context, YumPackageName yumPackage) {
+        return queryInstalled(terminal, context, yumPackage.getName()).map(yumPackage::isSubsetOf).orElse(false);
+    }
+
+    static Optional<YumPackageName> queryInstalled(Terminal terminal, TaskContext context, String packageName) {
+        CommandResult commandResult = terminal.newCommandLine(context)
+                .add("rpm", "-q", packageName, "--queryformat", RPM_QUERYFORMAT)
+                .ignoreExitCode()
+                .executeSilently();
+
+        if (commandResult.getExitCode() != 0) return Optional.empty();
+
+        YumPackageName.Builder builder = new YumPackageName.Builder();
+        List<Function<String, YumPackageName.Builder>> builders = PACKAGE_NAME_BUILDERS_GENERATOR.apply(builder);
+        List<Optional<String>> lines = commandResult.mapEachLine(line -> Optional.of(line).filter(s -> !"(none)".equals(s)));
+        if (lines.size() != builders.size()) throw new IllegalStateException(String.format(
+                "Unexpected response from rpm, expected %d lines, got %s", builders.size(), commandResult.getOutput()));
+
+        IntStream.range(0, builders.size()).forEach(i -> lines.get(i).ifPresent(builders.get(i)::apply));
+        return Optional.of(builder.build());
+    }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTester.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTester.java
index 589362e747f..e47d71cbdf7 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTester.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTester.java
@@ -34,15 +34,15 @@ public class YumTester extends Yum {
     }
 
     public GenericYumCommandExpectation expectInstall(String... packages) {
-        return new GenericYumCommandExpectation("install", packages);
+        return new GenericYumCommandExpectation(CommandType.install, packages);
     }
 
     public GenericYumCommandExpectation expectUpdate(String... packages) {
-        return new GenericYumCommandExpectation("upgrade", packages);
+        return new GenericYumCommandExpectation(CommandType.upgrade, packages);
     }
 
     public GenericYumCommandExpectation expectRemove(String... packages) {
-        return new GenericYumCommandExpectation("remove", packages);
+        return new GenericYumCommandExpectation(CommandType.remove, packages);
     }
 
     public InstallFixedCommandExpectation expectInstallFixedVersion(String yumPackage) {
@@ -55,12 +55,12 @@ public class YumTester extends Yum {
 
 
     public class GenericYumCommandExpectation {
-        private final String command;
+        private final CommandType commandType;
         protected final List<YumPackageName> packages;
         private List<String> enableRepos = List.of();
 
-        private GenericYumCommandExpectation(String command, String... packages) {
-            this.command = command;
+        private GenericYumCommandExpectation(CommandType commandType, String... packages) {
+            this.commandType = commandType;
             this.packages = Stream.of(packages).map(YumPackageName::fromString).collect(Collectors.toList());
         }
 
@@ -72,11 +72,12 @@ public class YumTester extends Yum {
         /** Mock the return value of the converge(TaskContext) method for this operation (true iff system was modified) */
         public YumTester andReturn(boolean value) {
             if (value) return execute("Success");
-            switch (command) {
-                case "install": return execute("Nothing to do");
-                case "upgrade": return execute("No packages marked for update");
-                case "remove": return execute("No Packages marked for removal");
-                default: throw new IllegalArgumentException("Unknown command: " + command);
+            switch (commandType) {
+                case installFixed:
+                case install: return execute("Nothing to do");
+                case upgrade: return execute("No packages marked for update");
+                case remove: return execute("No Packages marked for removal");
+                default: throw new IllegalArgumentException("Unknown command type: " + commandType);
             }
         }
 
@@ -85,12 +86,24 @@ public class YumTester extends Yum {
         }
 
         private YumTester execute(String output) {
+            if (commandType == CommandType.install)
+                terminal.interceptCommand("rpm query", cmd -> new TestChildProcess2(1, "Not installed"));
+            if (commandType == CommandType.remove) { // Pretend the first package is installed so we can continue to yum commands
+                YumPackageName pkg = packages.get(0);
+                terminal.interceptCommand("rpm query", cmd -> new TestChildProcess2(0, String.join("\n",
+                        pkg.getName(),
+                        pkg.getEpoch().orElse("(none)"),
+                        pkg.getVersion().orElse("1.2.3"),
+                        pkg.getRelease().orElse("1"),
+                        pkg.getArchitecture().orElse("(none)"))));
+            }
+
             StringBuilder cmd = new StringBuilder();
-            cmd.append("yum ").append(command).append(" --assumeyes");
+            cmd.append("yum ").append(commandType.command).append(" --assumeyes");
             enableRepos.forEach(repo -> cmd.append(" --enablerepo=").append(repo));
-            if ("install".equals(command) && packages.size() > 1)
+            if (commandType == CommandType.install && packages.size() > 1)
                 cmd.append(" --setopt skip_missing_names_on_install=False");
-            if ("upgrade".equals(command) && packages.size() > 1)
+            if (commandType == CommandType.upgrade && packages.size() > 1)
                 cmd.append(" --setopt skip_missing_names_on_update=False");
             packages.forEach(pkg -> {
                 String name = pkg.toName(yumVersion);
@@ -109,7 +122,7 @@ public class YumTester extends Yum {
 
     public class InstallFixedCommandExpectation extends GenericYumCommandExpectation {
         private InstallFixedCommandExpectation(String yumPackage) {
-            super("install", yumPackage);
+            super(CommandType.installFixed, yumPackage);
         }
 
         @Override
@@ -150,4 +163,13 @@ public class YumTester extends Yum {
         }
     }
 
+    private enum CommandType {
+        install("install"), upgrade("upgrade"), remove("remove"), installFixed("install");
+
+        private final String command;
+        CommandType(String command) {
+            this.command = command;
+        }
+    }
+
 }
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 37695ca9504..92f8f78d255 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
@@ -78,6 +78,7 @@ public class YumTest {
 
     @Test
     public void testAlreadyInstalled() {
+        mockRpmQuery("package-1", null);
         mockYumVersion();
         terminal.expectCommand(
                 "yum install --assumeyes --enablerepo=repo1 --enablerepo=repo2 --setopt skip_missing_names_on_install=False package-1 package-2 2>&1",
@@ -90,6 +91,7 @@ public class YumTest {
                 .converge(taskContext));
 
         // RHEL 8
+        mockRpmQuery("package-1", null);
         mockYumVersion(YumVersion.rhel8);
         terminal.expectCommand(
                 "yum install --assumeyes --enablerepo=repo1 --enablerepo=repo2 --setopt skip_missing_names_on_install=False package-1 package-2 2>&1",
@@ -125,6 +127,7 @@ public class YumTest {
 
     @Test
     public void testAlreadyRemoved() {
+        mockRpmQuery("package-1", YumPackageName.fromString("package-1-1.2.3-1"));
         mockYumVersion();
         terminal.expectCommand(
                 "yum remove --assumeyes package-1 package-2 2>&1",
@@ -136,6 +139,7 @@ public class YumTest {
                 .converge(taskContext));
 
         // RHEL 8
+        mockRpmQuery("package-1", YumPackageName.fromString("package-1-1.2.3-1"));
         mockYumVersion(YumVersion.rhel8);
         terminal.expectCommand(
                 "yum remove --assumeyes package-1 package-2 2>&1",
@@ -147,7 +151,15 @@ public class YumTest {
     }
 
     @Test
+    public void skipsYumRemoveNotInRpm() {
+        mockRpmQuery("package-1", null);
+        mockRpmQuery("package-2", null);
+        assertFalse(yum.remove("package-1", "package-2").converge(taskContext));
+    }
+
+    @Test
     public void testInstall() {
+        mockRpmQuery("package-1", null);
         mockYumVersion();
         terminal.expectCommand(
                 "yum install --assumeyes --setopt skip_missing_names_on_install=False package-1 package-2 2>&1",
@@ -160,7 +172,15 @@ public class YumTest {
     }
 
     @Test
+    public void skipsYumInstallIfInRpm() {
+        mockRpmQuery("package-1", YumPackageName.fromString("package-1-1.2.3-1"));
+        mockRpmQuery("package-2", YumPackageName.fromString("1:package-2-1.2.3-1.el7.x86_64"));
+        assertFalse(yum.install("package-1-1.2.3-1", "package-2").converge(taskContext));
+    }
+
+    @Test
     public void testInstallWithEnablerepo() {
+        mockRpmQuery("package-1", null);
         mockYumVersion();
         terminal.expectCommand(
                 "yum install --assumeyes --enablerepo=repo-name --setopt skip_missing_names_on_install=False package-1 package-2 2>&1",
@@ -273,6 +293,7 @@ public class YumTest {
 
     @Test(expected = ChildProcessFailureException.class)
     public void testFailedInstall() {
+        mockRpmQuery("package-1", null);
         mockYumVersion();
         terminal.expectCommand(
                 "yum install --assumeyes --enablerepo=repo-name --setopt skip_missing_names_on_install=False package-1 package-2 2>&1",
@@ -288,6 +309,7 @@ public class YumTest {
 
     @Test
     public void testUnknownPackages() {
+        mockRpmQuery("package-1", null);
         mockYumVersion();
         terminal.expectCommand(
                 "yum install --assumeyes --setopt skip_missing_names_on_install=False package-1 package-2 package-3 2>&1",
@@ -328,4 +350,7 @@ public class YumTest {
         mockYumVersion(YumVersion.rhel7);
     }
 
+    private void mockRpmQuery(String packageName, YumPackageName installedOrNull) {
+        new YumTester(terminal).expectQueryInstalled(packageName).andReturn(installedOrNull);
+    }
 }