Always use a fallback provisioner in hosted Vespa

If we introduce a new cluster or change the number of nodes in an
application between versions we will try to load the model (at startup,
so ActivatedModelsBuilder will be used) and discover that we need to provision
additional nodes. Since ActivatedModelsBuilder had set the fallback provisioner
to null this failed spectacularly. Make sure to always use node repository
provisioner as fallback in this case.

5 files changed, 24 insertions, 31 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
index 0ebb83f5cd1..b6346677d6b 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ActivatedModelsBuilder.java
@@ -1,7 +1,6 @@
 // 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.modelfactory;
 
-import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ConfigDefinitionRepo;
@@ -16,6 +15,7 @@ import com.yahoo.config.provision.Version;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.config.server.ConfigServerSpec;
 import com.yahoo.vespa.config.server.GlobalComponentRegistry;
+import com.yahoo.vespa.config.server.provision.HostProvisionerProvider;
 import com.yahoo.vespa.config.server.tenant.Rotations;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.config.server.application.Application;
@@ -54,7 +54,8 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
     public ActivatedModelsBuilder(TenantName tenant, long appGeneration, SessionZooKeeperClient zkClient, GlobalComponentRegistry globalComponentRegistry) {
         super(globalComponentRegistry.getModelFactoryRegistry(),
               globalComponentRegistry.getConfigserverConfig(),
-              globalComponentRegistry.getZone());
+              globalComponentRegistry.getZone(),
+              HostProvisionerProvider.from(globalComponentRegistry.getHostProvisioner()));
         this.tenant = tenant;
         this.appGeneration = appGeneration;
         this.zkClient = zkClient;
