Move disk limit check to NodeResourceLimits

12 files changed, 72 insertions, 66 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostSystem.java b/config-model/src/main/java/com/yahoo/vespa/model/HostSystem.java
index 00a1078b294..098d917c4e0 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/HostSystem.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/HostSystem.java
@@ -161,6 +161,11 @@ public class HostSystem extends TreeConfigProducer<Host> {
             deployLogger.log(level, message);
         }
 
+        @Override
+        public void logApplicationPackage(Level level, String message) {
+            deployLogger.logApplicationPackage(level, message);
+        }
+
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
index 5bb73643de5..ea4988f3029 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
@@ -290,28 +290,9 @@ public class NodesSpecification {
                                          .loadBalancerSettings(zoneEndpoint)
                                          .stateful(stateful)
                                          .build();
-        logInsufficientDiskResources(clusterId, clusterType, logger);
         return hostSystem.allocateHosts(cluster, Capacity.from(min, max, groupSize, required, canFail, cloudAccount, info), logger);
     }
 
-    /** Log a message if requested disk may not fit core/heap dumps */
-    private void logInsufficientDiskResources(ClusterSpec.Id clusterId, ClusterSpec.Type clusterType, DeployLogger deployLogger) {
-        NodeResources resources = min.nodeResources();
-        if (resources.diskGbIsUnspecified() || resources.memoryGbIsUnspecified()) return;
-        double minDiskGb = resources.memoryGb() * switch (clusterType) {
-            case combined, content -> 3;
-            case container -> 2;
-            default -> 0; // No constraint on other types
-        };
-        if (resources.diskGb() < minDiskGb) {
-            // TODO(mpolden): Consider enforcing this on Vespa 9
-            deployLogger.logApplicationPackage(Level.WARNING, "Requested disk (" + resources.diskGb() +
-                                                              "Gb) in " + clusterId + " is not large enough to fit " +
-                                                              "core/heap dumps. Minimum recommended disk resources " +
-                                                              "is " + minDiskGb + "Gb");
-        }
-    }
-
     private static Pair<NodeResources, NodeResources> nodeResources(ModelElement nodesElement) {
         ModelElement resources = nodesElement.child("resources");
         if (resources != null) {
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 b1f47c54d54..c128b9af6e0 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
@@ -2577,44 +2577,6 @@ public class ModelProvisioningTest {
         assertEquals((long) ((128 - memoryOverheadGb) * GB * 0.08), cfg.flush().memory().each().maxmemory()); // from default node flavor tuning
     }
 
-    @Test
-    public void warn_on_insufficient_disk_resources() {
-        String services = """
-                <?xml version='1.0' encoding='utf-8' ?>
-                <services>
-                  <container version='1.0' id='c1'>
-                    <nodes count='1'>
-                      <resources vcpu='1' memory='24Gb' disk='40Gb'/>
-                    </nodes>
-                  </container>
-                  <container version='1.0' id='c2'>
-                    <nodes count='1'>
-                      <resources vcpu='1' memory='24Gb' disk='50Gb'/>
-                    </nodes>
-                  </container>
-                  <content version='1.0' id='c3'>
-                    <redundancy>1</redundancy>
-                    <documents>
-                      <document type='type1' mode='index'/>
-                    </documents>
-                    <nodes count='1'>
-                      <resources vcpu='1' memory='24Gb' disk='50Gb'/>
-                    </nodes>
-                  </content>
-                </services>
-                """;
-        VespaModelTester tester = new VespaModelTester();
-        tester.addHosts(new NodeResources(1, 24, 50, 1, DiskSpeed.fast), 10);
-        TestLogger testLogger = new TestLogger();
-        VespaModel model = tester.createModel(services, true, new DeployState.Builder().deployLogger(testLogger));
-        assertEquals(1, model.getContainerClusters().get("c1").getContainers().size());
-        assertEquals(1, model.getContainerClusters().get("c2").getContainers().size());
-        assertEquals(1, model.getContentClusters().get("c3").getSearch().getSearchNodes().size());
-        assertEquals(List.of(new TestLogger.LogMessage(Level.WARNING, "Requested disk (40.0Gb) in cluster 'c1' is not large enough to fit core/heap dumps. Minimum recommended disk resources is 48.0Gb"),
-                             new TestLogger.LogMessage(Level.WARNING, "Requested disk (50.0Gb) in cluster 'c3' is not large enough to fit core/heap dumps. Minimum recommended disk resources is 72.0Gb")),
-                     testLogger.msgs());
-    }
-
     private static ProtonConfig getProtonConfig(VespaModel model, String configId) {
         ProtonConfig.Builder builder = new ProtonConfig.Builder();
         model.getConfig(builder, configId);
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/ProvisionLogger.java b/config-provisioning/src/main/java/com/yahoo/config/provision/ProvisionLogger.java
index 5a22056de1b..9d72f274419 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/ProvisionLogger.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/ProvisionLogger.java
@@ -10,6 +10,16 @@ import java.util.logging.Level;
  */
 public interface ProvisionLogger {
 
+    /** Log a message unrelated to the application package, e.g. internal error/status. */
     void log(Level level, String message);
 
+    /**
+     * Log a message related to the application package. These messages should be actionable by the user, f.ex. to
+     * signal usage of invalid/deprecated syntax.
+     * This default implementation just forwards to {@link #log(Level, String)}
+     */
+    default void logApplicationPackage(Level level, String message) {
+        log(level, message);
+    }
+
 }
diff --git a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
index 63655da0784..450239f7b12 100644
--- a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
@@ -326,7 +326,6 @@ public class QueryTestCase {
 
     @Test
     void testBooleanParameterNoQueryProfile() {
-        QueryProfile profile = new QueryProfile("myProfile");
         Query query = new Query("/?query=something&ranking.softtimeout.enable=false");
         assertFalse(query.properties().getBoolean("ranking.softtimeout.enable"));
         assertFalse(query.getRanking().getSoftTimeout().getEnable());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index 9da66413b9c..f3d69fdf103 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -205,7 +205,7 @@ public class NodeRepository extends AbstractComponent {
      */
     public boolean exclusiveAllocation(ClusterSpec clusterSpec) {
         return clusterSpec.isExclusive() ||
-               ( clusterSpec.type().isContainer() && zone.system().isPublic() &&  !zone.environment().isTest() ) ||
+               ( clusterSpec.type().isContainer() && zone.system().isPublic() && !zone.environment().isTest() ) ||
                ( !zone().cloud().allowHostSharing() && !sharedHosts.value().isEnabled(clusterSpec.type().name()));
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
index ffd2805bcff..506564f9ffa 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
@@ -81,11 +81,10 @@ public class NodeRepositoryProvisioner implements Provisioner {
      * The nodes are ordered by increasing index number.
      */
     @Override
-    public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, Capacity requested,
-                                  ProvisionLogger logger) {
+    public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, Capacity requested, ProvisionLogger logger) {
         log.log(Level.FINE, "Received deploy prepare request for " + requested +
                             " for application " + application + ", cluster " + cluster);
-        validate(application, cluster, requested);
+        validate(application, cluster, requested, logger);
 
         int groups;
         NodeResources resources;
@@ -113,7 +112,7 @@ public class NodeRepositoryProvisioner implements Provisioner {
                              requireCompatibleResources(resources, cluster));
     }
 
-    private void validate(ApplicationId application, ClusterSpec cluster, Capacity requested) {
+    private void validate(ApplicationId application, ClusterSpec cluster, Capacity requested, ProvisionLogger logger) {
         if (cluster.group().isPresent()) throw new IllegalArgumentException("Node requests cannot specify a group");
 
         nodeResourceLimits.ensureWithinAdvertisedLimits("Min", requested.minResources().nodeResources(), application, cluster);
@@ -121,6 +120,18 @@ public class NodeRepositoryProvisioner implements Provisioner {
 
         if ( ! requested.minResources().nodeResources().gpuResources().equals(requested.maxResources().nodeResources().gpuResources()))
             throw new IllegalArgumentException(requested + " is invalid: Gpu capacity cannot have ranges");
+
+        logInsufficientDiskResources(cluster, requested, logger);
+    }
+
+    private void logInsufficientDiskResources(ClusterSpec cluster, Capacity requested, ProvisionLogger logger) {
+        var resources = requested.minResources().nodeResources();
+        if ( ! nodeResourceLimits.isWithinAdvertisedDiskLimits(resources, cluster)) {
+            logger.logApplicationPackage(Level.WARNING, "Requested disk (" + resources.diskGb() +
+                                                        "Gb) in " + cluster.id() + " is not large enough to fit " +
+                                                        "core/heap dumps. Minimum recommended disk resources " +
+                                                        "is 3x memory for content and 2x memory for containers");
+        }
     }
 
     private NodeResources getNodeResources(ClusterSpec cluster, NodeResources nodeResources, ApplicationId applicationId) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeResourceLimits.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeResourceLimits.java
index 9ded1a2735c..61975ca0dc1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeResourceLimits.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeResourceLimits.java
@@ -2,14 +2,17 @@
 package com.yahoo.vespa.hosted.provision.provisioning;
 
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
+import com.yahoo.config.provision.ProvisionLogger;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 
 import java.util.Locale;
+import java.util.logging.Level;
 
 /**
  * Defines the resource limits for nodes in various zones
@@ -35,6 +38,17 @@ public class NodeResourceLimits {
             illegal(type, "diskGb", "Gb", cluster, requested.diskGb(), minAdvertisedDiskGb(requested, cluster.isExclusive()));
     }
 
+    // TODO: Move this check into the above when we are ready to fail, not just warn on this. */
+    public boolean isWithinAdvertisedDiskLimits(NodeResources requested, ClusterSpec cluster) {
+        if (requested.diskGbIsUnspecified() || requested.memoryGbIsUnspecified()) return true;
+        double minDiskGb = requested.memoryGb() * switch (cluster.type()) {
+            case combined, content -> 3;
+            case container -> 2;
+            default -> 0; // No constraint on other types
+        };
+        return requested.diskGb() >= minDiskGb;
+    }
+
     /** Returns whether the real resources we'll end up with on a given tenant node are within limits */
     public boolean isWithinRealLimits(NodeCandidate candidateNode, ApplicationId applicationId, ClusterSpec cluster) {
         if (candidateNode.type() != NodeType.tenant) return true; // Resource limits only apply to tenant nodes
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
index dcde521bfda..053633da80a 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
@@ -261,7 +261,7 @@ public class MockDeployer implements Deployer {
         public ClusterSpec cluster() { return cluster; }
 
         private List<HostSpec> prepare(NodeRepositoryProvisioner provisioner) {
-            return provisioner.prepare(id, cluster, capacity, null);
+            return provisioner.prepare(id, cluster, capacity, new SystemOutProvisionLogger());
         }
 
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/SystemOutProvisionLogger.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/SystemOutProvisionLogger.java
new file mode 100644
index 00000000000..ebcf72a11ff
--- /dev/null
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/SystemOutProvisionLogger.java
@@ -0,0 +1,23 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.provision.testutils;
+
+import com.yahoo.config.provision.ProvisionLogger;
+
+import java.util.logging.Level;
+
+/**
+ * @author bratseth
+ */
+public class SystemOutProvisionLogger implements ProvisionLogger {
+
+    @Override
+    public void log(Level level, String message) {
+        System.out.println("ProvisionLogger system " + level + ": " + message);
+    }
+
+    @Override
+    public void logApplicationPackage(Level level, String message) {
+        System.out.println("ProvisionLogger application " + level + ": " + message);
+    }
+
+}
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
index 4d2816cb14f..3d5297ad573 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
@@ -30,6 +30,7 @@ import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
 import com.yahoo.vespa.hosted.provision.testutils.ServiceMonitorStub;
 import com.yahoo.vespa.service.duper.InfraApplication;
 import com.yahoo.vespa.service.duper.TenantHostApplication;
+import com.yahoo.vespa.hosted.provision.testutils.SystemOutProvisionLogger;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -270,7 +271,7 @@ public class NodeFailTester {
     }
 
     public void activate(ApplicationId applicationId, ClusterSpec cluster, Capacity capacity) {
-        List<HostSpec> hosts = provisioner.prepare(applicationId, cluster, capacity, null);
+        List<HostSpec> hosts = provisioner.prepare(applicationId, cluster, capacity, new SystemOutProvisionLogger());
         try (var lock = provisioner.lock(applicationId)) {
             NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
             provisioner.activate(hosts, new ActivationContext(0), new ApplicationTransaction(lock, transaction));
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
index 7e85131eaf4..9bbc5c5e25a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
@@ -773,7 +773,7 @@ public class ProvisioningTester {
 
     }
 
-    private static class NullProvisionLogger implements ProvisionLogger {
+    public static class NullProvisionLogger implements ProvisionLogger {
         @Override public void log(Level level, String message) { }
     }