Add support for version ACK-dependent tasks in cluster controller

Used to enable synchronous operation for set-node-state calls, which
ensure that side-effects of the call are visible when the response
returns.

If controller leadership is lost before state is published, tasks will
be failed back to the client.

6 files changed, 138 insertions, 40 deletions

diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/errors/UnknownMasterException.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/errors/UnknownMasterException.java
index 7108a941277..eade2e807c9 100644
--- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/errors/UnknownMasterException.java
+++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/errors/UnknownMasterException.java
@@ -3,6 +3,10 @@ package com.yahoo.vespa.clustercontroller.utils.staterestapi.errors;
 
 public class UnknownMasterException extends NotMasterException {
 
+    public UnknownMasterException(String message) {
+        super(message);
+    }
+
     public UnknownMasterException() {
         super("No known master cluster controller currently exists.");
     }
diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java
index db2b33c68e8..5a9b85e734b 100644
--- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java
+++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java
@@ -16,7 +16,7 @@ public interface SetUnitStateRequest extends UnitRequest {
 
         public final int value;
 
-        private Condition(int value) {
+        Condition(int value) {
             this.value = value;
         }
 
@@ -24,9 +24,34 @@ public interface SetUnitStateRequest extends UnitRequest {
             try {
                 return Condition.valueOf(value.toUpperCase());
             } catch (IllegalArgumentException e) {
-                throw new InvalidContentException("Invalid value for my enum Condition: " + value);
+                throw new InvalidContentException(String.format("Invalid value for condition: '%s', expected one of 'force', 'safe'", value));
             }
         }
     }
     Condition getCondition();
+
+    enum ResponseWait {
+        WAIT_UNTIL_CLUSTER_ACKED("wait-until-cluster-acked"), // Wait for state change to be ACKed by cluster
+        NO_WAIT("no-wait"); // Return without waiting for state change to be ACKed by cluster
+
+        private final String name;
+
+        ResponseWait(String name) { this.name = name; }
+
+        public String getName() { return this.name; }
+
+        @Override
+        public String toString() { return name; }
+
+        public static ResponseWait fromString(String value) throws InvalidContentException {
+            if (value.equalsIgnoreCase(WAIT_UNTIL_CLUSTER_ACKED.name)) {
+                return WAIT_UNTIL_CLUSTER_ACKED;
+            } else if (value.equalsIgnoreCase(NO_WAIT.name)) {
+                return NO_WAIT;
+            }
+            throw new InvalidContentException(String.format("Invalid value for response-wait: '%s', expected one of '%s', '%s'",
+                    value, WAIT_UNTIL_CLUSTER_ACKED.name, NO_WAIT.name));
+        }
+    }
+    ResponseWait getResponseWait();
 }
diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java
index f5ab406179b..04dcb582389 100644
--- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java
+++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java
@@ -35,9 +35,12 @@ public class JsonReader {
     static class SetRequestData {
         final Map<String, UnitState> stateMap;
         final SetUnitStateRequest.Condition condition;
-        public SetRequestData(Map<String, UnitState> stateMap, SetUnitStateRequest.Condition condition) {
+        final SetUnitStateRequest.ResponseWait responseWait;
+        public SetRequestData(Map<String, UnitState> stateMap, SetUnitStateRequest.Condition condition,
+                              SetUnitStateRequest.ResponseWait responseWait) {
             this.stateMap = stateMap;
             this.condition = condition;
+            this.responseWait = responseWait;
         }
     }
 
@@ -47,11 +50,15 @@ public class JsonReader {
         final SetUnitStateRequest.Condition condition;
 
         if (json.has("condition")) {
-            condition = SetUnitStateRequest.Condition.valueOf(json.getString("condition"));
+            condition = SetUnitStateRequest.Condition.fromString(json.getString("condition"));
         } else {
             condition = SetUnitStateRequest.Condition.FORCE;
         }
 
+        final SetUnitStateRequest.ResponseWait responseWait = json.has("response-wait")
+                ? SetUnitStateRequest.ResponseWait.fromString(json.getString("response-wait"))
+                : SetUnitStateRequest.ResponseWait.WAIT_UNTIL_CLUSTER_ACKED;
+
         Map<String, UnitState> stateMap = new HashMap<>();
         if (!json.has("state")) {
             throw new InvalidContentException("Set state requests must contain a state object");
@@ -90,7 +97,7 @@ public class JsonReader {
             }
             stateMap.put(type, new UnitStateImpl(code, reason));
         }
-        return new SetRequestData(stateMap, condition);
+        return new SetRequestData(stateMap, condition, responseWait);
     }
 
 }
diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java
index 9a8fc084d60..fcdf3214c45 100644
--- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java
+++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java
@@ -58,18 +58,20 @@ public class RestApiHandler implements HttpRequestHandler {
                 });
                 return new JsonHttpResult().setJson(jsonWriter.createJson(data));
             } else {
-                final JsonReader.SetRequestData setRequestdata = jsonReader.getStateRequestData(request);
+                final JsonReader.SetRequestData setRequestData = jsonReader.getStateRequestData(request);
                 SetResponse setResponse = restApi.setUnitState(new SetUnitStateRequest() {
                     @Override
                     public Map<String, UnitState> getNewState() {
-                        return setRequestdata.stateMap;
+                        return setRequestData.stateMap;
                     }
                     @Override
                     public String[] getUnitPath() {
                         return unitPath;
                     }
                     @Override
-                    public Condition getCondition() { return setRequestdata.condition; }
+                    public Condition getCondition() { return setRequestData.condition; }
+                    @Override
+                    public ResponseWait getResponseWait() { return setRequestData.responseWait; }
                 });
                 return new JsonHttpResult().setJson(jsonWriter.createJson(setResponse));
             }
diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/DummyStateApi.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/DummyStateApi.java
index c1d801d4759..a54653ddd13 100644
--- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/DummyStateApi.java
+++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/DummyStateApi.java
@@ -179,7 +179,8 @@ public class DummyStateApi implements StateRestAPI {
         }
         n.state = newState.get("current").getId();
         n.reason = newState.get("current").getReason();
-        return new SetResponse("DummyStateAPI", true);
+        String reason = String.format("DummyStateAPI %s call", request.getResponseWait().getName());
+        return new SetResponse(reason, true);
     }
 
     private void checkForInducedException() throws StateRestApiException {
diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/StateRestAPITest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/StateRestAPITest.java
index d8dabeb23d8..8328ecc491f 100644
--- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/StateRestAPITest.java
+++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/StateRestAPITest.java
@@ -5,14 +5,18 @@ import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperatio
 import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncUtils;
 import com.yahoo.vespa.clustercontroller.utils.communication.http.HttpRequest;
 import com.yahoo.vespa.clustercontroller.utils.communication.http.HttpResult;
-import com.yahoo.vespa.clustercontroller.utils.communication.http.JsonHttpResult;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.errors.*;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.server.RestApiHandler;
 import com.yahoo.vespa.clustercontroller.utils.test.TestTransport;
-import junit.framework.TestCase;
 import org.codehaus.jettison.json.JSONObject;
+import org.junit.Test;
 
-public class StateRestAPITest extends TestCase {
+import java.util.Optional;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class StateRestAPITest {
 
     private static void populateDummyBackend(DummyBackend backend) {
         backend.addCluster(new DummyBackend.Cluster("foo")
@@ -68,6 +72,7 @@ public class StateRestAPITest extends TestCase {
         return (JSONObject) result.getContent();
     }
 
+    @Test
     public void testTopLevelList() throws Exception {
         setupDummyStateApi();
         HttpResult result = execute(new HttpRequest().setPath("/cluster/v2"));
@@ -80,6 +85,7 @@ public class StateRestAPITest extends TestCase {
         assertEquals(expected, ((JSONObject) result.getContent()).toString(2));
     }
 
+    @Test
     public void testClusterState() throws Exception {
         setupDummyStateApi();
         HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo"));
@@ -92,6 +98,7 @@ public class StateRestAPITest extends TestCase {
         assertEquals(expected, ((JSONObject) result.getContent()).toString(2));
     }
 
+    @Test
     public void testNodeState() throws Exception {
         setupDummyStateApi();
         HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo/3"));
@@ -108,6 +115,7 @@ public class StateRestAPITest extends TestCase {
         assertEquals(expected, ((JSONObject) result.getContent()).toString(2));
     }
 
+    @Test
     public void testRecursiveMode() throws Exception {
         setupDummyStateApi();
         {
@@ -198,6 +206,25 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    private String retireAndExpectHttp200Response(Optional<String> responseWait) throws Exception {
+        JSONObject json = new JSONObject()
+                .put("state", new JSONObject()
+                        .put("current", new JSONObject()
+                                .put("state", "retired")
+                                .put("reason", "No reason")))
+                .put("condition", "FORCE");
+        if (responseWait.isPresent()) {
+            json.put("response-wait", responseWait.get());
+        }
+        HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo/3").setPostContent(json));
+        assertEquals(result.toString(true), 200, result.getHttpReturnCode());
+        assertEquals(result.toString(true), "application/json", result.getHeader("Content-Type"));
+        StringBuilder print = new StringBuilder();
+        result.printContent(print);
+        return print.toString();
+    }
+
+    @Test
     public void testSetNodeState() throws Exception {
         setupDummyStateApi();
         {
@@ -240,26 +267,41 @@ public class StateRestAPITest extends TestCase {
                     + "}";
             assertEquals(json.toString(2), expected, json.toString(2));
         }
+    }
+
+    @Test
+    public void set_node_state_response_wait_type_is_propagated_to_handler() throws Exception {
+        setupDummyStateApi();
         {
-            JSONObject json = new JSONObject()
-                    .put("state", new JSONObject()
-                            .put("current", new JSONObject()
-                                    .put("state", "retired")
-                                    .put("reason", "No reason")))
-                    .put("condition", "FORCE");
-            HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo/3").setPostContent(json));
-            assertEquals(result.toString(true), 200, result.getHttpReturnCode());
-            assertEquals(result.toString(true), "application/json", result.getHeader("Content-Type"));
-            StringBuilder print = new StringBuilder();
-            result.printContent(print);
-            assertEquals(print.toString(),
+            String result = retireAndExpectHttp200Response(Optional.of("wait-until-cluster-acked"));
+            assertEquals(result,
                     "JSON: {\n" +
                             "  \"wasModified\": true,\n" +
-                            "  \"reason\": \"DummyStateAPI\"\n" +
+                            "  \"reason\": \"DummyStateAPI wait-until-cluster-acked call\"\n" +
+                            "}");
+        }
+        {
+            String result = retireAndExpectHttp200Response(Optional.of("no-wait"));
+            assertEquals(result,
+                    "JSON: {\n" +
+                            "  \"wasModified\": true,\n" +
+                            "  \"reason\": \"DummyStateAPI no-wait call\"\n" +
                             "}");
         }
     }
 
+    @Test
+    public void set_node_state_response_wait_type_is_cluster_acked_by_default() throws Exception {
+        setupDummyStateApi();
+        String result = retireAndExpectHttp200Response(Optional.empty());
+        assertEquals(result,
+                "JSON: {\n" +
+                        "  \"wasModified\": true,\n" +
+                        "  \"reason\": \"DummyStateAPI wait-until-cluster-acked call\"\n" +
+                        "}");
+    }
+
+    @Test
     public void testMissingUnits() throws Exception {
         setupDummyStateApi();
         {
@@ -278,6 +320,7 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    @Test
     public void testUnknownMaster() throws Exception {
         setupDummyStateApi();
         stateApi.induceException(new UnknownMasterException());
@@ -290,6 +333,7 @@ public class StateRestAPITest extends TestCase {
         assertTrue(result.getHeader("Location") == null);
     }
 
+    @Test
     public void testOtherMaster() throws Exception {
         setupDummyStateApi();
         {
@@ -314,6 +358,7 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    @Test
     public void testRuntimeException() throws Exception {
         setupDummyStateApi();
         stateApi.induceException(new RuntimeException("Moahaha"));
@@ -325,6 +370,7 @@ public class StateRestAPITest extends TestCase {
         assertEquals(expected, result.getContent().toString());
     }
 
+    @Test
     public void testClientFailures() throws Exception {
         setupDummyStateApi();
         {
@@ -358,6 +404,7 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    @Test
     public void testInternalFailure() throws Exception {
         setupDummyStateApi();
         {
@@ -371,6 +418,7 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    @Test
     public void testInvalidRecursiveValues() throws Exception {
         setupDummyStateApi();
         {
@@ -391,6 +439,7 @@ public class StateRestAPITest extends TestCase {
         }
     }
 
+    @Test
     public void testInvalidJsonInSetStateRequest() throws Exception {
         setupDummyStateApi();
         {
@@ -437,24 +486,34 @@ public class StateRestAPITest extends TestCase {
             assertTrue(result.toString(true), result.getContent().toString().contains("value of state->current->reason is not a string"));
         }
         {
-            JSONObject json = new JSONObject()
-                    .put("state", new JSONObject()
-                            .put("current", new JSONObject()
-                                    .put("state", "retired")
-                                    .put("reason", "No reason")))
-                    .put("condition", "Non existing condition");
-            HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo/3").setPostContent(json));
-            assertEquals(result.toString(true), 500, result.getHttpReturnCode());
-            assertEquals(result.toString(true), "application/json", result.getHeader("Content-Type"));
-            StringBuilder print = new StringBuilder();
-            result.printContent(print);
-            assertEquals(print.toString(),
-                    "JSON: {\"message\": \"java.lang.IllegalArgumentException: No enum constant " +
-                            "com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest." +
-                            "Condition.Non existing condition\"}");
+            String result = retireAndExpectHttp400Response("Non existing condition", "no-wait");
+            assertEquals(result,
+                    "JSON: {\"message\": \"Invalid value for condition: 'Non existing condition', expected one of 'force', 'safe'\"}");
+        }
+        {
+            String result = retireAndExpectHttp400Response("FORCE", "banana");
+            assertEquals(result,
+                    "JSON: {\"message\": \"Invalid value for response-wait: 'banana', expected one of 'wait-until-cluster-acked', 'no-wait'\"}");
         }
     }
 
+    private String retireAndExpectHttp400Response(String condition, String responseWait) throws Exception {
+        JSONObject json = new JSONObject()
+                .put("state", new JSONObject()
+                        .put("current", new JSONObject()
+                                .put("state", "retired")
+                                .put("reason", "No reason")))
+                .put("condition", condition)
+                .put("response-wait", responseWait);
+        HttpResult result = execute(new HttpRequest().setPath("/cluster/v2/foo/3").setPostContent(json));
+        assertEquals(result.toString(true), 400, result.getHttpReturnCode());
+        assertEquals(result.toString(true), "application/json", result.getHeader("Content-Type"));
+        StringBuilder print = new StringBuilder();
+        result.printContent(print);
+        return print.toString();
+    }
+
+    @Test
     public void testInvalidPathPrefix() throws Exception {
         DummyBackend backend = new DummyBackend();
         stateApi = new DummyStateApi(backend);