Merge pull request #17931 from vespa-engine/hmusum/warn-when-specifying-resource-limits-in-hosted

Warn when specifying resource limits in hosted Vespa

6 files changed, 68 insertions, 25 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterResourceLimits.java b/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterResourceLimits.java
index c7d56c9b4b5..70f2acd3c7b 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterResourceLimits.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/ClusterResourceLimits.java
@@ -1,6 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.content;
 
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import com.yahoo.vespa.model.content.cluster.DomResourceLimitsBuilder;
 
@@ -35,28 +36,31 @@ public class ClusterResourceLimits {
     public static class Builder {
 
         private final boolean enableFeedBlockInDistributor;
+        private final boolean hostedVespa;
+        private final DeployLogger deployLogger;
         private ResourceLimits.Builder ctrlBuilder = new ResourceLimits.Builder();
         private ResourceLimits.Builder nodeBuilder = new ResourceLimits.Builder();
 
-        public Builder(boolean enableFeedBlockInDistributor) {
+        public Builder(boolean enableFeedBlockInDistributor, boolean hostedVespa, DeployLogger deployLogger) {
             this.enableFeedBlockInDistributor = enableFeedBlockInDistributor;
+            this.hostedVespa = hostedVespa;
+            this.deployLogger = deployLogger;
         }
 
         public ClusterResourceLimits build(ModelElement clusterElem) {
-            ModelElement tuningElem = clusterElem.childByPath("tuning");
-            if (tuningElem != null) {
-                ctrlBuilder = DomResourceLimitsBuilder.createBuilder(tuningElem);
-            }
-
-            ModelElement protonElem = clusterElem.childByPath("engine.proton");
-            if (protonElem != null) {
-                nodeBuilder = DomResourceLimitsBuilder.createBuilder(protonElem);
-            }
+            ctrlBuilder = createBuilder(clusterElem.childByPath("tuning"));
+            nodeBuilder = createBuilder(clusterElem.childByPath("engine.proton"));
 
             deriveLimits();
             return new ClusterResourceLimits(this);
         }
 
+        private ResourceLimits.Builder createBuilder(ModelElement element) {
+            return element == null
+                    ? new ResourceLimits.Builder()
+                    : DomResourceLimitsBuilder.createBuilder(element, hostedVespa, deployLogger);
+        }
+
         public void setClusterControllerBuilder(ResourceLimits.Builder builder) {
             ctrlBuilder = builder;
         }
@@ -88,7 +92,7 @@ public class ClusterResourceLimits {
                                                                   Optional<Double> contentNodeLimit,
                                                                   Consumer<Double> setter) {
             // TODO: remove this when feed block in distributor is default enabled.
-            if (!clusterControllerLimit.isPresent() && !contentNodeLimit.isPresent()) {
+            if (clusterControllerLimit.isEmpty() && contentNodeLimit.isEmpty()) {
                 setter.accept(0.8);
             }
         }
@@ -96,7 +100,7 @@ public class ClusterResourceLimits {
         private void deriveClusterControllerLimit(Optional<Double> clusterControllerLimit,
                                                   Optional<Double> contentNodeLimit,
                                                   Consumer<Double> setter) {
-            if (!clusterControllerLimit.isPresent()) {
+            if (clusterControllerLimit.isEmpty()) {
                 contentNodeLimit.ifPresent(limit ->
                         // TODO: emit warning when feed block in distributor is default enabled.
                         setter.accept(Double.max(0.0, (limit - 0.01))));
@@ -106,7 +110,7 @@ public class ClusterResourceLimits {
         private void deriveContentNodeLimit(Optional<Double> contentNodeLimit,
                                             Optional<Double> clusterControllerLimit,
                                             Consumer<Double> setter) {
-            if (!contentNodeLimit.isPresent()) {
+            if (contentNodeLimit.isEmpty()) {
                 clusterControllerLimit.ifPresent(limit ->
                         setter.accept(calcContentNodeLimit(limit)));
             }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
index f6a45842bd9..b9ee973664e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
@@ -134,7 +134,10 @@ public class ContentCluster extends AbstractConfigProducer implements
                                                   globallyDistributedDocuments, routingSelection,
                                                   deployState.zone(), deployState.isHosted());
             boolean enableFeedBlockInDistributor = deployState.getProperties().featureFlags().enableFeedBlockInDistributor();
-            var resourceLimits = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor).build(contentElement);
+            var resourceLimits = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor,
+                                                                   stateIsHosted(deployState),
+                                                                   deployState.getDeployLogger())
+                    .build(contentElement);
             c.clusterControllerConfig = new ClusterControllerConfig.Builder(getClusterId(contentElement),
                     contentElement,
                     resourceLimits.getClusterControllerLimits()).build(deployState, c, contentElement.getXml());
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DomResourceLimitsBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DomResourceLimitsBuilder.java
index 210f062f9b2..7640f1d737e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DomResourceLimitsBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/DomResourceLimitsBuilder.java
@@ -1,9 +1,12 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.content.cluster;
 
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import com.yahoo.vespa.model.content.ResourceLimits;
 
+import java.util.logging.Level;
+
 /**
  * Builder for feed block resource limits.
  *
@@ -11,10 +14,15 @@ import com.yahoo.vespa.model.content.ResourceLimits;
  */
 public class DomResourceLimitsBuilder {
 
-    public static ResourceLimits.Builder createBuilder(ModelElement contentXml) {
+    public static ResourceLimits.Builder createBuilder(ModelElement contentXml, boolean hostedVespa, DeployLogger deployLogger) {
         ResourceLimits.Builder builder = new ResourceLimits.Builder();
         ModelElement resourceLimits = contentXml.child("resource-limits");
-        if (resourceLimits == null) {
+        if (resourceLimits == null) { return builder; }
+
+        if (hostedVespa) {
+            deployLogger.log(Level.WARNING, "Element " + resourceLimits + " is not allowed, default limits will be used");
+            // TODO: Throw exception when we are sure nobody is using this
+            //throw new IllegalArgumentException("Element " + element + " is not allowed to be set, default limits will be used");
             return builder;
         }
         if (resourceLimits.child("disk") != 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 326bf577acc..2ddddba19d9 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
@@ -2056,9 +2056,6 @@ public class ModelProvisioningTest {
                 "    </nodes>",
                 "    <engine>",
                 "      <proton>",
-                "        <resource-limits>",
-                "          <memory>0.92</memory>",
-                "        </resource-limits>",
                 "        <tuning>",
                 "          <searchnode>",
                 "            <flushstrategy>",
@@ -2084,7 +2081,6 @@ public class ModelProvisioningTest {
         assertEquals(2000, cfg.flush().memory().maxtlssize()); // from config override
         assertEquals(1000, cfg.flush().memory().maxmemory()); // from explicit tuning
         assertEquals((long) ((128 - reservedMemoryGb) * GB / 8), cfg.flush().memory().each().maxmemory()); // from default node flavor tuning
-        assertEquals(0.92, cfg.writefilter().memorylimit(), 0.0001); // from explicit resource-limits
     }
 
     private static ProtonConfig getProtonConfig(VespaModel model, String configId) {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/ClusterResourceLimitsTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/ClusterResourceLimitsTest.java
index da2d5e2e3a0..469e4649c14 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/ClusterResourceLimitsTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/ClusterResourceLimitsTest.java
@@ -1,6 +1,11 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.content;
 
+import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.config.model.application.provider.BaseDeployLogger;
+import com.yahoo.searchdefinition.derived.TestableDeployLogger;
+import com.yahoo.text.XML;
+import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import org.junit.Test;
 
 import java.util.Optional;
@@ -43,7 +48,7 @@ public class ClusterResourceLimitsTest {
             return this;
         }
         public ClusterResourceLimits build() {
-            var builder = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor);
+            var builder = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor, false, new BaseDeployLogger());
             builder.setClusterControllerBuilder(ctrlBuilder);
             builder.setContentNodeBuilder(nodeBuilder);
             return builder.build();
@@ -114,6 +119,29 @@ public class ClusterResourceLimitsTest {
                 new Fixture(true));
     }
 
+    @Test
+    // TODO: Change to expect exception being thrown when no one uses this in hosted
+    public void default_resource_limits_when_hosted_and_warning_is_logged() {
+        TestableDeployLogger logger = new TestableDeployLogger();
+        final boolean hosted = true;
+
+        ClusterResourceLimits.Builder builder = new ClusterResourceLimits.Builder(true, hosted, logger);
+        ClusterResourceLimits limits = builder.build(new ModelElement(XML.getDocument("<cluster id=\"test\">" +
+                                                       "  <tuning>\n" +
+                                                       "    <resource-limits>\n" +
+                                                       "      <memory>0.92</memory>\n" +
+                                                       "    </resource-limits>\n" +
+                                                       "  </tuning>\n" +
+                                                       "</cluster>")
+                                          .getDocumentElement()));
+
+        assertLimits(0.8, 0.8, limits.getClusterControllerLimits());
+        assertLimits(0.9, 0.9, limits.getContentNodeLimits());
+
+        assertEquals(1, logger.warnings.size());
+        assertEquals("Element resource-limits is not allowed, default limits will be used", logger.warnings.get(0));
+    }
+
     private void assertLimits(Double expCtrlDisk, Double expCtrlMemory, Double expNodeDisk, Double expNodeMemory, Fixture f) {
         var limits = f.build();
         assertLimits(expCtrlDisk, expCtrlMemory, limits.getClusterControllerLimits());
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java
index b1bd44d93b4..22e38b30959 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/FleetControllerClusterTest.java
@@ -1,6 +1,7 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.content;
 
+import com.yahoo.config.model.application.provider.BaseDeployLogger;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.deploy.TestProperties;
 import com.yahoo.vespa.config.content.FleetcontrollerConfig;
@@ -22,9 +23,12 @@ public class FleetControllerClusterTest {
         MockRoot root = new MockRoot("", deployState);
         var clusterElement = new ModelElement(doc.getDocumentElement());
         return new ClusterControllerConfig.Builder("storage",
-                clusterElement,
-                new ClusterResourceLimits.Builder(enableFeedBlockInDistributor).build(clusterElement).getClusterControllerLimits()).
-                build(root.getDeployState(), root, clusterElement.getXml());
+                                                   clusterElement,
+                                                   new ClusterResourceLimits.Builder(enableFeedBlockInDistributor,
+                                                                                     false,
+                                                                                     new BaseDeployLogger())
+                                                           .build(clusterElement).getClusterControllerLimits())
+                .build(root.getDeployState(), root, clusterElement.getXml());
     }
 
     private ClusterControllerConfig parse(String xml) {