diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2023-12-18 15:04:39 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-12-18 15:04:39 +0100 |
commit | c594e1ce3a1bb59fbe97f81d17a8c67361d7994c (patch) | |
tree | e1c64368225d6bd6142f2d799a8a1d604fab9fec /config-model/src | |
parent | 6adad88bd950d1af73a676752639d8c9cd4a74c1 (diff) | |
parent | 45fc6575935c2188131d6b8e310570cb0c56bbde (diff) |
Merge pull request #29694 from vespa-engine/jonmv/more-careful-resource-parsing
More careful resource parsing
Diffstat (limited to 'config-model/src')
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()); |