Do not wait for version ack for failed set-node-state

5 files changed, 82 insertions, 6 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/Request.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/Request.java
index 5ac15e75127..4ef62ad3fdf 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/Request.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/Request.java
@@ -17,8 +17,8 @@ public abstract class Request<Result> extends RemoteClusterControllerTask {
     // TODO a lot of this logic could be replaced with a CompleteableFuture
 
     private Exception failure = null;
-    private boolean resultSet = false;
-    private Result result = null;
+    protected boolean resultSet = false;
+    protected Result result = null;
     private final MasterState masterState;
 
 
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java
index ada068808f7..849d8cc6e7b 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java
@@ -32,26 +32,31 @@ public class SetNodeStateRequest extends Request<SetResponse> {
     private final Map<String, UnitState> newStates;
     private final SetUnitStateRequest.Condition condition;
     private final SetUnitStateRequest.ResponseWait responseWait;
-
+    private final WantedStateSetter wantedState;
 
     public SetNodeStateRequest(Id.Node id, SetUnitStateRequest setUnitStateRequest) {
+        this(id, setUnitStateRequest, SetNodeStateRequest::setWantedState);
+    }
+
+    /** Public for tests. */
+    public SetNodeStateRequest(Id.Node id, SetUnitStateRequest setUnitStateRequest, WantedStateSetter wantedState) {
         super(MasterState.MUST_BE_MASTER);
         this.id = id;
         this.newStates = setUnitStateRequest.getNewState();
         this.condition = setUnitStateRequest.getCondition();
         this.responseWait = setUnitStateRequest.getResponseWait();
+        this.wantedState = wantedState;
     }
 
     @Override
     public SetResponse calculateResult(RemoteClusterControllerTask.Context context) throws StateRestApiException {
-        SetResponse setResponse = setWantedState(
+        return wantedState.set(
                 context.cluster,
                 condition,
                 newStates,
                 id.getNode(),
                 context.nodeStateOrHostInfoChangeHandler,
                 context.currentConsolidatedState);
-        return setResponse;
     }
 
     static NodeState getRequestedNodeState(Map<String, UnitState> newStates, Node n) throws StateRestApiException {
@@ -73,6 +78,12 @@ public class SetNodeStateRequest extends Request<SetResponse> {
         return (responseWait == SetUnitStateRequest.ResponseWait.WAIT_UNTIL_CLUSTER_ACKED);
     }
 
+    @Override
+    public boolean isFailed() {
+        // Failure to set a node state is propagated as a 200 with wasModified false.
+        return super.isFailed() || (resultSet && !result.getWasModified());
+    }
+
     static SetResponse setWantedState(
             ContentCluster cluster,
             SetUnitStateRequest.Condition condition,
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java
index 3f9e2b48eb5..1246ba62313 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java
@@ -2,7 +2,8 @@
 package com.yahoo.vespa.clustercontroller.core.restapiv2.requests;
 
 import com.yahoo.vdslib.distribution.ConfiguredNode;
-import com.yahoo.vdslib.state.*;
+import com.yahoo.vdslib.state.Node;
+import com.yahoo.vdslib.state.NodeType;
 import com.yahoo.vespa.clustercontroller.core.RemoteClusterControllerTask;
 import com.yahoo.vespa.clustercontroller.core.restapiv2.Id;
 import com.yahoo.vespa.clustercontroller.core.restapiv2.Request;
@@ -77,4 +78,10 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> {
         // 'true' here means the current state now equals the request's wanted state.
         return new SetResponse("ok", true);
     }
+
+    @Override
+    public boolean isFailed() {
+        // Failure to set a node state is propagated as a 200 with wasModified false.
+        return super.isFailed() || (resultSet && !result.getWasModified());
+    }
 }
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java
new file mode 100644
index 00000000000..6fa7d536c67
--- /dev/null
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java
@@ -0,0 +1,26 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.clustercontroller.core.restapiv2.requests;
+
+import com.yahoo.vdslib.state.ClusterState;
+import com.yahoo.vdslib.state.Node;
+import com.yahoo.vespa.clustercontroller.core.ContentCluster;
+import com.yahoo.vespa.clustercontroller.core.listeners.NodeStateOrHostInfoChangeHandler;
+import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.StateRestApiException;
+import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest;
+import com.yahoo.vespa.clustercontroller.utils.staterestapi.response.SetResponse;
+import com.yahoo.vespa.clustercontroller.utils.staterestapi.response.UnitState;
+
+import java.util.Map;
+
+/**
+ * @author hakon
+ */
+@FunctionalInterface
+public interface WantedStateSetter {
+    SetResponse set(ContentCluster cluster,
+                    SetUnitStateRequest.Condition condition,
+                    Map<String, UnitState> newStates,
+                    Node node,
+                    NodeStateOrHostInfoChangeHandler stateListener,
+                    ClusterState currentClusterState) throws StateRestApiException;
+}
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
index 4fb244666a4..3f977273054 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
@@ -2,12 +2,15 @@
 package com.yahoo.vespa.clustercontroller.core.restapiv2;
 
 import com.yahoo.vdslib.state.NodeType;
+import com.yahoo.vespa.clustercontroller.core.MasterInterface;
 import com.yahoo.vespa.clustercontroller.core.RemoteClusterControllerTask;
 import com.yahoo.vespa.clustercontroller.core.restapiv2.requests.SetNodeStateRequest;
+import com.yahoo.vespa.clustercontroller.core.restapiv2.requests.WantedStateSetter;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.DeadlineExceededException;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.InvalidContentException;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.MissingUnitException;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.OperationNotSupportedForUnitException;
+import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.StateRestApiException;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.UnknownMasterException;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.response.SetResponse;
@@ -26,6 +29,9 @@ import static org.hamcrest.core.StringContains.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class SetNodeStateTest extends StateRestApiTest {
 
@@ -426,4 +432,30 @@ public class SetNodeStateTest extends StateRestApiTest {
         request.getResult();
     }
 
+    @Test
+    public void no_fail_if_modified() throws StateRestApiException {
+        assertFalse(isFailed(true));
+    }
+
+    @Test
+    public void fail_if_not_modified() throws StateRestApiException {
+        assertTrue(isFailed(false));
+    }
+
+    private boolean isFailed(boolean wasModified) throws StateRestApiException {
+        WantedStateSetter wantedStateSetter = mock(WantedStateSetter.class);
+        SetNodeStateRequest request = new SetNodeStateRequest(
+                createDummyId(),
+                new SetUnitStateRequestImpl("music/storage/1").setNewState("user", "maintenance", "whatever reason."),
+                wantedStateSetter);
+        SetResponse response = new SetResponse("some reason", wasModified);
+        when(wantedStateSetter.set(any(), any(), any(), any(), any(), any())).thenReturn(response);
+
+        RemoteClusterControllerTask.Context context = mock(RemoteClusterControllerTask.Context.class);
+        MasterInterface masterInterface = mock(MasterInterface.class);
+        context.masterInfo = masterInterface;
+        when(masterInterface.isMaster()).thenReturn(true);
+        request.doRemoteFleetControllerTask(context);
+        return request.isFailed();
+    }
 }