Merge pull request #29694 from vespa-engine/jonmv/more-careful-resource-parsing

More careful resource parsing

2 files changed, 99 insertions, 10 deletions

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 baf752cb4be..93d65426b61 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
@@ -25,6 +25,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.function.ToDoubleFunction;
 import java.util.logging.Level;
 
 /**
@@ -313,10 +314,10 @@ public class NodesSpecification {
     }
 
     private static Pair<NodeResources, NodeResources> nodeResourcesFromResourcesElement(ModelElement element) {
-        Pair<Double, Double> vcpu       = toRange(element.stringAttribute("vcpu"),   .0, Double::parseDouble);
-        Pair<Double, Double> memory     = toRange(element.stringAttribute("memory"), .0, s -> parseGbAmount(s, "B"));
-        Pair<Double, Double> disk       = toRange(element.stringAttribute("disk"),   .0, s -> parseGbAmount(s, "B"));
-        Pair<Double, Double> bandwith   = toRange(element.stringAttribute("bandwidth"),      .3, s -> parseGbAmount(s, "BPS"));
+        Pair<Double, Double> vcpu       = toRange("vcpu", element,      .0, Double::parseDouble);
+        Pair<Double, Double> memory     = toRange("memory", element,    .0, s -> parseGbAmount(s, "B"));
+        Pair<Double, Double> disk       = toRange("disk", element,      .0, s -> parseGbAmount(s, "B"));
+        Pair<Double, Double> bandwith   = toRange("bandwidth", element, .3, s -> parseGbAmount(s, "BPS"));
         NodeResources.DiskSpeed   diskSpeed     = parseOptionalDiskSpeed(element.stringAttribute("disk-speed"));
         NodeResources.StorageType storageType   = parseOptionalStorageType(element.stringAttribute("storage-type"));
         NodeResources.Architecture architecture = parseOptionalArchitecture(element.stringAttribute("architecture"));
@@ -342,7 +343,7 @@ public class NodesSpecification {
         if (byteAmount.endsWith(unit))
             byteAmount = byteAmount.substring(0, byteAmount.length() - unit.length());
 
-        double multiplier = Math.pow(1000, -3);
+        double multiplier = -1;
         if (byteAmount.endsWith("K"))
             multiplier = Math.pow(1000, -2);
         else if (byteAmount.endsWith("M"))
@@ -360,7 +361,11 @@ public class NodesSpecification {
         else if (byteAmount.endsWith("Y"))
             multiplier = Math.pow(1000, 5);
 
-        byteAmount = byteAmount.substring(0, byteAmount.length() -1 ).strip();
+        if (multiplier == -1)
+            multiplier = Math.pow(1000, -3);
+        else
+            byteAmount = byteAmount.substring(0, byteAmount.length() -1).strip();
+
         try {
             return Double.parseDouble(byteAmount) * multiplier;
         }
@@ -477,20 +482,27 @@ public class NodesSpecification {
     }
 
     /** Parses a value ("value") or value range ("[min-value, max-value]") */
