diff options
author | jonmv <venstad@gmail.com> | 2023-12-18 14:36:41 +0100 |
---|---|---|
committer | jonmv <venstad@gmail.com> | 2023-12-18 14:36:41 +0100 |
commit | 50b4132d1daf81dcc0b289b56ecc0d1489d5704e (patch) | |
tree | 645dd02ea64ef1aec67023d5c924c96122c769ac | |
parent | ee6bc94776a8998ce78353bbf685c8c121b3b41b (diff) |
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..3f91ddc9f9d 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 must be less than or equal to 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()); |