Only upgrade OS of active hosts

2 files changed, 23 insertions, 9 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java
index e10ff3d24cd..be474eddf97 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.os;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.curator.Lock;
+import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter;
 import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient;
@@ -91,7 +92,7 @@ public class OsVersions {
             }
 
             if (!force && oldTarget.filter(v -> v.isAfter(newTarget)).isPresent()) {
-                throw new IllegalArgumentException("Cannot set target OS version to " + newTarget +
+                throw new IllegalArgumentException("Cannot set target OS version to " + newTarget.toFullString() +
                                                    " without setting 'force', as it's lower than the current version: "
                                                    + oldTarget.get());
             }
@@ -119,14 +120,14 @@ public class OsVersions {
 
     /** Trigger upgrade of nodes of given type*/
     private void upgrade(NodeType type, Version version) {
-        var nodes = nodeRepository.list().nodeType(type);
-        var numberToUpgrade = Math.max(0, maxActiveUpgrades - nodes.changingOsVersionTo(version).size());
-        var nodesToUpgrade = nodes.not().changingOsVersionTo(version)
-                                  .not().onOsVersion(version)
-                                  .byIncreasingOsVersion()
-                                  .first(numberToUpgrade);
+        var activeNodes = nodeRepository.list().nodeType(type).state(Node.State.active);
+        var numberToUpgrade = Math.max(0, maxActiveUpgrades - activeNodes.changingOsVersionTo(version).size());
+        var nodesToUpgrade = activeNodes.not().changingOsVersionTo(version)
+                                        .not().onOsVersion(version)
+                                        .byIncreasingOsVersion()
+                                        .first(numberToUpgrade);
         if (nodesToUpgrade.size() == 0) return;
-        log.info("Upgrading " + nodesToUpgrade.size() + " nodes of type " + type + " to OS version " + version);
+        log.info("Upgrading " + nodesToUpgrade.size() + " nodes of type " + type + " to OS version " + version.toFullString());
         nodeRepository.upgradeOs(NodeListFilter.from(nodesToUpgrade.asList()), Optional.of(version));
     }
 
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 5e859cd3d25..5f4bde85c88 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
@@ -2,9 +2,11 @@
 package com.yahoo.vespa.hosted.provision.os;
 
 import com.yahoo.component.Version;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
+import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.OsVersion;
 import com.yahoo.vespa.hosted.provision.node.Status;
 import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
@@ -29,11 +31,13 @@ import static org.junit.Assert.fail;
 public class OsVersionsTest {
 
     private final ProvisioningTester tester = new ProvisioningTester.Builder().build();
+    private final ApplicationId infraApplication = ApplicationId.from("hosted-vespa", "tenant-host", "default");
 
     @Test
     public void test_versions() {
         var versions = new OsVersions(tester.nodeRepository(), Integer.MAX_VALUE);
         tester.makeReadyNodes(10, "default", NodeType.host);
+        tester.prepareAndActivateInfraApplication(infraApplication, NodeType.host);
         Supplier<List<Node>> hostNodes = () -> tester.nodeRepository().getNodes(NodeType.host);
 
         // Upgrade OS
@@ -78,7 +82,8 @@ public class OsVersionsTest {
         int maxActiveUpgrades = 5;
         var versions = new OsVersions(tester.nodeRepository(), maxActiveUpgrades);
         tester.makeReadyNodes(totalNodes, "default", NodeType.host);
-        Supplier<NodeList> hostNodes = () -> tester.nodeRepository().list().nodeType(NodeType.host);
+        Supplier<NodeList> hostNodes = () -> tester.nodeRepository().list().state(Node.State.active).nodeType(NodeType.host);
+        tester.prepareAndActivateInfraApplication(infraApplication, NodeType.host);
 
         // 5 nodes have no version. The other 15 are spread across different versions
         var hostNodesList = hostNodes.get().asList();
@@ -86,6 +91,13 @@ public class OsVersionsTest {
             setCurrentVersion(List.of(hostNodesList.get(i)), new Version(7, 0, i));
         }
 
+        // Deprovisioned hosts are not considered
+        for (var host : tester.makeReadyNodes(10, "default", NodeType.host)) {
+            tester.nodeRepository().fail(host.hostname(), Agent.system, OsVersions.class.getSimpleName());
+            tester.nodeRepository().removeRecursively(host.hostname());
+        }
+        assertEquals(10, tester.nodeRepository().getNodes(Node.State.deprovisioned).size());
+
         // Set target
         var version1 = Version.fromString("7.1");
         versions.setTarget(NodeType.host, version1, false);
@@ -117,6 +129,7 @@ public class OsVersionsTest {
     public void test_newer_upgrade_aborts_upgrade_to_stale_version() {
         var versions = new OsVersions(tester.nodeRepository(), Integer.MAX_VALUE);
         tester.makeReadyNodes(10, "default", NodeType.host);
+        tester.prepareAndActivateInfraApplication(infraApplication, NodeType.host);
         Supplier<NodeList> hostNodes = () -> tester.nodeRepository().list().nodeType(NodeType.host);
 
         // Some nodes are targeting an older version