From c174b1652c7251516971f04f8be764810b31b886 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Mon, 29 Oct 2018 09:57:49 +0100 Subject: Revert "Enforce CC timeouts in Orchestrator 2" --- .../restapiv2/requests/SetNodeStateRequest.java | 27 ++-- .../requests/SetNodeStatesForClusterRequest.java | 5 +- .../core/restapiv2/requests/WantedStateSetter.java | 3 +- .../core/restapiv2/SetNodeStateTest.java | 9 +- .../requests/SetNodeStateRequestTest.java | 41 ++---- .../staterestapi/requests/SetUnitStateRequest.java | 3 - .../utils/staterestapi/server/JsonReader.java | 10 +- .../utils/staterestapi/server/RestApiHandler.java | 2 - .../server/application/OrchestratorMock.java | 5 + jaxrs_client_utils/pom.xml | 6 - .../vespa/jaxrs/client/JaxRsClientFactory.java | 44 ------ .../yahoo/vespa/jaxrs/client/JaxRsStrategy.java | 4 - .../yahoo/vespa/jaxrs/client/JaxRsTimeouts.java | 22 --- .../jaxrs/client/JerseyJaxRsClientFactory.java | 28 ++-- .../vespa/jaxrs/client/LegacyJaxRsTimeouts.java | 26 ---- .../vespa/jaxrs/client/RetryingJaxRsStrategy.java | 14 +- .../yahoo/vespa/jaxrs/client/HttpPatchTest.java | 3 +- .../jaxrs/client/RetryingJaxRsStrategyTest.java | 48 ++++--- .../provision/testutils/OrchestratorMock.java | 6 + .../orchestrator/restapi/HostSuspensionApi.java | 8 +- orchestrator/pom.xml | 6 - .../com/yahoo/vespa/orchestrator/Orchestrator.java | 9 ++ .../vespa/orchestrator/OrchestratorContext.java | 70 +++------- .../yahoo/vespa/orchestrator/OrchestratorImpl.java | 37 +++-- .../controller/ClusterControllerClientImpl.java | 41 ++++-- .../ClusterControllerClientTimeouts.java | 125 ----------------- .../RetryingClusterControllerClientFactory.java | 34 ++--- ...ngleInstanceClusterControllerClientFactory.java | 54 ++++++++ .../yahoo/vespa/orchestrator/model/NodeGroup.java | 2 +- .../orchestrator/status/InMemoryStatusService.java | 7 +- .../vespa/orchestrator/status/StatusService.java | 8 +- .../status/ZookeeperStatusService.java | 29 ++-- .../vespa/orchestrator/OrchestratorImplTest.java | 16 +-- .../controller/ClusterControllerClientTest.java | 8 +- .../ClusterControllerClientTimeoutsTest.java | 151 --------------------- ...RetryingClusterControllerClientFactoryTest.java | 54 -------- ...InstanceClusterControllerClientFactoryTest.java | 116 ++++++++++++++++ .../orchestrator/resources/HostResourceTest.java | 10 +- .../status/ZookeeperStatusServiceTest.java | 21 ++- .../src/main/java/com/yahoo/time/TimeBudget.java | 12 -- 40 files changed, 391 insertions(+), 733 deletions(-) delete mode 100644 jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java delete mode 100644 jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java delete mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java create mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java delete mode 100644 orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java delete mode 100644 orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java create mode 100644 orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java 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 eecdcc75228..4d6738940a8 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 @@ -37,7 +37,6 @@ public class SetNodeStateRequest extends Request { private final SetUnitStateRequest.ResponseWait responseWait; private final WantedStateSetter wantedState; private final TimeBudget timeBudget; - private final boolean probe; public SetNodeStateRequest(Id.Node id, SetUnitStateRequest setUnitStateRequest) { this(id, setUnitStateRequest, SetNodeStateRequest::setWantedState); @@ -52,7 +51,6 @@ public class SetNodeStateRequest extends Request { this.responseWait = setUnitStateRequest.getResponseWait(); this.wantedState = wantedState; this.timeBudget = setUnitStateRequest.timeBudget(); - this.probe = setUnitStateRequest.isProbe(); } @Override @@ -63,8 +61,7 @@ public class SetNodeStateRequest extends Request { newStates, id.getNode(), context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState, - probe); + context.currentConsolidatedState); } static NodeState getRequestedNodeState(Map newStates, Node n) throws StateRestApiException { @@ -103,8 +100,7 @@ public class SetNodeStateRequest extends Request { Map newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState, - boolean probe) throws StateRestApiException { + ClusterState currentClusterState) throws StateRestApiException { if ( ! cluster.hasConfiguredNode(node.getIndex())) { throw new MissingIdException(cluster.getName(), node); } @@ -130,8 +126,7 @@ public class SetNodeStateRequest extends Request { condition, nodeInfo, cluster, - stateListener, - probe); + stateListener); // If the state was successfully set, just return an "ok" message back. String reason = success ? "ok" : result.getReason(); @@ -148,10 +143,9 @@ public class SetNodeStateRequest extends Request { SetUnitStateRequest.Condition condition, NodeInfo nodeInfo, ContentCluster cluster, - NodeStateOrHostInfoChangeHandler stateListener, - boolean probe) { + NodeStateOrHostInfoChangeHandler stateListener) { if (result.settingWantedStateIsAllowed()) { - setNewWantedState(nodeInfo, newWantedState, stateListener, probe); + setNewWantedState(nodeInfo, newWantedState, stateListener); } // True if the wanted state was or has just been set to newWantedState @@ -162,7 +156,7 @@ public class SetNodeStateRequest extends Request { // of the distributor. E.g. setting the storage node to maintenance may cause // feeding issues unless distributor is also set down. - setDistributorWantedState(cluster, nodeInfo.getNodeIndex(), newWantedState, stateListener, probe); + setDistributorWantedState(cluster, nodeInfo.getNodeIndex(), newWantedState, stateListener); } return success; @@ -175,8 +169,7 @@ public class SetNodeStateRequest extends Request { private static void setDistributorWantedState(ContentCluster cluster, int index, NodeState newStorageWantedState, - NodeStateOrHostInfoChangeHandler stateListener, - boolean probe) { + NodeStateOrHostInfoChangeHandler stateListener) { Node distributorNode = new Node(NodeType.DISTRIBUTOR, index); NodeInfo nodeInfo = cluster.getNodeInfo(distributorNode); if (nodeInfo == null) { @@ -207,15 +200,13 @@ public class SetNodeStateRequest extends Request { if (newWantedState.getState() != currentWantedState.getState() || !Objects.equals(newWantedState.getDescription(), currentWantedState.getDescription())) { - setNewWantedState(nodeInfo, newWantedState, stateListener, probe); + setNewWantedState(nodeInfo, newWantedState, stateListener); } } private static void setNewWantedState(NodeInfo nodeInfo, NodeState newWantedState, - NodeStateOrHostInfoChangeHandler stateListener, - boolean probe) { - if (probe) return; + NodeStateOrHostInfoChangeHandler stateListener) { nodeInfo.setWantedState(newWantedState); stateListener.handleNewWantedNodeState(nodeInfo, newWantedState); } 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 d7820722887..b4d189bcd55 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 @@ -27,7 +27,6 @@ public class SetNodeStatesForClusterRequest extends Request { private final Map newStates; private final SetUnitStateRequest.Condition condition; private final TimeBudget timeBudget; - private final boolean probe; public SetNodeStatesForClusterRequest(Id.Cluster cluster, SetUnitStateRequest request) { @@ -36,7 +35,6 @@ public class SetNodeStatesForClusterRequest extends Request { this.newStates = request.getNewState(); this.condition = request.getCondition(); this.timeBudget = request.timeBudget(); - this.probe = request.isProbe(); } @Override @@ -71,8 +69,7 @@ public class SetNodeStatesForClusterRequest extends Request { newStates, node, context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState, - probe); + context.currentConsolidatedState); if (!setResponse.getWasModified()) { throw new InternalFailure("We have not yet implemented the meaning of " + 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 index c3090a5e832..6fa7d536c67 100644 --- 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 @@ -22,6 +22,5 @@ public interface WantedStateSetter { Map newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState, - boolean probe) throws StateRestApiException; + 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 6cf4b7989e7..f3a4be5ac2f 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 @@ -33,7 +33,6 @@ 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.Matchers.anyBoolean; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -47,7 +46,6 @@ public class SetNodeStateTest extends StateRestApiTest { private Condition condition = Condition.FORCE; private ResponseWait responseWait = ResponseWait.WAIT_UNTIL_CLUSTER_ACKED; private TimeBudget timeBudget = TimeBudget.fromNow(Clock.systemUTC(), Duration.ofSeconds(10)); - private boolean probe = false; public SetUnitStateRequestImpl(String req) { super(req, 0); @@ -100,11 +98,6 @@ public class SetNodeStateTest extends StateRestApiTest { public TimeBudget timeBudget() { return timeBudget; } - - @Override - public boolean isProbe() { - return probe; - } } private void verifyStateSet(String state, String reason) throws Exception { @@ -465,7 +458,7 @@ public class SetNodeStateTest extends StateRestApiTest { 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(), anyBoolean())).thenReturn(response); + when(wantedStateSetter.set(any(), any(), any(), any(), any(), any())).thenReturn(response); RemoteClusterControllerTask.Context context = mock(RemoteClusterControllerTask.Context.class); MasterInterface masterInterface = mock(MasterInterface.class); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java index 7161fb1be79..9239b8774b0 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java @@ -23,23 +23,20 @@ import java.util.Map; import java.util.Optional; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class SetNodeStateRequestTest { - private static final String REASON = "operator"; - private ContentCluster cluster = mock(ContentCluster.class); - private SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; - private Map newStates = new HashMap<>(); - private UnitState unitState = mock(UnitState.class); + public static final String REASON = "operator"; + ContentCluster cluster = mock(ContentCluster.class); + SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; + Map newStates = new HashMap<>(); + UnitState unitState = mock(UnitState.class); private final int NODE_INDEX = 2; - private Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); - private NodeStateOrHostInfoChangeHandler stateListener = mock(NodeStateOrHostInfoChangeHandler.class); - private ClusterState currentClusterState = mock(ClusterState.class); - private boolean probe = false; + Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); + NodeStateOrHostInfoChangeHandler stateListener = mock(NodeStateOrHostInfoChangeHandler.class); + ClusterState currentClusterState = mock(ClusterState.class); @Before public void setUp() { @@ -55,16 +52,6 @@ public class SetNodeStateRequestTest { Optional.of(State.MAINTENANCE), Optional.of(State.DOWN)); } - @Test - public void testProbingDoesntChangeState() throws StateRestApiException { - probe = true; - testSetStateRequest( - "maintenance", - State.UP, State.UP, - NodeStateChangeChecker.Result.allowSettingOfWantedState(), - Optional.empty(), Optional.empty()); - } - @Test public void testUpToDown() throws StateRestApiException { testSetStateRequest( @@ -137,9 +124,6 @@ public class SetNodeStateRequestTest { when(cluster.getNodeInfo(distributorNode)).thenReturn(distributorNodeInfo); NodeState distributorNodeState = new NodeState(distributorNode.getType(), distributorWantedState); - if (distributorWantedState != State.UP) { - distributorNodeState.setDescription(REASON); - } when(distributorNodeInfo.getUserWantedState()).thenReturn(distributorNodeState); setWantedState(); @@ -149,9 +133,6 @@ public class SetNodeStateRequestTest { new NodeState(NodeType.STORAGE, expectedNewStorageWantedState.get()); verify(storageNodeInfo).setWantedState(expectedNewStorageNodeState); verify(stateListener).handleNewWantedNodeState(storageNodeInfo, expectedNewStorageNodeState); - } else { - verify(storageNodeInfo, times(0)).setWantedState(any()); - verify(stateListener, times(0)).handleNewWantedNodeState(eq(storageNodeInfo), any()); } if (expectedNewDistributorWantedState.isPresent()) { @@ -159,9 +140,6 @@ public class SetNodeStateRequestTest { new NodeState(NodeType.DISTRIBUTOR, expectedNewDistributorWantedState.get()); verify(distributorNodeInfo).setWantedState(expectedNewDistributorNodeState); verify(stateListener).handleNewWantedNodeState(distributorNodeInfo, expectedNewDistributorNodeState); - } else { - verify(distributorNodeInfo, times(0)).setWantedState(any()); - verify(stateListener, times(0)).handleNewWantedNodeState(eq(distributorNodeInfo), any()); } } @@ -172,7 +150,6 @@ public class SetNodeStateRequestTest { newStates, storageNode, stateListener, - currentClusterState, - probe); + currentClusterState); } } \ No newline at end of file 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 27f18c3664b..a28ddb3539b 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 @@ -64,7 +64,4 @@ public interface SetUnitStateRequest extends UnitRequest { ResponseWait getResponseWait(); TimeBudget timeBudget(); - - /** A probe request is a non-committal request to see if an identical (but non-probe) request would have succeeded. */ - boolean isProbe(); } 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 dab6895cc9d..d871a8ed6bc 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 @@ -33,16 +33,13 @@ public class JsonReader { } static class SetRequestData { - final boolean probe; final Map stateMap; final SetUnitStateRequest.Condition condition; final SetUnitStateRequest.ResponseWait responseWait; - public SetRequestData(boolean probe, - Map stateMap, + public SetRequestData(Map stateMap, SetUnitStateRequest.Condition condition, SetUnitStateRequest.ResponseWait responseWait) { - this.probe = probe; this.stateMap = stateMap; this.condition = condition; this.responseWait = responseWait; @@ -52,9 +49,8 @@ public class JsonReader { public SetRequestData getStateRequestData(HttpRequest request) throws Exception { JSONObject json = new JSONObject(request.getPostContent().toString()); - final boolean probe = json.has("probe") && json.getBoolean("probe"); - final SetUnitStateRequest.Condition condition; + if (json.has("condition")) { condition = SetUnitStateRequest.Condition.fromString(json.getString("condition")); } else { @@ -104,6 +100,6 @@ public class JsonReader { stateMap.put(type, new UnitStateImpl(code, reason)); } - return new SetRequestData(probe, stateMap, condition, responseWait); + 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 46f5d964245..c38f7aec8c6 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 @@ -97,8 +97,6 @@ public class RestApiHandler implements HttpRequestHandler { public ResponseWait getResponseWait() { return setRequestData.responseWait; } @Override public TimeBudget timeBudget() { return TimeBudget.from(clock, start, timeout); } - @Override - public boolean isProbe() { return setRequestData.probe; } }); return new JsonHttpResult().setJson(jsonWriter.createJson(setResponse)); } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java index 73abd70a5ae..e61d3710fac 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java @@ -46,6 +46,11 @@ public class OrchestratorMock implements Orchestrator { suspendedHosts.add(hostName); } + @Override + public void suspendGroup(NodeGroup nodeGroup) { + nodeGroup.getHostNames().forEach(this::suspend); + } + @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) diff --git a/jaxrs_client_utils/pom.xml b/jaxrs_client_utils/pom.xml index 43fbc66a9e6..e3de5b8163d 100644 --- a/jaxrs_client_utils/pom.xml +++ b/jaxrs_client_utils/pom.xml @@ -16,12 +16,6 @@ container-plugin ${project.artifactId} - - com.yahoo.vespa - vespajlib - ${project.version} - provided - com.yahoo.vespa application-model diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java index 78076bdc04b..d004ac3af45 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java @@ -3,55 +3,11 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; -import java.net.URI; -import java.time.Duration; - /** * Interface for creating a JAX-RS client API instance for a single server endpoint. * * @author bakksjo */ public interface JaxRsClientFactory { - class Params { - private final Class apiClass; - private final URI uri; - - private Duration connectTimeout = Duration.ofSeconds(30); - private Duration readTimeout = Duration.ofSeconds(30); - - public Params(Class apiClass, URI uri) { - this.apiClass = apiClass; - this.uri = uri; - } - - public Class apiClass() { - return apiClass; - } - - public URI uri() { - return uri; - } - - public void setConnectTimeout(Duration timeout) { - this.connectTimeout = timeout; - } - - public Duration connectTimeout() { - return connectTimeout; - } - - public void setReadTimeout(Duration timeout) { - readTimeout = timeout; - } - - public Duration readTimeout() { - return readTimeout; - } - } - - default T createClient(Params params) { - throw new UnsupportedOperationException("Not implemented"); - } - T createClient(Class apiClass, HostName hostName, int port, String pathPrefix, String scheme); } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java index cd7d8684cbc..72af76fe54c 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java @@ -11,8 +11,4 @@ import java.util.function.Function; */ public interface JaxRsStrategy { R apply(final Function function) throws IOException; - - default R apply(final Function function, JaxRsTimeouts timeouts) throws IOException { - return apply(function); - } } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java deleted file mode 100644 index 6758d6f94d6..00000000000 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java +++ /dev/null @@ -1,22 +0,0 @@ -package com.yahoo.vespa.jaxrs.client; - -import java.time.Duration; - -/** - * @author hakonhall - */ -public interface JaxRsTimeouts { - /** - * The connect timeout, which must be at least 1ms. - * - * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. - */ - Duration getConnectTimeoutOrThrow(); - - /** - * The read timeout, which must be at least 1ms. - * - * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. - */ - Duration getReadTimeoutOrThrow(); -} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java index 8aa880fb0e4..9321f8e290d 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java @@ -23,20 +23,34 @@ import java.util.Collections; */ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { + private static final int DEFAULT_CONNECT_TIMEOUT_MS = 30000; + private static final int DEFAULT_READ_TIMEOUT_MS = 30000; + // Client is a heavy-weight object with a finalizer so we create only one and re-use it private final Client client; public JerseyJaxRsClientFactory() { - this(null, null, null); + this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS); } public JerseyJaxRsClientFactory(SSLContext sslContext, HostnameVerifier hostnameVerifier, String userAgent) { + this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS, sslContext, hostnameVerifier, userAgent); + } + + public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs) { + this(connectTimeoutMs, readTimeoutMs, null, null, null); + } + + public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs, SSLContext sslContext, + HostnameVerifier hostnameVerifier, String userAgent) { /* * Configure client with some workarounds for HTTP/JAX-RS/Jersey issues. See: * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/ClientProperties.html#SUPPRESS_HTTP_COMPLIANCE_VALIDATION * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/HttpUrlConnectorProvider.html#SET_METHOD_WORKAROUND */ ClientBuilder builder = ClientBuilder.newBuilder() + .property(ClientProperties.CONNECT_TIMEOUT, connectTimeoutMs) + .property(ClientProperties.READ_TIMEOUT, readTimeoutMs) .property(ClientProperties.SUPPRESS_HTTP_COMPLIANCE_VALIDATION, true) // Allow empty PUT. TODO: Fix API. .property(HttpUrlConnectorProvider.SET_METHOD_WORKAROUND, true) // Allow e.g. PATCH method. .property(ClientProperties.FOLLOW_REDIRECTS, true); @@ -52,17 +66,11 @@ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { this.client = builder.build(); } - @Override - public T createClient(Params params) { - WebTarget target = client.target(params.uri()); - target.property(ClientProperties.CONNECT_TIMEOUT, (int) params.connectTimeout().toMillis()); - target.property(ClientProperties.READ_TIMEOUT, (int) params.readTimeout().toMillis()); - return WebResourceFactory.newResource(params.apiClass(), target); - } - @Override public T createClient(Class apiClass, HostName hostName, int port, String pathPrefix, String scheme) { UriBuilder uriBuilder = UriBuilder.fromPath(pathPrefix).host(hostName.s()).port(port).scheme(scheme); - return createClient(new Params<>(apiClass, uriBuilder.build())); + WebTarget target = client.target(uriBuilder); + return WebResourceFactory.newResource(apiClass, target); } + } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java deleted file mode 100644 index 3f2139f6bf0..00000000000 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java +++ /dev/null @@ -1,26 +0,0 @@ -package com.yahoo.vespa.jaxrs.client; - -import java.time.Duration; - -/** - * Legacy defaults for timeouts. - * - * Clients should instead define their own JaxRsTimeouts tailored to their use-case. - * - * @author hakonhall - */ -// Immutable -public class LegacyJaxRsTimeouts implements JaxRsTimeouts { - private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(30); - private static final Duration READ_TIMEOUT = Duration.ofSeconds(30); - - @Override - public Duration getConnectTimeoutOrThrow() { - return CONNECT_TIMEOUT; - } - - @Override - public Duration getReadTimeoutOrThrow() { - return READ_TIMEOUT; - } -} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java index 4c97147d61e..65b302ef4ff 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java @@ -4,9 +4,7 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; import javax.ws.rs.ProcessingException; -import javax.ws.rs.core.UriBuilder; import java.io.IOException; -import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -67,21 +65,11 @@ public class RetryingJaxRsStrategy implements JaxRsStrategy { @Override public R apply(final Function function) throws IOException { - return apply(function, new LegacyJaxRsTimeouts()); - } - - - @Override - public R apply(final Function function, JaxRsTimeouts timeouts) throws IOException { ProcessingException sampleException = null; for (int i = 0; i < maxIterations; ++i) { for (final HostName hostName : hostNames) { - URI uri = UriBuilder.fromPath(pathPrefix).port(port).scheme(scheme).host(hostName.s()).build(); - JaxRsClientFactory.Params params = new JaxRsClientFactory.Params<>(apiClass, uri); - params.setConnectTimeout(timeouts.getConnectTimeoutOrThrow()); - params.setReadTimeout(timeouts.getReadTimeoutOrThrow()); - final T jaxRsClient = jaxRsClientFactory.createClient(params); + final T jaxRsClient = jaxRsClientFactory.createClient(apiClass, hostName, port, pathPrefix, scheme); try { return function.apply(jaxRsClient); } catch (ProcessingException e) { diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java index 8161602cdac..63e2b814c24 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java @@ -74,7 +74,8 @@ public class HttpPatchTest extends JerseyTest { final JaxRsStrategy client = factory.apiNoRetries(TestResourceApi.class, apiPath); final String responseBody; - responseBody = client.apply(api -> api.doPatch(REQUEST_BODY)); + responseBody = client.apply(api -> + api.doPatch(REQUEST_BODY)); assertThat(testResourceSingleton.invocation.get(60, TimeUnit.SECONDS), is(REQUEST_BODY)); assertThat(responseBody, is(REQUEST_BODY)); diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java index dbe886b7896..e31920febd6 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java @@ -5,10 +5,7 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.defaults.Defaults; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -import org.mockito.runners.MockitoJUnitRunner; import org.mockito.stubbing.OngoingStubbing; import javax.ws.rs.GET; @@ -18,26 +15,24 @@ import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.Set; -import java.util.stream.Collectors; -import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@RunWith(MockitoJUnitRunner.class) public class RetryingJaxRsStrategyTest { private static final String API_PATH = "/"; - @Captor - ArgumentCaptor> paramsCaptor; - @Path(API_PATH) private interface TestJaxRsApi { @GET @@ -58,7 +53,8 @@ public class RetryingJaxRsStrategyTest { @Before public void setup() { - when(jaxRsClientFactory.createClient(any())).thenReturn(mockApi); + when(jaxRsClientFactory.createClient(eq(TestJaxRsApi.class), any(HostName.class), anyInt(), anyString(), anyString())) + .thenReturn(mockApi); } @Test @@ -67,12 +63,11 @@ public class RetryingJaxRsStrategyTest { verify(mockApi, times(1)).doSomething(); - verify(jaxRsClientFactory, times(1)).createClient(paramsCaptor.capture()); - JaxRsClientFactory.Params params = paramsCaptor.getValue(); - assertEquals(REST_PORT, params.uri().getPort()); - assertEquals(API_PATH, params.uri().getPath()); - assertEquals("http", params.uri().getScheme()); - assertThat(SERVER_HOSTS, hasItem(new HostName(params.uri().getHost()))); + // Check that one of the supplied hosts is contacted. + final ArgumentCaptor hostNameCaptor = ArgumentCaptor.forClass(HostName.class); + verify(jaxRsClientFactory, times(1)) + .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); + assertThat(SERVER_HOSTS.contains(hostNameCaptor.getValue()), is(true)); } @Test @@ -104,10 +99,10 @@ public class RetryingJaxRsStrategyTest { @Test public void testRetryLoopsOverAvailableServers() throws Exception { when(mockApi.doSomething()) - .thenThrow(new ProcessingException("Fake socket timeout 1 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 2 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 3 induced by test")) - .thenThrow(new ProcessingException("Fake socket timeout 4 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 1 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 2 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 3 induced by test")) + .thenThrow(new ProcessingException("Fake timeout 4 induced by test")) .thenReturn("a response"); jaxRsStrategy.apply(TestJaxRsApi::doSomething); @@ -147,9 +142,12 @@ public class RetryingJaxRsStrategyTest { verifyAllServersContacted(jaxRsClientFactory); } - private void verifyAllServersContacted(final JaxRsClientFactory jaxRsClientFactory) { - verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())).createClient(paramsCaptor.capture()); - final Set> actualServerHostsContacted = new HashSet<>(paramsCaptor.getAllValues()); - assertEquals(actualServerHostsContacted.stream().map(x -> new HostName(x.uri().getHost())).collect(Collectors.toSet()), SERVER_HOSTS); + private static void verifyAllServersContacted( + final JaxRsClientFactory jaxRsClientFactory) { + final ArgumentCaptor hostNameCaptor = ArgumentCaptor.forClass(HostName.class); + verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())) + .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); + final Set actualServerHostsContacted = new HashSet<>(hostNameCaptor.getAllValues()); + assertThat(actualServerHostsContacted, equalTo(SERVER_HOSTS)); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java index 70750dd6672..c71b0783d69 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java @@ -5,6 +5,7 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.orchestrator.Host; import com.yahoo.vespa.orchestrator.Orchestrator; +import com.yahoo.vespa.orchestrator.model.NodeGroup; import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; @@ -44,6 +45,11 @@ public class OrchestratorMock implements Orchestrator { suspendedHosts.add(hostName); } + @Override + public void suspendGroup(NodeGroup nodeGroup) { + nodeGroup.getHostNames().forEach(this::suspend); + } + @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) diff --git a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java index 9535096af4f..603d6a1adac 100644 --- a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java +++ b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java @@ -22,9 +22,13 @@ public interface HostSuspensionApi { String PATH_PREFIX = "/v1/suspensions/hosts"; /** - * Ask for permission to temporarily suspend all services on a set of hosts (nodes). + * Ask for permission to temporarily suspend all services on a set of hosts. * - * See HostApi::suspend for semantics of suspending a node. + * See HostApi::suspend for semantics of suspending a host. + * + * On failure, it tries to resume ALL hosts. It needs to try to resume all hosts because any or all hosts + * may have been suspended in an earlier attempt. Ending with resumption of all hosts makes sure other + * batch-requests for suspension of hosts succeed. */ @PUT @Path("/{hostname}") diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml index 37d308111ae..ae05a1908c9 100644 --- a/orchestrator/pom.xml +++ b/orchestrator/pom.xml @@ -116,12 +116,6 @@ ${project.version} provided - - com.yahoo.vespa - testutil - ${project.version} - test - org.apache.curator curator-test diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java index df124f2f690..b2be4fe52ec 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java @@ -74,6 +74,15 @@ public interface Orchestrator { */ void acquirePermissionToRemove(HostName hostName) throws OrchestrationException; + /** + * Suspend normal operations for a group of nodes in the same application. + * + * @param nodeGroup The group of nodes in an application. + * @throws HostStateChangeDeniedException if the request cannot be meet due to policy constraints. + * @throws HostNameNotFoundException if any hostnames in the node group is not recognized + */ + void suspendGroup(NodeGroup nodeGroup) throws HostStateChangeDeniedException, HostNameNotFoundException; + /** * Suspend several hosts. On failure, all hosts are resumed before exiting the method with an exception. */ diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java index 25c6c780130..6577b4b96cc 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java @@ -1,74 +1,40 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator; +import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.time.TimeBudget; -import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts; import java.time.Clock; import java.time.Duration; -import java.time.Instant; -import java.util.Optional; /** - * Context for an operation (or suboperation) of the Orchestrator that needs to pass through to the backend, - * e.g. timeout management and probing. + * Context for the Orchestrator, e.g. timeout management. * - * @author hakonhall + * @author hakon */ public class OrchestratorContext { - private static final Duration DEFAULT_TIMEOUT_FOR_SINGLE_OP = Duration.ofSeconds(10); - private static final Duration DEFAULT_TIMEOUT_FOR_BATCH_OP = Duration.ofSeconds(60); + private static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(10); - private final Clock clock; - private final TimeBudget timeBudget; - private boolean probe; + private TimeBudget timeBudget; - public static OrchestratorContext createContextForMultiAppOp(Clock clock) { - return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_BATCH_OP), true); + public OrchestratorContext(Clock clock) { + this.timeBudget = TimeBudget.fromNow(clock, DEFAULT_TIMEOUT); } - public static OrchestratorContext createContextForSingleAppOp(Clock clock) { - return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_SINGLE_OP), true); + /** Get the original timeout in seconds. */ + public long getOriginalTimeoutInSeconds() { + return timeBudget.originalTimeout().get().getSeconds(); } - private OrchestratorContext(Clock clock, TimeBudget timeBudget, boolean probe) { - this.clock = clock; - this.timeBudget = timeBudget; - this.probe = probe; - } - - public Duration getTimeLeft() { - return timeBudget.timeLeftOrThrow().get(); - } - - public ClusterControllerClientTimeouts getClusterControllerTimeouts(String clusterName) { - return new ClusterControllerClientTimeouts(clusterName, timeBudget.timeLeftAsTimeBudget()); - } - - - /** Mark this operation as a non-committal probe. */ - public OrchestratorContext markAsProbe() { - this.probe = true; - return this; - } - - /** Whether the operation is a no-op probe to test whether it would have succeeded, if it had been committal. */ - public boolean isProbe() { - return probe; - } - - /** Create an OrchestratorContext to use within an application lock. */ - public OrchestratorContext createSubcontextForApplication() { - Instant now = clock.instant(); - Instant deadline = timeBudget.deadline().get(); - Instant maxDeadline = now.plus(DEFAULT_TIMEOUT_FOR_SINGLE_OP); - if (maxDeadline.compareTo(deadline) < 0) { - deadline = maxDeadline; + /** + * Get timeout for a suboperation that should take up {@code shareOfRemaining} of the + * remaining time, or throw an {@link UncheckedTimeoutException} if timed out. + */ + public float getSuboperationTimeoutInSeconds(float shareOfRemaining) { + if (!(0f <= shareOfRemaining && shareOfRemaining <= 1.0f)) { + throw new IllegalArgumentException("Share of remaining time must be between 0 and 1: " + shareOfRemaining); } - return new OrchestratorContext( - clock, - TimeBudget.from(clock, now, Optional.of(Duration.between(now, deadline))), - probe); + return shareOfRemaining * timeBudget.timeLeftOrThrow().get().toMillis() / 1000.0f; } } 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 3a2673d9dbc..ad8a35312e4 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java @@ -105,9 +105,7 @@ public class OrchestratorImpl implements Orchestrator { @Override public void setNodeStatus(HostName hostName, HostStatus status) throws OrchestrationException { ApplicationInstanceReference reference = getApplicationInstance(hostName).reference(); - OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); - try (MutableStatusRegistry statusRegistry = statusService - .lockApplicationInstance_forCurrentThreadOnly(reference, context.getTimeLeft())) { + try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly(reference)) { statusRegistry.setHostState(hostName, status); } } @@ -129,10 +127,10 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); - OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + OrchestratorContext context = new OrchestratorContext(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getTimeLeft())) { + context.getOriginalTimeoutInSeconds())) { final HostStatus currentHostState = statusRegistry.getHostStatus(hostName); if (HostStatus.NO_REMARKS == currentHostState) { @@ -150,7 +148,7 @@ public class OrchestratorImpl implements Orchestrator { public void suspend(HostName hostName) throws HostStateChangeDeniedException, HostNameNotFoundException { ApplicationInstance appInstance = getApplicationInstance(hostName); NodeGroup nodeGroup = new NodeGroup(appInstance, hostName); - suspendGroup(OrchestratorContext.createContextForSingleAppOp(clock), nodeGroup); + suspendGroup(nodeGroup); } @Override @@ -158,10 +156,10 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); NodeGroup nodeGroup = new NodeGroup(appInstance, hostName); - OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + OrchestratorContext context = new OrchestratorContext(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getTimeLeft())) { + context.getOriginalTimeoutInSeconds())) { ApplicationApi applicationApi = new ApplicationApiImpl( nodeGroup, statusRegistry, @@ -171,19 +169,16 @@ public class OrchestratorImpl implements Orchestrator { } } - /** - * Suspend normal operations for a group of nodes in the same application. - * - * @param nodeGroup The group of nodes in an application. - * @throws HostStateChangeDeniedException if the request cannot be meet due to policy constraints. - */ - void suspendGroup(OrchestratorContext context, NodeGroup nodeGroup) throws HostStateChangeDeniedException { + // Public for testing purposes + @Override + public void suspendGroup(NodeGroup nodeGroup) throws HostStateChangeDeniedException, HostNameNotFoundException { ApplicationInstanceReference applicationReference = nodeGroup.getApplicationReference(); + OrchestratorContext context = new OrchestratorContext(clock); try (MutableStatusRegistry hostStatusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( applicationReference, - context.getTimeLeft())) { + context.getOriginalTimeoutInSeconds())) { ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(applicationReference).getApplicationInstanceStatus(); if (appStatus == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { return; @@ -229,12 +224,14 @@ public class OrchestratorImpl implements Orchestrator { throw new BatchHostNameNotFoundException(parentHostname, hostNames, e); } - OrchestratorContext context = OrchestratorContext.createContextForMultiAppOp(clock); for (NodeGroup nodeGroup : nodeGroupsOrderedByApplication) { try { - suspendGroup(context.createSubcontextForApplication(), nodeGroup); + suspendGroup(nodeGroup); } catch (HostStateChangeDeniedException e) { throw new BatchHostStateChangeDeniedException(parentHostname, nodeGroup, e); + } catch (HostNameNotFoundException e) { + // Should never get here since since we would have received HostNameNotFoundException earlier. + throw new BatchHostNameNotFoundException(parentHostname, hostNames, e); } catch (RuntimeException e) { throw new BatchInternalErrorException(parentHostname, nodeGroup, e); } @@ -304,12 +301,12 @@ public class OrchestratorImpl implements Orchestrator { private void setApplicationStatus(ApplicationId appId, ApplicationInstanceStatus status) throws ApplicationStateChangeDeniedException, ApplicationIdNotFoundException{ - OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + OrchestratorContext context = new OrchestratorContext(clock); ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId, instanceLookupService); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appRef, - context.getTimeLeft())) { + context.getOriginalTimeoutInSeconds())) { // Short-circuit if already in wanted state if (status == statusRegistry.getApplicationInstanceStatus()) return; 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 d7e63ccfc76..467b534f809 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 @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.controller; -import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; @@ -16,6 +15,23 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ public static final String REQUEST_REASON = "Orchestrator"; + // On setNodeState calls against the CC ensemble. + // + // We'd like to set a timeout for the request to the first CC such that if the first + // CC is faulty, there's sufficient time to send the request to the second and third CC. + // The timeouts would be: + // timeout(1. request) = SHARE_REMAINING_TIME * T + // timeout(2. request) = SHARE_REMAINING_TIME * T * (1 - SHARE_REMAINING_TIME) + // timeout(3. request) = SHARE_REMAINING_TIME * T * (1 - SHARE_REMAINING_TIME)^2 + // + // Using a share of 50% gives approximately: + // timeout(1. request) = T * 0.5 + // timeout(2. request) = T * 0.25 + // timeout(3. request) = T * 0.125 + // + // which seems fine + public static final float SHARE_REMAINING_TIME = 0.5f; + private final JaxRsStrategy clusterControllerApi; private final String clusterName; @@ -36,16 +52,15 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ ClusterControllerNodeState wantedState) throws IOException { ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.SAFE); - ClusterControllerClientTimeouts timeouts = context.getClusterControllerTimeouts(clusterName); try { return clusterControllerApi.apply(api -> api.setNodeState( clusterName, storageNodeIndex, - timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, - stateRequest), - timeouts); - } catch (IOException | UncheckedTimeoutException e) { + context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), + stateRequest) + ); + } catch (IOException e) { String message = String.format( "Giving up setting %s for storage node with index %d in cluster %s", stateRequest, @@ -64,18 +79,16 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ @Override public ClusterControllerStateResponse setApplicationState( OrchestratorContext context, - ClusterControllerNodeState wantedState) throws IOException { - ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); - ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.FORCE); - ClusterControllerClientTimeouts timeouts = context.getClusterControllerTimeouts(clusterName); + final ClusterControllerNodeState wantedState) throws IOException { + final ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); + final ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.FORCE); try { return clusterControllerApi.apply(api -> api.setClusterState( clusterName, - timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, - stateRequest), - timeouts); - } catch (IOException | UncheckedTimeoutException e) { + context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), + stateRequest)); + } catch (IOException e) { final String message = String.format( "Giving up setting %s for cluster %s", stateRequest, diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java deleted file mode 100644 index 5b0685e88a0..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java +++ /dev/null @@ -1,125 +0,0 @@ -package com.yahoo.vespa.orchestrator.controller; - -import com.google.common.util.concurrent.UncheckedTimeoutException; -import com.yahoo.time.TimeBudget; -import com.yahoo.vespa.jaxrs.client.JaxRsTimeouts; - -import java.time.Duration; - -/** - * Calculates various timeouts associated with a REST call from the Orchestrator to the Cluster Controller. - * - *

Timeout handling of HTTP messaging is fundamentally flawed in various Java implementations. - * We would like to specify a max time for the whole operation (connect, send request, and receive response). - * Jersey JAX-RS implementation and the Apache HTTP client library provides a way to set the connect timeout C - * and read timeout R. So if the operation takes NR reads, and the writes takes TW time, - * the theoretical max time is: T = C + R * NR + TW. With both NR and TW unknown, there's no way to - * set a proper C and R.

- * - *

The various timeouts is set according to the following considerations:

- * - *
    - *
  1. Some time is reserved for the execution in this process, e.g. execution leading to the REST call, - * handling of the response, exception handling, etc, such that we can finish processing this request - * before the {@link #timeBudget} deadline. This is typically in the order of ms.
    2. - *
  2. A timeout will be passed to the Cluster Controller backend. We'll give a timeout such that if one - * CC times out, the next CC will be given exactly the same timeout. This may or may not be a good strategy: - * (A) There's typically a 3rd CC. But if the first 2 fails with timeout, the chance the last is OK - * is negligible. (B) If picking the CC is random, then giving the full timeout to the first - * should be sufficient since a later retry will hit the healthy CC. (C) Because we have been using - * DROP in networking rules, clients may time out (host out of app or whatever). This would suggest - * allowing more than 1 full request.
    3. - *
  3. The timeout passed to the CC backend should be such that if it honors that, the Orchestrator - * should not time out. This means some kernel and network overhead should be subtracted from the timeout - * passed to the CC.
    4. - *
  4. We're only able to set the connect and read/write timeouts(!) Since we're communicating within - * data center, assume connections are in the order of ms, while a single read may stall close up to the CC - * timeout.
    5. - *
- * - * @author hakonhall - */ -public class ClusterControllerClientTimeouts implements JaxRsTimeouts { - // In data center connect timeout - static final Duration CONNECT_TIMEOUT = Duration.ofMillis(50); - // Per call overhead - static final Duration IN_PROCESS_OVERHEAD_PER_CALL = Duration.ofMillis(50); - // In data center kernel and network overhead. - static final Duration NETWORK_OVERHEAD_PER_CALL = CONNECT_TIMEOUT; - // Minimum time reserved for post-RPC processing to finish BEFORE the deadline, including ZK write. - static final Duration IN_PROCESS_OVERHEAD = Duration.ofMillis(100); - // Number of JAX-RS RPC calls to account for within the time budget. - static final int NUM_CALLS = 2; - // Minimum server-side timeout - static final Duration MIN_SERVER_TIMEOUT = Duration.ofMillis(10); - - private final String clusterName; - private final TimeBudget timeBudget; - private final Duration maxClientTimeout; - - /** - * Creates a timeouts instance. - * - * The {@link #timeBudget} SHOULD be the time budget for a single logical call to the Cluster Controller. - * A logical call to CC may in fact call the CC several times, if the first onces are down and/or not - * the master. - * - * @param clusterName The name of the content cluster this request is for. - * @param timeBudget The time budget for a single logical call to the the Cluster Controller. - */ - public ClusterControllerClientTimeouts(String clusterName, TimeBudget timeBudget) { - this.clusterName = clusterName; - this.timeBudget = timeBudget; - - // timeLeft = inProcessOverhead + numCalls * clientTimeout - maxClientTimeout = timeBudget.originalTimeout().get().minus(IN_PROCESS_OVERHEAD).dividedBy(NUM_CALLS); - } - - @Override - public Duration getConnectTimeoutOrThrow() { - return CONNECT_TIMEOUT; - } - - @Override - public Duration getReadTimeoutOrThrow() { - Duration timeLeft = timeBudget.timeLeft().get(); - if (timeLeft.toMillis() <= 0) { - throw new UncheckedTimeoutException("Exceeded the timeout " + timeBudget.originalTimeout().get() + - " against content cluster '" + clusterName + "' by " + timeLeft.negated()); - } - - Duration clientTimeout = min(timeLeft, maxClientTimeout); - verifyPositive(timeLeft, clientTimeout); - - // clientTimeout = overheadPerCall + connectTimeout + readTimeout - Duration readTimeout = clientTimeout.minus(IN_PROCESS_OVERHEAD_PER_CALL).minus(CONNECT_TIMEOUT); - verifyPositive(timeLeft, readTimeout); - - return readTimeout; - } - - public Duration getServerTimeoutOrThrow() { - // readTimeout = networkOverhead + serverTimeout - Duration serverTimeout = getReadTimeoutOrThrow().minus(NETWORK_OVERHEAD_PER_CALL); - if (serverTimeout.toMillis() < MIN_SERVER_TIMEOUT.toMillis()) { - throw new UncheckedTimeoutException("Server would be given too little time to complete: " + - serverTimeout + ". Original timeout was " + timeBudget.originalTimeout().get()); - } - - return serverTimeout; - } - - private static Duration min(Duration a, Duration b) { - return a.compareTo(b) < 0 ? a : b; - } - - private void verifyLargerThan(Duration timeLeft, Duration availableDuration) { - if (availableDuration.toMillis() <= 0) { - throw new UncheckedTimeoutException("Too little time left (" + timeLeft + - ") to call content cluster '" + clusterName + - "', original timeout was " + timeBudget.originalTimeout().get()); - } - } - - private void verifyPositive(Duration timeLeft, Duration duration) { verifyLargerThan(timeLeft, duration); } -} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java index 7a0b96c2231..5571eedeec6 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java @@ -8,8 +8,9 @@ import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; import com.yahoo.vespa.jaxrs.client.JaxRsStrategyFactory; import com.yahoo.vespa.jaxrs.client.JerseyJaxRsClientFactory; -import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; /** * @author bakksjo @@ -20,12 +21,14 @@ public class RetryingClusterControllerClientFactory implements ClusterController public static final int HARDCODED_CLUSTERCONTROLLER_PORT = 19050; public static final String CLUSTERCONTROLLER_API_PATH = "/"; public static final String CLUSTERCONTROLLER_SCHEME = "http"; + private static final int CLUSTER_CONTROLLER_CONNECT_TIMEOUT_MS = 1000; + private static final int CLUSTER_CONTROLLER_READ_TIMEOUT_MS = 1000; private JaxRsClientFactory jaxRsClientFactory; @Inject public RetryingClusterControllerClientFactory() { - this(new JerseyJaxRsClientFactory()); + this(new JerseyJaxRsClientFactory(CLUSTER_CONTROLLER_CONNECT_TIMEOUT_MS, CLUSTER_CONTROLLER_READ_TIMEOUT_MS)); } public RetryingClusterControllerClientFactory(JaxRsClientFactory jaxRsClientFactory) { @@ -33,20 +36,19 @@ public class RetryingClusterControllerClientFactory implements ClusterController } @Override - public ClusterControllerClient createClient(List clusterControllers, String clusterName) { - JaxRsStrategy jaxRsApi = - new JaxRsStrategyFactory( - new HashSet<>(clusterControllers), - HARDCODED_CLUSTERCONTROLLER_PORT, - jaxRsClientFactory, - CLUSTERCONTROLLER_SCHEME) - .apiWithRetries(ClusterControllerJaxRsApi.class, CLUSTERCONTROLLER_API_PATH) - // Use max iteration 1. The JaxRsStrategyFactory will try host 1, 2, then 3: - // - If host 1 responds, it will redirect to master if necessary. Otherwise - // - If host 2 responds, it will redirect to master if necessary. Otherwise - // - If host 3 responds, it may redirect to master if necessary (if they're up - // after all), but more likely there's no quorum and this will fail too. - .setMaxIterations(1); + public ClusterControllerClient createClient(List clusterControllers, + String clusterName) { + Set clusterControllerSet = clusterControllers.stream().collect(Collectors.toSet()); + JaxRsStrategy jaxRsApi + = new JaxRsStrategyFactory(clusterControllerSet, HARDCODED_CLUSTERCONTROLLER_PORT, jaxRsClientFactory, CLUSTERCONTROLLER_SCHEME) + .apiWithRetries(ClusterControllerJaxRsApi.class, CLUSTERCONTROLLER_API_PATH) + // Use max iteration 1. The JaxRsStrategyFactory will try host 1, 2, then 3: + // - If host 1 responds, it will redirect to master if necessary. Otherwise + // - If host 2 responds, it will redirect to master if necessary. Otherwise + // - If host 3 responds, it may redirect to master if necessary (if they're up + // after all), but more likely there's no quorum and this will fail too. + .setMaxIterations(1); return new ClusterControllerClientImpl(jaxRsApi, clusterName); } + } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java new file mode 100644 index 00000000000..7459f0a6b11 --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java @@ -0,0 +1,54 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.orchestrator.controller; + +import com.yahoo.log.LogLevel; +import com.yahoo.vespa.applicationmodel.HostName; +import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; +import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; +import com.yahoo.vespa.jaxrs.client.NoRetryJaxRsStrategy; + +import java.util.List; +import java.util.logging.Logger; + +/** + * @author bakksjo + */ +public class SingleInstanceClusterControllerClientFactory implements ClusterControllerClientFactory { + + public static final int CLUSTERCONTROLLER_HARDCODED_PORT = 19050; + public static final String CLUSTERCONTROLLER_HARDCODED_SCHEME = "http"; + public static final String CLUSTERCONTROLLER_API_PATH = "/"; + + private static final Logger log = Logger.getLogger(SingleInstanceClusterControllerClientFactory.class.getName()); + + private JaxRsClientFactory jaxRsClientFactory; + + public SingleInstanceClusterControllerClientFactory(JaxRsClientFactory jaxRsClientFactory) { + this.jaxRsClientFactory = jaxRsClientFactory; + } + + @Override + public ClusterControllerClient createClient(List clusterControllers, + String clusterName) { + if (clusterControllers.isEmpty()) { + throw new IllegalArgumentException("No cluster controller instances found"); + } + HostName controllerHostName = clusterControllers.iterator().next(); + int port = CLUSTERCONTROLLER_HARDCODED_PORT; // TODO: Get this from service monitor. + + log.log(LogLevel.DEBUG, () -> + "For cluster '" + clusterName + "' with controllers " + clusterControllers + + ", creating api client for " + controllerHostName.s() + ":" + port); + + JaxRsStrategy strategy = new NoRetryJaxRsStrategy<>( + controllerHostName, + port, + jaxRsClientFactory, + ClusterControllerJaxRsApi.class, + CLUSTERCONTROLLER_API_PATH, + CLUSTERCONTROLLER_HARDCODED_SCHEME); + + return new ClusterControllerClientImpl(strategy, clusterName); + } + +} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java index 74b4b534acc..ed506c82079 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java @@ -43,7 +43,7 @@ public class NodeGroup { } public List getHostNames() { - return hostNames.stream().sorted().collect(Collectors.toList()); + return hostNames.stream().collect(Collectors.toList()).stream().sorted().collect(Collectors.toList()); } public String toCommaSeparatedString() { diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java index bd5eb6f3e29..c5ae553a98c 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java @@ -4,7 +4,6 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.HostName; -import java.time.Duration; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -43,11 +42,15 @@ public class InMemoryStatusService implements StatusService { }; } + @Override + public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference) { + return lockApplicationInstance_forCurrentThreadOnly(applicationInstanceReference, 10); + } @Override public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - Duration timeout) { + long timeoutSeconds) { Lock lock = instanceLockService.get(applicationInstanceReference); return new InMemoryMutableStatusRegistry(lock, applicationInstanceReference); } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java index 76adef72b2b..c47be096242 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; -import java.time.Duration; import java.util.Set; /** @@ -25,7 +24,7 @@ public interface StatusService { * possibly inconsistent snapshot values. It is not recommended that this method is used for anything other * than monitoring, logging, debugging, etc. It should never be used for multi-step operations (e.g. * read-then-write) where consistency is required. For those cases, use - * {@link #lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference, Duration)}. + * {@link #lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference)}. */ ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference); @@ -53,9 +52,12 @@ public interface StatusService { * this case, subsequent mutating operations will fail, but previous mutating operations are NOT rolled back. * This may leave the registry in an inconsistent state (as judged by the client code). */ + MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference); + + /** Lock application instance with timeout. */ MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - Duration timeout); + long timeoutSeconds); /** * Returns all application instances that are allowed to be down. The intention is to use this diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java index 7df29e038c1..deece6a4a65 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java @@ -56,6 +56,21 @@ public class ZookeeperStatusService implements StatusService { }; } + /** + * 1) locks the status service for an application instance. + * 2) fails all operations in this thread when the session is lost, + * since session loss might cause the lock to be lost. + * Since it only fails operations in this thread, + * all operations depending on a lock, including the locking itself, must be done in this thread. + * Note that since it is the thread that fails, all status operations in this thread will fail + * even if they're not supposed to be guarded by this lock + * (i.e. the request is for another applicationInstanceReference) + */ + @Override + public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference) { + return lockApplicationInstance_forCurrentThreadOnly(applicationInstanceReference, 10); + } + @Override public Set getAllSuspendedApplications() { try { @@ -78,23 +93,13 @@ public class ZookeeperStatusService implements StatusService { } } - /** - * 1) locks the status service for an application instance. - * 2) fails all operations in this thread when the session is lost, - * since session loss might cause the lock to be lost. - * Since it only fails operations in this thread, - * all operations depending on a lock, including the locking itself, must be done in this thread. - * Note that since it is the thread that fails, all status operations in this thread will fail - * even if they're not supposed to be guarded by this lock - * (i.e. the request is for another applicationInstanceReference) - */ @Override public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - Duration timeout) { + long timeoutSeconds) { String lockPath = applicationInstanceLock2Path(applicationInstanceReference); Lock lock = new Lock(lockPath, curator); - lock.acquire(timeout); + lock.acquire(Duration.ofSeconds(timeoutSeconds)); try { return new ZkMutableStatusRegistry(lock, applicationInstanceReference); 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 80174d05a54..76d9398c44e 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java @@ -43,8 +43,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -247,8 +245,6 @@ public class OrchestratorImplTest { // A spy is preferential because suspendAll() relies on delegating the hard work to suspend() and resume(). OrchestratorImpl orchestrator = spy(this.orchestrator); - OrchestratorContext context = mock(OrchestratorContext.class); - orchestrator.suspendAll( new HostName("parentHostname"), Arrays.asList( @@ -261,9 +257,9 @@ public class OrchestratorImplTest { // TEST6: tenant-id-3:application-instance-3:default // TEST1: test-tenant-id:application:instance InOrder order = inOrder(orchestrator); - order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST3_NODE_GROUP)); - order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); - order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST1_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP); + order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); + order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST1_NODE_GROUP); order.verifyNoMoreInteractions(); } @@ -276,7 +272,7 @@ public class OrchestratorImplTest { DummyInstanceLookupService.TEST6_HOST_NAME, "some-constraint", "error message"); - doThrow(supensionFailure).when(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); + doThrow(supensionFailure).when(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); try { orchestrator.suspendAll( @@ -295,8 +291,8 @@ public class OrchestratorImplTest { } InOrder order = inOrder(orchestrator); - order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST3_NODE_GROUP)); - order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP); + order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); order.verifyNoMoreInteractions(); } diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java index b12cd5aa7be..228174a9b3d 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java @@ -6,9 +6,7 @@ import com.yahoo.vespa.jaxrs.client.LocalPassThroughJaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; import org.junit.Test; -import java.time.Duration; - -import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyFloat; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -30,9 +28,7 @@ public class ClusterControllerClientTest { final ClusterControllerNodeState wantedState = ClusterControllerNodeState.MAINTENANCE; OrchestratorContext context = mock(OrchestratorContext.class); - ClusterControllerClientTimeouts timeouts = mock(ClusterControllerClientTimeouts.class); - when(context.getClusterControllerTimeouts(any())).thenReturn(timeouts); - when(timeouts.getServerTimeoutOrThrow()).thenReturn(Duration.ofSeconds(1)); + when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); clusterControllerClient.setNodeState(context, STORAGE_NODE_INDEX, wantedState); final ClusterControllerStateRequest expectedNodeStateRequest = new ClusterControllerStateRequest( diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java deleted file mode 100644 index 9fe65447ac1..00000000000 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java +++ /dev/null @@ -1,151 +0,0 @@ -package com.yahoo.vespa.orchestrator.controller; - -import com.google.common.util.concurrent.UncheckedTimeoutException; -import com.yahoo.test.ManualClock; -import com.yahoo.time.TimeBudget; -import org.junit.Before; -import org.junit.Test; - -import java.time.Duration; -import java.util.Optional; - -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.CONNECT_TIMEOUT; -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.IN_PROCESS_OVERHEAD; -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.IN_PROCESS_OVERHEAD_PER_CALL; -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.MIN_SERVER_TIMEOUT; -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.NETWORK_OVERHEAD_PER_CALL; -import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.NUM_CALLS; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.fail; - -public class ClusterControllerClientTimeoutsTest { - // The minimum time left for any invocation of prepareForImmediateJaxRsCall(). - private static final Duration MINIMUM_TIME_LEFT = IN_PROCESS_OVERHEAD_PER_CALL - .plus(CONNECT_TIMEOUT) - .plus(NETWORK_OVERHEAD_PER_CALL) - .plus(MIN_SERVER_TIMEOUT); - static { - assertEquals(Duration.ofMillis(160), MINIMUM_TIME_LEFT); - } - - // The minimum time left (= original time) which is required to allow any requests to the CC. - private static final Duration MINIMUM_ORIGINAL_TIMEOUT = MINIMUM_TIME_LEFT - .multipliedBy(NUM_CALLS) - .plus(IN_PROCESS_OVERHEAD); - static { - assertEquals(Duration.ofMillis(420), MINIMUM_ORIGINAL_TIMEOUT); - } - - private final ManualClock clock = new ManualClock(); - - private Duration originalTimeout; - private TimeBudget timeBudget; - private ClusterControllerClientTimeouts timeouts; - - private void makeTimeouts(Duration originalTimeout) { - this.originalTimeout = originalTimeout; - this.timeBudget = TimeBudget.from(clock, clock.instant(), Optional.of(originalTimeout)); - this.timeouts = new ClusterControllerClientTimeouts("clustername", timeBudget); - } - - @Before - public void setUp() { - makeTimeouts(Duration.ofSeconds(3)); - } - - @Test - public void makes2RequestsWithMaxProcessingTime() { - assertStandardTimeouts(); - - Duration maxProcessingTime = IN_PROCESS_OVERHEAD_PER_CALL - .plus(CONNECT_TIMEOUT) - .plus(timeouts.getReadTimeoutOrThrow()); - assertEquals(1450, maxProcessingTime.toMillis()); - clock.advance(maxProcessingTime); - - assertStandardTimeouts(); - - clock.advance(maxProcessingTime); - - try { - timeouts.getServerTimeoutOrThrow(); - fail(); - } catch (UncheckedTimeoutException e) { - assertEquals( - "Too little time left (PT0.1S) to call content cluster 'clustername', original timeout was PT3S", - e.getMessage()); - } - } - - @Test - public void makesAtLeast3RequestsWithShortProcessingTime() { - assertStandardTimeouts(); - - Duration shortProcessingTime = Duration.ofMillis(200); - clock.advance(shortProcessingTime); - - assertStandardTimeouts(); - - clock.advance(shortProcessingTime); - - assertStandardTimeouts(); - } - - private void assertStandardTimeouts() { - assertEquals(Duration.ofMillis(50), timeouts.getConnectTimeoutOrThrow()); - assertEquals(Duration.ofMillis(1350), timeouts.getReadTimeoutOrThrow()); - assertEquals(Duration.ofMillis(1300), timeouts.getServerTimeoutOrThrow()); - } - - @Test - public void alreadyTimedOut() { - clock.advance(Duration.ofSeconds(4)); - - try { - timeouts.getServerTimeoutOrThrow(); - fail(); - } catch (UncheckedTimeoutException e) { - assertEquals( - "Exceeded the timeout PT3S against content cluster 'clustername' by PT1S", - e.getMessage()); - } - } - - @Test - public void justTooLittleTime() { - clock.advance(originalTimeout.minus(MINIMUM_TIME_LEFT).plus(Duration.ofMillis(1))); - try { - timeouts.getServerTimeoutOrThrow(); - fail(); - } catch (UncheckedTimeoutException e) { - assertEquals( - "Server would be given too little time to complete: PT0.009S. Original timeout was PT3S", - e.getMessage()); - } - } - - @Test - public void justEnoughTime() { - clock.advance(originalTimeout.minus(MINIMUM_TIME_LEFT)); - timeouts.getServerTimeoutOrThrow(); - } - - @Test - public void justTooLittleInitialTime() { - makeTimeouts(MINIMUM_ORIGINAL_TIMEOUT.minus(Duration.ofMillis(1))); - try { - timeouts.getServerTimeoutOrThrow(); - fail(); - } catch (UncheckedTimeoutException e) { - assertEquals( - "Server would be given too little time to complete: PT0.0095S. Original timeout was PT0.419S", - e.getMessage()); - } - } - - @Test - public void justEnoughInitialTime() { - makeTimeouts(MINIMUM_ORIGINAL_TIMEOUT); - timeouts.getServerTimeoutOrThrow(); - } -} \ No newline at end of file diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java deleted file mode 100644 index 1f505991476..00000000000 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java +++ /dev/null @@ -1,54 +0,0 @@ -package com.yahoo.vespa.orchestrator.controller; - -import com.yahoo.test.ManualClock; -import com.yahoo.vespa.applicationmodel.HostName; -import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; -import com.yahoo.vespa.orchestrator.OrchestratorContext; -import org.junit.Test; -import org.mockito.ArgumentCaptor; - -import java.io.IOException; -import java.time.Clock; -import java.util.Arrays; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import static org.junit.Assert.assertEquals; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -public class RetryingClusterControllerClientFactoryTest { - private final Clock clock = new ManualClock(); - - @Test - public void verifyJerseyCallForSetNodeState() throws IOException { - JaxRsClientFactory clientFactory = mock(JaxRsClientFactory.class); - ClusterControllerJaxRsApi api = mock(ClusterControllerJaxRsApi.class); - when(clientFactory.createClient(any())).thenReturn(api); - RetryingClusterControllerClientFactory factory = new RetryingClusterControllerClientFactory(clientFactory); - String clusterName = "clustername"; - HostName host1 = new HostName("host1"); - HostName host2 = new HostName("host2"); - HostName host3 = new HostName("host3"); - List clusterControllers = Arrays.asList(host1, host2, host3); - ClusterControllerClient client = factory.createClient(clusterControllers, clusterName); - OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); - int storageNode = 2; - ClusterControllerNodeState wantedState = ClusterControllerNodeState.MAINTENANCE; - client.setNodeState(context, storageNode, wantedState); - - ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(ClusterControllerStateRequest.class); - - verify(api, times(1)).setNodeState(eq(clusterName), eq(storageNode), eq(4.8f), requestCaptor.capture()); - ClusterControllerStateRequest request = requestCaptor.getValue(); - assertEquals(ClusterControllerStateRequest.Condition.SAFE, request.condition); - Map expectedState = new HashMap<>(); - expectedState.put("user", new ClusterControllerStateRequest.State(wantedState, "Orchestrator")); - assertEquals(expectedState, request.state); - } -} \ No newline at end of file diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java new file mode 100644 index 00000000000..4dabae14add --- /dev/null +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java @@ -0,0 +1,116 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.orchestrator.controller; + +import com.yahoo.vespa.applicationmodel.ConfigId; +import com.yahoo.vespa.applicationmodel.HostName; +import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; +import com.yahoo.vespa.orchestrator.OrchestratorContext; +import org.junit.Before; +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +import static org.hamcrest.CoreMatchers.anyOf; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyFloat; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.argThat; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class SingleInstanceClusterControllerClientFactoryTest { + private static final int PORT = SingleInstanceClusterControllerClientFactory.CLUSTERCONTROLLER_HARDCODED_PORT; + private static final String PATH = SingleInstanceClusterControllerClientFactory.CLUSTERCONTROLLER_API_PATH; + + private static final HostName HOST_NAME_1 = new HostName("host1"); + private static final HostName HOST_NAME_2 = new HostName("host2"); + private static final HostName HOST_NAME_3 = new HostName("host3"); + + OrchestratorContext context = mock(OrchestratorContext.class); + private final ClusterControllerJaxRsApi mockApi = mock(ClusterControllerJaxRsApi.class); + private final JaxRsClientFactory jaxRsClientFactory = mock(JaxRsClientFactory.class); + private final ClusterControllerClientFactory clientFactory + = new SingleInstanceClusterControllerClientFactory(jaxRsClientFactory); + + @Before + public void setup() { + when( + jaxRsClientFactory.createClient( + eq(ClusterControllerJaxRsApi.class), + any(HostName.class), + anyInt(), + anyString(), + anyString())) + .thenReturn(mockApi); + } + + @Test + public void testCreateClientWithNoClusterControllerInstances() throws Exception { + final List clusterControllers = Arrays.asList(); + + try { + clientFactory.createClient(clusterControllers, "clusterName"); + fail(); + } catch (IllegalArgumentException e) { + // As expected. + } + } + + @Test + public void testCreateClientWithSingleClusterControllerInstance() throws Exception { + final List clusterControllers = Arrays.asList(HOST_NAME_1); + + when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); + clientFactory.createClient(clusterControllers, "clusterName") + .setNodeState(context, 0, ClusterControllerNodeState.MAINTENANCE); + + verify(jaxRsClientFactory).createClient( + ClusterControllerJaxRsApi.class, + HOST_NAME_1, + PORT, + PATH, + "http"); + } + + @Test + public void testCreateClientWithoutClusterControllerInstances() throws Exception { + final List clusterControllers = Arrays.asList(); + + try { + clientFactory.createClient(clusterControllers, "clusterName"); + fail(); + } catch (IllegalArgumentException e) { + // As expected. + } + } + + @Test + public void testCreateClientWithThreeClusterControllerInstances() throws Exception { + final List clusterControllers = Arrays.asList(HOST_NAME_1, HOST_NAME_2, HOST_NAME_3); + + when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); + clientFactory.createClient(clusterControllers, "clusterName") + .setNodeState(context, 0, ClusterControllerNodeState.MAINTENANCE); + + verify(jaxRsClientFactory).createClient( + eq(ClusterControllerJaxRsApi.class), + argThat(is(anyOf( + equalTo(HOST_NAME_1), + equalTo(HOST_NAME_2), + equalTo(HOST_NAME_3)))), + eq(PORT), + eq(PATH), + eq("http")); + } + + private static ConfigId clusterControllerConfigId(final int index) { + return new ConfigId("admin/cluster-controllers/" + index); + } +} diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java index a9b8127e7fe..45ba862c8f1 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.resources; +import com.yahoo.jdisc.Timer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -31,7 +32,6 @@ import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; import com.yahoo.vespa.orchestrator.status.StatusService; -import org.junit.Before; import org.junit.Test; import javax.ws.rs.BadRequestException; @@ -41,7 +41,6 @@ import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; import java.net.URI; import java.time.Clock; -import java.time.Instant; import java.util.Arrays; import java.util.Collections; import java.util.Optional; @@ -75,7 +74,7 @@ public class HostResourceTest { static { when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.forApplicationInstance(eq(APPLICATION_INSTANCE_REFERENCE))) .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); - when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(eq(APPLICATION_INSTANCE_REFERENCE), any())) + when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(eq(APPLICATION_INSTANCE_REFERENCE))) .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); when(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY.getHostStatus(any())) .thenReturn(HostStatus.NO_REMARKS); @@ -151,11 +150,6 @@ public class HostResourceTest { private final UriInfo uriInfo = mock(UriInfo.class); - @Before - public void setUp() { - when(clock.instant()).thenReturn(Instant.now()); - } - @Test public void returns_200_on_success() { HostResource hostResource = diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java index 44847666670..2e914718e20 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java @@ -17,7 +17,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; -import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -82,8 +81,7 @@ public class ZookeeperStatusServiceTest { @Test public void setting_host_state_is_idempotent() { try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, - Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { //shuffling to catch "clean database" failures for all cases. for (HostStatus hostStatus: shuffledList(HostStatus.values())) { @@ -107,12 +105,11 @@ public class ZookeeperStatusServiceTest { final CompletableFuture lockedSuccessfullyFuture; try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, - Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> { try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2 - .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) + .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE)) { } }); @@ -134,13 +131,13 @@ public class ZookeeperStatusServiceTest { ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { //must run in separate thread, since having 2 locks in the same thread fails CompletableFuture resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> { try { zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(1)); + TestIds.APPLICATION_INSTANCE_REFERENCE,1); fail("Both zookeeper host status services locked simultaneously for the same application instance"); } catch (RuntimeException e) { } @@ -214,7 +211,7 @@ public class ZookeeperStatusServiceTest { // Suspend try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } @@ -226,7 +223,7 @@ public class ZookeeperStatusServiceTest { // Resume try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.NO_REMARKS); } @@ -244,12 +241,12 @@ public class ZookeeperStatusServiceTest { assertThat(suspendedApps.size(), is(0)); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE)) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE2, Duration.ofSeconds(10))) { + TestIds.APPLICATION_INSTANCE_REFERENCE2)) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } diff --git a/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java b/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java index 449b0d6bd05..a7963df0208 100644 --- a/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java +++ b/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java @@ -66,18 +66,6 @@ public class TimeBudget { }); } - /** Returns the time left, possibly negative if the deadline has passed. */ - public Optional timeLeft() { - return timeout.map(timeout -> timeout.minus(timePassed())); - } - - /** Returns the time left as a new TimeBudget. */ - public TimeBudget timeLeftAsTimeBudget() { - Instant now = clock.instant(); - Optional deadline = deadline(); - return new TimeBudget(clock, now, deadline.map(d -> Duration.between(now, d))); - } - private static Duration nonNegativeBetween(Instant start, Instant end) { return makeNonNegative(Duration.between(start, end)); } -- cgit v1.2.3