Only consider operational nodes for upgrade

6 files changed, 29 insertions, 14 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgrader.java
index 012f9f0ff57..d7fc71e4b08 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgrader.java
@@ -73,7 +73,7 @@ public class SystemUpgrader extends Maintainer {
         try {
             return controller().configServer()
                                .nodeRepository()
-                               .list(zone, application)
+                               .listOperational(zone, application)
                                .stream()
                                .map(versionField)
                                .min(Comparator.naturalOrder())
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
index 880ca98d252..696dc62f979 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
@@ -129,7 +129,7 @@ public class VersionStatus {
         ListMap<Version, HostName> versions = new ListMap<>();
         for (ZoneId zone : zones) {
             for (SystemApplication application : SystemApplication.all()) {
-                for (Node node : controller.configServer().nodeRepository().list(zone, application.id())) {
+                for (Node node : controller.configServer().nodeRepository().listOperational(zone, application.id())) {
                     versions.put(node.currentVersion(), node.hostname());
                 }
             }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerMock.java
index 9fb076880af..e9eec0682f3 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerMock.java
@@ -65,7 +65,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
                                             .mapToObj(i -> new Node(
                                                     HostName.from("node-" + i + "-" + application.id().application()
                                                                                                  .value()),
-                                                    application.nodeType(),
+                                                    Node.State.active, application.nodeType(),
                                                     Optional.of(application.id()),
                                                     initialVersion,
                                                     initialVersion
@@ -98,7 +98,8 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     public void setVersion(Version version, ZoneId zone, List<SystemApplication> applications) {
         for (SystemApplication application : applications) {
             for (Node node : nodeRepository().list(zone, application.id())) {
-                nodeRepository().add(zone, new Node(node.hostname(), node.type(), node.owner(), version, version));
+                nodeRepository().add(zone, new Node(node.hostname(), node.state(), node.type(), node.owner(),
+                                                    version, version));
             }
         }
     }
@@ -155,7 +156,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
                 application.activate();
                 for (Node node : nodeRepository.list(deployment.zoneId(), deployment.applicationId())) {
                     nodeRepository.add(deployment.zoneId(), new Node(node.hostname(),
-                                                                     node.type(),
+                                                                     node.state(), node.type(),
                                                                      node.owner(),
                                                                      node.currentVersion(),
                                                                      application.version().get()));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/NodeRepositoryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/NodeRepositoryMock.java
index 1c42fb3501c..a720b0efa82 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/NodeRepositoryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/NodeRepositoryMock.java
@@ -54,8 +54,8 @@ public class NodeRepositoryMock implements NodeRepository {
         nodeRepository.getOrDefault(zone, Collections.emptyMap()).values()
                       .stream()
                       .filter(node -> node.type() == type)
-                      .map(node -> new Node(node.hostname(), node.type(), node.owner(), node.currentVersion(),
-                                            version))
+                      .map(node -> new Node(node.hostname(), node.state(), node.type(), node.owner(),
+                                            node.currentVersion(), version))
                       .forEach(node -> add(zone, node));
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgraderTest.java
index 991448e8fca..5790ab2a0c8 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgraderTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemUpgraderTest.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.List;
 import java.util.function.Function;
 
 import static org.junit.Assert.assertEquals;
@@ -39,6 +40,9 @@ public class SystemUpgraderTest {
 
         Version version1 = Version.fromString("6.5");
         tester.configServer().bootstrap(Arrays.asList(zone1, zone2, zone3, zone4));
+        // Fail a few nodes. Failed nodes should not affect versions
+        failNodeIn(zone1, SystemApplication.configServer);
+        failNodeIn(zone3, SystemApplication.zone);
         tester.upgradeSystem(version1);
         tester.systemUpgrader().maintain();
         assertCurrentVersion(SystemApplication.configServer, version1, zone1, zone2, zone3, zone4);
@@ -127,14 +131,24 @@ public class SystemUpgraderTest {
     /** Simulate upgrade of nodes allocated to given application. In a real system this is done by the node itself */
     private void completeUpgrade(SystemApplication application, Version version, ZoneId... zones) {
         for (ZoneId zone : zones) {
-            for (Node node : nodeRepository().list(zone, application.id())) {
-                nodeRepository().add(zone, new Node(node.hostname(), node.type(), node.owner(), node.wantedVersion(),
-                                                    node.wantedVersion()));
+            for (Node node : nodeRepository().listOperational(zone, application.id())) {
+                nodeRepository().add(zone, new Node(node.hostname(), node.state(), node.type(), node.owner(),
+                                                    node.wantedVersion(), node.wantedVersion()));
             }
             assertCurrentVersion(application, version, zone);
         }
     }
 
+    private void failNodeIn(ZoneId zone, SystemApplication application) {
+        List<Node> nodes = nodeRepository().list(zone, application.id());
+        if (nodes.isEmpty()) {
+            throw new IllegalArgumentException("No nodes allocated to " + application.id());
+        }
+        Node node = nodes.get(0);
+        nodeRepository().add(zone, new Node(node.hostname(), Node.State.failed, node.type(), node.owner(),
+                                            node.currentVersion(), node.wantedVersion()));
+    }
+
     private void assertWantedVersion(SystemApplication application, Version version, ZoneId... zones) {
         assertVersion(application.id(), version, Node::wantedVersion, zones);
     }
@@ -143,10 +157,10 @@ public class SystemUpgraderTest {
         assertVersion(application.id(), version, Node::currentVersion, zones);
     }
 
-    private void assertVersion(ApplicationId applicationId, Version version, Function<Node, Version> versionField,
+    private void assertVersion(ApplicationId application, Version version, Function<Node, Version> versionField,
                                ZoneId... zones) {
         for (ZoneId zone : zones) {
-            for (Node node : nodeRepository().list(zone, applicationId)) {
+            for (Node node : nodeRepository().listOperational(zone, application)) {
                 assertEquals(version, versionField.apply(node));
             }
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
index 9450b6223b1..57676ffc3f5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java
@@ -61,8 +61,8 @@ public class VersionStatusTest {
         Version oldest = new Version(5);
         for (ZoneId zone : tester.zoneRegistry().zones().all().ids()) {
             for (Node node : tester.configServer().nodeRepository().list(zone, SystemApplication.configServer.id())) {
-                tester.configServer().nodeRepository().add(zone, new Node(node.hostname(), node.type(), node.owner(),
-                                                                          oldest, node.wantedVersion()));
+                tester.configServer().nodeRepository().add(zone, new Node(node.hostname(), node.state(), node.type(),
+                                                                          node.owner(), oldest, node.wantedVersion()));
                 break;
             }
         }