Differentiate between trying and requiring node state change

6 files changed, 42 insertions, 24 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
index db18a71c805..5a9d75a9d41 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
@@ -425,7 +425,7 @@ public class OrchestratorImpl implements Orchestrator {
                 ClusterControllerClient client = clusterControllerClientFactory.createClient(clusterControllers, cluster.clusterId().s());
                 for (ServiceInstance service : cluster.serviceInstances()) {
                     try {
-                        if ( ! client.setNodeState(context, service.hostName(), VespaModelUtil.getStorageNodeIndex(service.configId()), MAINTENANCE))
+                        if ( ! client.trySetNodeState(context, service.hostName(), VespaModelUtil.getStorageNodeIndex(service.configId()), MAINTENANCE))
                             return false;
                     }
                     catch (Exception e) {
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClient.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClient.java
index 2c31b475b21..98ca9f805b4 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClient.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClient.java
@@ -18,8 +18,16 @@ public interface ClusterControllerClient {
      * @return false is this was a probe operation, and permission would be denied.
      * @throws HostStateChangeDeniedException if operation fails, or is otherwise disallowed.
      */
-    boolean setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex,
-                         ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException;
+    boolean trySetNodeState(OrchestratorContext context, HostName host, int storageNodeIndex,
+                            ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException;
+
+    /**
+     * Requests that a cluster controller sets the requested node to the requested state.
+     *
+     * @throws HostStateChangeDeniedException if operation fails, or is disallowed.
+     */
+    void setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex,
+                      ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException;
 
     /**
      * Requests that a cluster controller sets all nodes in the cluster to the requested state.
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java
index b4966e71bfa..7c7dca3b03e 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java
@@ -52,8 +52,8 @@ public class ClusterControllerClientImpl implements ClusterControllerClient {
         this.client = client;
     }
 
-    @Override
-    public boolean setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex, ClusterControllerNodeState wantedState) {
+    private boolean setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex,
+                              ClusterControllerNodeState wantedState, boolean throwOnFailure) {
         try {
             ClusterControllerClientTimeouts timeouts = context.getClusterControllerTimeouts();
             Inspector response = client.send(strategy(hosts), Method.POST)
@@ -64,13 +64,12 @@ public class ClusterControllerClientImpl implements ClusterControllerClient {
                                        .throwing(retryOnRedirect)
                                        .read(SlimeUtils::jsonToSlime).get();
             if ( ! response.field("wasModified").asBool()) {
-                if (context.isProbe())
-                    return false;
-
-                throw new HostStateChangeDeniedException(host,
-                                                         HostedVespaPolicy.SET_NODE_STATE_CONSTRAINT,
-                                                         "Failed to set state to " + wantedState +
-                                                         " in cluster controller: " + response.field("reason").asString());
+                if (throwOnFailure)
+                    throw new HostStateChangeDeniedException(host,
+                                                             HostedVespaPolicy.SET_NODE_STATE_CONSTRAINT,
+                                                             "Failed to set state to " + wantedState +
+                                                             " in cluster controller: " + response.field("reason").asString());
+                return false;
             }
             return true;
         }
@@ -100,6 +99,16 @@ public class ClusterControllerClientImpl implements ClusterControllerClient {
     }
 
     @Override
+    public boolean trySetNodeState(OrchestratorContext context, HostName host, int storageNodeIndex, ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException {
+        return setNodeState(context, host, storageNodeIndex, wantedState, false);
+    }
+
+    @Override
+    public void setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex, ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException {
+        setNodeState(context, host, storageNodeIndex, wantedState, true);
+    }
+
+    @Override
     public void setApplicationState(OrchestratorContext context, ApplicationInstanceId applicationId,
                                     ClusterControllerNodeState wantedState) throws ApplicationStateChangeDeniedException {
         try {
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
index 6e8e1eb0ccb..f5b7771d5f4 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
@@ -459,16 +459,16 @@ public class OrchestratorImplTest {
                                             applicationApiFactory,
                                             flagSource);
 
-        when(fooClient.setNodeState(any(), any(), eq(1), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
-        when(fooClient.setNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
-        when(barClient.setNodeState(any(), any(), eq(0), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
-        when(barClient.setNodeState(any(), any(), eq(3), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
+        when(fooClient.trySetNodeState(any(), any(), eq(1), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
+        when(fooClient.trySetNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
+        when(barClient.trySetNodeState(any(), any(), eq(0), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
+        when(barClient.trySetNodeState(any(), any(), eq(3), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(true);
         assertTrue(orchestrator.isQuiescent(id));
 
-        when(fooClient.setNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(false);
+        when(fooClient.trySetNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenReturn(false);
         assertFalse(orchestrator.isQuiescent(id));
 
-        when(fooClient.setNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenThrow(new RuntimeException());
+        when(fooClient.trySetNodeState(any(), any(), eq(2), eq(ClusterControllerNodeState.MAINTENANCE))).thenThrow(new RuntimeException());
         assertFalse(orchestrator.isQuiescent(id));
     }
 
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientFactoryMock.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientFactoryMock.java
index 27928618eeb..597487ccfc5 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientFactoryMock.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientFactoryMock.java
@@ -53,13 +53,14 @@ public class ClusterControllerClientFactoryMock implements ClusterControllerClie
     @Override
     public ClusterControllerClient createClient(List<HostName> clusterControllers, String clusterName) {
         return new ClusterControllerClient() {
-            @Override public boolean setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex,
-                                               ClusterControllerNodeState wantedState) throws HostStateChangeDeniedException {
+            @Override public boolean trySetNodeState(OrchestratorContext context, HostName host, int storageNodeIndex, ClusterControllerNodeState wantedState) {
                 nodes.put(clusterName + "/" + storageNodeIndex, wantedState);
                 return true;
             }
-            @Override public void setApplicationState(OrchestratorContext context, ApplicationInstanceId applicationId,
-                                                      ClusterControllerNodeState wantedState) throws ApplicationStateChangeDeniedException {
+            @Override public void setNodeState(OrchestratorContext context, HostName host, int storageNodeIndex, ClusterControllerNodeState wantedState) {
+                trySetNodeState(context, host, storageNodeIndex, wantedState);
+            }
+            @Override public void setApplicationState(OrchestratorContext context, ApplicationInstanceId applicationId, ClusterControllerNodeState wantedState) {
                 nodes.replaceAll((key, state) -> key.startsWith(clusterName + "/") ? wantedState : state);
             }
         };
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImplTest.java
index 7746eff457e..2570a8035bb 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImplTest.java
@@ -66,7 +66,7 @@ public class ClusterControllerClientImplTest {
                         return "{ \"wasModified\": true }";
                     },
                     200);
-        assertTrue(client.setNodeState(context, host, 2, DOWN));
+        client.setNodeState(context, host, 2, DOWN);
 
         clock.advance(Duration.ofSeconds(9));
         wire.expect((url, body) -> {
@@ -93,7 +93,7 @@ public class ClusterControllerClientImplTest {
                         return "{ \"wasModified\": false, \"reason\": \"no reason\" }";
                     },
                     200);
-        assertFalse(client.setNodeState(OrchestratorContext.createContextForBatchProbe(clock), host, 2, MAINTENANCE));
+        assertFalse(client.trySetNodeState(OrchestratorContext.createContextForBatchProbe(clock), host, 2, MAINTENANCE));
     }
 
     @Test