Use current system version for non-converging system applications

4 files changed, 65 insertions, 22 deletions

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 9b3423e9f10..2e7f204e50c 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
@@ -6,6 +6,7 @@ import com.yahoo.collections.ListMap;
 import com.yahoo.component.Version;
 import com.yahoo.component.Vtag;
 import com.yahoo.config.provision.HostName;
+import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
@@ -130,12 +131,15 @@ public class VersionStatus {
         ListMap<Version, HostName> versions = new ListMap<>();
         for (ZoneId zone : zones) {
             for (SystemApplication application : SystemApplication.all()) {
-                if (!application.configConvergedIn(zone, controller)) {
-                    throw new IllegalStateException("Config for " + application.id() + " in " + zone + " has not converged");
+                boolean configConverged = application.configConvergedIn(zone, controller);
+                if (!configConverged) {
+                    log.log(LogLevel.WARNING, "Config for " + application.id() + " in " + zone + " has not converged");
                 }
                 for (Node node : controller.configServer().nodeRepository().list(zone, application.id(),
                                                                                  SystemApplication.activeStates())) {
-                    versions.put(node.currentVersion(), node.hostname());
+                    // Only use current node version if config has converged
+                    Version nodeVersion = configConverged ? node.currentVersion() : controller.systemVersion();
+                    versions.put(nodeVersion, node.hostname());
                 }
             }
         }
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 04e149327b6..6bbc024b829 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
@@ -30,17 +30,15 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzClientFactoryMock;
 import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock;
 import com.yahoo.vespa.hosted.controller.integration.MockMetricsService;
-import com.yahoo.vespa.hosted.controller.maintenance.JobControl;
-import com.yahoo.vespa.hosted.controller.maintenance.VersionStatusUpdater;
 import com.yahoo.vespa.hosted.controller.persistence.ApplicationSerializer;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
 import com.yahoo.vespa.hosted.controller.routing.MockRoutingGenerator;
 import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
+import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
 import com.yahoo.vespa.hosted.rotation.config.RotationsConfig;
 
-import java.time.Duration;
 import java.util.Optional;
 import java.util.OptionalLong;
 import java.util.function.Supplier;
@@ -286,7 +284,7 @@ public final class ControllerTester {
                                                new MockLogStore(),
                                                () -> "test-controller");
         // Calculate initial versions
-        new VersionStatusUpdater(controller, Duration.ofDays(1), new JobControl(curator)).run();
+        controller.updateVersionStatus(VersionStatus.compute(controller));
         return controller;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
index 2aad2353c23..684a1f8e47d 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
@@ -23,7 +23,7 @@ import com.yahoo.vespa.hosted.controller.maintenance.JobControl;
 import com.yahoo.vespa.hosted.controller.maintenance.ReadyJobsTrigger;
 import com.yahoo.vespa.hosted.controller.maintenance.SystemUpgrader;
 import com.yahoo.vespa.hosted.controller.maintenance.Upgrader;
-import com.yahoo.vespa.hosted.controller.maintenance.VersionStatusUpdater;
+import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
 
 import java.time.Duration;
 import java.util.List;
@@ -50,7 +50,6 @@ public class DeploymentTester {
     private final Upgrader upgrader;
     private final SystemUpgrader systemUpgrader;
     private final ReadyJobsTrigger readyJobTrigger;
-    private final VersionStatusUpdater versionStatusUpdater;
 
     public DeploymentTester() {
         this(new ControllerTester());
@@ -64,7 +63,6 @@ public class DeploymentTester {
         this.upgrader = new Upgrader(tester.controller(), maintenanceInterval, jobControl, tester.curator());
         this.systemUpgrader = new SystemUpgrader(tester.controller(), maintenanceInterval, jobControl);
         this.readyJobTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval, jobControl);
-        this.versionStatusUpdater = new VersionStatusUpdater(tester.controller(), maintenanceInterval, jobControl);
     }
 
     public SystemUpgrader systemUpgrader() {
@@ -101,7 +99,7 @@ public class DeploymentTester {
 
     /** Re-compute and write version status */
     public void computeVersionStatus() {
-        versionStatusUpdater.run();
+        controller().updateVersionStatus(VersionStatus.compute(controller()));
     }
 
     /** Upgrade controller to given version */
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 0e9518a63e9..5ff1c84cbfd 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 org.junit.Before;
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Function;
 
@@ -59,7 +60,7 @@ public class SystemUpgraderTest {
         // Controller upgrades
         Version version2 = Version.fromString("6.6");
         tester.upgradeController(version2);
-        assertEquals(version2, tester.controller().versionStatus().controllerVersion().get().versionNumber());
+        assertControllerVersion(version2);
 
         // System upgrade starts
         tester.systemUpgrader().maintain();
@@ -115,7 +116,7 @@ public class SystemUpgraderTest {
 
         // System version remains unchanged until final application upgrades
         tester.computeVersionStatus();
-        assertEquals(version1, tester.controller().versionStatus().systemVersion().get().versionNumber());
+        assertSystemVersion(version1);
 
         // zone 4: zone-application upgrades
         tester.systemUpgrader().maintain();
@@ -124,10 +125,10 @@ public class SystemUpgraderTest {
 
         // zone 4: System version remains unchanged until config converges
         tester.computeVersionStatus();
-        assertEquals(version1, tester.controller().versionStatus().systemVersion().get().versionNumber());
+        assertSystemVersion(version1);
         convergeServices(SystemApplication.zone, zone4);
         tester.computeVersionStatus();
-        assertEquals(version2, tester.controller().versionStatus().systemVersion().get().versionNumber());
+        assertSystemVersion(version2);
 
         // Next run does nothing as system is now upgraded
         tester.systemUpgrader().maintain();
@@ -136,6 +137,41 @@ public class SystemUpgraderTest {
     }
 
     @Test
+    public void upgrade_controller_with_non_converging_application() {
+        tester.controllerTester().zoneRegistry().setUpgradePolicy(UpgradePolicy.create().upgrade(zone1));
+
+        // Bootstrap system
+        tester.configServer().bootstrap(Collections.singletonList(zone1), SystemApplication.configServer,
+                                        SystemApplication.zone);
+        Version version1 = Version.fromString("6.5");
+        tester.upgradeSystem(version1);
+
+        // Controller upgrades
+        Version version2 = Version.fromString("6.6");
+        tester.upgradeController(version2);
+
+        // zone 1: System applications upgrade
+        tester.systemUpgrader().maintain();
+        completeUpgrade(SystemApplication.configServer, version2, zone1);
+        tester.systemUpgrader().maintain();
+        completeUpgrade(SystemApplication.zone, version2, zone1);
+        tester.computeVersionStatus();
+        assertSystemVersion(version1); // Unchanged until zone-application converges
+
+        // Controller upgrades again
+        Version version3 = Version.fromString("6.7");
+        tester.upgradeController(version3);
+        assertSystemVersion(version1);
+        assertControllerVersion(version3);
+
+        // zone 1: zone-application converges and system version changes
+        convergeServices(SystemApplication.zone, zone1);
+        tester.computeVersionStatus();
+        assertSystemVersion(version2);
+        assertControllerVersion(version3);
+    }
+
+    @Test
     public void upgrade_system_containing_host_applications() {
         tester.controllerTester().zoneRegistry().setUpgradePolicy(
                 UpgradePolicy.create()
@@ -153,7 +189,7 @@ public class SystemUpgraderTest {
         // Controller upgrades
         Version version2 = Version.fromString("6.6");
         tester.upgradeController(version2);
-        assertEquals(version2, tester.controller().versionStatus().controllerVersion().get().versionNumber());
+        assertControllerVersion(version2);
 
         // System upgrades in zone 1:
         tester.systemUpgrader().maintain();
@@ -187,22 +223,21 @@ public class SystemUpgraderTest {
 
     @Test
     public void never_downgrades_system() {
-        ZoneId zone = ZoneId.from("prod", "eu-west-1");
-        tester.controllerTester().zoneRegistry().setUpgradePolicy(UpgradePolicy.create().upgrade(zone));
+        tester.controllerTester().zoneRegistry().setUpgradePolicy(UpgradePolicy.create().upgrade(zone1));
 
         Version version = Version.fromString("6.5");
         tester.upgradeSystem(version);
         tester.systemUpgrader().maintain();
-        assertWantedVersion(SystemApplication.configServer, version, zone);
-        assertWantedVersion(SystemApplication.zone, version, zone);
+        assertWantedVersion(SystemApplication.configServer, version, zone1);
+        assertWantedVersion(SystemApplication.zone, version, zone1);
 
         // Controller is downgraded
         tester.upgradeController(Version.fromString("6.4"));
 
         // Wanted version for zone remains unchanged
         tester.systemUpgrader().maintain();
-        assertWantedVersion(SystemApplication.configServer, version, zone);
-        assertWantedVersion(SystemApplication.zone, version, zone);
+        assertWantedVersion(SystemApplication.configServer, version, zone1);
+        assertWantedVersion(SystemApplication.zone, version, zone1);
     }
 
     /** Simulate upgrade of nodes allocated to given application. In a real system this is done by the node itself */
@@ -238,6 +273,14 @@ public class SystemUpgraderTest {
                                             node.currentVersion(), node.wantedVersion()));
     }
 
+    private void assertSystemVersion(Version version) {
+        assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber());
+    }
+
+    private void assertControllerVersion(Version version) {
+        assertEquals(version, tester.controller().versionStatus().controllerVersion().get().versionNumber());
+    }
+
     private void assertWantedVersion(SystemApplication application, Version version, ZoneId... zones) {
         assertVersion(application, version, Node::wantedVersion, zones);
     }