Merge pull request #23033 from vespa-engine/jonmv/allow-less-model-building-failure

Disallow model building failures on any version with a major MERGEOK

6 files changed, 27 insertions, 54 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 563989ddf05..361d966ebc5 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
@@ -92,6 +92,9 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         Instant start = Instant.now();
         log.log(Level.FINE, () -> "Will build models for " + applicationId);
         Set<Version> versions = modelFactoryRegistry.allVersions();
+        if (applicationPackage.getMajorVersion().isPresent() && applicationPackage.getMajorVersion().get() != wantedNodeVespaVersion.getMajor())
+            throw new IllegalArgumentException("requested node version (" + wantedNodeVespaVersion + ") has a different major version " +
+                                               "than specified in deployment.xml (" + applicationPackage.getMajorVersion().get() + ")");
 
         // If the application specifies a major, skip models on a newer major
         Optional<Integer> requestedMajorVersion = applicationPackage.getMajorVersion();
@@ -128,11 +131,8 @@ 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)) {
+                if (shouldSkipCreatingMajorVersionOnError(majorVersions, majorVersion, wantedNodeVespaVersion, allocatedHosts)) {
                     log.log(Level.INFO, applicationId + ": Skipping major version " + majorVersion, e);
                 }
                 else {
@@ -157,9 +157,15 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         return allApplicationModels;
     }
 
-    private boolean shouldSkipCreatingMajorVersionOnError(List<Integer> majorVersions, Integer majorVersion) {
-        if (majorVersion.equals(Collections.min(majorVersions))) return false;
-        // Note: This needs to be updated when we no longer want to support successfully deploying
+    private boolean shouldSkipCreatingMajorVersionOnError(List<Integer> majorVersions, Integer majorVersion, Version wantedVersion,
+                                                          AllocatedHostsFromAllModels allHosts) {
+        if (majorVersion.equals(wantedVersion.getMajor())) return false;        // Ensure we are valid for our targeted major.
+        if (allHosts.toAllocatedHosts().getHosts().stream()
+                    .flatMap(host -> host.version().stream())
+                    .map(Version::getMajor)
+                    .anyMatch(majorVersion::equals)) return false;              // Ensure we are valid for our currently deployed major.
+        if (majorVersion.equals(Collections.min(majorVersions))) return false;  // Probably won't happen if the other two are both false ... ?
+        // Note: This needs to be bumped when we no longer want to support successfully deploying
         // applications that are not working on version 8, but are working on a lower major version (unless
         // apps have explicitly defined major version to deploy to in application package)
         return majorVersion >= 8;
@@ -180,7 +186,7 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         if (buildLatestModelForThisMajor) {
             latest = Optional.of(findLatest(versions));
             // load latest application version
-            MODELRESULT latestModelVersion = tryBuildModelVersion(modelFactoryRegistry.getFactory(latest.get()),
+            MODELRESULT latestModelVersion = buildModelVersion(modelFactoryRegistry.getFactory(latest.get()),
                                                                applicationPackage,
                                                                applicationId,
                                                                wantedDockerImageRepository,
@@ -195,7 +201,7 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
             if (latest.isPresent() && version.equals(latest.get())) continue; // already loaded
 
             try {
-                MODELRESULT modelVersion = tryBuildModelVersion(modelFactoryRegistry.getFactory(version),
+                MODELRESULT modelVersion = buildModelVersion(modelFactoryRegistry.getFactory(version),
                                                              applicationPackage,
                                                              applicationId,
                                                              wantedDockerImageRepository,
@@ -255,25 +261,6 @@ 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/apps/app-major-version-2/deployment.xml b/configserver/src/test/apps/app-major-version-7/deployment.xml
index 6d95ca8a16c..7412557b8e7 100644
--- a/configserver/src/test/apps/app-major-version-2/deployment.xml
+++ b/configserver/src/test/apps/app-major-version-7/deployment.xml
@@ -1,2 +1,2 @@
 <!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -->
-<deployment version='1.0' major-version='2'/>
+<deployment version='1.0' major-version='7'/>
diff --git a/configserver/src/test/apps/app-major-version-2/hosts.xml b/configserver/src/test/apps/app-major-version-7/hosts.xml
index a515a4e97da..a515a4e97da 100644
--- a/configserver/src/test/apps/app-major-version-2/hosts.xml
+++ b/configserver/src/test/apps/app-major-version-7/hosts.xml
diff --git a/configserver/src/test/apps/app-major-version-2/schemas/music.sd b/configserver/src/test/apps/app-major-version-7/schemas/music.sd
index f4b11d1e8e4..f4b11d1e8e4 100644
--- a/configserver/src/test/apps/app-major-version-2/schemas/music.sd
+++ b/configserver/src/test/apps/app-major-version-7/schemas/music.sd
diff --git a/configserver/src/test/apps/app-major-version-2/services.xml b/configserver/src/test/apps/app-major-version-7/services.xml
index 5b80c10dee2..5b80c10dee2 100644
--- a/configserver/src/test/apps/app-major-version-2/services.xml
+++ b/configserver/src/test/apps/app-major-version-7/services.xml
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
index 89f1548e9eb..f43d8ebebd3 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
@@ -236,37 +236,19 @@ public class SessionRepositoryTest {
     @Test
     public void require_that_an_application_package_can_limit_to_one_major_version() throws Exception {
         MockModelFactory failingFactory = new MockModelFactory();
-        failingFactory.vespaVersion = new Version(3, 0, 0);
+        failingFactory.vespaVersion = new Version(8, 0, 0);
         failingFactory.throwErrorOnLoad = true;
 
         MockModelFactory okFactory = new MockModelFactory();
-        okFactory.vespaVersion = new Version(2, 0, 0);
+        okFactory.vespaVersion = new Version(7, 0, 0);
         okFactory.throwErrorOnLoad = false;
 
         setup(new ModelFactoryRegistry(List.of(okFactory, failingFactory)));
 
-        File testApp = new File("src/test/apps/app-major-version-2");
-        deploy(applicationId, testApp);
+        File testApp = new File("src/test/apps/app-major-version-7");
+        deploy(new PrepareParams.Builder().applicationId(applicationId).vespaVersion(okFactory.vespaVersion).build(), testApp);
 
-        // Does not cause an error because model version 3 is skipped
-    }
-
-    @Test
-    public void require_that_an_application_package_can_limit_to_one_higher_major_version() throws Exception {
-        MockModelFactory failingFactory = new MockModelFactory();
-        failingFactory.vespaVersion = new Version(3, 0, 0);
-        failingFactory.throwErrorOnLoad = true;
-
-        MockModelFactory okFactory = new MockModelFactory();
-        okFactory.vespaVersion = new Version(1, 0, 0);
-        okFactory.throwErrorOnLoad = false;
-
-        setup(new ModelFactoryRegistry(List.of(okFactory, failingFactory)));
-
-        File testApp = new File("src/test/apps/app-major-version-2");
-        deploy(applicationId, testApp);
-
-        // Does not cause an error because model version 3 is skipped
+        // Does not cause an error because model version 8 is skipped
     }
 
     @Test
@@ -341,8 +323,12 @@ public class SessionRepositoryTest {
     }
 
     private long deploy(ApplicationId applicationId, File testApp) {
-        applicationRepository.deploy(testApp, new PrepareParams.Builder().applicationId(applicationId).build());
-        return applicationRepository.getActiveSession(applicationId).get().getSessionId();
+        return deploy(new PrepareParams.Builder().applicationId(applicationId).build(), testApp);
+    }
+
+    private long deploy(PrepareParams params, File testApp) {
+        applicationRepository.deploy(testApp, params);
+        return applicationRepository.getActiveSession(params.getApplicationId()).get().getSessionId();
     }
 
     private static class MockModelFactory implements ModelFactory {