Merge pull request #12075 from vespa-engine/freva/reset-want-to-deprovision

Automatically set and clear wantToDeprovision when adding first/remov…

6 files changed, 49 insertions, 23 deletions

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 fd6094ae111..7885cec6b65 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
@@ -71,6 +71,6 @@ public class Reports {
             return this;
         }
 
-        public Reports build() { return new Reports(Collections.unmodifiableMap(reportMap)); }
+        public Reports build() { return new Reports(reportMap); }
     }
 }
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 09cb5dad0a9..577e28a4237 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
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeResources;
+import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.io.IOUtils;
 import com.yahoo.slime.Inspector;
@@ -34,8 +35,8 @@ import java.util.stream.Collectors;
 
 import static com.yahoo.config.provision.NodeResources.DiskSpeed.fast;
 import static com.yahoo.config.provision.NodeResources.DiskSpeed.slow;
-import static com.yahoo.config.provision.NodeResources.StorageType.remote;
 import static com.yahoo.config.provision.NodeResources.StorageType.local;
+import static com.yahoo.config.provision.NodeResources.StorageType.remote;
 
 /**
  * A class which can take a partial JSON node/v2 node JSON structure and apply it to a node object.
@@ -46,7 +47,6 @@ import static com.yahoo.config.provision.NodeResources.StorageType.local;
 public class NodePatcher {
 
     private static final String WANT_TO_RETIRE = "wantToRetire";
-    private static final String WANT_TO_DEPROVISION = "wantToDeprovision";
 
     private final NodeFlavors nodeFlavors;
     private final Inspector inspector;
@@ -100,7 +100,6 @@ public class NodePatcher {
     private List<Node> applyFieldRecursive(List<Node> childNodes, String name, Inspector value) {
         switch (name) {
             case WANT_TO_RETIRE:
-            case WANT_TO_DEPROVISION:
                 return childNodes.stream()
                         .map(child -> applyField(child, name, value))
                         .collect(Collectors.toList());
@@ -139,7 +138,9 @@ public class NodePatcher {
                 return IP.Config.verify(node.with(node.ipConfig().with(IP.Pool.of(asStringSet(value)))), nodes);
             case WANT_TO_RETIRE :
                 return node.withWantToRetire(asBoolean(value), Agent.operator, clock.instant());
-            case WANT_TO_DEPROVISION :
+            case "wantToDeprovision" :
+                if (node.type() != NodeType.host && asBoolean(value))
+                    throw new IllegalArgumentException("wantToDeprovision can only be set for hosts");
                 return node.with(node.status().withWantToDeprovision(asBoolean(value)));
             case "reports" :
                 return nodeWithPatchedReports(node, value);
@@ -172,22 +173,42 @@ public class NodePatcher {
     }
 
     private Node nodeWithPatchedReports(Node node, Inspector reportsInspector) {
+        Node patchedNode;
         // "reports": null clears the reports
-        if (reportsInspector.type() == Type.NIX) return node.with(new Reports());
+        if (reportsInspector.type() == Type.NIX) {
+            patchedNode = node.with(new Reports());
+        } else {
+            var reportsBuilder = new Reports.Builder(node.reports());
+            reportsInspector.traverse((ObjectTraverser) (reportId, reportInspector) -> {
+                if (reportInspector.type() == Type.NIX) {
+                    // ... "reports": { "reportId": null } clears the report "reportId"
+                    reportsBuilder.clearReport(reportId);
+                } else {
+                    // ... "reports": { "reportId": {...} } overrides the whole report "reportId"
+                    reportsBuilder.setReport(Report.fromSlime(reportId, reportInspector));
+                }
+            });
+            patchedNode = node.with(reportsBuilder.build());
+        }
 
-        var reportsBuilder = new Reports.Builder(node.reports());
+        boolean hadHardFailReports = node.reports().getReports().stream()
+                .anyMatch(r -> r.getType() == Report.Type.HARD_FAIL);
+        boolean hasHardFailReports = patchedNode.reports().getReports().stream()
+                .anyMatch(r -> r.getType() == Report.Type.HARD_FAIL);
 
-        reportsInspector.traverse((ObjectTraverser) (reportId, reportInspector) -> {
-            if (reportInspector.type() == Type.NIX) {
-                // ... "reports": { "reportId": null } clears the report "reportId"
-                reportsBuilder.clearReport(reportId);
-            } else {
-                // ... "reports": { "reportId": {...} } overrides the whole report "reportId"
-                reportsBuilder.setReport(Report.fromSlime(reportId, reportInspector));
-            }
-        });
+        // If this patch resulted in going from not having HARD_FAIL report to having one, or vice versa
+        if (hadHardFailReports != hasHardFailReports) {
+            // Do not automatically change wantToDeprovision when
+            // 1. Transitioning to having a HARD_FAIL report and being in state failed:
+            //    To allow operators manually unset before the host is parked and deleted.
+            // 2. When in parked state: Deletion is imminent, possibly already underway
+            if ((hasHardFailReports && node.state() == Node.State.failed) || node.state() == Node.State.parked)
+                return patchedNode;
+
+            patchedNode = patchedNode.with(patchedNode.status().withWantToDeprovision(hasHardFailReports));
+        }
 
-        return node.with(reportsBuilder.build());
+        return patchedNode;
     }
 
     private Set<String> asStringSet(Inspector field) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
index 6dfb1aed47e..c26614c630c 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
@@ -218,9 +218,6 @@ public class RestApiTest {
                                    Utf8.toBytes("{\"wantToRetire\": true}"), Request.Method.PATCH),
                        "{\"message\":\"Updated host4.yahoo.com\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
-                        Utf8.toBytes("{\"wantToDeprovision\": true}"), Request.Method.PATCH),
-                "{\"message\":\"Updated host4.yahoo.com\"}");
-        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
                         Utf8.toBytes("{\"currentVespaVersion\": \"6.43.0\",\"currentDockerImage\": \"docker-registry.domain.tld:8080/dist/vespa:6.45.0\"}"), Request.Method.PATCH),
                         "{\"message\":\"Updated host4.yahoo.com\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
@@ -229,6 +226,9 @@ public class RestApiTest {
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/dockerhost1.yahoo.com",
                                    Utf8.toBytes("{\"modelName\": \"foo\"}"), Request.Method.PATCH),
                        "{\"message\":\"Updated dockerhost1.yahoo.com\"}");
+        assertResponse(new Request("http://localhost:8080/nodes/v2/node/dockerhost1.yahoo.com",
+                        Utf8.toBytes("{\"wantToDeprovision\": true}"), Request.Method.PATCH),
+                "{\"message\":\"Updated dockerhost1.yahoo.com\"}");
         assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/dockerhost1.yahoo.com"), "\"modelName\":\"foo\"");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/dockerhost1.yahoo.com",
                                    Utf8.toBytes("{\"modelName\": null}"), Request.Method.PATCH),
@@ -541,11 +541,13 @@ public class RestApiTest {
                                 "    \"actualCpuCores\": {" +
                                 "      \"createdMillis\": 1, " +
                                 "      \"description\": \"Actual number of CPU cores (2) differs from spec (4)\"," +
+                                "      \"type\": \"HARD_FAIL\"," +
                                 "      \"value\":2" +
                                 "    }," +
                                 "    \"diskSpace\": {" +
                                 "      \"createdMillis\": 2, " +
                                 "      \"description\": \"Actual disk space (2TB) differs from spec (3TB)\"," +
+                                "      \"type\": \"HARD_FAIL\"," +
                                 "      \"details\": {" +
                                 "        \"inGib\": 3," +
                                 "        \"disks\": [\"/dev/sda1\", \"/dev/sdb3\"]" +
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
index ad5c28d6a80..af3552945d9 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
@@ -35,7 +35,7 @@
   "currentDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.45.0",
   "failCount": 1,
   "wantToRetire": true,
-  "wantToDeprovision": true,
+  "wantToDeprovision": false,
   "history": [
     {
       "event": "provisioned",
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports-2.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports-2.json
index d33c1c9e743..a3d53798d7c 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports-2.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports-2.json
@@ -30,7 +30,7 @@
   "currentRebootGeneration": 0,
   "failCount": 0,
   "wantToRetire": false,
-  "wantToDeprovision": false,
+  "wantToDeprovision": true,
   "history": [
     {
       "event": "provisioned",
@@ -65,6 +65,7 @@
     "diskSpace": {
       "createdMillis": 2,
       "description": "Actual disk space (2TB) differs from spec (3TB)",
+      "type":"HARD_FAIL",
       "details": {
         "inGib": 3,
         "disks": [
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports.json
index 4119e46e225..67b8d67c7f1 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node6-reports.json
@@ -30,7 +30,7 @@
   "currentRebootGeneration": 0,
   "failCount": 0,
   "wantToRetire": false,
-  "wantToDeprovision": false,
+  "wantToDeprovision": true,
   "history": [
     {
       "event": "provisioned",
@@ -62,11 +62,13 @@
     "actualCpuCores": {
       "createdMillis": 1,
       "description": "Actual number of CPU cores (2) differs from spec (4)",
+      "type":"HARD_FAIL",
       "value": 2
     },
     "diskSpace": {
       "createdMillis": 2,
       "description": "Actual disk space (2TB) differs from spec (3TB)",
+      "type":"HARD_FAIL",
       "details": {
         "inGib": 3,
         "disks": [