Merge pull request #18023 from vespa-engine/hmusum/throw-exception-when-specifyin-resource-limits

Throw exception if resource-limits is specified

8 files changed, 49 insertions, 46 deletions

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 5158f3ec488..d2892917a2e 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
@@ -96,6 +96,7 @@ public interface ModelContext {
         @ModelFeatureFlag(owners = {"tokle", "bjorncs"}) default boolean enableCustomAclMapping() { return false; }
         @ModelFeatureFlag(owners = {"geirst", "vekterli"}) default int numDistributorStripes() { return 0; }
         @ModelFeatureFlag(owners = {"arnej"}) default boolean requireConnectivityCheck() { return false; }
+        @ModelFeatureFlag(owners = {"hmusum"}) default boolean throwIfResourceLimitsSpecified() { return false; }
     }
 
     /** Warning: As elsewhere in this package, do not make backwards incompatible changes that will break old config models! */
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 70f2acd3c7b..8db656a5f2c 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,7 +1,6 @@
 // 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;
 
@@ -37,14 +36,14 @@ public class ClusterResourceLimits {
 
         private final boolean enableFeedBlockInDistributor;
         private final boolean hostedVespa;
-        private final DeployLogger deployLogger;
+        private final boolean throwIfSpecified;
         private ResourceLimits.Builder ctrlBuilder = new ResourceLimits.Builder();
         private ResourceLimits.Builder nodeBuilder = new ResourceLimits.Builder();
 
-        public Builder(boolean enableFeedBlockInDistributor, boolean hostedVespa, DeployLogger deployLogger) {
+        public Builder(boolean enableFeedBlockInDistributor, boolean hostedVespa, boolean throwIfSpecified) {
             this.enableFeedBlockInDistributor = enableFeedBlockInDistributor;
             this.hostedVespa = hostedVespa;
-            this.deployLogger = deployLogger;
+            this.throwIfSpecified = throwIfSpecified;
         }
 
         public ClusterResourceLimits build(ModelElement clusterElem) {
@@ -58,7 +57,7 @@ public class ClusterResourceLimits {
         private ResourceLimits.Builder createBuilder(ModelElement element) {
             return element == null
                     ? new ResourceLimits.Builder()
-                    : DomResourceLimitsBuilder.createBuilder(element, hostedVespa, deployLogger);
+                    : DomResourceLimitsBuilder.createBuilder(element, hostedVespa, throwIfSpecified);
         }
 
         public void setClusterControllerBuilder(ResourceLimits.Builder builder) {
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 e0d311e6df6..97093203758 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
@@ -123,7 +123,7 @@ public class ContentCluster extends AbstractConfigProducer<AbstractConfigProduce
             boolean enableFeedBlockInDistributor = deployState.getProperties().featureFlags().enableFeedBlockInDistributor();
             var resourceLimits = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor,
                                                                    stateIsHosted(deployState),
-                                                                   deployState.getDeployLogger())
+                                                                   deployState.featureFlags().throwIfResourceLimitsSpecified())
                     .build(contentElement);
             c.clusterControllerConfig = new ClusterControllerConfig.Builder(getClusterId(contentElement),
                     contentElement,
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 9f4852629d0..bab991efe51 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,12 +1,9 @@
-// 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.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.
  *
@@ -14,18 +11,14 @@ import java.util.logging.Level;
  */
 public class DomResourceLimitsBuilder {
 
-    public static ResourceLimits.Builder createBuilder(ModelElement contentXml, boolean hostedVespa, DeployLogger deployLogger) {
+    public static ResourceLimits.Builder createBuilder(ModelElement contentXml, boolean hostedVespa, boolean throwIfSpecified) {
         ResourceLimits.Builder builder = new ResourceLimits.Builder();
         ModelElement resourceLimits = contentXml.child("resource-limits");
         if (resourceLimits == null) { return builder; }
 
-        if (hostedVespa) {
-            deployLogger.logApplicationPackage(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 (hostedVespa && throwIfSpecified)
+            throw new IllegalArgumentException("Element '" + resourceLimits + "' is not allowed to be set");
+
         if (resourceLimits.child("disk") != null) {
             builder.setDiskLimit(resourceLimits.childAsDouble("disk"));
         }
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 469e4649c14..ad1f5331a91 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,15 +1,16 @@
 // 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.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.w3c.dom.Document;
 
 import java.util.Optional;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 
@@ -48,13 +49,16 @@ public class ClusterResourceLimitsTest {
             return this;
         }
         public ClusterResourceLimits build() {
-            var builder = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor, false, new BaseDeployLogger());
+            var builder = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor, false, false);
             builder.setClusterControllerBuilder(ctrlBuilder);
             builder.setContentNodeBuilder(nodeBuilder);
             return builder.build();
         }
     }
 
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
     @Test
     public void content_node_limits_are_derived_from_cluster_controller_limits_if_not_set() {
         assertLimits(0.4, 0.7, 0.7, 0.85,
@@ -120,26 +124,25 @@ public class ClusterResourceLimitsTest {
     }
 
     @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();
+    public void exception_is_thrown_when_resource_limits_are_specified() {
         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));
+        Document clusterXml = XML.getDocument("<cluster id=\"test\">" +
+                                            "  <tuning>\n" +
+                                            "    <resource-limits>\n" +
+                                            "      <memory>0.92</memory>\n" +
+                                            "    </resource-limits>\n" +
+                                            "  </tuning>\n" +
+                                            "</cluster>");
+
+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage(containsString("Element 'resource-limits' is not allowed to be set"));
+        ClusterResourceLimits.Builder builder = new ClusterResourceLimits.Builder(true, hosted, true);
+        builder.build(new ModelElement(clusterXml.getDocumentElement()));
+
+        expectedException = ExpectedException.none();
+        ClusterResourceLimits.Builder builder2 = new ClusterResourceLimits.Builder(true, hosted, false);
+        builder2.build(new ModelElement(clusterXml.getDocumentElement()));
     }
 
     private void assertLimits(Double expCtrlDisk, Double expCtrlMemory, Double expNodeDisk, Double expNodeMemory, Fixture f) {
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 22e38b30959..2eecfa9440e 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,12 +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.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;
 import com.yahoo.config.model.test.MockRoot;
 import com.yahoo.text.XML;
+import com.yahoo.vespa.config.content.FleetcontrollerConfig;
 import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import org.junit.Test;
 import org.w3c.dom.Document;
@@ -24,9 +23,7 @@ public class FleetControllerClusterTest {
         var clusterElement = new ModelElement(doc.getDocumentElement());
         return new ClusterControllerConfig.Builder("storage",
                                                    clusterElement,
-                                                   new ClusterResourceLimits.Builder(enableFeedBlockInDistributor,
-                                                                                     false,
-                                                                                     new BaseDeployLogger())
+                                                   new ClusterResourceLimits.Builder(enableFeedBlockInDistributor, false, false)
                                                            .build(clusterElement).getClusterControllerLimits())
                 .build(root.getDeployState(), root, clusterElement.getXml());
     }
@@ -115,7 +112,7 @@ public class FleetControllerClusterTest {
         assertLimits(0.8, 0.7, getConfigForResourceLimitsTuning(null, 0.7));
     }
 
-    private static double DELTA = 0.00001;
+    private static final double DELTA = 0.00001;
 
     private void assertLimits(double expDisk, double expMemory, FleetcontrollerConfig config) {
         var limits = config.cluster_feed_block_limit();
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 8805c339482..5cd52e36339 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
@@ -180,6 +180,7 @@ public class ModelContextImpl implements ModelContext {
         private final boolean requireConnectivityCheck;
         private final int maxConcurrentMergesPerContentNode;
         private final int maxMergeQueueSize;
+        private final boolean throwIfResourceLimitsSpecified;
 
         public FeatureFlags(FlagSource source, ApplicationId appId) {
             this.dedicatedClusterControllerFlavor = parseDedicatedClusterControllerFlavor(flagValue(source, appId, Flags.DEDICATED_CLUSTER_CONTROLLER_FLAVOR));
@@ -205,6 +206,7 @@ public class ModelContextImpl implements ModelContext {
             this.requireConnectivityCheck = flagValue(source, appId, Flags.REQUIRE_CONNECTIVITY_CHECK);
             this.maxConcurrentMergesPerContentNode = flagValue(source, appId, Flags.MAX_CONCURRENT_MERGES_PER_NODE);
             this.maxMergeQueueSize = flagValue(source, appId, Flags.MAX_MERGE_QUEUE_SIZE);
+            this.throwIfResourceLimitsSpecified = flagValue(source, appId, Flags.THROW_EXCEPTION_IF_RESOURCE_LIMITS_SPECIFIED);
         }
 
         @Override public Optional<NodeResources> dedicatedClusterControllerFlavor() { return Optional.ofNullable(dedicatedClusterControllerFlavor); }
@@ -232,6 +234,7 @@ public class ModelContextImpl implements ModelContext {
         @Override public boolean requireConnectivityCheck() { return requireConnectivityCheck; }
         @Override public int maxConcurrentMergesPerNode() { return maxConcurrentMergesPerContentNode; }
         @Override public int maxMergeQueueSize() { return maxMergeQueueSize; }
+        @Override public boolean throwIfResourceLimitsSpecified() { return throwIfResourceLimitsSpecified; }
 
         private static <V> V flagValue(FlagSource source, ApplicationId appId, UnboundFlag<? extends V, ?, ?> flag) {
             return flag.bindTo(source)
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 cfa14979d3f..9904ef77513 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
@@ -266,6 +266,13 @@ public class Flags {
             "Takes effect on next restart",
             ZONE_ID, APPLICATION_ID);
 
+    public static final UnboundBooleanFlag THROW_EXCEPTION_IF_RESOURCE_LIMITS_SPECIFIED = defineFeatureFlag(
+            "throw-exception-if-resource-limits-specified", false,
+            List.of("mpolden"), "2021-06-07", "2021-07-07",
+            "Whether to throw an exception in hosted Vespa if the application specifies resource limits in services.xml",
+            "Takes effect on next deployment through controller",
+            APPLICATION_ID);
+
     /** WARNING: public for testing: All flags should be defined in {@link Flags}. */
     public static UnboundBooleanFlag defineFeatureFlag(String flagId, boolean defaultValue, List<String> owners,
                                                        String createdAt, String expiresAt, String description,