More testing of setting resource limits and add TODOs

2 files changed, 53 insertions, 20 deletions

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 37adb73bc15..32b0f5b6477 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
@@ -24,10 +24,12 @@ public class DomResourceLimitsBuilder {
 
         if (hostedVespa) {
             String message = "Element '" + resourceLimits + "' is not allowed to be set";
-            if (throwIfSpecified)
-                throw new IllegalArgumentException(message);
-            else
-                deployLogger.logApplicationPackage(Level.WARNING, message);
+            if (throwIfSpecified) throw new IllegalArgumentException(message);
+
+
+            deployLogger.logApplicationPackage(Level.WARNING, message);
+            // TODO: return (default values will then be used). Cannot be done now as an app needs current behavior
+            //return builder;
         }
 
         if (resourceLimits.child("disk") != null) {
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 4324f257922..22618ea433b 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
@@ -6,6 +6,7 @@ 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.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -24,15 +25,23 @@ public class ClusterResourceLimitsTest {
 
     private static class Fixture {
         private final boolean enableFeedBlockInDistributor;
+        private final boolean hostedVespa;
+        private final boolean throwIfSpecified;
         private final ResourceLimits.Builder ctrlBuilder = new ResourceLimits.Builder();
         private final ResourceLimits.Builder nodeBuilder = new ResourceLimits.Builder();
 
         public Fixture() {
-            this.enableFeedBlockInDistributor = false;
+            this(false);
         }
 
         public Fixture(boolean enableFeedBlockInDistributor) {
+            this(enableFeedBlockInDistributor, false, false);
+        }
+
+        public Fixture(boolean enableFeedBlockInDistributor, boolean hostedVespa, boolean throwIfSpecified) {
             this.enableFeedBlockInDistributor = enableFeedBlockInDistributor;
+            this.hostedVespa = hostedVespa;
+            this.throwIfSpecified = throwIfSpecified;
         }
 
         public Fixture ctrlDisk(double limit) {
@@ -53,8 +62,8 @@ public class ClusterResourceLimitsTest {
         }
         public ClusterResourceLimits build() {
             var builder = new ClusterResourceLimits.Builder(enableFeedBlockInDistributor,
-                                                            false,
-                                                            false,
+                                                            hostedVespa,
+                                                            throwIfSpecified,
                                                             new BaseDeployLogger());
             builder.setClusterControllerBuilder(ctrlBuilder);
             builder.setContentNodeBuilder(nodeBuilder);
@@ -130,27 +139,49 @@ public class ClusterResourceLimitsTest {
     }
 
     @Test
-    public void exception_is_thrown_when_resource_limits_are_specified() {
+    @Ignore // TODO: Remove hosted_limits_are_used_if_app_is_allowed_to_set_limits and enable this when code is fixed to do sp
+    public void hosted_log_when_resource_limits_are_specified() {
         TestableDeployLogger logger = new TestableDeployLogger();
 
-        buildClusterResourceLimitsAndLogIfSpecified(logger);
+        var limits = hostedBuildAndLogIfSpecified(logger);
         assertEquals(1, logger.warnings.size());
         assertEquals("Element 'resource-limits' is not allowed to be set", logger.warnings.get(0));
 
+        // Verify that default limits are used
+        assertLimits(0.8, 0.8, limits.getClusterControllerLimits());
+        assertLimits(0.9, 0.9, limits.getContentNodeLimits());
+    }
+
+    @Test
+    public void hosted_exception_is_thrown_when_resource_limits_are_specified() {
         expectedException.expect(IllegalArgumentException.class);
         expectedException.expectMessage(containsString("Element 'resource-limits' is not allowed to be set"));
-        buildClusterResourceLimitsAndThrowIfSpecified(logger);
+        hostedBuildAndThrowIfSpecified();
+    }
+
+    @Test
+    // TODO: Remove this and enable hosted_log_when_resource_limits_are_specified when code is fixed
+    public void hosted_limits_are_used_if_app_is_allowed_to_set_limits() {
+        TestableDeployLogger logger = new TestableDeployLogger();
+
+        var limits = hostedBuildAndLogIfSpecified(logger);
+        assertEquals(1, logger.warnings.size());
+        assertEquals("Element 'resource-limits' is not allowed to be set", logger.warnings.get(0));
+
+        // Verify that limits in XML are used
+        assertLimits(0.8, 0.92, limits.getClusterControllerLimits());
+        assertLimits(0.9, 0.96, limits.getContentNodeLimits());
     }
 
-    private void buildClusterResourceLimitsAndThrowIfSpecified(DeployLogger deployLogger) {
-        buildClusterResourceLimits(true, deployLogger);
+    private void hostedBuildAndThrowIfSpecified() {
+        hostedBuild(true, new TestableDeployLogger());
     }
 
-    private void buildClusterResourceLimitsAndLogIfSpecified(DeployLogger deployLogger) {
-        buildClusterResourceLimits(false, deployLogger);
+    private ClusterResourceLimits hostedBuildAndLogIfSpecified(DeployLogger deployLogger) {
+        return hostedBuild(false, deployLogger);
     }
 
-    private void buildClusterResourceLimits(boolean throwIfSpecified, DeployLogger deployLogger) {
+    private ClusterResourceLimits hostedBuild(boolean throwIfSpecified, DeployLogger deployLogger) {
         Document clusterXml = XML.getDocument("<cluster id=\"test\">" +
                                               "  <tuning>\n" +
                                               "    <resource-limits>\n" +
@@ -163,7 +194,7 @@ public class ClusterResourceLimitsTest {
                                                                                   true,
                                                                                   throwIfSpecified,
                                                                                   deployLogger);
-        builder.build(new ModelElement(clusterXml.getDocumentElement()));
+        return builder.build(new ModelElement(clusterXml.getDocumentElement()));
     }
 
     private void assertLimits(Double expCtrlDisk, Double expCtrlMemory, Double expNodeDisk, Double expNodeMemory, Fixture f) {
@@ -173,15 +204,15 @@ public class ClusterResourceLimitsTest {
     }
 
     private void assertLimits(Double expDisk, Double expMemory, ResourceLimits limits) {
-        assertLimit(expDisk, limits.getDiskLimit());
-        assertLimit(expMemory, limits.getMemoryLimit());
+        assertLimit(expDisk, limits.getDiskLimit(), "disk");
+        assertLimit(expMemory, limits.getMemoryLimit(), "memory");
     }
 
-    private void assertLimit(Double expLimit, Optional<Double> actLimit) {
+    private void assertLimit(Double expLimit, Optional<Double> actLimit, String limitType) {
         if (expLimit == null) {
             assertFalse(actLimit.isPresent());
         } else {
-            assertEquals(expLimit, actLimit.get(), 0.00001);
+            assertEquals(limitType + " limit not as expected", expLimit, actLimit.get(), 0.00001);
         }
     }