Merge pull request #14192 from vespa-engine/mpolden/never-downgrade

Never attempt to downgrade OS

6 files changed, 30 insertions, 10 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
index cbc5a44ae94..ad85235fc69 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
@@ -96,6 +96,11 @@ public class NodeList extends AbstractFilteringList<Node, NodeList> {
                                                          .orElse(Version.emptyVersion)));
     }
 
+    /** Returns the subset of nodes that are currently on a lower version than the given version */
+    public NodeList osVersionIsBefore(Version version) {
+        return matching(node -> node.status().osVersion().isBefore(version));
+    }
+
     /** Returns the subset of nodes that are currently on the given OS version */
     public NodeList onOsVersion(Version version) {
         return matching(node -> node.status().osVersion().matches(version));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivator.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivator.java
index be1190ccff4..0e3b6715ff1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivator.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivator.java
@@ -26,8 +26,8 @@ public class OsUpgradeActivator extends NodeRepositoryMaintainer {
     protected boolean maintain() {
         for (var nodeType : NodeType.values()) {
             if (!nodeType.isHost()) continue;
-            var active = canUpgradeOsOf(nodeType);
-            nodeRepository().osVersions().resumeUpgradeOf(nodeType, active);
+            boolean resume = canUpgradeOsOf(nodeType);
+            nodeRepository().osVersions().resumeUpgradeOf(nodeType, resume);
         }
         return true;
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java
index 1216c060181..0385e2e3df6 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java
@@ -43,6 +43,11 @@ public class OsVersion {
         return wanted.isPresent() && !current.equals(wanted);
     }
 
+    /** Returns whether this is before the given version */
+    public boolean isBefore(Version version) {
+        return current.isEmpty() || current.get().isBefore(version);
+    }
+
     /** Returns whether current version matches given version */
     public boolean matches(Version version) {
         return current.isPresent() && current.get().equals(version);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingUpgrader.java
index 03d04a5f6cf..74b288d77c5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/DelegatingUpgrader.java
@@ -38,7 +38,7 @@ public class DelegatingUpgrader implements Upgrader {
         NodeList activeNodes = nodeRepository.list().nodeType(target.nodeType()).state(Node.State.active);
         int numberToUpgrade = Math.max(0, maxActiveUpgrades - activeNodes.changingOsVersionTo(target.version()).size());
         NodeList nodesToUpgrade = activeNodes.not().changingOsVersionTo(target.version())
-                                             .not().onOsVersion(target.version())
+                                             .osVersionIsBefore(target.version())
                                              .byIncreasingOsVersion()
                                              .first(numberToUpgrade);
         if (nodesToUpgrade.size() == 0) return;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringUpgrader.java
index aebf14ab13f..b4e21b22cd2 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringUpgrader.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringUpgrader.java
@@ -46,7 +46,7 @@ public class RetiringUpgrader implements Upgrader {
         Instant retiredAt = target.lastRetiredAt().orElse(Instant.EPOCH);
         if (now.isBefore(retiredAt.plus(nodeBudget))) return; // Budget has not been spent yet
 
-        activeNodes.not().onOsVersion(target.version())
+        activeNodes.osVersionIsBefore(target.version())
                    .not().deprovisioning()
                    .byIncreasingOsVersion()
                    .first(1)
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
index 914008af227..6a41e766ace 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
@@ -38,7 +38,7 @@ public class OsVersionsTest {
     private final ApplicationId infraApplication = ApplicationId.from("hosted-vespa", "infra", "default");
 
     @Test
-    public void versions() {
+    public void upgrade() {
         var versions = new OsVersions(tester.nodeRepository(), new DelegatingUpgrader(tester.nodeRepository(), Integer.MAX_VALUE));
         provisionInfraApplication(10);
         Supplier<List<Node>> hostNodes = () -> tester.nodeRepository().getNodes(NodeType.host);
@@ -50,18 +50,28 @@ public class OsVersionsTest {
         assertEquals(version1, versions.targetFor(NodeType.host).get());
         assertTrue("Per-node wanted OS version remains unset", hostNodes.get().stream().allMatch(node -> node.status().osVersion().wanted().isEmpty()));
 
+        // One host upgrades to a later version outside the control of orchestration
+        Node hostOnLaterVersion = hostNodes.get().get(0);
+        setCurrentVersion(List.of(hostOnLaterVersion), Version.fromString("8.1"));
+
         // Upgrade OS again
         var version2 = Version.fromString("7.2");
         versions.setTarget(NodeType.host, version2, Optional.empty(), false);
         assertEquals(version2, versions.targetFor(NodeType.host).get());
 
-        // Target can be (de)activated
+        // Resume upgrade
         versions.resumeUpgradeOf(NodeType.host, true);
-        assertTrue("Target version activated", hostNodes.get().stream()
-                                                        .allMatch(node -> node.status().osVersion().wanted().isPresent()));
+        List<Node> allHosts = hostNodes.get();
+        assertTrue("Wanted version is set", allHosts.stream()
+                                                       .filter(node -> !node.equals(hostOnLaterVersion))
+                                                       .allMatch(node -> node.status().osVersion().wanted().isPresent()));
+        assertTrue("Wanted version is not set for host on later version",
+                   allHosts.get(0).status().osVersion().wanted().isEmpty());
+
+        // Halt upgrade
         versions.resumeUpgradeOf(NodeType.host, false);
-        assertTrue("Target version deactivated", hostNodes.get().stream()
-                                                          .allMatch(node -> node.status().osVersion().wanted().isEmpty()));
+        assertTrue("Wanted version is unset", hostNodes.get().stream()
+                                                       .allMatch(node -> node.status().osVersion().wanted().isEmpty()));
 
         // Downgrading fails
         try {