Preserve only specific fields when reprovisioning

9 files changed, 43 insertions, 27 deletions

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 05b171f36b0..08741dccd89 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
@@ -349,13 +349,13 @@ public class NodeRepository extends AbstractComponent {
 
     /**
      * Adds a list of (newly created) nodes to the node repository as <i>provisioned</i> nodes.
-     * If any of the nodes already exists in the deprovisioned state, they will be moved back to provisioned instead
-     * and the returned list will contain the existing (moved) node.
+     * If any of the nodes already exists in the deprovisioned state, the new node will be merged
+     * with the history of that node.
      */
     public List<Node> addNodes(List<Node> nodes, Agent agent) {
         try (Mutex lock = lockUnallocated()) {
             List<Node> nodesToAdd =  new ArrayList<>();
-            List<Node> nodesToMove = new ArrayList<>();
+            List<Node> nodesToRemove = new ArrayList<>();
             for (int i = 0; i < nodes.size(); i++) {
                 var node = nodes.get(i);
 
@@ -369,15 +369,19 @@ public class NodeRepository extends AbstractComponent {
                 if (existing.isPresent()) {
                     if (existing.get().state() != State.deprovisioned)
                         illegal("Cannot add " + node + ": A node with this name already exists");
-                    nodesToMove.add(existing.get());
-                }
-                else {
-                    nodesToAdd.add(node);
+                    node = node.with(existing.get().history());
+                    node = node.with(existing.get().reports());
+                    node = node.with(node.status().withFailCount(existing.get().status().failCount()));
+                    if (existing.get().status().firmwareVerifiedAt().isPresent())
+                        node = node.with(node.status().withFirmwareVerifiedAt(existing.get().status().firmwareVerifiedAt().get()));
+                    nodesToRemove.add(existing.get());
                 }
+
+                nodesToAdd.add(node);
             }
             List<Node> resultingNodes = new ArrayList<>();
             resultingNodes.addAll(db.addNodesInState(IP.Config.verify(nodesToAdd, list(lock)), State.provisioned));
-            nodesToMove.forEach(node -> resultingNodes.add(move(node, State.provisioned, agent, Optional.empty())));
+            db.removeNodes(nodesToRemove);
             return resultingNodes;
         }
     }
@@ -609,14 +613,10 @@ public class NodeRepository extends AbstractComponent {
                 children.forEach(child -> requireRemovable(child, true, force));
                 db.removeNodes(children);
                 List<Node> removed = new ArrayList<>(children);
-                if (zone.cloud().value().equals("aws")) {
+                if (zone.cloud().value().equals("aws"))
                     db.removeNodes(List.of(node));
-                }
-                else {
-                    node = node.withWantToRetire(false, Agent.system, clock.instant());
-                    node = node.with(node.status().withWantToDeprovision(false));
+                else
                     move(node, State.deprovisioned, Agent.system, Optional.empty());
-                }
                 removed.add(node);
                 return removed;
             }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Report.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Report.java
index 65cbc17aff5..37141d8f25b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Report.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Report.java
@@ -17,10 +17,13 @@ import java.util.Arrays;
  * @author hakonhall
  */
 public class Report {
+
     /** The time the report was created, in milliseconds since Epoch. */
     public static final String CREATED_FIELD = "createdMillis";
+
     /** The type of the report. */
     public static final String TYPE_FIELD = "type";
+
     /** The description of the report. */
     public static final String DESCRIPTION_FIELD = "description";
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Reports.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Reports.java
index 7885cec6b65..8680c569d44 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Reports.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Reports.java
@@ -17,6 +17,7 @@ import java.util.TreeMap;
  */
 // @Immutable
 public class Reports {
+
     private final Map<String, Report> reports;
 
     public Reports() { this(Collections.emptyMap()); }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java
index 15f3c481fe3..6b52bd68e73 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java
@@ -64,7 +64,7 @@ public class Status {
 
     public Status withDecreasedFailCount() { return new Status(reboot, vespaVersion, dockerImage, failCount - 1, wantToRetire, wantToDeprovision, osVersion, firmwareVerifiedAt); }
 
-    public Status setFailCount(Integer value) { return new Status(reboot, vespaVersion, dockerImage, value, wantToRetire, wantToDeprovision, osVersion, firmwareVerifiedAt); }
+    public Status withFailCount(int value) { return new Status(reboot, vespaVersion, dockerImage, value, wantToRetire, wantToDeprovision, osVersion, firmwareVerifiedAt); }
 
     /** Returns how many times this node has been moved to the failed state. */
     public int failCount() { return failCount; }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
index a060f9c8b55..12b245ede7f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
@@ -124,7 +124,7 @@ public class NodePatcher {
             case "currentFirmwareCheck":
                 return node.withFirmwareVerifiedAt(Instant.ofEpochMilli(asLong(value)));
             case "failCount" :
-                return node.with(node.status().setFailCount(asLong(value).intValue()));
+                return node.with(node.status().withFailCount(asLong(value).intValue()));
             case "flavor" :
                 return node.with(nodeFlavors.getFlavorOrThrow(asString(value)));
             case "parentHostname" :
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 56b848a5cf2..24c7a191dc5 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
@@ -148,19 +148,22 @@ public class NodeRepositoryTest {
     }
 
     @Test
-    public void deprovisioned_hosts_are_resurrected_on_add() {
+    public void relevant_information_from_deprovisioned_hosts_are_merged_into_readded_host() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
         Instant testStart = tester.nodeRepository().clock().instant();
 
-        ((ManualClock)tester.nodeRepository().clock()).advance(Duration.ofSeconds(1));
+        tester.clock().advance(Duration.ofSeconds(1));
         tester.addNode("id1", "host1", "default", NodeType.host);
         tester.addNode("id2", "host2", "default", NodeType.host);
         assertFalse(tester.nodeRepository().getNode("host1").get().history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
 
-        // Set host 1 properties and remove it
+        // Set host 1 properties and deprovision it
         Node host1 = tester.nodeRepository().getNode("host1").get();
         host1 = host1.withWantToRetire(true, Agent.system, tester.nodeRepository().clock().instant());
         host1 = host1.with(host1.status().withWantToDeprovision(true));
+        host1 = host1.withFirmwareVerifiedAt(tester.clock().instant());
+        host1 = host1.with(host1.status().withIncreasedFailCount());
+        host1 = host1.with(host1.reports().withReport(Report.basicReport("id", Report.Type.HARD_FAIL, tester.clock().instant(), "Test report")));
         tester.nodeRepository().write(host1, tester.nodeRepository().lock(host1));
         tester.nodeRepository().removeRecursively("host1");
 
@@ -168,12 +171,19 @@ public class NodeRepositoryTest {
         host1 = tester.nodeRepository().getNode("host1").get();
         assertEquals(Node.State.deprovisioned, host1.state());
         assertTrue(host1.history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
-        assertFalse(host1.status().wantToRetire());
-        assertFalse(host1.status().wantToDeprovision());
 
-        // Adding it again moves it from deprovisioned
-        host1 = tester.addNode("id1", "host1", "default", NodeType.host);
-        assertTrue(host1.history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
+        // Adding it again preserves some information from the deprovisioned host and removes is
+        tester.addNode("id2", "host1", "default", NodeType.host);
+        host1 = tester.nodeRepository().getNode("host1").get();
+        assertEquals("This is the newly added node", "id2", host1.id());
+        assertFalse("The old 'host1' is removed",
+                    tester.nodeRepository().getNode("host1", Node.State.deprovisioned).isPresent());
+        assertFalse("Not transferred from deprovisioned host", host1.status().wantToRetire());
+        assertFalse("Not transferred from deprovisioned host", host1.status().wantToDeprovision());
+        assertTrue("Transferred from deprovisioned host", host1.history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
+        assertTrue("Transferred from deprovisioned host", host1.status().firmwareVerifiedAt().isPresent());
+        assertEquals("Transferred from deprovisioned host", 1, host1.status().failCount());
+        assertEquals("Transferred from deprovisioned host", 1, host1.reports().getReports().size());
     }
 
     @Test
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
index 57c45636df9..8cf114e2bbb 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
@@ -26,7 +26,7 @@ public class NodeRepositoryTester {
 
     private final NodeFlavors nodeFlavors;
     private final NodeRepository nodeRepository;
-    private final Clock clock;
+    private final ManualClock clock;
     private final MockCurator curator;
 
     public NodeRepositoryTester() {
@@ -81,4 +81,6 @@ public class NodeRepositoryTester {
         return b.build();
     }
 
+    public ManualClock clock() { return clock; }
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
index 6665a6dcf69..77ed7fed4a3 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
@@ -291,7 +291,7 @@ public class FailedExpirerTest {
         public FailureScenario failNode(int times, String... hostname) {
             Stream.of(hostname).forEach(h -> {
                 Node node = get(h);
-                nodeRepository.write(node.with(node.status().setFailCount(times)), () -> {});
+                nodeRepository.write(node.with(node.status().withFailCount(times)), () -> {});
                 nodeRepository.fail(h, Agent.system, "Failed by unit test");
             });
             return this;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
index 21f25dcede4..58ebfa555d6 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
@@ -220,7 +220,7 @@ public class SerializationTest {
                              node.flavor().resources(),
                              clock.instant());
 
-        node = node.with(node.status().setFailCount(0));
+        node = node.with(node.status().withFailCount(0));
         Node copy2 = nodeSerializer.fromJson(Node.State.provisioned, nodeSerializer.toJson(node));
 
         assertEquals(0, copy2.status().failCount());