Merge pull request #5867 from vespa-engine/mpolden/upgrade-host-applications

Upgrade host applications

6 files changed, 131 insertions, 38 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index db749713483..8b0dc35e16b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -341,16 +341,16 @@ public class ApplicationController {
 
     /** Deploy a system application to given zone */
     public void deploy(SystemApplication application, ZoneId zone, Version version) {
-        if (!application.hasApplicationPackage()) {
+        if (application.hasApplicationPackage()) {
+            ApplicationPackage applicationPackage = new ApplicationPackage(
+                    artifactRepository.getSystemApplicationPackage(application.id(), zone, version)
+            );
+            DeployOptions options = withVersion(version, DeployOptions.none());
+            deploy(application.id(), applicationPackage, zone, options, Collections.emptySet(), Collections.emptySet());
+        } else {
             // Deploy by calling node repository directly
             configServer().nodeRepository().upgrade(zone, application.nodeType(), version);
-            return;
         }
-        ApplicationPackage applicationPackage = new ApplicationPackage(
-                artifactRepository.getSystemApplicationPackage(application.id(), zone, version)
-        );
-        DeployOptions options = withVersion(version, DeployOptions.none());
-        deploy(application.id(), applicationPackage, zone, options, Collections.emptySet(), Collections.emptySet());
     }
 
     private ActivateResult deploy(ApplicationId application, ApplicationPackage applicationPackage,
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/SystemApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/SystemApplication.java
index d3daa68741e..70814ba6508 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/SystemApplication.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/SystemApplication.java
@@ -12,17 +12,23 @@ import java.util.List;
  *
  * @author mpolden
  */
-public enum  SystemApplication {
+public enum SystemApplication {
 
+    // Note that the enum declaration order decides the upgrade order
+    configServerHost(ApplicationId.from("hosted-vespa", "configserver-host", "default"), NodeType.confighost),
+    proxyHost(ApplicationId.from("hosted-vespa", "proxy-host", "default"), NodeType.proxyhost),
     configServer(ApplicationId.from("hosted-vespa", "zone-config-servers", "default"), NodeType.config),
-    zone(ApplicationId.from("hosted-vespa", "routing", "default"), NodeType.proxy);
+    zone(ApplicationId.from("hosted-vespa", "routing", "default"), NodeType.proxy,
+         configServerHost, proxyHost, configServer);
 
     private final ApplicationId id;
     private final NodeType nodeType;
+    private final List<SystemApplication> prerequisites;
 
-    SystemApplication(ApplicationId id, NodeType nodeType) {
+    SystemApplication(ApplicationId id, NodeType nodeType, SystemApplication... prerequisites) {
         this.id = id;
         this.nodeType = nodeType;
+        this.prerequisites = Arrays.asList(prerequisites);
     }
 
     public ApplicationId id() {
@@ -34,7 +40,10 @@ public enum  SystemApplication {
         return nodeType;
     }
 
-    /** Returns whether this system application has its own application package */
+    /** Returns the system applications that should upgrade before this */
+    public List<SystemApplication> prerequisites() { return prerequisites; }
+
+    /** Returns whether this system application has an application package */
     public boolean hasApplicationPackage() {
         return nodeType == NodeType.proxy;
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 75c01dcb9b3..e902206ad8b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -97,7 +97,7 @@ public class DeploymentTrigger {
      * trigger next.
      */
     public void notifyOfCompletion(JobReport report) {
-        log.log(LogLevel.INFO, String.format("Notified of %s for %s of %s (%d).",
+        log.log(LogLevel.INFO, String.format("Notified of %s for %s of %s (%d)",
                                              report.jobError().map(e -> e.toString() + " error")
                                                    .orElse("success"),
                                              report.jobType(),
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 d7fc71e4b08..7cb967e0c82 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
@@ -11,6 +11,7 @@ import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 import com.yahoo.yolean.Exceptions;
 
 import java.time.Duration;
+import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
@@ -37,51 +38,62 @@ public class SystemUpgrader extends Maintainer {
         if (!target.isPresent()) {
             return;
         }
+        // TODO: Change to SystemApplication.all() once host applications support upgrade
+        try {
+            deploy(Arrays.asList(SystemApplication.configServer, SystemApplication.zone), target.get());
+        } catch (Exception e) {
+            log.log(Level.WARNING, "Failed to upgrade system. Retrying in " + maintenanceInterval(), e);
+        }
+    }
+
+    /** Deploy a list of system applications until they converge on the given version */
+    private void deploy(List<SystemApplication> applications, Version target) {
         for (List<ZoneId> zones : controller().zoneRegistry().upgradePolicy().asList()) {
-            // The order here is important. Config servers should always upgrade first
-            if (!deploy(zones, SystemApplication.configServer, target.get())) {
-                break;
-            }
-            if (!deploy(zones, SystemApplication.zone, target.get())) {
-                break;
+            boolean converged = true;
+            for (SystemApplication application : applications) {
+                if (application.prerequisites().stream().allMatch(prerequisite -> converged(zones, prerequisite, target))) {
+                    deploy(zones, application, target);
+                }
+                converged &= converged(zones, application, target);
             }
+            if (!converged) break;
         }
     }
 
     /** Deploy application on given version. Returns true when all allocated nodes are on requested version */
-    private boolean deploy(List<ZoneId> zones, SystemApplication application, Version version) {
-        boolean completed = true;
+    private void deploy(List<ZoneId> zones, SystemApplication application, Version target) {
         for (ZoneId zone : zones) {
-            if (!wantedVersion(zone, application.id()).equals(version)) {
-                log.info(String.format("Deploying %s version %s in %s", application.id(), version, zone));
-                controller().applications().deploy(application, zone, version);
+            if (!wantedVersion(zone, application.id(), target).equals(target)) {
+                log.info(String.format("Deploying %s version %s in %s", application.id(), target, zone));
+                controller().applications().deploy(application, zone, target);
             }
-            completed = completed && currentVersion(zone, application.id()).equals(version);
         }
-        return completed;
     }
 
-    private Version wantedVersion(ZoneId zone, ApplicationId application) {
-        return minVersion(zone, application, Node::wantedVersion);
+    private boolean converged(List<ZoneId> zones, SystemApplication application, Version target) {
+        return zones.stream().allMatch(zone -> currentVersion(zone, application.id(), target).equals(target));
+    }
+
+    private Version wantedVersion(ZoneId zone, ApplicationId application, Version defaultVersion) {
+        return minVersion(zone, application, Node::wantedVersion).orElse(defaultVersion);
     }
 
-    private Version currentVersion(ZoneId zone, ApplicationId application) {
-        return minVersion(zone, application, Node::currentVersion);
+    private Version currentVersion(ZoneId zone, ApplicationId application, Version defaultVersion) {
+        return minVersion(zone, application, Node::currentVersion).orElse(defaultVersion);
     }
 
-    private Version minVersion(ZoneId zone, ApplicationId application, Function<Node, Version> versionField) {
+    private Optional<Version> minVersion(ZoneId zone, ApplicationId application, Function<Node, Version> versionField) {
         try {
             return controller().configServer()
                                .nodeRepository()
                                .listOperational(zone, application)
                                .stream()
                                .map(versionField)
-                               .min(Comparator.naturalOrder())
-                               .orElse(Version.emptyVersion);
+                               .min(Comparator.naturalOrder());
         } catch (Exception e) {
             log.log(Level.WARNING, String.format("Failed to get version for %s in %s: %s", application, zone,
                                                  Exceptions.toMessageString(e)));
-            return Version.emptyVersion;
+            return Optional.empty();
         }
     }
 
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 e9eec0682f3..99b913709df 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
@@ -54,13 +54,17 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
 
     @Inject
     public ConfigServerMock(ZoneRegistryMock zoneRegistry) {
-        bootstrap(zoneRegistry.zones().all().ids());
+        bootstrap(zoneRegistry.zones().all().ids(), SystemApplication.all());
     }
 
-    public void bootstrap(List<ZoneId> zones) {
+    public void bootstrap(List<ZoneId> zones, SystemApplication... applications) {
+        bootstrap(zones, Arrays.asList(applications));
+    }
+
+    public void bootstrap(List<ZoneId> zones, List<SystemApplication> applications) {
         nodeRepository().clear();
         for (ZoneId zone : zones) {
-            for (SystemApplication application : SystemApplication.all()) {
+            for (SystemApplication application : applications) {
                 List<Node> nodes = IntStream.rangeClosed(1, 3)
                                             .mapToObj(i -> new Node(
                                                     HostName.from("node-" + i + "-" + application.id().application()
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 5790ab2a0c8..3fbda16cb4a 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
@@ -9,6 +9,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.zone.UpgradePolicy;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
+import org.junit.Ignore;
 import org.junit.Test;
 
 import java.util.Arrays;
@@ -16,6 +17,7 @@ import java.util.List;
 import java.util.function.Function;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author mpolden
@@ -39,7 +41,9 @@ public class SystemUpgraderTest {
         );
 
         Version version1 = Version.fromString("6.5");
-        tester.configServer().bootstrap(Arrays.asList(zone1, zone2, zone3, zone4));
+        // Bootstrap a system without host applications
+        tester.configServer().bootstrap(Arrays.asList(zone1, zone2, zone3, zone4), SystemApplication.configServer,
+                                        SystemApplication.zone);
         // Fail a few nodes. Failed nodes should not affect versions
         failNodeIn(zone1, SystemApplication.configServer);
         failNodeIn(zone3, SystemApplication.zone);
@@ -68,6 +72,8 @@ public class SystemUpgraderTest {
         tester.systemUpgrader().maintain();
         assertWantedVersion(SystemApplication.zone, version2, zone1);
         completeUpgrade(SystemApplication.zone, version2, zone1);
+        assertTrue("Deployed zone application",
+                   tester.configServer().application(SystemApplication.zone.id()).isPresent());
 
         // zone 2, 3 and 4: still targets old version
         assertWantedVersion(SystemApplication.configServer, version1, zone2, zone3, zone4);
@@ -109,6 +115,55 @@ public class SystemUpgraderTest {
     }
 
     @Test
+    @Ignore // TODO: Unignore once host applications support upgrade
+    public void upgrade_system_containing_host_applications() {
+        tester.controllerTester().zoneRegistry().setUpgradePolicy(
+                UpgradePolicy.create()
+                             .upgrade(zone1)
+                             .upgradeInParallel(zone2, zone3)
+                             .upgrade(zone4)
+        );
+
+        Version version1 = Version.fromString("6.5");
+        tester.configServer().bootstrap(Arrays.asList(zone1, zone2, zone3, zone4), SystemApplication.all());
+        tester.upgradeSystem(version1);
+        tester.systemUpgrader().maintain();
+        assertCurrentVersion(SystemApplication.all(), version1, zone1, zone2, zone3, zone4);
+
+        // Controller upgrades
+        Version version2 = Version.fromString("6.6");
+        tester.upgradeController(version2);
+        assertEquals(version2, tester.controller().versionStatus().controllerVersion().get().versionNumber());
+
+        // System upgrades in zone 1:
+        tester.systemUpgrader().maintain();
+        List<SystemApplication> allExceptZone = Arrays.asList(SystemApplication.configServerHost,
+                                                              SystemApplication.proxyHost,
+                                                              SystemApplication.configServer);
+        completeUpgrade(allExceptZone, version2, zone1);
+        tester.systemUpgrader().maintain();
+        completeUpgrade(SystemApplication.zone, version2, zone1);
+        assertWantedVersion(SystemApplication.all(), version1, zone2, zone3, zone4);
+
+        // zone 2 and 3:
+        tester.systemUpgrader().maintain();
+        completeUpgrade(allExceptZone, version2, zone2, zone3);
+        tester.systemUpgrader().maintain();
+        completeUpgrade(SystemApplication.zone, version2, zone2, zone3);
+        assertWantedVersion(SystemApplication.all(), version1, zone4);
+
+        // zone 4:
+        tester.systemUpgrader().maintain();
+        completeUpgrade(allExceptZone, version2, zone4);
+        tester.systemUpgrader().maintain();
+        completeUpgrade(SystemApplication.zone, version2, zone4);
+
+        // All done
+        tester.systemUpgrader().maintain();
+        assertWantedVersion(SystemApplication.all(), version2, zone1, zone2, zone3, zone4);
+    }
+
+    @Test
     public void never_downgrades_system() {
         ZoneId zone = ZoneId.from("prod", "eu-west-1");
         tester.controllerTester().zoneRegistry().setUpgradePolicy(UpgradePolicy.create().upgrade(zone));
@@ -130,6 +185,7 @@ 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) {
+        assertWantedVersion(application, version, zones);
         for (ZoneId zone : zones) {
             for (Node node : nodeRepository().listOperational(zone, application.id())) {
                 nodeRepository().add(zone, new Node(node.hostname(), node.state(), node.type(), node.owner(),
@@ -139,6 +195,10 @@ public class SystemUpgraderTest {
         }
     }
 
+    private void completeUpgrade(List<SystemApplication> applications, Version version, ZoneId... zones) {
+        applications.forEach(application -> completeUpgrade(application, version, zones));
+    }
+
     private void failNodeIn(ZoneId zone, SystemApplication application) {
         List<Node> nodes = nodeRepository().list(zone, application.id());
         if (nodes.isEmpty()) {
@@ -157,11 +217,19 @@ public class SystemUpgraderTest {
         assertVersion(application.id(), version, Node::currentVersion, zones);
     }
 
+    private void assertWantedVersion(List<SystemApplication> applications, Version version, ZoneId... zones) {
+        applications.forEach(application -> assertVersion(application.id(), version, Node::wantedVersion, zones));
+    }
+
+    private void assertCurrentVersion(List<SystemApplication> applications, Version version, ZoneId... zones) {
+        applications.forEach(application -> assertVersion(application.id(), version, Node::currentVersion, zones));
+    }
+
     private void assertVersion(ApplicationId application, Version version, Function<Node, Version> versionField,
                                ZoneId... zones) {
         for (ZoneId zone : zones) {
             for (Node node : nodeRepository().listOperational(zone, application)) {
-                assertEquals(version, versionField.apply(node));
+                assertEquals(application + " version", version, versionField.apply(node));
             }
         }
     }