@@ -74,6 +75,7 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
                                             Instant now) {
         log.log(LogLevel.DEBUG, String.format("Loading model version %s for session %s application %s",
                                               modelFactory.getVersion(), appGeneration, applicationId));
+        ModelContext.Properties modelContextProperties = createModelContextProperties(applicationId);
         ModelContext modelContext = new ModelContextImpl(
                 applicationPackage,
                 Optional.empty(),
@@ -81,8 +83,8 @@ public class ActivatedModelsBuilder extends ModelsBuilder<Application> {
                 logger,
                 configDefinitionRepo,
                 getForVersionOrLatest(applicationPackage.getFileRegistryMap(), modelFactory.getVersion()).orElse(new MockFileRegistry()),
-                createStaticProvisioner(applicationPackage.getAllocatedHosts()),
-                createModelContextProperties(applicationId),
+                createStaticProvisioner(applicationPackage.getAllocatedHosts(), modelContextProperties),
+                modelContextProperties,
                 Optional.empty(),
                 new com.yahoo.component.Version(modelFactory.getVersion().toString()),
                 wantedNodeVespaVersion);
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 15834a9eaa0..27343e71a87 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
@@ -5,6 +5,7 @@ import com.google.common.util.concurrent.UncheckedTimeoutException;
 import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.model.api.HostProvisioner;
+import com.yahoo.config.model.api.ModelContext;
 import com.yahoo.config.model.api.ModelFactory;
 import com.yahoo.config.provision.AllocatedHosts;
 import com.yahoo.config.provision.ApplicationId;
@@ -16,6 +17,8 @@ import com.yahoo.lang.SettableOptional;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.config.server.http.InternalServerException;
 import com.yahoo.vespa.config.server.http.UnknownVespaVersionException;
+import com.yahoo.vespa.config.server.provision.HostProvisionerProvider;
+import com.yahoo.vespa.config.server.provision.ProvisionerAdapter;
 import com.yahoo.vespa.config.server.provision.StaticProvisioner;
 
 import java.time.Instant;
@@ -48,11 +51,15 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
 
     private final Zone zone;
 
-    protected ModelsBuilder(ModelFactoryRegistry modelFactoryRegistry, ConfigserverConfig configserverConfig, Zone zone) {
+    private final HostProvisionerProvider hostProvisionerProvider;
+
+    ModelsBuilder(ModelFactoryRegistry modelFactoryRegistry, ConfigserverConfig configserverConfig,
+                  Zone zone, HostProvisionerProvider hostProvisionerProvider) {
         this.modelFactoryRegistry = modelFactoryRegistry;
         this.configserverConfig = configserverConfig;
         this.hosted = configserverConfig.hostedVespa();
         this.zone = zone;
+        this.hostProvisionerProvider = hostProvisionerProvider;
     }
 
     /** Returns the zone this is running in */
@@ -211,10 +218,15 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
      * returns empty otherwise, which may either mean that no hosts are allocated or that we are running
      * non-hosted and should default to use hosts defined in the application package, depending on context
      */
-    protected Optional<HostProvisioner> createStaticProvisioner(Optional<AllocatedHosts> allocatedHosts) {
+    Optional<HostProvisioner> createStaticProvisioner(Optional<AllocatedHosts> allocatedHosts, ModelContext.Properties properties) {
         if (hosted && allocatedHosts.isPresent())
-            return Optional.of(new StaticProvisioner(allocatedHosts.get()));
+            return Optional.of(new StaticProvisioner(allocatedHosts.get(), createNodeRepositoryProvisioner(properties).get()));
         return Optional.empty();
     }
 
+    Optional<HostProvisioner> createNodeRepositoryProvisioner(ModelContext.Properties properties) {
+        return hostProvisionerProvider.getHostProvisioner().map(
+                provisioner -> new ProvisionerAdapter(provisioner, properties.applicationId()));
+    }
+
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
index 56bdd432d90..4c43abf5faa 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/PreparedModelsBuilder.java
@@ -25,7 +25,6 @@ import com.yahoo.vespa.config.server.application.PermanentApplicationPackage;
 import com.yahoo.vespa.config.server.deploy.ModelContextImpl;
 import com.yahoo.vespa.config.server.filedistribution.FileDistributionProvider;
 import com.yahoo.vespa.config.server.provision.HostProvisionerProvider;
-import com.yahoo.vespa.config.server.provision.ProvisionerAdapter;
 import com.yahoo.vespa.config.server.provision.StaticProvisioner;
 import com.yahoo.vespa.config.server.session.FileDistributionFactory;
 import com.yahoo.vespa.config.server.session.PrepareParams;
@@ -52,7 +51,6 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
     private final DeployLogger logger;
     private final PrepareParams params;
     private final FileDistributionFactory fileDistributionFactory;
-    private final HostProvisionerProvider hostProvisionerProvider;
     private final Optional<ApplicationSet> currentActiveApplicationSet;
     private final ModelContext.Properties properties;
 
@@ -67,12 +65,11 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
                                  Optional<ApplicationSet> currentActiveApplicationSet,
                                  ModelContext.Properties properties,
                                  ConfigserverConfig configserverConfig) {
-        super(modelFactoryRegistry, configserverConfig, properties.zone());
+        super(modelFactoryRegistry, configserverConfig, properties.zone(), hostProvisionerProvider);
         this.permanentApplicationPackage = permanentApplicationPackage;
         this.configDefinitionRepo = configDefinitionRepo;
 
         this.fileDistributionFactory = fileDistributionFactory;
-        this.hostProvisionerProvider = hostProvisionerProvider;
 
         this.context = context;
         this.logger = logger;
@@ -127,12 +124,8 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
         Optional<HostProvisioner> nodeRepositoryProvisioner = createNodeRepositoryProvisioner(properties);
         if ( ! allocatedHosts.isPresent()) return nodeRepositoryProvisioner;
         
-        Optional<HostProvisioner> staticProvisioner = createStaticProvisioner(allocatedHosts);
+        Optional<HostProvisioner> staticProvisioner = createStaticProvisioner(allocatedHosts, properties);
         if ( ! staticProvisioner.isPresent()) return Optional.empty(); // Since we have hosts allocated this means we are on non-hosted
-
-        // The following option should not be possible, but since there is a right action for it we can take it
-        if ( ! nodeRepositoryProvisioner.isPresent())
-            return Optional.of(new StaticProvisioner(allocatedHosts.get()));
             
         // Nodes are already allocated by a model and we should use them unless this model requests hosts from a
         // previously unallocated cluster. This allows future models to stop allocate certain clusters.
@@ -154,12 +147,6 @@ public class PreparedModelsBuilder extends ModelsBuilder<PreparedModelsBuilder.P
                 .collect(Collectors.toList()));
     }
 
-    private Optional<HostProvisioner> createNodeRepositoryProvisioner(ModelContext.Properties properties) {
-        return hostProvisionerProvider.getHostProvisioner().map(
-                provisioner -> new ProvisionerAdapter(provisioner, properties.applicationId()));
-    }
-
-
     /** The result of preparing a single model version */
     public static class PreparedModelResult implements ModelResult {
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java b/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java
index 7e97690331f..b31f4501767 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/provision/StaticProvisioner.java
@@ -5,7 +5,6 @@ import com.yahoo.config.model.api.HostProvisioner;
 import com.yahoo.config.provision.*;
 
 import java.util.List;
-import java.util.Optional;
 import java.util.stream.Collectors;
 
 /**
@@ -21,13 +20,6 @@ public class StaticProvisioner implements HostProvisioner {
     private final HostProvisioner fallback;
 
     /**
-     * Creates a static host provisioner with no fallback
-     */
-    public StaticProvisioner(AllocatedHosts allocatedHosts) {
-        this(allocatedHosts, null);
-    }
-
-    /**
      * Creates a static host provisioner which will fall back to using the given provisioner
      * if a request is made for nodes in a cluster which is not present in this allocation.
      */
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/provision/StaticProvisionerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/provision/StaticProvisionerTest.java
index badcdf53b77..17ad741d182 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/provision/StaticProvisionerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/provision/StaticProvisionerTest.java
@@ -30,7 +30,7 @@ public class StaticProvisionerTest {
         InMemoryProvisioner inMemoryHostProvisioner = new InMemoryProvisioner(false, "host1.yahoo.com", "host2.yahoo.com", "host3.yahoo.com", "host4.yahoo.com");
         VespaModel firstModel = createModel(app, inMemoryHostProvisioner);
 
-        StaticProvisioner staticProvisioner = new StaticProvisioner(firstModel.allocatedHosts());
+        StaticProvisioner staticProvisioner = new StaticProvisioner(firstModel.allocatedHosts(), null);
         VespaModel secondModel = createModel(app, staticProvisioner);
 
         assertModelConfig(firstModel, secondModel);