Merge pull request #8516 from vespa-engine/hakonhall/loose-allocation-when-retiring-or-hw-failing

Lose allocation when retiring or hw failing

9 files changed, 29 insertions, 16 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index a02508c3474..8d127f6c547 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -250,6 +250,13 @@ public final class Node {
         return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type);
     }
 
+    /** Returns a new Node without an allocation. */
+    public Node withoutAllocation() {
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state,
+                Optional.empty(), history, type, reports, modelName);
+    }
+
+
     /** Returns a copy of this node with the IP addresses set to the given value. */
     public Node withIpAddresses(Set<String> ipAddresses) {
         return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state,
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index e48d8a11c27..bc2e729b791 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -452,7 +452,7 @@ public class NodeRepository extends AbstractComponent {
      * @throws NoSuchNodeException if the node is not found
      */
     public Node fail(String hostname, Agent agent, String reason) {
-        return move(hostname, Node.State.failed, agent, Optional.of(reason));
+        return move(hostname, true, Node.State.failed, agent, Optional.of(reason));
     }
 
     /**
@@ -470,8 +470,8 @@ public class NodeRepository extends AbstractComponent {
      * @return the node in its new state
      * @throws NoSuchNodeException if the node is not found
      */
-    public Node park(String hostname, Agent agent, String reason) {
-        return move(hostname, Node.State.parked, agent, Optional.of(reason));
+    public Node park(String hostname, boolean keepAllocation, Agent agent, String reason) {
+        return move(hostname, keepAllocation, Node.State.parked, agent, Optional.of(reason));
     }
 
     /**
@@ -490,7 +490,7 @@ public class NodeRepository extends AbstractComponent {
      * @throws NoSuchNodeException if the node is not found
      */
     public Node reactivate(String hostname, Agent agent, String reason) {
-        return move(hostname, Node.State.active, agent, Optional.of(reason));
+        return move(hostname, true, Node.State.active, agent, Optional.of(reason));
     }
 
     private List<Node> moveRecursively(String hostname, Node.State toState, Agent agent, Optional<String> reason) {
@@ -498,13 +498,18 @@ public class NodeRepository extends AbstractComponent {
                                          .map(child -> move(child, toState, agent, reason))
                                          .collect(Collectors.toList());
 
-        moved.add(move(hostname, toState, agent, reason));
+        moved.add(move(hostname, true, toState, agent, reason));
         return moved;
     }
 
-    private Node move(String hostname, Node.State toState, Agent agent, Optional<String> reason) {
+    private Node move(String hostname, boolean keepAllocation, Node.State toState, Agent agent, Optional<String> reason) {
         Node node = getNode(hostname).orElseThrow(() ->
                 new NoSuchNodeException("Could not move " + hostname + " to " + toState + ": Node not found"));
+
+        if (!keepAllocation && node.allocation().isPresent()) {
+            node = node.withoutAllocation();
+        }
+
         return move(node, toState, agent, reason);
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
index 52ef5b4d1c6..1b18dfc46c1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
@@ -107,8 +107,8 @@ public class FailedExpirer extends Maintainer {
                                       .collect(Collectors.toList());
 
                 if (unparkedChildren.isEmpty()) {
-                    nodeRepository.park(candidate.hostname(), Agent.system,
-                                        "Parked by FailedExpirer due to hardware issue");
+                    nodeRepository.park(candidate.hostname(), false, Agent.system,
+                            "Parked by FailedExpirer due to hardware issue");
                 } else {
                     log.info(String.format("Expired failed node %s with hardware issue was not parked because of " +
                                            "unparked children: %s", candidate.hostname(),
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
index 0d5525370ee..1c0e788250e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
@@ -40,7 +40,7 @@ public class InactiveExpirer extends Expirer {
     protected void expire(List<Node> expired) {
         expired.forEach(node -> {
             if (node.status().wantToRetire()) {
-                nodeRepository.park(node.hostname(), Agent.system, "Expired by InactiveExpirer");
+                nodeRepository.park(node.hostname(), false, Agent.system, "Expired by InactiveExpirer");
             } else {
                 nodeRepository.setDirty(node, Agent.system, "Expired by InactiveExpirer");
             }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java
index b170d4eb66e..3113aaf45cc 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java
@@ -87,7 +87,7 @@ public class NodeRetirer extends Maintainer {
 
                             retirementPolicy.shouldRetire(nodeToRetire).ifPresent(reason -> {
                                 nodeRepository().write(nodeToRetire.with(nodeToRetire.status().withWantToDeprovision(true)));
-                                nodeRepository().park(nodeToRetire.hostname(), Agent.NodeRetirer, reason);
+                                nodeRepository().park(nodeToRetire.hostname(), false, Agent.NodeRetirer, reason);
                                 iter.remove();
                             });
                         }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
index 98a3020e38e..77a6ff675c4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
@@ -33,7 +33,7 @@ public class NodeRepositoryTest {
 
         assertEquals(3, tester.nodeRepository().getNodes().size());
         
-        tester.nodeRepository().park("host2", Agent.system, "Parking to unit test");
+        tester.nodeRepository().park("host2", true, Agent.system, "Parking to unit test");
         tester.nodeRepository().removeRecursively("host2");
 
         assertEquals(2, tester.nodeRepository().getNodes().size());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTest.java
index 6eeeb3dfe08..399ff8582bd 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTest.java
@@ -159,7 +159,7 @@ public class NodeRetirerTest {
 
         // Now 2 of those finish retiring and go to parked
         nodesToRetire.stream().limit(2).forEach(node ->
-                tester.nodeRepository.park(node.hostname(), Agent.system, "Parked for unit testing"));
+                tester.nodeRepository.park(node.hostname(), false, Agent.system, "Parked for unit testing"));
         long actualOneRetired = retirer.getNumberNodesAllowToRetireForCluster(tester.nodeRepository.getNodes(app), 2);
         assertEquals(1, actualOneRetired);
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
index 64613f1bf56..5e33e863df7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
@@ -41,6 +41,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
+import java.util.stream.LongStream;
 
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Matchers.any;
@@ -163,9 +164,9 @@ public class NodeRetirerTester {
     }
 
     void assertParkedCountsByApplication(long... nums) {
-        Map<ApplicationId, Long> expected = expectedCountsByApplication(nums);
-        Map<ApplicationId, Long> actual = nodeRepository.getNodes(Node.State.parked).stream()
-                .collect(Collectors.groupingBy(node -> node.allocation().get().owner(), Collectors.counting()));
+        // Nodes lose allocation when parked, so just do a sum.
+        long expected = LongStream.of(nums).filter(value -> value > 0L).sum();
+        long actual = (long) nodeRepository.getNodes(Node.State.parked).size();
         assertEquals(expected, actual);
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
index 8a10c0207b4..67fa610e2e0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
@@ -89,7 +89,7 @@ public class PeriodicApplicationMaintainerTest {
         // Fail and park some nodes
         nodeRepository.fail(nodeRepository.getNodes(fixture.app1).get(3).hostname(), Agent.system, "Failing to unit test");
         nodeRepository.fail(nodeRepository.getNodes(fixture.app2).get(0).hostname(), Agent.system, "Failing to unit test");
-        nodeRepository.park(nodeRepository.getNodes(fixture.app2).get(4).hostname(), Agent.system, "Parking to unit test");
+        nodeRepository.park(nodeRepository.getNodes(fixture.app2).get(4).hostname(), true, Agent.system, "Parking to unit test");
         int failedInApp1 = 1;
         int failedOrParkedInApp2 = 2;
         assertEquals(fixture.wantedNodesApp1 - failedInApp1, nodeRepository.getNodes(fixture.app1, Node.State.active).size());