Merge pull request #23017 from vespa-engine/jonmv/always-fail-when-requested-model-version-faild-to-build

Always fail model building when wanted version fails

2 files changed, 58 insertions, 2 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
index f6f6a9ea24c..563989ddf05 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java
@@ -128,6 +128,9 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                 // caused by the state of the system, not the model version/application combination
                 throw e;
             }
+            catch (RequireModelVersionFailureException e) {
+                throw e.getCause();
+            }
             catch (RuntimeException e) {
                 if (shouldSkipCreatingMajorVersionOnError(majorVersions, majorVersion)) {
                     log.log(Level.INFO, applicationId + ": Skipping major version " + majorVersion, e);
@@ -177,7 +180,7 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         if (buildLatestModelForThisMajor) {
             latest = Optional.of(findLatest(versions));
             // load latest application version
-            MODELRESULT latestModelVersion = buildModelVersion(modelFactoryRegistry.getFactory(latest.get()),
+            MODELRESULT latestModelVersion = tryBuildModelVersion(modelFactoryRegistry.getFactory(latest.get()),
                                                                applicationPackage,
                                                                applicationId,
                                                                wantedDockerImageRepository,
@@ -192,7 +195,7 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
             if (latest.isPresent() && version.equals(latest.get())) continue; // already loaded
 
             try {
-                MODELRESULT modelVersion = buildModelVersion(modelFactoryRegistry.getFactory(version),
+                MODELRESULT modelVersion = tryBuildModelVersion(modelFactoryRegistry.getFactory(version),
                                                              applicationPackage,
                                                              applicationId,
                                                              wantedDockerImageRepository,
@@ -252,6 +255,25 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                                .anyMatch(host -> host.version().isPresent() && host.version().get().equals(version));
     }
 
+    private MODELRESULT tryBuildModelVersion(ModelFactory modelFactory, ApplicationPackage applicationPackage,
+                                             ApplicationId applicationId, Optional<DockerImage> dockerImageRepository,
+                                             Version wantedNodeVespaVersion) {
+        try {
+            return buildModelVersion(modelFactory, applicationPackage, applicationId, dockerImageRepository, wantedNodeVespaVersion);
+        }
+        catch (RuntimeException e) {
+            if (modelFactory.version().equals(wantedNodeVespaVersion))
+                throw new RequireModelVersionFailureException(e);
+
+            throw e;
+        }
+    }
+
+    private static class RequireModelVersionFailureException extends RuntimeException {
+        RequireModelVersionFailureException(RuntimeException cause) { super(cause); }
+        @Override public RuntimeException getCause() { return (RuntimeException) super.getCause(); }
+    }
+
     protected abstract MODELRESULT buildModelVersion(ModelFactory modelFactory, ApplicationPackage applicationPackage,
                                                      ApplicationId applicationId, Optional<DockerImage> dockerImageRepository,
                                                      Version wantedNodeVespaVersion);
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
index 10e3ab1ec97..5dfc1bbd3ef 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
@@ -52,6 +52,7 @@ import static com.yahoo.vespa.config.server.deploy.DeployTester.createHostedMode
 import static java.util.stream.Collectors.toList;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -276,6 +277,39 @@ public class HostedDeployTest {
      * that are still using features that do not work on version 8.x)
      */
     @Test
+    public void testWantedVersionIsRequiredAlsoWhenThereIsAnOlderMajorThatDoesNotFailModelBuilding() {
+        int oldMajor = 7;
+        int newMajor = 8;
+        Version wantedVersion = new Version(newMajor, 1, 2);
+        Version oldVersion = new Version(oldMajor, 2, 3);
+        List<Host> hosts = createHosts(9, oldVersion.toFullString(), wantedVersion.toFullString());
+
+        CountingModelFactory oldFactory = createHostedModelFactory(oldVersion);
+        ModelFactory newFactory = createFailingModelFactory(wantedVersion);
+        List<ModelFactory> modelFactories = List.of(oldFactory, newFactory);
+
+        DeployTester tester = createTester(hosts, modelFactories, prodZone);
+
+        // Not OK when failing version is requested.
+        assertEquals("Invalid application package",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> tester.deployApp("src/test/apps/hosted/", wantedVersion.toFullString()))
+                             .getMessage());
+
+        // OK when older version is requested.
+        tester.deployApp("src/test/apps/hosted/", oldVersion.toFullString());
+        assertEquals(9, tester.getAllocatedHostsOf(tester.applicationId()).getHosts().size());
+
+        // Check >0 not ==0 as the session watcher thread is running and will redeploy models in the background
+        assertTrue(oldFactory.creationCount() > 0);
+    }
+
+    /**
+     * Tests that we create the minimal set of models and that version 7.x is created
+     * if creating version 8.x fails (to support upgrades to new major version for applications
+     * that are still using features that do not work on version 8.x)
+     */
+    @Test
     public void testCreateLatestMajorOnPreviousMajorIfItFailsOnMajorVersion8() {
         deployWithModelForLatestMajorVersionFailing(8);
     }