Merge pull request #6299 from vespa-engine/mpolden/system-version-change-requires-config-convergence

Ensure that config has converged before changing system version

9 files changed, 73 insertions, 35 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 98ec05e563a..05ac311c514 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
@@ -3,7 +3,12 @@ package com.yahoo.vespa.hosted.controller.application;
 
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.NodeType;
+import com.yahoo.config.provision.RegionName;
+import com.yahoo.vespa.hosted.controller.Controller;
+import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
+import com.yahoo.vespa.hosted.controller.api.integration.configserver.ServiceConvergence;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 
 import java.util.Arrays;
 import java.util.List;
@@ -49,6 +54,20 @@ public enum SystemApplication {
         return nodeType == NodeType.proxy;
     }
 
+    /** Returns whether config for this application has converged in given zone */
+    public boolean configConvergedIn(ZoneId zone, Controller controller) {
+        if (!hasApplicationPackage()) {
+            return true;
+        }
+        // TODO: Remove this hack once Docker hosts are removed from zone-application.
+        if (isAws(zone.region())) {
+            return true; // Skip checking config convergence on AWS as Docker hosts do not have cloud config
+        }
+        return controller.configServer().serviceConvergence(new DeploymentId(id(), zone))
+                         .map(ServiceConvergence::converged)
+                         .orElse(false);
+    }
+
     /** All known system applications */
     public static List<SystemApplication> all() {
         return Arrays.asList(values());
@@ -64,4 +83,8 @@ public enum SystemApplication {
         return String.format("system application %s of type %s", id, nodeType);
     }
 
+    private static boolean isAws(RegionName region) {
+        return region.value().startsWith("cd-aws-") || region.value().startsWith("aws-");
+    }
+
 }
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 e9dd093fae8..953e95e25c8 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
@@ -3,9 +3,7 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.component.Version;
 import com.yahoo.vespa.hosted.controller.Controller;
-import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
-import com.yahoo.vespa.hosted.controller.api.integration.configserver.ServiceConvergence;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
@@ -71,21 +69,11 @@ public class SystemUpgrader extends Maintainer {
             if (convergedOn(target, application.dependencies(), zone)) {
                 deploy(target, application, zone);
             }
-            converged &= convergedOn(target, application, zone) & configConverged(application, zone);
+            converged &= convergedOn(target, application, zone) & application.configConvergedIn(zone, controller());
         }
         return converged;
     }
 
-    /** Returns whether config for given application has converged */
-    private boolean configConverged(SystemApplication application, ZoneId zone) {
-        if (!application.hasApplicationPackage()) {
-            return true;
-        }
-        return controller().configServer().serviceConvergence(new DeploymentId(application.id(), zone))
-                           .map(ServiceConvergence::converged)
-                           .orElse(false);
-    }
-
     /** Deploy application on given version idempotently */
     private void deploy(Version target, SystemApplication application, ZoneId zone) {
         if (!wantedVersion(zone, application, target).equals(target)) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VersionStatusUpdater.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VersionStatusUpdater.java
index dea991bc653..565acf6ebd5 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VersionStatusUpdater.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VersionStatusUpdater.java
@@ -1,10 +1,11 @@
 // 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.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
+import com.yahoo.yolean.Exceptions;
 
-import java.io.UncheckedIOException;
 import java.time.Duration;
 
 /**
@@ -25,8 +26,9 @@ public class VersionStatusUpdater extends Maintainer {
         try {
             VersionStatus newStatus = VersionStatus.compute(controller());
             controller().updateVersionStatus(newStatus);
-        } catch (UncheckedIOException e) {
-            log.warning("Failed to compute version status. This is likely a transient error: " + e.getMessage());
+        } catch (Exception e) {
+            log.log(LogLevel.WARNING, "Failed to compute version status: " + Exceptions.toMessageString(e) +
+                                      ". Retrying in " + maintenanceInterval());
         }
     }
 
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 dcb7947728c..9b3423e9f10 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
@@ -130,6 +130,9 @@ 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");
+                }
                 for (Node node : controller.configServer().nodeRepository().list(zone, application.id(),
                                                                                  SystemApplication.activeStates())) {
                     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 a023abd1a1d..ab33d6901a5 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
@@ -75,6 +75,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
                                             ))
                                             .collect(Collectors.toList());
                 nodeRepository().add(zone, nodes);
+                convergeServices(application.id(), zone);
             }
         }
     }
@@ -103,12 +104,10 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     }
 
     /** Set version for system applications in given zone */
-    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.state(), node.type(), node.owner(),
-                                                    version, version));
-            }
+    public void setVersion(ApplicationId application, ZoneId zone, Version version) {
+        for (Node node : nodeRepository().list(zone, application)) {
+            nodeRepository().add(zone, new Node(node.hostname(), node.state(), node.type(), node.owner(),
+                                                version, version));
         }
     }
 
