Build latest model version only for latest major, unless that fails

3 files changed, 82 insertions, 37 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 19a568d9e01..ba6d1364311 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
@@ -71,8 +71,8 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
      * @param allocatedHosts the newest version (major and minor) (which is loaded first) decides the allocated hosts
      *                       and assigns to this SettableOptional such that it can be used after this method returns
      */
-    public List<MODELRESULT> buildModels(ApplicationId applicationId, 
-                                         com.yahoo.component.Version wantedNodeVespaVersion, 
+    public List<MODELRESULT> buildModels(ApplicationId applicationId,
+                                         Version wantedNodeVespaVersion,
                                          ApplicationPackage applicationPackage,
                                          SettableOptional<AllocatedHosts> allocatedHosts,
                                          Instant now) {
@@ -87,9 +87,9 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                                                        requestedMajorVersion.get() + " are present");
         }
 
-        // Load models by one major version at the time as new major versions are allowed to be non-loadable
-        // in the case where an existing application is incompatible with a new major version
-        // (which is possible by the definition of major)
+        // Load models one major version at a time (in reverse order) as new major versions are allowed
+        // to be non-loadable in the case where an existing application is incompatible with a new
+        // major version (which is possible by the definition of major)
         List<Integer> majorVersions = versions.stream()
                                               .map(Version::getMajor)
                                               .distinct()
@@ -97,15 +97,19 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                                               .collect(Collectors.toList());
 
         List<MODELRESULT> allApplicationModels = new ArrayList<>();
