Merge pull request #12643 from vespa-engine/mpolden/avoid-magic-combined-redux-4k

Allocate dedicated nodes when no nodes are specified

8 files changed, 102 insertions, 27 deletions

diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
index ada3119f5bb..0c061dd8222 100644
--- a/config-model-api/abi-spec.json
+++ b/config-model-api/abi-spec.json
@@ -884,7 +884,8 @@
       "public boolean useNewAthenzFilter()",
       "public boolean usePhraseSegmenting()",
       "public java.lang.String proxyProtocol()",
-      "public java.util.Optional athenzDomain()"
+      "public java.util.Optional athenzDomain()",
+      "public boolean useDedicatedNodesWhenUnspecified()"
     ],
     "fields": []
   },
diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java
index 7fcde1b5e6b..b9ada59cb0b 100644
--- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java
+++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java
@@ -67,6 +67,7 @@ public interface ModelContext {
         default boolean usePhraseSegmenting() { return false; }
         default String proxyProtocol() { return "https-only"; }
         default Optional<AthenzDomain> athenzDomain() { return Optional.empty(); }
+        default boolean useDedicatedNodesWhenUnspecified() { return false; }
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java
index e49ceffabc1..802fdcc1dda 100644
--- a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java
+++ b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java
@@ -44,6 +44,7 @@ public class TestProperties implements ModelContext.Properties {
     private Optional<EndpointCertificateSecrets> endpointCertificateSecrets = Optional.empty();
     private boolean useNewAthenzFilter = false;
     private boolean usePhraseSegmenting = false;
+    private boolean useDedicatedNodesWhenUnspecified = false;
     private AthenzDomain athenzDomain;
 
     @Override public boolean multitenant() { return multitenant; }
@@ -66,6 +67,7 @@ public class TestProperties implements ModelContext.Properties {
     @Override public boolean useBucketSpaceMetric() { return true; }
     @Override public boolean useNewAthenzFilter() { return useNewAthenzFilter; }
     @Override public boolean usePhraseSegmenting() { return usePhraseSegmenting; }
+    @Override public boolean useDedicatedNodesWhenUnspecified() { return useDedicatedNodesWhenUnspecified; }
     @Override public Optional<AthenzDomain> athenzDomain() { return Optional.ofNullable(athenzDomain); }
 
     public TestProperties setDefaultTermwiseLimit(double limit) {
@@ -123,6 +125,11 @@ public class TestProperties implements ModelContext.Properties {
         return this;
     }
 
+    public TestProperties setUseDedicatedNodesWhenUnspecified(boolean useDedicatedNodesWhenUnspecified) {
+        this.useDedicatedNodesWhenUnspecified = useDedicatedNodesWhenUnspecified;
+        return this;
+    }
+
     public TestProperties setAthenzDomain(AthenzDomain domain) {
         this.athenzDomain = domain;
         return this;
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index 165404ce9d4..1b2df3d47e3 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -570,12 +570,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
 
     private void addNodesFromXml(ApplicationContainerCluster cluster, Element containerElement, ConfigModelContext context) {
         Element nodesElement = XML.getChild(containerElement, "nodes");
-        if (nodesElement == null) { // default single node on localhost
-            ApplicationContainer node = new ApplicationContainer(cluster, "container.0", 0, cluster.isHostedVespa());
-            HostResource host = allocateSingleNodeHost(cluster, log, containerElement, context);
-            node.setHostResource(host);
-            node.initService(context.getDeployLogger());
-            cluster.addContainers(Collections.singleton(node));
+        if (nodesElement == null) {
+            cluster.addContainers(allocateWithoutNodesTag(cluster, containerElement, context));
         } else {
             List<ApplicationContainer> nodes = createNodes(cluster, nodesElement, context);
 
@@ -652,29 +648,53 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         }
     }
 
-    /** Creates a single host when there is no nodes tag */
-    private HostResource allocateSingleNodeHost(ApplicationContainerCluster cluster, DeployLogger logger, Element containerElement, ConfigModelContext context) {
+    /** Allocate a container cluster without a nodes tag */
+    private List<ApplicationContainer> allocateWithoutNodesTag(ApplicationContainerCluster cluster, Element containerElement, ConfigModelContext context) {
         DeployState deployState = context.getDeployState();
         HostSystem hostSystem = cluster.hostSystem();
         if (deployState.isHosted()) {
-            Optional<HostResource> singleContentHost = getHostResourceFromContentClusters(cluster, containerElement, context);
-            if (singleContentHost.isPresent()) { // there is a content cluster; put the container on its first node 
-                return singleContentHost.get();
+            // TODO(mpolden): The old way of allocating. Remove when 7.198 is the oldest model in production and the
+            //                feature flag is set to true in all zones.
+            if (!context.properties().useDedicatedNodesWhenUnspecified()) {
+                Optional<HostResource> singleContentHost = getHostResourceFromContentClusters(cluster, containerElement, context);
+                if (singleContentHost.isPresent()) { // there is a content cluster; put the container on its first node
+                    return singleHostContainerCluster(cluster, singleContentHost.get(), context);
+                }
+                else { // request 1 node
+                    ClusterSpec clusterSpec = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from(cluster.getName()))
+                                                         .vespaVersion(deployState.getWantedNodeVespaVersion())
+                                                         .dockerImageRepo(deployState.getWantedDockerImageRepo())
+                                                         .build();
+                    Capacity capacity = Capacity.fromCount(1,
+                                                           Optional.empty(),
+                                                           false,
+                                                           ! deployState.getProperties().isBootstrap());
+                    HostResource host = hostSystem.allocateHosts(clusterSpec, capacity, 1, log).keySet().iterator().next();
+                    return singleHostContainerCluster(cluster, host, context);
+                }
             }
-            else { // request 1 node
-                ClusterSpec clusterSpec = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from(cluster.getName()))
-                        .vespaVersion(deployState.getWantedNodeVespaVersion())
-                        .dockerImageRepo(deployState.getWantedDockerImageRepo())
-                        .build();
-                Capacity capacity = Capacity.fromCount(1,
-                                                       Optional.empty(),
-                                                       false,
-                                                       ! deployState.getProperties().isBootstrap());
-                return hostSystem.allocateHosts(clusterSpec, capacity, 1, logger).keySet().iterator().next();
-            }
-        } else {
-            return hostSystem.getHost(Container.SINGLENODE_CONTAINER_SERVICESPEC);
-        }
+            // request just enough nodes to satisfy environment capacity requirement
+            ClusterSpec clusterSpec = ClusterSpec.request(ClusterSpec.Type.container,
+                                                          ClusterSpec.Id.from(cluster.getName()))
+                                                 .vespaVersion(deployState.getWantedNodeVespaVersion())
+                                                 .dockerImageRepo(deployState.getWantedDockerImageRepo())
+                                                 .build();
+            int nodeCount = deployState.zone().environment().isProduction() ? 2 : 1;
+            Capacity capacity = Capacity.fromCount(nodeCount,
+                                                   Optional.empty(),
+                                                   false,
+                                                   !deployState.getProperties().isBootstrap());
+            var hosts = hostSystem.allocateHosts(clusterSpec, capacity, 1, log);
+            return createNodesFromHosts(log, hosts, cluster);
+        }
+        return singleHostContainerCluster(cluster, hostSystem.getHost(Container.SINGLENODE_CONTAINER_SERVICESPEC), context);
+    }
+
+    private List<ApplicationContainer> singleHostContainerCluster(ApplicationContainerCluster cluster, HostResource host, ConfigModelContext context) {
+        ApplicationContainer node = new ApplicationContainer(cluster, "container.0", 0, cluster.isHostedVespa());
+        node.setHostResource(host);
+        node.initService(context.getDeployLogger());
+        return List.of(node);
     }
 
     private List<ApplicationContainer> createNodesFromNodeCount(ApplicationContainerCluster cluster, Element nodesElement, ConfigModelContext context) {
diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
index d215fdbb7a0..1670ac23ba4 100644
--- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
+++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
@@ -1398,6 +1398,31 @@ public class ModelProvisioningTest {
     }
 
     @Test
+    public void testNoNodeTagMeansTwoNodesInContainerClusterWithFeatureFlag() {
+        String services =
+                "<?xml version='1.0' encoding='utf-8' ?>\n" +
+                "<services>" +
+                "  <container id='foo' version='1.0'>" +
+                "    <search/>" +
+                "    <document-api/>" +
+                "  </container>" +
+                "  <content version='1.0' id='bar'>" +
+                "     <documents>" +
+                "       <document type='type1' mode='index'/>" +
+                "     </documents>" +
+                "  </content>" +
+                "</services>";
+        VespaModelTester tester = new VespaModelTester();
+        tester.setUseDedicatedNodesWhenUnspecified(true);
+        tester.addHosts(3);
+        VespaModel model = tester.createModel(services, true);
+        assertEquals(3, model.getRoot().hostSystem().getHosts().size());
+        assertEquals(2, model.getAdmin().getSlobroks().size());
+        assertEquals(2, model.getContainerClusters().get("foo").getContainers().size());
+        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes());
+    }
+
+    @Test
     public void testNoNodeTagMeans1NodeNoContent() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
index b6180ab78b9..fd837c6dea3 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
@@ -48,6 +48,7 @@ public class VespaModelTester {
     private Map<NodeResources, Collection<Host>> hostsByResources = new HashMap<>();
     private ApplicationId applicationId = ApplicationId.defaultId();
     private boolean useDedicatedNodeForLogserver = false;
+    private boolean useDedicatedNodesWhenUnspecified = false;
 
     public VespaModelTester() {
         this(new NullConfigModelRegistry());
@@ -97,6 +98,10 @@ public class VespaModelTester {
         this.useDedicatedNodeForLogserver = useDedicatedNodeForLogserver;
     }
 
+    public void setUseDedicatedNodesWhenUnspecified(boolean useDedicatedNodesWhenUnspecified) {
+        this.useDedicatedNodesWhenUnspecified = useDedicatedNodesWhenUnspecified;
+    }
+
     /** Creates a model which uses 0 as start index and fails on out of capacity */
     public VespaModel createModel(String services, String ... retiredHostNames) {
         return createModel(Zone.defaultZone(), services, true, retiredHostNames);
@@ -137,7 +142,8 @@ public class VespaModelTester {
                 .setMultitenant(true)
                 .setHostedVespa(hosted)
                 .setApplicationId(applicationId)
-                .setUseDedicatedNodeForLogserver(useDedicatedNodeForLogserver);
+                .setUseDedicatedNodeForLogserver(useDedicatedNodeForLogserver)
+                .setUseDedicatedNodesWhenUnspecified(useDedicatedNodesWhenUnspecified);
 
         DeployState deployState = new DeployState.Builder()
                 .applicationPackage(appPkg)
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java
index 930bdaadcea..a292ea65d9d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java
@@ -146,6 +146,7 @@ public class ModelContextImpl implements ModelContext {
         private final boolean usePhraseSegmenting;
         private final String proxyProtocol;
         private final Optional<AthenzDomain> athenzDomain;
+        private final boolean useDedicatedNodesWhenUnspecified;
 
         public Properties(ApplicationId applicationId,
                           boolean multitenantFromConfig,
@@ -186,6 +187,8 @@ public class ModelContextImpl implements ModelContext {
             this.proxyProtocol = Flags.PROXY_PROTOCOL.bindTo(flagSource)
                     .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value();
             this.athenzDomain = athenzDomain;
+            this.useDedicatedNodesWhenUnspecified = Flags.DEDICATED_NODES_WHEN_UNSPECIFIED.bindTo(flagSource)
+                    .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value();
         }
 
         @Override
@@ -251,6 +254,12 @@ public class ModelContextImpl implements ModelContext {
 
         @Override
         public Optional<AthenzDomain> athenzDomain() { return athenzDomain; }
+
+        @Override
+        public boolean useDedicatedNodesWhenUnspecified() {
+            return useDedicatedNodesWhenUnspecified;
+        }
+
     }
 
 }
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
index 3a06337d386..b549fb74db4 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
@@ -273,6 +273,12 @@ public class Flags {
             "Takes effect immediately",
             APPLICATION_ID);
 
+    public static final UnboundBooleanFlag DEDICATED_NODES_WHEN_UNSPECIFIED = defineFeatureFlag(
+            "dedicated-nodes-when-unspecified", false,
+            "Whether config-server should allocate dedicated container nodes when <nodes/> is not specified in services.xml",
+            "Takes effect on redeploy",
+            APPLICATION_ID);
+
     /** WARNING: public for testing: All flags should be defined in {@link Flags}. */
     public static UnboundBooleanFlag defineFeatureFlag(String flagId, boolean defaultValue, String description,
                                                        String modificationEffect, FetchVector.Dimension... dimensions) {