Merge pull request #7522 from vespa-engine/mpolden/remove-host-reboot-on-state-transition

Only reboot non-hosts on dirty state transition

9 files changed, 26 insertions, 21 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
index 05cd341d659..7291add1cf1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
@@ -214,17 +214,19 @@ public class CuratorDatabaseClient {
     private Status newNodeStatus(Node node, Node.State toState) {
         if (node.state() != Node.State.failed && toState == Node.State.failed) return node.status().withIncreasedFailCount();
         if (node.state() == Node.State.failed && toState == Node.State.active) return node.status().withDecreasedFailCount(); // fail undo
-        // Increase reboot generation when node is moved to dirty unless quick reuse is prioritized. 
-        // This gets rid of lingering processes, updates OS packages if necessary and tests that reboot succeeds.
-        if (node.state() != Node.State.dirty && toState == Node.State.dirty && !needsFastNodeReuse(zone))
+        if (rebootOnTransitionTo(toState, node)) {
             return node.status().withReboot(node.status().reboot().withIncreasedWanted());
-
+        }
         return node.status();
     }
 
-    /** In automated test environments, nodes need to be reused quickly to achieve fast test turnaround time */
-    private boolean needsFastNodeReuse(Zone zone) {
-        return zone.environment().isTest();
+    /** Returns whether to reboot node as part of transition to given state. This is done to get rid of any lingering
+     * unwanted state (e.g. processes) on non-host nodes. */
+    private boolean rebootOnTransitionTo(Node.State state, Node node) {
+        if (node.type().isDockerHost()) return false; // Reboot of host nodes is handled by NodeRebooter
+        if (zone.environment().isTest()) return false; // We want to reuse nodes quickly in test environments
+
+        return node.state() != Node.State.dirty && state == Node.State.dirty;
     }
 
     /**
@@ -238,7 +240,7 @@ public class CuratorDatabaseClient {
         for (Node.State state : states) {
             for (String hostname : curatorDatabase.getChildren(toPath(state))) {
                 Optional<Node> node = getNode(hostname, state);
-                if (node.isPresent()) nodes.add(node.get()); // node might disappear between getChildren and getNode
+                node.ifPresent(nodes::add); // node might disappear between getChildren and getNode
             }
         }
         return nodes;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java
index 57152d12416..fe29b3b9fe2 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java
@@ -22,8 +22,8 @@ public class NodeRebooterTest {
         MaintenanceTester tester = new MaintenanceTester();
         tester.createReadyTenantNodes(15);
         tester.createReadyHostNodes(15);
-        // New nodes are rebooted when transitioning from dirty to ready. Advance the time so that additional reboots
-        // will be performed.
+        // New non-host nodes are rebooted when transitioning from dirty to ready. Advance the time so that additional
+        // reboots will be performed.
         tester.clock.advance(rebootInterval);
         
         NodeRebooter rebooter = new NodeRebooter(tester.nodeRepository, tester.clock, rebootInterval, tester.jobControl());
@@ -34,12 +34,15 @@ public class NodeRebooterTest {
                      withCurrentRebootGeneration(2L, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready)).size());
         assertEquals("No nodes have 2 reboots scheduled",
                      0,
-                     withCurrentRebootGeneration(3L, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready)).size());
+                     withCurrentRebootGeneration(3L, tester.nodeRepository.getNodes(Node.State.ready)).size());
 
         maintenanceIntervals(rebooter, tester, 11);
-        assertEquals("Reboot interval is 10x iteration interval, so most nodes are now rebooted twice",
-                     30,
-                     withCurrentRebootGeneration(3L, tester.nodeRepository.getNodes(Node.State.ready)).size());
+        assertEquals("Reboot interval is 10x iteration interval, so tenant nodes are now rebooted 3 times",
+                     15,
+                     withCurrentRebootGeneration(3L, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready)).size());
+        assertEquals("Reboot interval is 10x iteration interval, so host nodes are now rebooted twice",
+                     15,
+                     withCurrentRebootGeneration(2L, tester.nodeRepository.getNodes(NodeType.host, Node.State.ready)).size());
     }
     
     private void maintenanceIntervals(NodeRebooter rebooter, MaintenanceTester tester, int iterations) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade-complete.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade-complete.json
index 203220d1b84..7dbe7a100dc 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade-complete.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade-complete.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "currentOsVersion": "7.5.2",
   "wantedOsVersion": "7.5.2",
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade.json
index eed164689f1..72280f91cc9 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1-os-upgrade.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "wantedOsVersion": "7.5.2",
   "failCount": 0,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1.json
index e92cf895a8a..fac2813f6c5 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node1.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "failCount": 0,
   "hardwareFailure": false,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node2.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node2.json
index b832fddf119..d90649cb7a5 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node2.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node2.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "failCount": 0,
   "hardwareFailure": false,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node3.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node3.json
index 17729207c1e..5fc6d432e29 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node3.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node3.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "failCount": 0,
   "hardwareFailure": false,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node4.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node4.json
index 2c3d112c69a..7624340c553 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node4.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node4.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "failCount": 0,
   "hardwareFailure": false,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node5.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node5.json
index 73c7f89ff0f..e9a7f153bb5 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node5.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/docker-node5.json
@@ -31,7 +31,7 @@
   "wantedDockerImage": "docker-registry.domain.tld:8080/dist/vespa:6.42.0",
   "wantedVespaVersion": "6.42.0",
   "allowedToBeDown": false,
-  "rebootGeneration": 1,
+  "rebootGeneration": 0,
   "currentRebootGeneration": 0,
   "failCount": 0,
   "hardwareFailure": false,