+        // Build latest model for latest major only, if that fails build latest model for previous major
+        boolean buildLatestModelForThisMajor = true;
         for (int i = 0; i < majorVersions.size(); i++) {
+            int majorVersion = majorVersions.get(i);
             try {
-                allApplicationModels.addAll(buildModelVersions(keepMajorVersion(majorVersions.get(i), versions),
+                allApplicationModels.addAll(buildModelVersions(keepMajorVersion(majorVersion, versions),
                                                                applicationId, wantedNodeVespaVersion, applicationPackage,
-                                                               allocatedHosts, now));
+                                                               allocatedHosts, now, buildLatestModelForThisMajor, majorVersion));
 
                 // skip old config models if requested after we have found a major version which works
                 if (allApplicationModels.size() > 0 && allApplicationModels.get(0).getModel().skipOldConfigModels(now))
                     break;
+                buildLatestModelForThisMajor = false; // We have successfully built latest model version, do it only for this major
             }
             catch (OutOfCapacityException | ApplicationLockException e) {
                 // Don't wrap this exception, and don't try to load other model versions as this is (most likely)
@@ -133,33 +137,39 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
     // versions is the set of versions for one particular major version
     private List<MODELRESULT> buildModelVersions(Set<Version> versions,
                                                  ApplicationId applicationId,
-                                                 com.yahoo.component.Version wantedNodeVespaVersion,
+                                                 Version wantedNodeVespaVersion,
                                                  ApplicationPackage applicationPackage,
                                                  SettableOptional<AllocatedHosts> allocatedHosts,
-                                                 Instant now) {
-        Version latest = findLatest(versions);
-        // load latest application version
-        MODELRESULT latestModelVersion = buildModelVersion(modelFactoryRegistry.getFactory(latest), 
-                                                           applicationPackage, 
-                                                           applicationId, 
-                                                           wantedNodeVespaVersion, 
-                                                           allocatedHosts.asOptional(),
-                                                           now);
-        allocatedHosts.set(latestModelVersion.getModel().allocatedHosts()); // Update with additional clusters allocated
-        List<MODELRESULT> allApplicationVersions = new ArrayList<>(Collections.singletonList(latestModelVersion));
-
-        if (latestModelVersion.getModel().skipOldConfigModels(now))
-            return allApplicationVersions;
+                                                 Instant now,
+                                                 boolean buildLatestModelForThisMajor,
+                                                 int majorVersion) {
+        List<MODELRESULT> allApplicationVersions = new ArrayList<>();
+        Optional<Version> latest = Optional.empty();
+        if (buildLatestModelForThisMajor) {
+            latest = Optional.of(findLatest(versions));
+            // load latest application version
+            MODELRESULT latestModelVersion = buildModelVersion(modelFactoryRegistry.getFactory(latest.get()),
+                                                               applicationPackage,
+                                                               applicationId,
+                                                               wantedNodeVespaVersion,
+                                                               allocatedHosts.asOptional(),
+                                                               now);
+            allocatedHosts.set(latestModelVersion.getModel().allocatedHosts()); // Update with additional clusters allocated
+            allApplicationVersions.add(latestModelVersion);
+
+            if (latestModelVersion.getModel().skipOldConfigModels(now))
+                return allApplicationVersions;
+        }
 
         // load old model versions
-        versions = versionsToBuild(versions, wantedNodeVespaVersion, latest.getMajor(), allocatedHosts.get());
+        versions = versionsToBuild(versions, wantedNodeVespaVersion, majorVersion, allocatedHosts.get());
         // TODO: We use the allocated hosts from the newest version when building older model versions.
         // This is correct except for the case where an old model specifies a cluster which the new version
         // does not. In that case we really want to extend the set of allocated hosts to include those of that
         // cluster as well. To do that, create a new provisioner which uses static provisioning for known
         // clusters and the node repository provisioner as fallback.
         for (Version version : versions) {
-            if (version.equals(latest)) continue; // already loaded
+            if (latest.isPresent() && version.equals(latest.get())) continue; // already loaded
 
             MODELRESULT modelVersion = buildModelVersion(modelFactoryRegistry.getFactory(version),
                                                          applicationPackage,
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 c6acbd93241..d64cf963be7 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
@@ -96,11 +96,14 @@ public class HostedDeployTest {
         assertEquals(3, tester.getAllocatedHostsOf(tester.applicationId()).getHosts().size());
     }
 
-    /** Test that only the minimal set of models are created (model versions used on hosts, the wanted version and the latest version) */
+    /**
+     * Test that only the minimal set of models are created (model versions used on hosts, the wanted version
+     * and the latest version for the latest major)
+     */
     @Test
     public void testCreateOnlyNeededModelVersions() {
         List<Host> hosts = Arrays.asList(createHost("host1", "6.0.0"),
-                                         createHost("host2", "6.2.0"),
+                                         createHost("host2", "6.1.0"),
                                          createHost("host3")); //Use a host with no version as well
         InMemoryProvisioner provisioner = new InMemoryProvisioner(new Hosts(hosts), true);
 
@@ -124,15 +127,18 @@ public class HostedDeployTest {
 
         // Check >0 not ==0 as the session watcher thread is running and will redeploy models in the background
         assertTrue(factory600.creationCount() > 0);
-        assertFalse(factory610.creationCount() > 0);
-        assertTrue(factory620.creationCount() > 0);
-        assertTrue(factory700.creationCount() > 0);
+        assertTrue(factory610.creationCount() > 0);
+        assertFalse(factory620.creationCount() > 0); // Latest model version on a major, but not for the latest major
+        assertTrue(factory700.creationCount() > 0);  // Wanted version, also needs to be built
         assertFalse(factory710.creationCount() > 0);
         assertTrue("Newest is always included", factory720.creationCount() > 0);
     }
 
 
-    /** Test that only the minimal set of models are created (the wanted version and the latest version per major, since nodes are without version) */
+    /**
+     * Test that only the minimal set of models are created (the wanted version and the latest version for
+     * the latest major, since nodes are without version)
+     */
     @Test
     public void testCreateOnlyNeededModelVersionsNewNodes() {
         List<Host> hosts = Arrays.asList(createHost("host1"), createHost("host2"), createHost("host3"));
@@ -141,9 +147,8 @@ public class HostedDeployTest {
         CountingModelFactory factory600 = DeployTester.createModelFactory(Version.fromString("6.0.0"));
         CountingModelFactory factory610 = DeployTester.createModelFactory(Version.fromString("6.1.0"));
         CountingModelFactory factory700 = DeployTester.createModelFactory(Version.fromString("7.0.0"));
-        CountingModelFactory factory710 = DeployTester.createModelFactory(Version.fromString("7.1.0"));
         CountingModelFactory factory720 = DeployTester.createModelFactory(Version.fromString("7.2.0"));
-        List<ModelFactory> modelFactories = Arrays.asList(factory600, factory610, factory700, factory710, factory720);
+        List<ModelFactory> modelFactories = Arrays.asList(factory600, factory610, factory700, factory720);
 
         DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(), Clock.systemUTC(), provisioner);
         // Deploy with version that does not exist on hosts, the model for this version should also be created
@@ -151,11 +156,34 @@ public class HostedDeployTest {
         assertEquals(3, tester.getAllocatedHostsOf(tester.applicationId()).getHosts().size());
 
         // Check >0 not ==0 as the session watcher thread is running and will redeploy models in the background
-        assertFalse(factory600.creationCount() > 0); // latest on major version 6
-        assertTrue("Newest per major version is always included", factory610.creationCount() > 0);
         assertTrue(factory700.creationCount() > 0);
-        assertFalse(factory710.creationCount() > 0);
-        assertTrue("Newest per major version is always included", factory720.creationCount() > 0);
+        assertTrue("Newest model for latest major version is always included", factory720.creationCount() > 0);
+    }
+
+    /**
+     * Test that we skip create the minimal set of models are created, but latest model version is created for
+     * previous major if creating latest model version on latest major version fails
+     **/
+    @Test
+    public void testCreateLatestMajorOnPreviousMajorIfItFailsOnNewestMajor() {
+        List<Host> hosts = Arrays.asList(createHost("host1", "6.0.0"),
+                                         createHost("host2", "6.1.0"),
+                                         createHost("host3", "6.1.0"));
+        InMemoryProvisioner provisioner = new InMemoryProvisioner(new Hosts(hosts), true);
+
+        CountingModelFactory factory600 = DeployTester.createModelFactory(Version.fromString("6.0.0"));
+        CountingModelFactory factory610 = DeployTester.createModelFactory(Version.fromString("6.1.0"));
+        ModelFactory factory720 = DeployTester.createFailingModelFactory(Version.fromString("7.2.0"));
+        List<ModelFactory> modelFactories = Arrays.asList(factory600, factory610, factory720);
+
+        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(), Clock.systemUTC(), provisioner);
+        tester.deployApp("src/test/apps/hosted/", "6.0.0", Instant.now());
+        assertEquals(3, 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(factory600.creationCount() > 0);
+        assertTrue("Latest model for previous major version is included if latest model for latest major version fails to build",
+                   factory610.creationCount() > 0);
     }
 
     /**
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
index 93f5f4f4ba4..3d5cfc40ec4 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.session;
 
+import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ModelContext;
 import com.yahoo.config.model.api.ModelCreateResult;
@@ -136,7 +137,7 @@ public class SessionPreparerTest {
     @Test
     public void require_that_application_is_prepared() throws Exception {
         prepare(testApp);
-        assertThat(fileDistributionFactory.mockFileDistributionProvider.timesCalled, is(2));
+        assertThat(fileDistributionFactory.mockFileDistributionProvider.timesCalled, is(1)); // Only builds the newest version
         assertTrue(configCurator.exists(sessionsPath.append(ConfigCurator.USERAPP_ZK_SUBPATH).append("services.xml").getAbsolute()));
     }
 
@@ -151,6 +152,12 @@ public class SessionPreparerTest {
 
     @Test(expected = InvalidApplicationException.class)
     public void require_that_prepare_fails_if_older_version_fails() throws IOException {
+        ConfigserverConfig config = new ConfigserverConfig.Builder()
+                .buildMinimalSetOfConfigModels(false)
+                .configServerDBDir(folder.newFolder("serverdb").getAbsolutePath())
+                .configDefinitionsDir(folder.newFolder("configdefinitions").getAbsolutePath())
+                .build();
+        componentRegistry = new TestComponentRegistry.Builder().curator(curator).configServerConfig(config).build();
         ModelFactoryRegistry modelFactoryRegistry = new ModelFactoryRegistry(Arrays.asList(
                 new TestModelFactory(version323),
                 new FailingModelFactory(version123, new IllegalArgumentException("BOOHOO"))));