Merge pull request #11757 from vespa-engine/mpolden/system-upgrader-target

Avoid checking every node when setting upgrade target

11 files changed, 126 insertions, 86 deletions

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 5ff564f7ad3..66015c76b06 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
@@ -64,7 +64,7 @@ public enum SystemApplication {
     }
 
     /** Returns whether this should receive OS upgrades */
-    public boolean isEligibleForOsUpgrades() {
+    public boolean shouldUpgradeOs() {
         return nodeType.isDockerHost();
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InfrastructureUpgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InfrastructureUpgrader.java
index 0dd6f0782bf..0a890834c6e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InfrastructureUpgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InfrastructureUpgrader.java
@@ -49,10 +49,12 @@ public abstract class InfrastructureUpgrader extends Maintainer {
                     converged &= upgradeAll(target, applications, zone);
                 } catch (UnreachableNodeRepositoryException e) {
                     converged = false;
-                    log.warning(String.format("%s: Failed to communicate with node repository in %s, continuing with next parallel zone: %s", this, zone, Exceptions.toMessageString(e)));
+                    log.warning(String.format("%s: Failed to communicate with node repository in %s, continuing with next parallel zone: %s",
+                                              this, zone, Exceptions.toMessageString(e)));
                 } catch (Exception e) {
                     converged = false;
-                    log.warning(String.format("%s: Failed to upgrade zone: %s, continuing with next parallel zone: %s", this, zone, Exceptions.toMessageString(e)));
+                    log.warning(String.format("%s: Failed to upgrade zone: %s, continuing with next parallel zone: %s",
+                                              this, zone, Exceptions.toMessageString(e)));
                 }
             }
             if (!converged) {
@@ -66,13 +68,10 @@ public abstract class InfrastructureUpgrader extends Maintainer {
         boolean converged = true;
         for (SystemApplication application : applications) {
             if (convergedOn(target, application.dependencies(), zone)) {
-                boolean currentAppConverged = convergedOn(target, application, zone);
-                // In dynamically provisioned zones there may be no tenant hosts at the time of upgrade, so we
-                // should always set the target version.
-                if (application == SystemApplication.tenantHost || !currentAppConverged) {
+                if (shouldUpgrade(target, application, zone)) {
                     upgrade(target, application, zone);
                 }
-                converged &= currentAppConverged;
+                converged &= convergedOn(target, application, zone);
             }
         }
         return converged;
@@ -82,6 +81,9 @@ public abstract class InfrastructureUpgrader extends Maintainer {
         return applications.stream().allMatch(application -> convergedOn(target, application, zone));
     }
 
+    /** Returns whether application in zone should be told to upgrade to given target */
+    protected abstract boolean shouldUpgrade(Version target, SystemApplication application, ZoneApi zone);
+
     /** Upgrade component to target version. Implementation should be idempotent */
     protected abstract void upgrade(Version target, SystemApplication application, ZoneApi zone);
 
@@ -94,7 +96,7 @@ public abstract class InfrastructureUpgrader extends Maintainer {
     /** Returns whether the upgrader should require given node to upgrade */
     protected abstract boolean requireUpgradeOf(Node node, SystemApplication application, ZoneApi zone);
 
-    /** Find the minimum value of a version field in a zone */
+    /** Find the minimum value of a version field in a zone by comparing all nodes */
     protected final Optional<Version> minVersion(ZoneApi zone, SystemApplication application, Function<Node, Version> versionField) {
         try {
             return controller().serviceRegistry().configServer()
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java
index 21fd0ec87d4..9c3c1dc1f5e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java
@@ -1,7 +1,6 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.maintenance;
 
-import com.google.common.collect.ImmutableSet;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.SystemName;
@@ -34,13 +33,15 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
     private final JobControl jobControl;
     private final ScheduledExecutorService service;
     private final String name;
-    private final Set<SystemName> permittedSystems;
+
+    /** The systems in which this maintainer should run */
+    private final Set<SystemName> activeSystems;
 
     public Maintainer(Controller controller, Duration interval, JobControl jobControl) {
         this(controller, interval, jobControl, null, EnumSet.allOf(SystemName.class));
     }
 
-    public Maintainer(Controller controller, Duration interval, JobControl jobControl, String name, Set<SystemName> permittedSystems) {
+    public Maintainer(Controller controller, Duration interval, JobControl jobControl, String name, Set<SystemName> activeSystems) {
         if (interval.isNegative() || interval.isZero())
             throw new IllegalArgumentException("Interval must be positive, but was " + interval);
 
@@ -48,7 +49,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
         this.maintenanceInterval = interval;
         this.jobControl = jobControl;
         this.name = name;
-        this.permittedSystems = ImmutableSet.copyOf(permittedSystems);
+        this.activeSystems = Set.copyOf(activeSystems);
 
         service = new ScheduledThreadPoolExecutor(1);
         long delay = staggeredDelay(controller.curator().cluster(), controller.hostname(), controller.clock().instant(), interval);
@@ -61,7 +62,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
     @Override
     public void run() {
         try {
-            if ( ! permittedSystems.contains(controller.system())) {
+            if ( ! activeSystems.contains(controller.system())) {
                 return;
             }
             if (jobControl.isActive(name())) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
index 93d1dac7382..dcb93937eb3 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
@@ -38,10 +38,6 @@ public class OsUpgrader extends InfrastructureUpgrader {
 
     @Override
     protected void upgrade(Version target, SystemApplication application, ZoneApi zone) {
-        if (!application.isEligibleForOsUpgrades()) return;
-        var existingTarget = targetVersion(zone, application);
-        if (existingTarget.isPresent() && existingTarget.get().equals(target)) return;
-
         log.info(String.format("Upgrading OS of %s to version %s in %s in cloud %s", application.id(), target, zone.getId(), zone.getCloudName()));
         controller().serviceRegistry().configServer().nodeRepository().upgradeOs(zone.getId(), application.nodeType(), target);
     }
@@ -65,20 +61,24 @@ public class OsUpgrader extends InfrastructureUpgrader {
                            .map(OsVersion::version);
     }
 
-    private Version currentVersion(ZoneApi zone, SystemApplication application, Version defaultVersion) {
-        return minVersion(zone, application, Node::currentOsVersion).orElse(defaultVersion);
-    }
-
-    private Optional<Version> targetVersion(ZoneApi zone, SystemApplication application) {
+    @Override
+    protected boolean shouldUpgrade(Version target, SystemApplication application, ZoneApi zone) {
+        if (!application.shouldUpgradeOs()) return false;                      // Never upgrade
         return controller().serviceRegistry().configServer().nodeRepository()
                            .targetVersionsOf(zone.getId())
-                           .osVersion(application.nodeType());
+                           .osVersion(application.nodeType())
+                           .map(target::isAfter)                               // Upgrade if target is after current
+                           .orElse(true);                                      // Upgrade if target is unset
+    }
+
+    private Version currentVersion(ZoneApi zone, SystemApplication application, Version defaultVersion) {
+        return minVersion(zone, application, Node::currentOsVersion).orElse(defaultVersion);
     }
 
     /** Returns whether node in application should be upgraded by this */
     public static boolean eligibleForUpgrade(Node node, SystemApplication application) {
         return upgradableNodeStates.contains(node.state()) &&
-               application.isEligibleForOsUpgrades();
+               application.shouldUpgradeOs();
     }
 
     private static String name(CloudName cloud) {
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 c43278f95b4..72c5639735d 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
@@ -30,12 +30,8 @@ public class SystemUpgrader extends InfrastructureUpgrader {
 
     @Override
     protected void upgrade(Version target, SystemApplication application, ZoneApi zone) {
-        // TODO(mpolden): Simplify this by comparing with version from NodeRepository#targetVersionsOf instead
-        if (minVersion(zone, application, Node::wantedVersion).map(target::isAfter)
-                                                              .orElse(true)) {
-            log.info(String.format("Deploying %s version %s in %s", application.id(), target, zone.getId()));
-            controller().applications().deploy(application, zone.getId(), target);
-        }
+        log.info(String.format("Deploying %s version %s in %s", application.id(), target, zone.getId()));
+        controller().applications().deploy(application, zone.getId(), target);
     }
 
     @Override
@@ -44,8 +40,8 @@ public class SystemUpgrader extends InfrastructureUpgrader {
         // Skip application convergence check if there are no nodes belonging to the application in the zone
         if (minVersion.isEmpty()) return true;
 
-        return     minVersion.get().equals(target)
-                && application.configConvergedIn(zone.getId(), controller(), Optional.of(target));
+        return minVersion.get().equals(target) &&
+               application.configConvergedIn(zone.getId(), controller(), Optional.of(target));
     }
 
     @Override
@@ -61,6 +57,28 @@ public class SystemUpgrader extends InfrastructureUpgrader {
                            .map(VespaVersion::versionNumber);
     }
 
+    @Override
+    protected boolean shouldUpgrade(Version target, SystemApplication application, ZoneApi zone) {
+        if (application.hasApplicationPackage()) {
+            // For applications with package we do not have a zone-wide version target. This means that we must check
+            // the wanted version of each node.
+            return minVersion(zone, application, Node::wantedVersion)
+                    // Upgrade if target is after any wanted version
+                    .map(target::isAfter)
+                    // Skip upgrade if there are no nodes allocated. This is overloaded to mean that the zone is not
+                    // expected to have a deployment of this application.
+                    // TODO(mpolden): Once all zones are either directly routed or not: Change this to
+                    //                always deploy proxy app and wait for convergence in zones that are not directly
+                    //                routed.
+                    .orElse(false);
+        }
+        return controller().serviceRegistry().configServer().nodeRepository()
+                           .targetVersionsOf(zone.getId())
+                           .vespaVersion(application.nodeType())
+                           .map(target::isAfter)                              // Upgrade if target is after current
+                           .orElse(true);                                     // Upgrade if target is unset
+    }
+
     /** Returns whether node in application should be upgraded by this */
     public static boolean eligibleForUpgrade(Node node) {
         return upgradableNodeStates.contains(node.state());
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
index d5e83d99cdd..1773a9c122e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
@@ -65,7 +65,7 @@ public class OsVersionStatus {
         controller.osVersions().forEach(osVersion -> osVersions.put(osVersion, new ArrayList<>()));
 
         for (var application : SystemApplication.all()) {
-            if (!application.isEligibleForOsUpgrades()) continue;
+            if (!application.shouldUpgradeOs()) continue;
             for (var zone : zonesToUpgrade(controller)) {
                 var targetOsVersion = controller.serviceRegistry().configServer().nodeRepository()
                                                 .targetVersionsOf(zone.getId())
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
index 82d35701e7e..84bdedba33c 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
@@ -234,6 +234,9 @@ public final class ControllerTester {
     public void upgradeSystemApplications(Version version, List<SystemApplication> systemApplications) {
         for (ZoneApi zone : zoneRegistry().zones().all().zones()) {
             for (SystemApplication application : systemApplications) {
+                if (!application.hasApplicationPackage()) {
+                    configServer().nodeRepository().upgrade(zone.getId(), application.nodeType(), version);
+                }
                 configServer().setVersion(application.id(), zone.getId(), version);
                 configServer().convergeServices(application.id(), zone.getId());
             }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
index 23630484dd6..f99396b3b02 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
@@ -145,7 +145,14 @@ public class NodeRepositoryMock implements NodeRepository {
 
     @Override
     public void upgrade(ZoneId zone, NodeType type, Version version) {
-        nodeRepository.getOrDefault(zone, Collections.emptyMap()).values()
+        this.targetVersions.compute(zone, (ignored, targetVersions) -> {
+            if (targetVersions == null) {
+                targetVersions = TargetVersions.EMPTY;
+            }
+            return targetVersions.withVespaVersion(type, version);
+        });
+        // Bump wanted version of each node. This is done by InfrastructureProvisioner in a real node repository.
+        nodeRepository.getOrDefault(zone, Map.of()).values()
                       .stream()
                       .filter(node -> node.type() == type)
                       .map(node -> new Node.Builder(node).wantedVersion(version).build())
@@ -160,6 +167,12 @@ public class NodeRepositoryMock implements NodeRepository {
             }
             return targetVersions.withOsVersion(type, version);
         });
+        // Bump wanted version of each node. This is done by OsUpgradeActivator in a real node repository.
+        nodeRepository.getOrDefault(zone, Map.of()).values()
+                      .stream()
+                      .filter(node -> node.type() == type)
+                      .map(node -> new Node.Builder(node).wantedOsVersion(version).build())
+                      .forEach(node -> putByHostname(zone, node));
     }
 
     @Override
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
index 99eb4f049b6..e0fb2aa0ee1 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
@@ -9,6 +9,7 @@ import com.yahoo.config.provision.zone.UpgradePolicy;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
+import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
@@ -19,6 +20,7 @@ import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.util.Comparator;
 import java.util.List;
 
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.productionUsWest1;
@@ -250,6 +252,9 @@ public class MetricsReporterTest {
             assertEquals(0, getNodesFailingUpgrade());
 
             // 1/3 nodes upgrade within timeout
+            assertEquals("Wanted version is raised for all nodes", version,
+                         tester.configServer().nodeRepository().list(zone1.getId(), SystemApplication.configServer.id()).stream()
+                               .map(Node::wantedVersion).min(Comparator.naturalOrder()).get());
             tester.configServer().setVersion(SystemApplication.configServer.id(), zone1.getId(), version, 1);
             tester.clock().advance(Duration.ofMinutes(30).plus(Duration.ofSeconds(1)));
             tester.computeVersionStatus();
@@ -303,6 +308,9 @@ public class MetricsReporterTest {
             assertEquals(0, getNodesFailingOsUpgrade());
 
             // 2/6 nodes upgrade within timeout
+            assertEquals("Wanted OS version is raised for all nodes", version,
+                         tester.configServer().nodeRepository().list(zone.getId(), SystemApplication.tenantHost.id()).stream()
+                               .map(Node::wantedOsVersion).min(Comparator.naturalOrder()).get());
             tester.configServer().setOsVersion(SystemApplication.tenantHost.id(), zone.getId(), version, 2);
             tester.clock().advance(Duration.ofMinutes(30 * 3 /* time allowance * node count */).plus(Duration.ofSeconds(1)));
             statusUpdater.maintain();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
index cf3d978ef62..9d910afd476 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
@@ -13,7 +13,6 @@ import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.integration.NodeRepositoryMock;
 import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock;
 import com.yahoo.vespa.hosted.controller.versions.NodeVersion;
-import org.junit.Before;
 import org.junit.Test;
 
 import java.time.Duration;
@@ -35,16 +34,9 @@ public class OsUpgraderTest {
     private static final ZoneApi zone4 = ZoneApiMock.newBuilder().withId("prod.us-east-3").build();
     private static final ZoneApi zone5 = ZoneApiMock.newBuilder().withId("prod.us-north-1").withCloud("other").build();
 
-    private ControllerTester tester;
-    private OsVersionStatusUpdater statusUpdater;
-
-    @Before
-    public void before() {
-        tester = new ControllerTester();
-        statusUpdater = new OsVersionStatusUpdater(tester.controller(), Duration.ofDays(1),
-                                                   new JobControl(tester.controller().curator()));
-    }
-
+    private final ControllerTester tester = new ControllerTester();
+    private final OsVersionStatusUpdater statusUpdater = new OsVersionStatusUpdater(tester.controller(), Duration.ofDays(1),
+                                                                                    new JobControl(tester.controller().curator()));
     @Test
     public void upgrade_os() {
         OsUpgrader osUpgrader = osUpgrader(
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 08dbd7e48db..140c37cdd94 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
@@ -10,13 +10,13 @@ import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.integration.NodeRepositoryMock;
 import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
-import org.junit.Before;
 import org.junit.Test;
 
 import java.time.Duration;
 import java.util.List;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
@@ -31,12 +31,7 @@ public class SystemUpgraderTest {
     private static final ZoneApi zone3 = ZoneApiMock.fromId("prod.us-central-1");
     private static final ZoneApi zone4 = ZoneApiMock.fromId("prod.us-east-3");
 
-    private ControllerTester tester;
-
-    @Before
-    public void before() {
-        tester = new ControllerTester();
-    }
+    private final ControllerTester tester = new ControllerTester();
 
     @Test
     public void upgrade_system() {
@@ -281,41 +276,48 @@ public class SystemUpgraderTest {
 
     @Test
     public void does_not_deploy_proxy_app_in_zones_without_proxy() {
-        List<SystemApplication> applications = List.of(
-                SystemApplication.configServerHost, SystemApplication.configServer, SystemApplication.tenantHost);
+        var applications = List.of(SystemApplication.configServerHost, SystemApplication.configServer,
+                                   SystemApplication.tenantHost);
         tester.configServer().bootstrap(List.of(zone1.getId()), applications);
         tester.configServer().disallowConvergenceCheck(SystemApplication.proxy.id());
+        var systemUpgrader = systemUpgrader(UpgradePolicy.create().upgrade(zone1));
 
-        SystemUpgrader systemUpgrader = systemUpgrader(UpgradePolicy.create().upgrade(zone1));
+        // System begins upgrade
+        var version1 = Version.fromString("6.5");
+        tester.upgradeController(version1);
+        systemUpgrader.maintain();
+        assertWantedVersion(applications, version1, zone1);
+        assertWantedVersion(SystemApplication.proxy, Version.emptyVersion, zone1);
 
-        Version version1 = Version.fromString("6.5");
-        tester.upgradeSystem(version1);
+        // System completes upgrade. Wanted version is not raised for proxy as it's is never deployed
+        completeUpgrade(applications, version1, zone1);
         systemUpgrader.maintain();
-        assertCurrentVersion(applications, version1, zone1);
+        assertWantedVersion(SystemApplication.proxy, Version.emptyVersion, zone1);
+        tester.computeVersionStatus();
+        assertEquals(version1, tester.controller().systemVersion());
     }
 
     /** 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, ZoneApi... zones) {
-        assertWantedVersion(application, version, zones);
-        for (ZoneApi zone : zones) {
+    private void completeUpgrade(SystemApplication application, Version version, ZoneApi first, ZoneApi... rest) {
+        assertWantedVersion(application, version, first, rest);
+        Stream.concat(Stream.of(first), Stream.of(rest)).forEach(zone -> {
             for (Node node : listNodes(zone, application)) {
                 nodeRepository().putByHostname(
                         zone.getId(),
                         new Node.Builder(node).currentVersion(node.wantedVersion()).build());
             }
-
             assertCurrentVersion(application, version, zone);
-        }
+        });
     }
 
-    private void convergeServices(SystemApplication application, ZoneApi... zones) {
-        for (ZoneApi zone : zones) {
+    private void convergeServices(SystemApplication application, ZoneApi first, ZoneApi... rest) {
+        Stream.concat(Stream.of(first), Stream.of(rest)).forEach(zone -> {
             tester.configServer().convergeServices(application.id(), zone.getId());
-        }
+        });
     }
 
-    private void completeUpgrade(List<SystemApplication> applications, Version version, ZoneApi... zones) {
-        applications.forEach(application -> completeUpgrade(application, version, zones));
+    private void completeUpgrade(List<SystemApplication> applications, Version version, ZoneApi zone, ZoneApi... rest) {
+        applications.forEach(application -> completeUpgrade(application, version, zone, rest));
     }
 
     private void failNodeIn(ZoneApi zone, SystemApplication application) {
@@ -337,29 +339,35 @@ public class SystemUpgraderTest {
         assertEquals(version, tester.controller().versionStatus().controllerVersion().get().versionNumber());
     }
 
-    private void assertWantedVersion(SystemApplication application, Version version, ZoneApi... zones) {
-        assertVersion(application, version, Node::wantedVersion, zones);
+    private void assertWantedVersion(SystemApplication application, Version version, ZoneApi first, ZoneApi... rest) {
+        Stream.concat(Stream.of(first), Stream.of(rest)).forEach(zone -> {
+            if (!application.hasApplicationPackage()) {
+                assertEquals("Target version set for " + application + " in " + zone.getId(), version,
+                             nodeRepository().targetVersionsOf(zone.getId()).vespaVersion(application.nodeType()).orElse(Version.emptyVersion));
+            }
+            assertVersion(application, version, Node::wantedVersion, zone);
+        });
     }
 
-    private void assertCurrentVersion(SystemApplication application, Version version, ZoneApi... zones) {
-        assertVersion(application, version, Node::currentVersion, zones);
+    private void assertCurrentVersion(SystemApplication application, Version version, ZoneApi first, ZoneApi... rest) {
+        assertVersion(application, version, Node::currentVersion, first, rest);
     }
 
-    private void assertWantedVersion(List<SystemApplication> applications, Version version, ZoneApi... zones) {
-        applications.forEach(application -> assertVersion(application, version, Node::wantedVersion, zones));
+    private void assertWantedVersion(List<SystemApplication> applications, Version version, ZoneApi first, ZoneApi... rest) {
+        applications.forEach(application -> assertWantedVersion(application, version, first, rest));
     }
 
-    private void assertCurrentVersion(List<SystemApplication> applications, Version version, ZoneApi... zones) {
-        applications.forEach(application -> assertVersion(application, version, Node::currentVersion, zones));
+    private void assertCurrentVersion(List<SystemApplication> applications, Version version, ZoneApi first, ZoneApi... rest) {
+        applications.forEach(application -> assertVersion(application, version, Node::currentVersion, first, rest));
     }
 
     private void assertVersion(SystemApplication application, Version version, Function<Node, Version> versionField,
-                               ZoneApi... zones) {
-        for (ZoneApi zone : requireNonEmpty(zones)) {
+                               ZoneApi first, ZoneApi... rest) {
+        Stream.concat(Stream.of(first), Stream.of(rest)).forEach(zone -> {
             for (Node node : listNodes(zone, application)) {
-                assertEquals(application + " version", version, versionField.apply(node));
+                assertEquals("Version of " + application.id() + " in " + zone.getId(), version, versionField.apply(node));
             }
-        }
+        });
     }
 
     private List<Node> listNodes(ZoneApi zone, SystemApplication application) {
@@ -378,9 +386,4 @@ public class SystemUpgraderTest {
                                   new JobControl(tester.curator()));
     }
 
-    private static <T> T[] requireNonEmpty(T[] args) {
-        if (args.length == 0) throw new IllegalArgumentException("Need at least one argument");
-        return args;
-    }
-
 }