Only include node states that are meterable in metering

3 files changed, 65 insertions, 6 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainer.java
index 0e14b61c5c5..20c4c3d4421 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainer.java
@@ -17,6 +17,7 @@ import java.time.Duration;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -74,7 +75,8 @@ public class ResourceMeterMaintainer extends Maintainer {
 
     private Collection<ResourceSnapshot> createResourceSnapshotsFromNodes(ZoneId zoneId, List<Node> nodes) {
         return nodes.stream()
-                .filter(unlessNodeOwnerIsHostedVespa())
+                .filter(this::unlessNodeOwnerIsHostedVespa)
+                .filter(this::isNodeStateMeterable)
                 .collect(Collectors.groupingBy(node ->
                                 node.owner().get(),
                                 Collectors.collectingAndThen(Collectors.toList(),
@@ -85,9 +87,23 @@ public class ResourceMeterMaintainer extends Maintainer {
                                 )).values();
     }
 
-    private Predicate<Node> unlessNodeOwnerIsHostedVespa() {
-        return node -> node.owner().map(owner ->
-                !owner.tenant().value().equals("hosted-vespa")
-        ).orElse(false);
+    private boolean unlessNodeOwnerIsHostedVespa(Node node) {
+        return node.owner()
+                .map(owner -> !owner.tenant().value().equals("hosted-vespa"))
+                .orElse(false);
+    }
+
+    /**
+     * Checks if the node is in some state where it is in active use by the tenant,
+     * and not transitioning out of use, in a failed state, etc.
+     */
+    private static final Set<Node.State> METERABLE_NODE_STATES = Set.of(
+            Node.State.reserved,   // an application will soon use this node
+            Node.State.active,     // an application is currently using this node
+            Node.State.inactive    // an application owner might set a node inactive
+    );
+
+    private boolean isNodeStateMeterable(Node node) {
+        return METERABLE_NODE_STATES.contains(node.state());
     }
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
index 36f962d653f..9c48c1adb85 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/NodeRepositoryMock.java
@@ -63,7 +63,10 @@ public class NodeRepositoryMock implements NodeRepository {
     }
 
     public void addNodes(ZoneId zone, List<Node> nodes) {
-        nodeRepository.put(zone, nodes.stream().collect(Collectors.toMap(Node::hostname, Function.identity())));
+        var existingNodes = nodeRepository.getOrDefault(zone, new HashMap<>());
+        var newNodes = nodes.stream().collect(Collectors.toMap(Node::hostname, Function.identity()));
+        newNodes.forEach(existingNodes::put);
+        nodeRepository.put(zone, existingNodes);
     }
 
     public void addFixedNodes(ZoneId zone) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainerTest.java
index 0245e7475f7..f23a59adc36 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ResourceMeterMaintainerTest.java
@@ -1,8 +1,13 @@
 // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.maintenance;
 
+import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.HostName;
+import com.yahoo.config.provision.NodeResources;
+import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
+import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
 import com.yahoo.vespa.hosted.controller.api.integration.resource.ResourceSnapshot;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockMeteringClient;
 import com.yahoo.vespa.hosted.controller.integration.MetricsMock;
@@ -10,7 +15,10 @@ import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 
@@ -60,5 +68,37 @@ public class ResourceMeterMaintainerTest {
         tester.configServer().nodeRepository().addFixedNodes(nonAwsZone.getId());
         tester.configServer().nodeRepository().addFixedNodes(awsZone1.getId());
         tester.configServer().nodeRepository().addFixedNodes(awsZone2.getId());
+        tester.configServer().nodeRepository().addNodes(
+                awsZone1.getId(),
+                createNodesInState(
+                        Node.State.provisioned,
+                        Node.State.ready,
+                        Node.State.dirty,
+                        Node.State.failed,
+                        Node.State.parked
+                )
+        );
+    }
+
+    private List<Node> createNodesInState(Node.State ...states) {
+        return Arrays.stream(states)
+                .map(state -> {
+                    return new Node.Builder()
+                            .hostname(HostName.from("host" + state))
+                            .parentHostname(HostName.from("parenthost" + state))
+                            .state(state)
+                            .type(NodeType.tenant)
+                            .owner(ApplicationId.from("tenant1", "app1", "default"))
+                            .currentVersion(Version.fromString("7.42"))
+                            .wantedVersion(Version.fromString("7.42"))
+                            .currentOsVersion(Version.fromString("7.6"))
+                            .wantedOsVersion(Version.fromString("7.6"))
+                            .serviceState(Node.ServiceState.expectedUp)
+                            .resources(new NodeResources(24, 24, 500, 1))
+                            .clusterId("clusterA")
+                            .clusterType(Node.ClusterType.container)
+                            .build();
+                })
+                .collect(Collectors.toUnmodifiableList());
     }
 }