@@ -174,6 +173,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
                                                                      node.currentVersion(),
                                                                      application.version().get()));
                 }
+                serviceStatus.remove(deployment); // Deployment is no longer converging after new deployment
 
                 PrepareResponse prepareResponse = new PrepareResponse();
                 prepareResponse.message = "foo";
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 e6ca68c90e1..04e149327b6 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,15 +30,17 @@ 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;
@@ -283,7 +285,8 @@ public final class ControllerTester {
                                                buildService,
                                                new MockLogStore(),
                                                () -> "test-controller");
-        controller.updateVersionStatus(VersionStatus.compute(controller));
+        // Calculate initial versions
+        new VersionStatusUpdater(controller, Duration.ofDays(1), new JobControl(curator)).run();
         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 bae1f667566..2aad2353c23 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
@@ -12,18 +12,18 @@ import com.yahoo.vespa.hosted.controller.ArtifactRepositoryMock;
 import com.yahoo.vespa.hosted.controller.ConfigServerMock;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
+import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
-import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 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.versions.VersionStatus;
+import com.yahoo.vespa.hosted.controller.maintenance.VersionStatusUpdater;
 
 import java.time.Duration;
 import java.util.List;
@@ -50,6 +50,7 @@ public class DeploymentTester {
     private final Upgrader upgrader;
     private final SystemUpgrader systemUpgrader;
     private final ReadyJobsTrigger readyJobTrigger;
+    private final VersionStatusUpdater versionStatusUpdater;
 
     public DeploymentTester() {
         this(new ControllerTester());
@@ -58,11 +59,12 @@ public class DeploymentTester {
     public DeploymentTester(ControllerTester tester) {
         this.tester = tester;
         tester.curator().writeUpgradesPerMinute(100);
-        this.upgrader = new Upgrader(tester.controller(), maintenanceInterval, new JobControl(tester.curator()),
-                                     tester.curator());
-        this.systemUpgrader = new SystemUpgrader(tester.controller(), maintenanceInterval, new JobControl(tester.curator()));
-        this.readyJobTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval,
-                                                    new JobControl(tester.curator()));
+
+        JobControl jobControl = new JobControl(tester.curator());
+        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() {
@@ -99,7 +101,7 @@ public class DeploymentTester {
 
     /** Re-compute and write version status */
     public void computeVersionStatus() {
-        controller().updateVersionStatus(VersionStatus.compute(controller()));
+        versionStatusUpdater.run();
     }
 
     /** Upgrade controller to given version */
@@ -111,7 +113,10 @@ public class DeploymentTester {
     /** Upgrade system applications in all zones to given version */
     public void upgradeSystemApplications(Version version) {
         for (ZoneId zone : tester.zoneRegistry().zones().all().ids()) {
-            tester.configServer().setVersion(version, zone, SystemApplication.all());
+            for (SystemApplication application : SystemApplication.all()) {
+                tester.configServer().setVersion(application.id(), zone, version);
+                tester.configServer().convergeServices(application.id(), zone);
+            }
         }
         computeVersionStatus();
     }
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 cddc0f5603e..0e9518a63e9 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
@@ -8,6 +8,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.Before;
 import org.junit.Test;
 
 import java.util.Arrays;
@@ -27,7 +28,12 @@ public class SystemUpgraderTest {
     private static final ZoneId zone3 = ZoneId.from("prod", "us-central-1");
     private static final ZoneId zone4 = ZoneId.from("prod", "us-east-3");
 
-    private final DeploymentTester tester = new DeploymentTester();
+    private DeploymentTester tester;
+
+    @Before
+    public void before() {
+        tester = new DeploymentTester();
+    }
 
     @Test
     public void upgrade_system() {
@@ -115,6 +121,11 @@ public class SystemUpgraderTest {
         tester.systemUpgrader().maintain();
         assertWantedVersion(SystemApplication.zone, version2, zone4);
         completeUpgrade(SystemApplication.zone, version2, zone4);
+
+        // zone 4: System version remains unchanged until config converges
+        tester.computeVersionStatus();
+        assertEquals(version1, tester.controller().versionStatus().systemVersion().get().versionNumber());
+        convergeServices(SystemApplication.zone, zone4);
         tester.computeVersionStatus();
         assertEquals(version2, tester.controller().versionStatus().systemVersion().get().versionNumber());
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
index c72edf7e403..b5a79eebcfd 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
@@ -57,7 +57,10 @@ public class ContainerTester {
     public void upgradeSystem(Version version) {
         controller().curator().writeControllerVersion(controller().hostname(), version);
         for (ZoneId zone : controller().zoneRegistry().zones().all().ids()) {
-            configServer().setVersion(version, zone, SystemApplication.all());
+            for (SystemApplication application : SystemApplication.all()) {
+                configServer().setVersion(application.id(), zone, version);
+                configServer().convergeServices(application.id(), zone);
+            }
         }
         computeVersionStatus();
     }