Make sure to build config model for wanted vespa version as well

When we build the needed config model versions based on
versions used on allocated hosts:
Add wanted vespa version to the set of models to be built, since
this is needed when wanted vespa version is not the latest vespa version
and there are no nodes having the wanted vespa version.

6 files changed, 67 insertions, 58 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 b0cb287ed0b..560f650dcba 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
@@ -94,9 +94,9 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         List<MODELRESULT> allApplicationModels = new ArrayList<>();
         for (int i = 0; i < majorVersions.size(); i++) {
             try {
-                allApplicationModels.addAll(buildModelVersion(filterByMajorVersion(majorVersions.get(i), versions),
-                                                              applicationId, wantedNodeVespaVersion, applicationPackage, 
-                                                              allocatedHosts, now));
+                allApplicationModels.addAll(buildModelVersions(filterByMajorVersion(majorVersions.get(i), versions),
+                                                               applicationId, wantedNodeVespaVersion, applicationPackage,
+                                                               allocatedHosts, now));
 
                 // 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))
@@ -125,11 +125,13 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         return allApplicationModels;
     }
 
-    private List<MODELRESULT> buildModelVersion(Set<Version> versions, ApplicationId applicationId,
-                                                com.yahoo.component.Version wantedNodeVespaVersion, 
-                                                ApplicationPackage applicationPackage,
-                                                SettableOptional<AllocatedHosts> allocatedHosts,
-                                                Instant now) {
+    // 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,
+                                                 ApplicationPackage applicationPackage,
+                                                 SettableOptional<AllocatedHosts> allocatedHosts,
+                                                 Instant now) {
         Version latest = findLatest(versions);
         // load latest application version
         MODELRESULT latestModelVersion = buildModelVersion(modelFactoryRegistry.getFactory(latest), 
@@ -152,6 +154,11 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                 || Arrays.asList("corp-us-east-1", "ap-southeast-1").contains(zone().region().value()))
             versions = keepThoseUsedOn(allocatedHosts.get(), versions);
 
+        // Make sure we build wanted version if we are building models for this major version
+        Version wantedVersion = Version.fromIntValues(wantedNodeVespaVersion.getMajor(), wantedNodeVespaVersion.getMinor(), wantedNodeVespaVersion.getMicro());
+        if (wantedVersion.getMajor() == latest.getMajor())
+            versions.add(wantedVersion);
+
         // 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
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/PrepareParams.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/PrepareParams.java
index b53ee9a5988..f53a48b3783 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/PrepareParams.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/PrepareParams.java
@@ -95,6 +95,11 @@ public final class PrepareParams {
             return this;
         }
 
+        public Builder vespaVersion(com.yahoo.config.provision.Version vespaVersion) {
+            this.vespaVersion = Optional.ofNullable(Version.fromString(vespaVersion.toSerializedForm()));
+            return this;
+        }
+
         public Builder rotations(String rotationsString) {
             this.rotations = new LinkedHashSet<>();
             if (rotationsString != null && !rotationsString.isEmpty()) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/TestComponentRegistry.java b/configserver/src/test/java/com/yahoo/vespa/config/server/TestComponentRegistry.java
index d2965b8a489..5f00499598a 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/TestComponentRegistry.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/TestComponentRegistry.java
@@ -23,7 +23,6 @@ import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.config.server.zookeeper.ConfigCurator;
 import com.yahoo.vespa.model.VespaModelFactory;
 
-import java.io.File;
 import java.time.Clock;
 import java.util.Collections;
 import java.util.Optional;
@@ -155,7 +154,7 @@ public class TestComponentRegistry implements GlobalComponentRegistry {
             final PermanentApplicationPackage permApp = this.permanentApplicationPackage
                     .orElse(new PermanentApplicationPackage(configserverConfig));
             FileDistributionFactory fileDistributionFactory = this.fileDistributionFactory
-                    .orElse(new MockFileDistributionFactory(new File(configserverConfig.fileReferencesDir())));
+                    .orElse(new MockFileDistributionFactory(configserverConfig));
             HostProvisionerProvider hostProvisionerProvider = hostProvisioner.isPresent() ?
                     HostProvisionerProvider.withProvisioner(hostProvisioner.get()) :
                     HostProvisionerProvider.empty();
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 b151ac57352..6ee4720b6b0 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
@@ -16,9 +16,9 @@ import com.yahoo.test.ManualClock;
 import static com.yahoo.vespa.config.server.deploy.DeployTester.CountingModelFactory;
 
 import com.yahoo.vespa.config.server.session.LocalSession;
-import org.junit.Ignore;
 import org.junit.Test;
 
+import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -38,7 +38,8 @@ public class HostedDeployTest {
 
     @Test
     public void testRedeployWithVersion() {
-        DeployTester tester = new DeployTester("src/test/apps/hosted/", createConfigserverConfig());
+        CountingModelFactory modelFactory = DeployTester.createModelFactory(Version.fromString("4.5.6"), Clock.systemUTC());
+        DeployTester tester = new DeployTester("src/test/apps/hosted/", Collections.singletonList(modelFactory), createConfigserverConfig());
         tester.deployApp("myApp", "4.5.6", Instant.now());
 
         Optional<com.yahoo.config.provision.Deployment> deployment = tester.redeployFromLocalActive();
@@ -73,14 +74,14 @@ public class HostedDeployTest {
         modelFactories.add(DeployTester.createModelFactory(Version.fromString("6.2.0"), clock));
         modelFactories.add(DeployTester.createModelFactory(Version.fromString("7.0.0"), clock));
         DeployTester tester = new DeployTester("src/test/apps/hosted/", modelFactories, createConfigserverConfig(), clock, Zone.defaultZone());
-        ApplicationId app = tester.deployApp("myApp", Instant.now());
+        ApplicationId app = tester.deployApp("myApp", "6.2.0", Instant.now());
         assertEquals(3, tester.getAllocatedHostsOf(app).getHosts().size());
 
     }
 
-    /** Test that unused versions are skipped in dev */
+    /** Test that only the minimal set of models are created (model versions used on hosts, the wanted version and the latest version) */
     @Test
-    public void testDeployMultipleVersionsInDev() {
+    public void testCreateOnlyNeededModelVersions() {
         List<Host> hosts = new ArrayList<>();
         hosts.add(createHost("host1", "6.0.0"));
         hosts.add(createHost("host2", "6.0.2"));
@@ -104,14 +105,15 @@ public class HostedDeployTest {
 
         DeployTester tester = new DeployTester("src/test/apps/hosted/", modelFactories, createConfigserverConfig(),
                                                clock, new Zone(Environment.dev, RegionName.defaultName()), provisioner);
-        ApplicationId app = tester.deployApp("myApp", Instant.now());
+        // Deploy with version that does not exist on hosts, the model for this version should also be created
+        ApplicationId app = tester.deployApp("myApp", "7.0.0", Instant.now());
         assertEquals(3, tester.getAllocatedHostsOf(app).getHosts().size());
 
         // 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);
-        assertFalse(factory700.creationCount() > 0);
+        assertTrue(factory700.creationCount() > 0);
         assertTrue(factory710.creationCount() > 0);
         assertTrue("Newest is always included", factory720.creationCount() > 0);
     }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/MockFileDistributionFactory.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/MockFileDistributionFactory.java
index 2c05017d449..e1874c622c2 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/MockFileDistributionFactory.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/MockFileDistributionFactory.java
@@ -14,9 +14,9 @@ public class MockFileDistributionFactory extends FileDistributionFactory {
 
     public final MockFileDistributionProvider mockFileDistributionProvider;
 
-    public MockFileDistributionFactory(File fileReferencesDir) {
-        super(new ConfigserverConfig(new ConfigserverConfig.Builder()));
-        mockFileDistributionProvider = new MockFileDistributionProvider(fileReferencesDir);
+    public MockFileDistributionFactory(ConfigserverConfig configserverConfig) {
+        super(configserverConfig);
+        mockFileDistributionProvider = new MockFileDistributionProvider(new File(configserverConfig.fileReferencesDir()));
     }
 
     @Override
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 92fb67fdd54..6f26323b558 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
@@ -21,6 +21,7 @@ import com.yahoo.vespa.config.server.*;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.vespa.config.server.application.MemoryTenantApplications;
 import com.yahoo.vespa.config.server.application.PermanentApplicationPackage;
+import com.yahoo.vespa.config.server.configchange.ConfigChangeActions;
 import com.yahoo.vespa.config.server.configchange.MockRestartAction;
 import com.yahoo.vespa.config.server.configchange.RestartActions;
 import com.yahoo.vespa.config.server.deploy.DeployHandlerLogger;
@@ -49,8 +50,7 @@ import static org.hamcrest.Matchers.contains;
 import static org.junit.Assert.*;
 
 /**
- * @author lulf
- * @since 5.1
+ * @author Ulf Lilleengen
  */
 public class SessionPreparerTest {
 
@@ -58,6 +58,10 @@ public class SessionPreparerTest {
     private static final Path sessionsPath = tenantPath.append("sessions").append("testapp");
     private static final File testApp = new File("src/test/apps/app");
     private static final File invalidTestApp = new File("src/test/apps/illegalApp");
+    private static final Version version123 = Version.fromIntValues(1, 2, 3);
+    private static final Version version124 = Version.fromIntValues(1, 2, 4);
+    private static final Version version321 = Version.fromIntValues(3, 2, 1);
+    private static final Version version323 = Version.fromIntValues(3, 2, 3);
 
     private MockCurator curator;
     private ConfigCurator configCurator;
@@ -83,9 +87,8 @@ public class SessionPreparerTest {
     }
 
     private SessionPreparer createPreparer(HostProvisionerProvider hostProvisionerProvider) {
-        ModelFactoryRegistry modelFactoryRegistry = new ModelFactoryRegistry(Arrays.asList(
-                new TestModelFactory(Version.fromIntValues(1, 2, 3)),
-                new TestModelFactory(Version.fromIntValues(3, 2, 1))));
+        ModelFactoryRegistry modelFactoryRegistry =
+                new ModelFactoryRegistry(Arrays.asList(new TestModelFactory(version123), new TestModelFactory(version321)));
         return createPreparer(modelFactoryRegistry, hostProvisionerProvider);
     }
 
@@ -104,37 +107,29 @@ public class SessionPreparerTest {
 
     @Test(expected = InvalidApplicationException.class)
     public void require_that_application_validation_exception_is_not_caught() throws IOException {
-        FilesApplicationPackage app = getApplicationPackage(invalidTestApp);
-        preparer.prepare(getContext(app), getLogger(), new PrepareParams.Builder().build(), Optional.empty(), tenantPath, Instant.now());
+        prepare(invalidTestApp);
     }
 
     @Test
     public void require_that_application_validation_exception_is_ignored_if_forced() throws IOException {
-        FilesApplicationPackage app = getApplicationPackage(invalidTestApp);
-        preparer.prepare(getContext(app), getLogger(),
-                         new PrepareParams.Builder().ignoreValidationErrors(true).timeoutBudget(TimeoutBudgetTest.day()).build(),
-                         Optional.empty(), tenantPath, Instant.now());
+        prepare(invalidTestApp, new PrepareParams.Builder().ignoreValidationErrors(true).timeoutBudget(TimeoutBudgetTest.day()).build());
     }
 
     @Test
     public void require_that_zookeeper_is_not_written_to_if_dryrun() throws IOException {
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(),
-                         new PrepareParams.Builder().dryRun(true).timeoutBudget(TimeoutBudgetTest.day()).build(),
-                         Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp, new PrepareParams.Builder().dryRun(true).timeoutBudget(TimeoutBudgetTest.day()).build());
         assertFalse(configCurator.exists(sessionsPath.append(ConfigCurator.USERAPP_ZK_SUBPATH).append("services.xml").getAbsolute()));
     }
 
     @Test
     public void require_that_filedistribution_is_ignored_on_dryrun() throws IOException {
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(),
-                                                       new PrepareParams.Builder().dryRun(true).timeoutBudget(TimeoutBudgetTest.day()).build(),
-                                                       Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp, new PrepareParams.Builder().dryRun(true).timeoutBudget(TimeoutBudgetTest.day()).build());
         assertThat(fileDistributionFactory.mockFileDistributionProvider.timesCalled, is(0));
     }
 
     @Test
     public void require_that_application_is_prepared() throws Exception {
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(), new PrepareParams.Builder().build(), Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp);
         assertThat(fileDistributionFactory.mockFileDistributionProvider.timesCalled, is(2));
         assertTrue(configCurator.exists(sessionsPath.append(ConfigCurator.USERAPP_ZK_SUBPATH).append("services.xml").getAbsolute()));
     }
@@ -142,19 +137,19 @@ public class SessionPreparerTest {
     @Test
     public void require_that_prepare_succeeds_if_newer_version_fails() throws IOException {
         ModelFactoryRegistry modelFactoryRegistry = new ModelFactoryRegistry(Arrays.asList(
-                new TestModelFactory(Version.fromIntValues(1, 2, 3)),
-                new FailingModelFactory(Version.fromIntValues(3, 2, 1), new IllegalArgumentException("BOOHOO"))));
+                new TestModelFactory(version123),
+                new FailingModelFactory(version321, new IllegalArgumentException("BOOHOO"))));
         preparer = createPreparer(modelFactoryRegistry, HostProvisionerProvider.empty());
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(), new PrepareParams.Builder().build(), Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp);
     }
 
     @Test(expected = InvalidApplicationException.class)
     public void require_that_prepare_fails_if_older_version_fails() throws IOException {
         ModelFactoryRegistry modelFactoryRegistry = new ModelFactoryRegistry(Arrays.asList(
-                new TestModelFactory(Version.fromIntValues(3, 2, 3)),
-                new FailingModelFactory(Version.fromIntValues(1, 2, 1), new IllegalArgumentException("BOOHOO"))));
+                new TestModelFactory(version323),
+                new FailingModelFactory(version123, new IllegalArgumentException("BOOHOO"))));
         preparer = createPreparer(modelFactoryRegistry, HostProvisionerProvider.empty());
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(), new PrepareParams.Builder().build(), Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp);
     }
 
     @Test(expected = InvalidApplicationException.class)
@@ -184,7 +179,7 @@ public class SessionPreparerTest {
                                .tenant(tenant)
                                .applicationName("foo").instanceName("quux").build();
         PrepareParams params = new PrepareParams.Builder().applicationId(origId).build();
-        preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(), params, Optional.empty(), tenantPath, Instant.now());
+        prepare(testApp, params);
         SessionZooKeeperClient zkc = new SessionZooKeeperClient(curator, sessionsPath);
         assertTrue(configCurator.exists(sessionsPath.append(SessionZooKeeperClient.APPLICATION_ID_PATH).getAbsolute()));
         assertThat(zkc.readApplicationId(), is(origId));
@@ -194,15 +189,10 @@ public class SessionPreparerTest {
     public void require_that_config_change_actions_are_collected_from_all_models() throws IOException {
         ServiceInfo service = new ServiceInfo("serviceName", "serviceType", null, new HashMap<>(), "configId", "hostName");
         ModelFactoryRegistry modelFactoryRegistry = new ModelFactoryRegistry(Arrays.asList(
-                new ConfigChangeActionsModelFactory(Version.fromIntValues(1, 2, 3),
-                        new MockRestartAction("change", Arrays.asList(service))),
-                new ConfigChangeActionsModelFactory(Version.fromIntValues(1, 2, 4),
-                        new MockRestartAction("other change", Arrays.asList(service)))));
+                new ConfigChangeActionsModelFactory(version123, new MockRestartAction("change", Arrays.asList(service))),
+                new ConfigChangeActionsModelFactory(version124, new MockRestartAction("other change", Arrays.asList(service)))));
         preparer = createPreparer(modelFactoryRegistry, HostProvisionerProvider.empty());
-        List<RestartActions.Entry> actions =
-                preparer.prepare(getContext(getApplicationPackage(testApp)), getLogger(),
-                                 new PrepareParams.Builder().build(), Optional.empty(), tenantPath, Instant.now())
-                        .getRestartActions().getEntries();
+        List<RestartActions.Entry> actions = prepare(testApp).getRestartActions().getEntries();
         assertThat(actions.size(), is(1));
         assertThat(actions.get(0).getMessages(), equalTo(ImmutableSet.of("change", "other change")));
     }
@@ -216,15 +206,13 @@ public class SessionPreparerTest {
         final String rotations = "mediasearch.msbe.global.vespa.yahooapis.com";
         final ApplicationId applicationId = applicationId("test");
         PrepareParams params = new PrepareParams.Builder().applicationId(applicationId).rotations(rotations).build();
-        File app = new File("src/test/resources/deploy/app");
-        preparer.prepare(getContext(getApplicationPackage(app)), getLogger(), params, Optional.empty(), tenantPath, Instant.now());
+        prepare(new File("src/test/resources/deploy/app"), params);
         assertThat(readRotationsFromZK(applicationId), contains(new Rotation(rotations)));
     }
 
     @Test
     public void require_that_rotations_are_read_from_zookeeper_and_used() throws IOException {
-        final Version vespaVersion = Version.fromIntValues(1, 2, 3);
-        final TestModelFactory modelFactory = new TestModelFactory(vespaVersion);
+        final TestModelFactory modelFactory = new TestModelFactory(version123);
         preparer = createPreparer(new ModelFactoryRegistry(Arrays.asList(modelFactory)),
                 HostProvisionerProvider.empty());
 
@@ -233,7 +221,7 @@ public class SessionPreparerTest {
         new Rotations(curator, tenantPath).writeRotationsToZooKeeper(applicationId, Collections.singleton(new Rotation(rotations)));
         final PrepareParams params = new PrepareParams.Builder().applicationId(applicationId).build();
         final File app = new File("src/test/resources/deploy/app");
-        preparer.prepare(getContext(getApplicationPackage(app)), getLogger(), params, Optional.empty(), tenantPath, Instant.now());
+        prepare(app, params);
 
         // check that the rotation from zookeeper were used
         final ModelContext modelContext = modelFactory.getModelContext();
@@ -244,6 +232,14 @@ public class SessionPreparerTest {
         assertThat(readRotationsFromZK(applicationId), contains(new Rotation(rotations)));
     }
 
+    private ConfigChangeActions prepare(File app) throws IOException {
+        return prepare(app, new PrepareParams.Builder().build());
+    }
+
+    private ConfigChangeActions prepare(File app, PrepareParams params) throws IOException {
+        return preparer.prepare(getContext(getApplicationPackage(app)), getLogger(), params, Optional.empty(), tenantPath, Instant.now());
+    }
+
     private SessionContext getContext(FilesApplicationPackage app) throws IOException {
         return new SessionContext(app, new SessionZooKeeperClient(curator, sessionsPath), app.getAppDir(), new MemoryTenantApplications(), new HostRegistry<>(), new SuperModelGenerationCounter(curator));
     }