Merge pull request #23891 from vespa-engine/jonmv/handle-major-upgrade-in-block-window-better

Jonmv/handle major upgrade in block window better

4 files changed, 61 insertions, 44 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
index 1d7d75d9193..e5e5c105614 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
@@ -54,6 +54,7 @@ import static com.yahoo.config.provision.Environment.staging;
 import static com.yahoo.config.provision.Environment.test;
 import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
+import static java.util.Comparator.reverseOrder;
 import static java.util.Objects.requireNonNull;
 import static java.util.function.BinaryOperator.maxBy;
 import static java.util.stream.Collectors.collectingAndThen;
@@ -168,6 +169,45 @@ public class DeploymentStatus {
         return allJobs.groupingBy(job -> job.id().application());
     }
 
+    /** Returns change potentially with a compatibility platform added, if required for the change to roll out to the given instance. */
+    public Change withCompatibilityPlatform(Change change, InstanceName instance) {
+        if (change.revision().isEmpty())
+            return change;
+
+        Optional<Version> compileVersion = change.revision()
+                                                 .map(application.revisions()::get)
+                                                 .flatMap(ApplicationVersion::compileVersion);
+
+        // If the revision requires a certain platform for compatibility, add that here.
+        VersionCompatibility compatibility = versionCompatibility.apply(instance);
+        Predicate<Version> compatibleWithCompileVersion = version -> compileVersion.map(compiled -> compatibility.accept(version, compiled)).orElse(true);
+        if (   application.productionDeployments().isEmpty() // TODO: replace with adding this for test jobs when needed
+            || application.productionDeployments().getOrDefault(instance, List.of()).stream()
+                     .anyMatch(deployment -> ! compatibleWithCompileVersion.test(deployment.version()))) {
+            return targetsForPolicy(versionStatus, systemVersion, application.deploymentSpec().requireInstance(instance).upgradePolicy())
+                    .stream() // Pick the latest platform which is compatible with the compile version, and is ready for this instance.
+                    .filter(compatibleWithCompileVersion)
+                    .findFirst()
+                    .map(platform -> change.withoutPin().with(platform))
+                    .orElse(change);
+        }
+        return change;
+    }
+
+    /** Returns target versions for given confidence, by descending version number. */
+    public static List<Version> targetsForPolicy(VersionStatus versions, Version systemVersion, DeploymentSpec.UpgradePolicy policy) {
+        if (policy == DeploymentSpec.UpgradePolicy.canary)
+            return List.of(systemVersion);
+
+        VespaVersion.Confidence target = policy == DeploymentSpec.UpgradePolicy.defaultPolicy ? VespaVersion.Confidence.normal : VespaVersion.Confidence.high;
+        return versions.deployableVersions().stream()
+                       .filter(version -> version.confidence().equalOrHigherThan(target))
+                       .map(VespaVersion::versionNumber)
+                       .sorted(reverseOrder())
+                       .collect(Collectors.toList());
+    }
+
+
     /**
      * The set of jobs that need to run for the changes of each instance of the application to be considered complete,
      * and any test jobs for any outstanding change, which will likely be needed to later deploy this change.
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 fa6da282172..7c0a4a83e4f 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
@@ -91,10 +91,9 @@ public class DeploymentTrigger {
                                             && acceptNewRevision(status, instanceName, outstanding.revision().get());
                 application = application.with(instanceName,
                                                instance -> withRemainingChange(instance,
-                                                                               withCompatibilityPlatform((deployOutstanding ? outstanding
-                                                                                                                            : Change.empty())
-                                                                                                                 .onTopOf(instance.change()),
-                                                                                                         status,
+                                                                               status.withCompatibilityPlatform((deployOutstanding ? outstanding
+                                                                                                                                   : Change.empty())
+                                                                                                                        .onTopOf(instance.change()),
                                                                                                          instanceName),
                                                                                status));
             }
@@ -102,44 +101,6 @@ public class DeploymentTrigger {
         });
     }
 
-    /** Returns any outstanding change for the given instance, coupled with any necessary platform upgrade. */
-    private Change withCompatibilityPlatform(Change revisionChange, DeploymentStatus status, InstanceName instance) {
-        Optional<Version> compileVersion = revisionChange.revision()
-                                                         .map(status.application().revisions()::get)
-                                                         .flatMap(ApplicationVersion::compileVersion);
-
-        // If the outstanding revision requires a certain platform for compatibility, add that here.
-        VersionCompatibility compatibility = applications().versionCompatibility(status.application().id().instance(instance));
-        Predicate<Version> compatibleWithCompileVersion = version -> compileVersion.map(compiled -> compatibility.accept(version, compiled)).orElse(true);
-        if (   status.application().productionDeployments().isEmpty()
-            || status.application().productionDeployments().getOrDefault(instance, List.of()).stream()
-                     .anyMatch(deployment -> ! compatibleWithCompileVersion.test(deployment.version()))) {
-            return targetsForPolicy(controller.readVersionStatus(), status.application().deploymentSpec().requireInstance(instance).upgradePolicy())
-                    .stream() // Pick the latest platform which is compatible with the compile version, and is ready for this instance.
-                    .filter(compatibleWithCompileVersion)
-                    .map(revisionChange::with)
-                    .filter(change -> status.instanceSteps().get(instance).readyAt(change)
-                                            .map(readyAt -> ! readyAt.isAfter(controller.clock().instant()))
-                                            .orElse(false))
-                    .findFirst().orElse(Change.empty());
-        }
-        return revisionChange;
-    }
-
-    /** Returns target versions for given confidence, by descending version number. */
-    public List<Version> targetsForPolicy(VersionStatus versions, DeploymentSpec.UpgradePolicy policy) {
-        Version systemVersion = controller.systemVersion(versions);
-        if (policy == DeploymentSpec.UpgradePolicy.canary)
-            return List.of(systemVersion);
-
-        VespaVersion.Confidence target = policy == DeploymentSpec.UpgradePolicy.defaultPolicy ? VespaVersion.Confidence.normal : VespaVersion.Confidence.high;
-        return versions.deployableVersions().stream()
-                       .filter(version -> version.confidence().equalOrHigherThan(target))
-                       .map(VespaVersion::versionNumber)
-                       .sorted(reverseOrder())
-                       .collect(Collectors.toList());
-    }
-
     /**
      * Records information when a job completes (successfully or not). This information is used when deciding what to
      * trigger next.
@@ -159,7 +120,6 @@ public class DeploymentTrigger {
 
     /**
      * Finds and triggers jobs that can and should run but are currently not, and returns the number of triggered jobs.
-     *
      * Only one job per type is triggered each run for test jobs, since their environments have limited capacity.
      */
     public TriggerResult triggerReadyJobs() {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index 269623de3f0..ce98643c25a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -9,6 +9,7 @@ import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.application.ApplicationList;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.InstanceList;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentStatus;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentStatusList;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel;
@@ -105,7 +106,7 @@ public class Upgrader extends ControllerMaintainer {
                                                                                                    .not().upgradingTo(targetAndNewer);
 
         Map<ApplicationId, Version> targets = new LinkedHashMap<>();
-        for (Version version : controller().applications().deploymentTrigger().targetsForPolicy(versionStatus, policy)) {
+        for (Version version : DeploymentStatus.targetsForPolicy(versionStatus, controller().systemVersion(versionStatus), policy)) {
             targetAndNewer.add(version);
             InstanceList eligible = eligibleForVersion(remaining, version, targetMajorVersion);
             InstanceList outdated = cancellationCriterion.apply(eligible);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
index 9fde1ae6d33..fda1f5f0b77 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
@@ -2207,6 +2207,22 @@ public class DeploymentTriggerTest {
                 .deploy();
         assertEquals(version1, tester.jobs().last(newApp.instanceId(), productionUsEast3).get().versions().targetPlatform());
         assertEquals(version1, newApp.application().revisions().get(tester.jobs().last(newApp.instanceId(), productionUsEast3).get().versions().targetRevision()).compileVersion().get());
+
+        // The new app enters a platform block window, and is pinned to the old platform;
+        // the new submission overrides both those settings, as the new revision should roll out regardless.
+        tester.atMondayMorning();
+        tester.deploymentTrigger().forceChange(newApp.instanceId(), Change.empty().withPin());
+        newApp.submit(new ApplicationPackageBuilder().compileVersion(version2)
+                                                     .systemTest()
+                                                     .blockChange(false, true, "mon", "0-23", "UTC")
+                                                     .region("us-east-3")
+                                                     .build());
+        RevisionId newRevision = newApp.lastSubmission().get();
+
+        assertEquals(Change.of(newRevision).with(version2), newApp.instance().change());
+        newApp.deploy();
+        assertEquals(version2, tester.jobs().last(newApp.instanceId(), productionUsEast3).get().versions().targetPlatform());
+        assertEquals(version2, newApp.application().revisions().get(tester.jobs().last(newApp.instanceId(), productionUsEast3).get().versions().targetRevision()).compileVersion().get());
     }
 
     @Test