-    private static <T> Pair<T, T> toRange(String s, T defaultValue, Function<String, T> valueParser) {
+    private static Pair<Double, Double> toRange(String name, ModelElement element, double defaultValue, ToDoubleFunction<String> valueParser) {
+        String s = element.stringAttribute(name);
         try {
+            Pair<Double, Double> pair;
             if (s == null) return new Pair<>(defaultValue, defaultValue);
             s = s.trim();
             if (s.startsWith("[") && s.endsWith("]")) {
                 String[] numbers = s.substring(1, s.length() - 1).split(",");
                 if (numbers.length != 2) throw new IllegalArgumentException();
-                return new Pair<>(valueParser.apply(numbers[0].trim()), valueParser.apply(numbers[1].trim()));
+                pair = new Pair<>(valueParser.applyAsDouble(numbers[0].trim()), valueParser.applyAsDouble(numbers[1].trim()));
+                if (pair.getFirst() > pair.getSecond())
+                    throw new IllegalArgumentException("first value cannot be larger than second value");
             } else {
-                return new Pair<>(valueParser.apply(s), valueParser.apply(s));
+                pair = new Pair<>(valueParser.applyAsDouble(s), valueParser.applyAsDouble(s));
             }
+            if (pair.getFirst() < 0 || pair.getSecond() < 0)
+                throw new IllegalArgumentException("values cannot be negative");
+            return pair;
         }
         catch (IllegalArgumentException e) {
-            throw new IllegalArgumentException("Expected a number or range on the form [min, max], but got '" + s + "'", e);
+            throw new IllegalArgumentException("Expected a number or range on the form [min, max] for node resource '" + name + "', but got '" + s + "'", e);
         }
     }
 
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecificationTest.java b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecificationTest.java
index b07d5132fec..a40d5f22939 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecificationTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecificationTest.java
@@ -1,6 +1,9 @@
 // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.builder.xml.dom;
 
+import com.yahoo.config.provision.NodeResources.Architecture;
+import com.yahoo.config.provision.NodeResources.DiskSpeed;
+import com.yahoo.config.provision.NodeResources.StorageType;
 import com.yahoo.text.XML;
 import org.junit.jupiter.api.Test;
 import org.w3c.dom.Document;
@@ -9,6 +12,7 @@ import com.yahoo.component.Version;
 import java.util.Optional;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -17,6 +21,79 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 public class NodesSpecificationTest {
 
     @Test
+    void validResources() {
+        var spec = nodesSpecification("""
+                                      <nodes count='3'>
+                                        <resources vcpu='2'
+                                                   memory='3Y'
+                                                   disk='4tB'
+                                                   bandwidth='1ZbPs'
+                                                   disk-speed='fast'
+                                                   storage-type='local'
+                                                   architecture='x86_64'>
+                                          <gpu count='1g' memory='3' />
+                                        </resources>
+                                      </nodes>
+                                      """);
+
+        assertEquals(3, spec.minResources().nodes());
+        assertEquals(3, spec.maxResources().nodes());
+
+        assertEquals(2, spec.minResources().nodeResources().vcpu(), 1e-9);
+        assertEquals(2, spec.maxResources().nodeResources().vcpu(), 1e-9);
+
+        assertEquals(3e15, spec.minResources().nodeResources().memoryGb(), 1e-9);
+        assertEquals(3e15, spec.maxResources().nodeResources().memoryGb(), 1e-9);
+
+        assertEquals(4e3, spec.minResources().nodeResources().diskGb(), 1e-9);
+        assertEquals(4e3, spec.maxResources().nodeResources().diskGb(), 1e-9);
+
+        assertEquals(1e12, spec.minResources().nodeResources().bandwidthGbps(), 1e-9);
+        assertEquals(1e12, spec.maxResources().nodeResources().bandwidthGbps(), 1e-9);
+
+        assertEquals(1 << 30, spec.minResources().nodeResources().gpuResources().count());
+        assertEquals(1 << 30, spec.maxResources().nodeResources().gpuResources().count());
+
+        assertEquals(3e-9, spec.minResources().nodeResources().gpuResources().memoryGb(), 1e-12);
+        assertEquals(3e-9, spec.maxResources().nodeResources().gpuResources().memoryGb(), 1e-12);
+
+        assertEquals(DiskSpeed.fast, spec.minResources().nodeResources().diskSpeed());
+        assertEquals(DiskSpeed.fast, spec.maxResources().nodeResources().diskSpeed());
+
+        assertEquals(StorageType.local, spec.minResources().nodeResources().storageType());
+        assertEquals(StorageType.local, spec.maxResources().nodeResources().storageType());
+
+        assertEquals(Architecture.x86_64, spec.minResources().nodeResources().architecture());
+        assertEquals(Architecture.x86_64, spec.maxResources().nodeResources().architecture());
+    }
+
+    @Test
+    void invalidResources() {
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources vcpu='-1' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources vcpu='' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='-1' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='1x' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources vcpu='[-1,]' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources vcpu='[1,0.5]' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='[,-1b]' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='[1mb,999kb]' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='b' /></nodes>"));
+        assertThrows(IllegalArgumentException.class,
+                     () -> nodesSpecification("<nodes><resources memory='Yb' /></nodes>"));
+    }
+
+    @Test
     void noExplicitGroupLimits() {
         var spec = nodesSpecification("<nodes count='30'/>");
         assertEquals(30, spec.minResources().nodes());