Merge pull request #11267 from vespa-engine/hmusum/create-all-needed-config-models-for-manually-deployed-apps

Always try to build config model for old versions

4 files changed, 106 insertions, 29 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
index 1dc0b838470..54807697da4 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
@@ -66,14 +66,20 @@ public class VespaModelFactory implements ModelFactory {
 
         this.clock = Clock.systemUTC();
     }
-    
+
+    // For testing only
     public VespaModelFactory(ConfigModelRegistry configModelRegistry) {
         this(configModelRegistry, Clock.systemUTC());
     }
+
+    // For testing only
     public VespaModelFactory(ConfigModelRegistry configModelRegistry, Clock clock) {
-        this(new Version(VespaVersion.major, VespaVersion.minor, VespaVersion.micro), configModelRegistry, clock);
+        this(new Version(VespaVersion.major, VespaVersion.minor, VespaVersion.micro), configModelRegistry,
+             clock, Zone.defaultZone());
     }
-    public VespaModelFactory(Version version, ConfigModelRegistry configModelRegistry, Clock clock) {
+
+    // For testing only
+    public VespaModelFactory(Version version, ConfigModelRegistry configModelRegistry, Clock clock, Zone zone) {
         this.version = version;
         if (configModelRegistry == null) {
             this.configModelRegistry = new NullConfigModelRegistry();
@@ -82,7 +88,7 @@ public class VespaModelFactory implements ModelFactory {
             this.configModelRegistry = configModelRegistry;
         }
         this.modelImporters = Collections.emptyList();
-        this.zone = Zone.defaultZone();
+        this.zone = zone;
         this.clock = clock;
     }
 
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 e25574c86d3..2de64ed145c 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
@@ -106,10 +106,6 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                 allApplicationModels.addAll(buildModelVersions(keepMajorVersion(majorVersion, versions),
                                                                applicationId, wantedNodeVespaVersion, applicationPackage,
                                                                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 | TransientException e) {
@@ -169,9 +165,6 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
                                                                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
@@ -184,14 +177,24 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> {
         for (Version version : versions) {
             if (latest.isPresent() && version.equals(latest.get())) continue; // already loaded
 
-            MODELRESULT modelVersion = buildModelVersion(modelFactoryRegistry.getFactory(version),
-                                                         applicationPackage,
-                                                         applicationId,
-                                                         wantedNodeVespaVersion,
-                                                         allocatedHosts.asOptional(),
-                                                         now);
-            allocatedHosts.set(modelVersion.getModel().allocatedHosts()); // Update with additional clusters allocated
-            allApplicationVersions.add(modelVersion);
+            MODELRESULT modelVersion;
+            try {
+                modelVersion = buildModelVersion(modelFactoryRegistry.getFactory(version),
+                                                 applicationPackage,
+                                                 applicationId,
+                                                 wantedNodeVespaVersion,
+                                                 allocatedHosts.asOptional(),
+                                                 now);
+                allocatedHosts.set(modelVersion.getModel().allocatedHosts()); // Update with additional clusters allocated
+                allApplicationVersions.add(modelVersion);
+            } catch (RuntimeException e) {
+                // allow failure to create old config models if there is a validation override that allow skipping old
+                // config models (which is always true for manually deployed zones)
+                if (allApplicationVersions.size() > 0 && allApplicationVersions.get(0).getModel().skipOldConfigModels(now))
+                    log.log(LogLevel.INFO, applicationId + ": Skipping old version (due to validation override)");
+                else
+                    throw e;
+            }
         }
         return allApplicationVersions;
     }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/DeployTester.java b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/DeployTester.java
index f619bd92bef..eacc2d5a4fc 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/DeployTester.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/DeployTester.java
@@ -143,12 +143,17 @@ public class DeployTester {
 
     /** Create a model factory for a particular version */
     public static CountingModelFactory createModelFactory(Version version) {
-        return new CountingModelFactory(version, Clock.systemUTC());
+        return createModelFactory(version, Clock.systemUTC());
     }
 
     /** Create a model factory for a particular version */
     public static CountingModelFactory createModelFactory(Version version, Clock clock) {
-        return new CountingModelFactory(version, clock);
+        return createModelFactory(version, clock, Zone.defaultZone());
+    }
+
+    /** Create a model factory for a particular version */
+    public static CountingModelFactory createModelFactory(Version version, Clock clock, Zone zone) {
+        return new CountingModelFactory(version, clock, zone);
     }
 
     /** Create a model factory which always fails validation */
@@ -299,8 +304,8 @@ public class DeployTester {
             this.wrapped = new VespaModelFactory(new NullConfigModelRegistry(), clock);
         }
 
-        public CountingModelFactory(Version version, Clock clock) {
-            this.wrapped = new VespaModelFactory(version, new NullConfigModelRegistry(), clock);
+        public CountingModelFactory(Version version, Clock clock, Zone zone) {
+            this.wrapped = new VespaModelFactory(version, new NullConfigModelRegistry(), clock, zone);
         }
 
         /** Returns the number of models created successfully by this instance */
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 74d3fd4ec74..26cc72672b9 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
@@ -135,7 +135,6 @@ public class HostedDeployTest {
         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 for
      * the latest major, since nodes are without version)
@@ -162,6 +161,63 @@ public class HostedDeployTest {
     }
 
     /**
+     * Test that deploying an application in a manually deployed zone creates all needed model versions
+     * (not just the latest one, manually deployed apps always have skipOldConfigModels set to true)
+     */
+    @Test
+    public void testCreateNeededModelVersionsForManuallyDeployedApps() {
+        List<Host> hosts = List.of(createHost("host1", "7.0.0"), createHost("host2", "7.0.0"),
+                                   createHost("host3", "7.0.0"), createHost("host4", "7.0.0"));
+        InMemoryProvisioner provisioner = new InMemoryProvisioner(new Hosts(hosts), true);
+
+        ManualClock clock = new ManualClock("2019-11-08T00:00:00");
+        Zone zone = new Zone(Environment.dev, RegionName.defaultName());
+        CountingModelFactory factory700 = DeployTester.createModelFactory(Version.fromString("7.0.0"), clock, zone);
+        CountingModelFactory factory710 = DeployTester.createModelFactory(Version.fromString("7.1.0"), clock, zone);
+        CountingModelFactory factory720 = DeployTester.createModelFactory(Version.fromString("7.2.0"), clock, zone);
+        List<ModelFactory> modelFactories = List.of(factory700, factory710, factory720);
+
+        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(zone),
+                                               clock, zone, provisioner);
+        // Deploy with version that does not exist on hosts, the model for this version should also be created
+        tester.deployApp("src/test/apps/hosted/", "7.2.0", Instant.now());
+        assertEquals(4, tester.getAllocatedHostsOf(tester.applicationId()).getHosts().size());
+
+        // Check >0 not ==0 as the session watcher thread is running and will redeploy models in the background
+        // Nodes are on 7.0.0 (should be created), no nodes on 7.1.0 (should not be created), 7.2.0 should always be creates
+        assertTrue(factory700.creationCount() > 0);
+        assertFalse(factory710.creationCount() > 0);
+        assertTrue("Newest model for latest major version is always included", factory720.creationCount() > 0);
+    }
+
+    /**
+     * Test that deploying an application in a manually deployed zone creates latest model version successfully,
+     * even if creating one of the older model fails
+     */
+    @Test
+    public void testCreateModelVersionsForManuallyDeployedAppsWhenCreatingFailsForOneVersion() {
+        List<Host> hosts = List.of(createHost("host1", "7.0.0"), createHost("host2", "7.0.0"),
+                                   createHost("host3", "7.0.0"), createHost("host4", "7.0.0"));
+        InMemoryProvisioner provisioner = new InMemoryProvisioner(new Hosts(hosts), true);
+
+        Zone zone = new Zone(Environment.dev, RegionName.defaultName());
+        ManualClock clock = new ManualClock("2019-11-08T00:00:00");
+        ModelFactory factory700 = DeployTester.createFailingModelFactory(Version.fromString("7.0.0"));
+        CountingModelFactory factory720 = DeployTester.createModelFactory(Version.fromString("7.2.0"), clock, zone);
+        List<ModelFactory> modelFactories = List.of(factory700, factory720);
+
+        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(zone),
+                                               clock, zone, provisioner);
+        // Deploy with version that does not exist on hosts, the model for this version should be created even
+        // if creating 7.0.0 fails
+        tester.deployApp("src/test/apps/hosted/", "7.2.0", Instant.now());
+        assertEquals(4, 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("Newest model for latest major version is always included", factory720.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)
@@ -244,8 +300,7 @@ public class HostedDeployTest {
         CountingModelFactory factory720 = DeployTester.createModelFactory(Version.fromString("7.2.0"), clock);
         List<ModelFactory> modelFactories = Arrays.asList(factory700, factory720);
 
-        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(),
-                                               clock, new Zone(Environment.dev, RegionName.defaultName()), provisioner);
+        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(), clock, provisioner);
         tester.deployApp("src/test/apps/hosted-routing-app/", "7.2.0", Instant.now());
         assertFalse(factory700.creationCount() > 0);
         assertTrue("Newest is always included", factory720.creationCount() > 0);
@@ -262,8 +317,9 @@ public class HostedDeployTest {
         CountingModelFactory factory620 = DeployTester.createModelFactory(Version.fromString("6.2.0"));
         List<ModelFactory> modelFactories = Arrays.asList(factory600, factory610, factory620);
 
-        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(),
-                                               Clock.systemUTC(), new Zone(Environment.prod, RegionName.defaultName()), provisioner);
+        Zone zone = new Zone(Environment.prod, RegionName.defaultName());
+        DeployTester tester = new DeployTester(modelFactories, createConfigserverConfig(zone),
+                                               Clock.systemUTC(), zone, provisioner);
         ApplicationId applicationId = tester.applicationId();
         // Deploy with oldest version
         tester.deployApp("src/test/apps/hosted/", "6.0.0", Instant.now());
@@ -339,11 +395,18 @@ public class HostedDeployTest {
     }
 
     private static ConfigserverConfig createConfigserverConfig() {
+        return createConfigserverConfig(Zone.defaultZone());
+    }
+
+    private static ConfigserverConfig createConfigserverConfig(Zone zone) {
         return new ConfigserverConfig(new ConfigserverConfig.Builder()
                                               .configServerDBDir(Files.createTempDir().getAbsolutePath())
                                               .configDefinitionsDir(Files.createTempDir().getAbsolutePath())
                                               .hostedVespa(true)
-                                              .multitenant(true));
+                                              .multitenant(true)
+                                              .region(zone.region().value())
+                                              .environment(zone.environment().value())
+                                              .system(zone.system().value()));
     }
 
     private Host createHost(String hostname, String version) {