Merge pull request #26375 from vespa-engine/ogronnesby/no-noderesources-add

Don't use NodeResources::add to create ResourceSnapshot

2 files changed, 71 insertions, 1 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java
index 4a849b831b7..f265ec3116c 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java
@@ -53,7 +53,7 @@ public class ResourceSnapshot {
 
         var resources = nodes.stream()
                 .map(Node::resources)
-                .reduce(NodeResources.zero(), NodeResources::add);
+                .reduce(NodeResources.zero(), ResourceSnapshot::addResources);
 
         return new ResourceSnapshot(applicationIds.iterator().next(), resources, timestamp, zoneId, versions.iterator().next());
     }
@@ -95,4 +95,24 @@ public class ResourceSnapshot {
     public int hashCode(){
         return Objects.hash(applicationId, resources, timestamp, zoneId, version);
     }
+
+    /* This function does pretty much the same thing as NodeResources::add, but it allows adding resources
+     * where some dimensions that are not relevant for billing (yet) are not the same.
+     *
+     * TODO: Make this code respect all dimensions.
+     */
+    private static NodeResources addResources(NodeResources a, NodeResources b) {
+        if (a.architecture() != b.architecture() && a.architecture() != NodeResources.Architecture.any && b.architecture() != NodeResources.Architecture.any) {
+            throw new IllegalArgumentException(a + " and " + b + " are not interchangeable for resource snapshots");
+        }
+        return new NodeResources(
+                a.vcpu() + b.vcpu(),
+                a.memoryGb() + b.memoryGb(),
+                a.diskGb() + b.diskGb(),
+                0,
+                NodeResources.DiskSpeed.any,
+                NodeResources.StorageType.any,
+                a.architecture(),
+                a.gpuResources().plus(b.gpuResources()));
+    }
 }
diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/resource/ResourceSnapshotTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/resource/ResourceSnapshotTest.java
new file mode 100644
index 00000000000..e5d615f8803
--- /dev/null
+++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/resource/ResourceSnapshotTest.java
@@ -0,0 +1,50 @@
+package com.yahoo.vespa.hosted.controller.api.resource;
+
+import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.NodeResources;
+import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
+import com.yahoo.vespa.hosted.controller.api.integration.resource.ResourceSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.time.Instant;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class ResourceSnapshotTest {
+    @Test
+    void test_adding_resources_collapse_dimensions() {
+        var nodes = List.of(
+                nodeWithResources(NodeResources.zero().with(NodeResources.DiskSpeed.fast)),
+                nodeWithResources(NodeResources.zero().with(NodeResources.DiskSpeed.slow)));
+
+        // This should be OK and not throw exception
+        var snapshot = ResourceSnapshot.from(nodes, Instant.EPOCH, ZoneId.defaultId());
+
+        assertEquals(NodeResources.DiskSpeed.any, snapshot.resources().diskSpeed());
+    }
+
+    @Test
+    void test_adding_resources_fail() {
+        var nodes = List.of(
+                nodeWithResources(NodeResources.zero().with(NodeResources.Architecture.x86_64)),
+                nodeWithResources(NodeResources.zero().with(NodeResources.Architecture.arm64)));
+
+        try {
+            ResourceSnapshot.from(nodes, Instant.EPOCH, ZoneId.defaultId());
+            fail("Should throw an exception");
+        } catch (IllegalArgumentException e) {
+            // this should happen
+        }
+    }
+
+    private Node nodeWithResources(NodeResources resources) {
+        return Node.builder()
+                .hostname("a")
+                .owner(ApplicationId.defaultId())
+                .resources(resources)
+                .build();
+    }
+}