diff options
54 files changed, 732 insertions, 424 deletions
diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java index d869b8ec4d9..45f2ef54f7f 100644 --- a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java @@ -5,6 +5,7 @@ import com.yahoo.container.plugin.osgi.ExportPackages.Export; import com.yahoo.container.plugin.osgi.ExportPackages.Parameter; import org.hamcrest.Description; import org.hamcrest.TypeSafeMatcher; +import org.junit.Ignore; import org.junit.Test; import java.util.List; @@ -102,6 +103,8 @@ public class ExportPackageParserTest { assertThat(export.getParameters(), contains(parameterMatching(versionParameter))); } + // TODO: MAVEN_OPTS are not propagated by the maven-surefire-plugin. Either try to fix the underlying problem or set -Xss in plugin config. + @Ignore // Frequently causes StackOverflowError @Test public void require_that_long_string_literals_do_not_cause_stack_overflow_error() { //From jersey-server-1.13.jar 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 4d6738940a8..eecdcc75228 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,6 +37,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { 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); @@ -51,6 +52,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { this.responseWait = setUnitStateRequest.getResponseWait(); this.wantedState = wantedState; this.timeBudget = setUnitStateRequest.timeBudget(); + this.probe = setUnitStateRequest.isProbe(); } @Override @@ -61,7 +63,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { newStates, id.getNode(), context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState); + context.currentConsolidatedState, + probe); } static NodeState getRequestedNodeState(Map<String, UnitState> newStates, Node n) throws StateRestApiException { @@ -100,7 +103,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { Map<String, UnitState> newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState) throws StateRestApiException { + ClusterState currentClusterState, + boolean probe) throws StateRestApiException { if ( ! cluster.hasConfiguredNode(node.getIndex())) { throw new MissingIdException(cluster.getName(), node); } @@ -126,7 +130,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { condition, nodeInfo, cluster, - stateListener); + stateListener, + probe); // If the state was successfully set, just return an "ok" message back. String reason = success ? "ok" : result.getReason(); @@ -143,9 +148,10 @@ public class SetNodeStateRequest extends Request<SetResponse> { SetUnitStateRequest.Condition condition, NodeInfo nodeInfo, ContentCluster cluster, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { if (result.settingWantedStateIsAllowed()) { - setNewWantedState(nodeInfo, newWantedState, stateListener); + setNewWantedState(nodeInfo, newWantedState, stateListener, probe); } // True if the wanted state was or has just been set to newWantedState @@ -156,7 +162,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { // 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); + setDistributorWantedState(cluster, nodeInfo.getNodeIndex(), newWantedState, stateListener, probe); } return success; @@ -169,7 +175,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { private static void setDistributorWantedState(ContentCluster cluster, int index, NodeState newStorageWantedState, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { Node distributorNode = new Node(NodeType.DISTRIBUTOR, index); NodeInfo nodeInfo = cluster.getNodeInfo(distributorNode); if (nodeInfo == null) { @@ -200,13 +207,15 @@ public class SetNodeStateRequest extends Request<SetResponse> { if (newWantedState.getState() != currentWantedState.getState() || !Objects.equals(newWantedState.getDescription(), currentWantedState.getDescription())) { - setNewWantedState(nodeInfo, newWantedState, stateListener); + setNewWantedState(nodeInfo, newWantedState, stateListener, probe); } } private static void setNewWantedState(NodeInfo nodeInfo, NodeState newWantedState, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { + if (probe) return; 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 b4d189bcd55..d7820722887 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,6 +27,7 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { private final Map<String, UnitState> newStates; private final SetUnitStateRequest.Condition condition; private final TimeBudget timeBudget; + private final boolean probe; public SetNodeStatesForClusterRequest(Id.Cluster cluster, SetUnitStateRequest request) { @@ -35,6 +36,7 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { this.newStates = request.getNewState(); this.condition = request.getCondition(); this.timeBudget = request.timeBudget(); + this.probe = request.isProbe(); } @Override @@ -69,7 +71,8 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { newStates, node, context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState); + context.currentConsolidatedState, + probe); 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 6fa7d536c67..c3090a5e832 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,5 +22,6 @@ public interface WantedStateSetter { Map<String, UnitState> newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState) throws StateRestApiException; + ClusterState currentClusterState, + boolean probe) 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 f3a4be5ac2f..6cf4b7989e7 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,6 +33,7 @@ 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; @@ -46,6 +47,7 @@ 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); @@ -98,6 +100,11 @@ 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 { @@ -458,7 +465,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())).thenReturn(response); + when(wantedStateSetter.set(any(), any(), any(), any(), any(), any(), anyBoolean())).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 9239b8774b0..7161fb1be79 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,20 +23,23 @@ 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 { - public static final String REASON = "operator"; - ContentCluster cluster = mock(ContentCluster.class); - SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; - Map<String, UnitState> newStates = new HashMap<>(); - UnitState unitState = mock(UnitState.class); + private static final String REASON = "operator"; + private ContentCluster cluster = mock(ContentCluster.class); + private SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; + private Map<String, UnitState> newStates = new HashMap<>(); + private UnitState unitState = mock(UnitState.class); private final int NODE_INDEX = 2; - Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); - NodeStateOrHostInfoChangeHandler stateListener = mock(NodeStateOrHostInfoChangeHandler.class); - ClusterState currentClusterState = mock(ClusterState.class); + 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; @Before public void setUp() { @@ -53,6 +56,16 @@ public class SetNodeStateRequestTest { } @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( "down", @@ -124,6 +137,9 @@ 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(); @@ -133,6 +149,9 @@ 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()) { @@ -140,6 +159,9 @@ 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()); } } @@ -150,6 +172,7 @@ public class SetNodeStateRequestTest { newStates, storageNode, stateListener, - currentClusterState); + currentClusterState, + probe); } }
\ 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 a28ddb3539b..27f18c3664b 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,4 +64,7 @@ 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 d871a8ed6bc..dab6895cc9d 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,13 +33,16 @@ public class JsonReader { } static class SetRequestData { + final boolean probe; final Map<String, UnitState> stateMap; final SetUnitStateRequest.Condition condition; final SetUnitStateRequest.ResponseWait responseWait; - public SetRequestData(Map<String, UnitState> stateMap, + public SetRequestData(boolean probe, + Map<String, UnitState> stateMap, SetUnitStateRequest.Condition condition, SetUnitStateRequest.ResponseWait responseWait) { + this.probe = probe; this.stateMap = stateMap; this.condition = condition; this.responseWait = responseWait; @@ -49,8 +52,9 @@ public class JsonReader { public SetRequestData getStateRequestData(HttpRequest request) throws Exception { JSONObject json = new JSONObject(request.getPostContent().toString()); - final SetUnitStateRequest.Condition condition; + 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 { @@ -100,6 +104,6 @@ public class JsonReader { stateMap.put(type, new UnitStateImpl(code, reason)); } - return new SetRequestData(stateMap, condition, responseWait); + return new SetRequestData(probe, 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 c38f7aec8c6..46f5d964245 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,6 +97,8 @@ 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/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java index e8142999433..66294d3fcef 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java @@ -20,10 +20,10 @@ public class ConfigProducerGroup<CHILD extends AbstractConfigProducer<?>> extend } public void addComponent(ComponentId id, CHILD producer) { - boolean wasAdded = producerById.put(id, producer) == null; - if (!wasAdded) { - throw new IllegalArgumentException("Two entities have the same component id '" + - id + "' in the same scope."); + CHILD existing = producerById.put(id, producer); + if ( existing != null) { + throw new IllegalArgumentException("Both " + producer + " and " + existing + " are configured" + + " with the id '" + id + "'. All components must have a unique id."); } addChild(producer); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java index f795e481f62..6b4f8c8f8b5 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java @@ -78,4 +78,9 @@ public class Chain<T extends ChainedComponent<?>> extends AbstractConfigProducer return TYPE; } + @Override + public String toString() { + return "chain '" + componentId + "'"; + } + } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java index 2605736e23b..ff211264a34 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java @@ -28,4 +28,9 @@ public class SearchChain extends Chain<Searcher<?>> { return Collections.emptyList(); } + @Override + public String toString() { + return "search chain '" + getId() + "'"; + } + } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java b/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java index 9122e855461..6ba75f1ff05 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java @@ -9,13 +9,11 @@ import org.junit.Before; import org.junit.Test; import org.w3c.dom.Element; -import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -import static org.hamcrest.Matchers.containsString; /** * @author gjoranv - * @since 5.1.11 */ public class SearchChainsTest2 { @@ -38,7 +36,8 @@ public class SearchChainsTest2 { chains.validate(); fail("Expected exception when inheriting a nonexistent search chain."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Missing chain 'nonexistent'")); + assertEquals("Missing chain 'nonexistent'.", + e.getMessage()); } } @@ -56,12 +55,13 @@ public class SearchChainsTest2 { ContainerModelBuilderTest.createModel(root, clusterElem); fail("Expected exception when declaring chains with duplicate id."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Two entities have the same component id 'same'")); + assertEquals("Both search chain 'same' and search chain 'same' are configured with the id 'same'. All components must have a unique id.", + e.getMessage()); } } @Test - public void fail_upon_user_declared_chain_with_same_id_as_builtin_chain() throws Exception { + public void fail_upon_user_declared_chain_with_same_id_as_builtin_chain() { final Element clusterElem = DomBuilderTest.parse( "<jdisc id='cluster1' version='1.0'>", ContainerModelBuilderTest.nodesXml, @@ -73,7 +73,8 @@ public class SearchChainsTest2 { ContainerModelBuilderTest.createModel(root, clusterElem); fail("Expected exception when taking the id from a builtin chain."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Two entities have the same component id 'vespa'")); + assertEquals("Both search chain 'vespa' and search chain 'vespa' are configured with the id 'vespa'. All components must have a unique id.", + e.getMessage()); } } } 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 e61d3710fac..73abd70a5ae 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 @@ -47,11 +47,6 @@ public class OrchestratorMock implements Orchestrator { } @Override - public void suspendGroup(NodeGroup nodeGroup) { - nodeGroup.getHostNames().forEach(this::suspend); - } - - @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS; diff --git a/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java b/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java index 1fad45a99e3..ca6fd44af50 100644 --- a/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java +++ b/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java @@ -8,7 +8,7 @@ import com.yahoo.search.query.profile.types.QueryProfileType; import java.util.Objects; /** - * Holds the settings for the soft-timeout feature. + * Settings for the soft-timeout feature. * * @author baldersheim */ @@ -41,22 +41,24 @@ public class SoftTimeout implements Cloneable { public void setFactor(double factor) { if ((factor < 0.0) || (factor > 1.0)) { - throw new IllegalArgumentException("factor must be in the range [0.0, 1.0]. It is " + factor); + throw new IllegalArgumentException("factor must be in the range [0.0, 1.0], got " + factor); } this.factor = factor; } + public Double getFactor() { return factor; } + public void setTailcost(double tailcost) { if ((tailcost < 0.0) || (tailcost > 1.0)) { - throw new IllegalArgumentException("tailcost must be in the range [0.0, 1.0]. It is " + tailcost); + throw new IllegalArgumentException("tailcost must be in the range [0.0, 1.0], got " + tailcost); } this.tailcost = tailcost; } + public Double getTailcost() { return tailcost; } /** Internal operation - DO NOT USE */ public void prepare(RankProperties rankProperties) { - if (enable != null) { rankProperties.put("vespa.softtimeout.enable", String.valueOf(enable)); } diff --git a/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java b/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java index 01501674ae8..83ef955a6d9 100644 --- a/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java @@ -37,7 +37,7 @@ public class SoftTimeoutTestCase { } catch (QueryException e) { assertEquals("Invalid request parameter", e.getMessage()); assertEquals("Could not set 'ranking.softtimeout." + key + "' to '" + value +"'", e.getCause().getMessage()); - assertEquals(key + " must be in the range [0.0, 1.0]. It is " + value, e.getCause().getCause().getMessage()); + assertEquals(key + " must be in the range [0.0, 1.0], got " + value, e.getCause().getCause().getMessage()); } } @Test diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java index b89cbaa4c82..22d738ff98c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java @@ -13,7 +13,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.RotationStatus; import com.yahoo.vespa.hosted.controller.authority.config.ApiAuthorityConfig; -import com.yahoo.yolean.Exceptions; +import org.apache.http.HttpResponse; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.ByteArrayEntity; import org.apache.http.impl.client.CloseableHttpClient; @@ -155,11 +155,14 @@ public class DeploymentMetricsMaintainer extends Maintainer { } private void feedMetrics(Slime slime) throws IOException { - String uri = baseUris.get(0) + "/metricforwarding/v1/deploymentmetrics/"; // For now, we only feed to one controller + String uri = baseUris.get(0) + "/metricforwarding/v1/deploymentmetrics"; // For now, we only feed to one controller CloseableHttpClient httpClient = HttpClientBuilder.create().build(); HttpPost httpPost = new HttpPost(uri); httpPost.setEntity(new ByteArrayEntity(SlimeUtils.toJsonBytes(slime))); - httpClient.execute(httpPost); + HttpResponse response = httpClient.execute(httpPost); + if (response.getStatusLine().getStatusCode() != 200) { + log.log(Level.WARNING, "Could not feed metrics. Reason: " + response.getStatusLine().getReasonPhrase()); + } } private static RotationStatus from(com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus status) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java index b1e3f8799d6..ccd943e7f0a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java @@ -81,10 +81,10 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase { @Override public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { Method method = getMethod(request); - if (isWhiteListedMethod(method)) return Optional.empty(); + Path path = new Path(request.getRequestURI()); + if (isWhiteListed(method, path)) return Optional.empty(); try { - Path path = new Path(request.getRequestURI()); AthenzPrincipal principal = getPrincipalOrThrow(request); if (isWhiteListedOperation(path, method)) { // no authz check @@ -106,8 +106,10 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase { } } - private static boolean isWhiteListedMethod(Method method) { - return WHITELISTED_METHODS.contains(method); + private static boolean isWhiteListed(Method method, Path path) { + return WHITELISTED_METHODS.contains(method) || + path.matches("/metricforwarding/v1/{*}") && method == POST || + path.matches("/contactinfo/v1/{*}") && method == POST; } private static boolean isWhiteListedOperation(Path path, Method method) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java index 91c847d10f1..f3112b09173 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java @@ -26,6 +26,7 @@ import com.yahoo.vespa.hosted.controller.restapi.StringResponse; import java.io.IOException; import java.util.HashMap; import java.util.Map; +import java.util.logging.Level; /** * This implements the metricforwarding/v1 API which allows feeding @@ -72,7 +73,8 @@ public class MetricForwardingApiHandler extends LoggingRequestHandler { }); }); } catch (IOException e) { - ErrorResponse.badRequest("Unable to parse request for metrics - " + e.getMessage()); + log.log(Level.WARNING, "Unable to parse request for cluster utilization metrics", e); + return ErrorResponse.badRequest("Unable to parse request for cluster utilization metrics - " + e.getMessage()); } return new StringResponse("Added cluster utilization metrics"); } @@ -99,7 +101,8 @@ public class MetricForwardingApiHandler extends LoggingRequestHandler { } }); } catch (IOException e) { - ErrorResponse.badRequest("Unable to parse request for metrics - " + e.getMessage()); + log.log(Level.WARNING, "Unable to parse request for deployment metrics", e); + return ErrorResponse.badRequest("Unable to parse request for deployment metrics - " + e.getMessage()); } return new StringResponse("Added deployment metrics"); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java index 5014f796933..fc43a7f9411 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java @@ -72,13 +72,13 @@ public class DeploymentMetricsMaintainerTest { metricsService.setZoneIn(assignedRotation, "proxy.prod.us-west-1.vip.test"); metricsService.setZoneOut(assignedRotation,"proxy.prod.us-east-3.vip.test"); - wireMockRule.stubFor(post(urlEqualTo("/metricforwarding/v1/deploymentmetrics/")) + wireMockRule.stubFor(post(urlEqualTo("/metricforwarding/v1/deploymentmetrics")) .willReturn(aResponse().withStatus(200))); maintainer.maintain(); List<ServeEvent> allServeEvents = getAllServeEvents(); assertEquals(1, allServeEvents.size()); - LoggedRequest request = findAll(postRequestedFor(urlEqualTo("/metricforwarding/v1/deploymentmetrics/"))).get(0); + LoggedRequest request = findAll(postRequestedFor(urlEqualTo("/metricforwarding/v1/deploymentmetrics"))).get(0); Slime slime = SlimeUtils.jsonToSlime(request.getBody()); Inspector inspector = slime.get().entry(0); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java index 19aa247edb4..6dce576e8a4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java @@ -58,6 +58,8 @@ public class ControllerAuthorizationFilterTest { ControllerAuthorizationFilter filter = createFilter(new ControllerTester()); assertIsAllowed(invokeFilter(filter, createRequest(PUT, "/application/v4/user", USER))); assertIsAllowed(invokeFilter(filter, createRequest(POST, "/application/v4/tenant/john", USER))); + assertIsAllowed(invokeFilter(filter, createRequest(POST, "/metricforwarding/v1/deploymentmetrics", USER))); + assertIsAllowed(invokeFilter(filter, createRequest(POST, "/contactinfo/v1/tenant/john/etc", USER))); } @Test diff --git a/jaxrs_client_utils/pom.xml b/jaxrs_client_utils/pom.xml index e3de5b8163d..43fbc66a9e6 100644 --- a/jaxrs_client_utils/pom.xml +++ b/jaxrs_client_utils/pom.xml @@ -18,6 +18,12 @@ <dependencies> <dependency> <groupId>com.yahoo.vespa</groupId> + <artifactId>vespajlib</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> <artifactId>application-model</artifactId> <version>${project.version}</version> <scope>provided</scope> 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 d004ac3af45..27d7024b9bd 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,11 +3,55 @@ 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<T> { + private final Class<T> apiClass; + private final URI uri; + + private Duration connectTimeout = Duration.ofSeconds(30); + private Duration readTimeout = Duration.ofSeconds(30); + + public Params(Class<T> apiClass, URI uri) { + this.apiClass = apiClass; + this.uri = uri; + } + + public Class<T> 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> T createClient(Params<T> params) { + return createClient(params.apiClass, new HostName(params.uri.getHost()), params.uri.getPort(), params.uri.getPath(), params.uri.getScheme()); + } + <T> T createClient(Class<T> 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 72af76fe54c..cd7d8684cbc 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,4 +11,8 @@ import java.util.function.Function; */ public interface JaxRsStrategy<T> { <R> R apply(final Function<T, R> function) throws IOException; + + default <R> R apply(final Function<T, R> 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 new file mode 100644 index 00000000000..914a9d7b42c --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java @@ -0,0 +1,22 @@ +package com.yahoo.vespa.jaxrs.client; + +import java.time.Duration; + +/** + * @author hakonhall + */ +public interface JaxRsTimeouts { + /** + * The connect timeout, which must be at least 1ms. Called once per real REST call. + * + * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. + */ + Duration getConnectTimeoutOrThrow(); + + /** + * The read timeout, which must be at least 1ms. Called once per real REST call. + * + * 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 9321f8e290d..8aa880fb0e4 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,34 +23,20 @@ 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(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS); + this(null, null, null); } 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); @@ -67,10 +53,16 @@ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { } @Override + public <T> T createClient(Params<T> 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> T createClient(Class<T> apiClass, HostName hostName, int port, String pathPrefix, String scheme) { UriBuilder uriBuilder = UriBuilder.fromPath(pathPrefix).host(hostName.s()).port(port).scheme(scheme); - WebTarget target = client.target(uriBuilder); - return WebResourceFactory.newResource(apiClass, target); + return createClient(new Params<>(apiClass, uriBuilder.build())); } - } 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 new file mode 100644 index 00000000000..3f2139f6bf0 --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java @@ -0,0 +1,26 @@ +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 65b302ef4ff..c964dfce2c7 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,7 +4,9 @@ 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; @@ -65,14 +67,25 @@ public class RetryingJaxRsStrategy<T> implements JaxRsStrategy<T> { @Override public <R> R apply(final Function<T, R> function) throws IOException { + return apply(function, new LegacyJaxRsTimeouts()); + } + + + @Override + public <R> R apply(final Function<T, R> function, JaxRsTimeouts timeouts) throws IOException { ProcessingException sampleException = null; for (int i = 0; i < maxIterations; ++i) { for (final HostName hostName : hostNames) { - final T jaxRsClient = jaxRsClientFactory.createClient(apiClass, hostName, port, pathPrefix, scheme); + URI uri = UriBuilder.fromPath(pathPrefix).port(port).scheme(scheme).host(hostName.s()).build(); + JaxRsClientFactory.Params<T> params = new JaxRsClientFactory.Params<>(apiClass, uri); + params.setConnectTimeout(timeouts.getConnectTimeoutOrThrow()); + params.setReadTimeout(timeouts.getReadTimeoutOrThrow()); + final T jaxRsClient = jaxRsClientFactory.createClient(params); try { return function.apply(jaxRsClient); } catch (ProcessingException e) { + // E.g. java.net.SocketTimeoutException thrown on read timeout is wrapped as a ProcessingException sampleException = e; logger.log(Level.INFO, "Failed REST API call to " + hostName + ":" + port + pathPrefix + " (in retry loop): " 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 63e2b814c24..8161602cdac 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,8 +74,7 @@ public class HttpPatchTest extends JerseyTest { final JaxRsStrategy<TestResourceApi> 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 e31920febd6..dbe886b7896 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,7 +5,10 @@ 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; @@ -15,24 +18,26 @@ 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.equalTo; -import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.hasItem; 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<JaxRsClientFactory.Params<TestJaxRsApi>> paramsCaptor; + @Path(API_PATH) private interface TestJaxRsApi { @GET @@ -53,8 +58,7 @@ public class RetryingJaxRsStrategyTest { @Before public void setup() { - when(jaxRsClientFactory.createClient(eq(TestJaxRsApi.class), any(HostName.class), anyInt(), anyString(), anyString())) - .thenReturn(mockApi); + when(jaxRsClientFactory.createClient(any())).thenReturn(mockApi); } @Test @@ -63,11 +67,12 @@ public class RetryingJaxRsStrategyTest { verify(mockApi, times(1)).doSomething(); - // Check that one of the supplied hosts is contacted. - final ArgumentCaptor<HostName> 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)); + verify(jaxRsClientFactory, times(1)).createClient(paramsCaptor.capture()); + JaxRsClientFactory.Params<TestJaxRsApi> 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()))); } @Test @@ -99,10 +104,10 @@ public class RetryingJaxRsStrategyTest { @Test public void testRetryLoopsOverAvailableServers() throws Exception { when(mockApi.doSomething()) - .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")) + .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")) .thenReturn("a response"); jaxRsStrategy.apply(TestJaxRsApi::doSomething); @@ -142,12 +147,9 @@ public class RetryingJaxRsStrategyTest { verifyAllServersContacted(jaxRsClientFactory); } - private static void verifyAllServersContacted( - final JaxRsClientFactory jaxRsClientFactory) { - final ArgumentCaptor<HostName> 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<HostName> actualServerHostsContacted = new HashSet<>(hostNameCaptor.getAllValues()); - assertThat(actualServerHostsContacted, equalTo(SERVER_HOSTS)); + private void verifyAllServersContacted(final JaxRsClientFactory jaxRsClientFactory) { + verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())).createClient(paramsCaptor.capture()); + final Set<JaxRsClientFactory.Params<TestJaxRsApi>> actualServerHostsContacted = new HashSet<>(paramsCaptor.getAllValues()); + assertEquals(actualServerHostsContacted.stream().map(x -> new HostName(x.uri().getHost())).collect(Collectors.toSet()), 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 c71b0783d69..70750dd6672 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,7 +5,6 @@ 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; @@ -46,11 +45,6 @@ public class OrchestratorMock implements Orchestrator { } @Override - public void suspendGroup(NodeGroup nodeGroup) { - nodeGroup.getHostNames().forEach(this::suspend); - } - - @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS; 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 603d6a1adac..9535096af4f 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,13 +22,9 @@ public interface HostSuspensionApi { String PATH_PREFIX = "/v1/suspensions/hosts"; /** - * Ask for permission to temporarily suspend all services on a set of hosts. + * Ask for permission to temporarily suspend all services on a set of hosts (nodes). * - * 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. + * See HostApi::suspend for semantics of suspending a node. */ @PUT @Path("/{hostname}") diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml index ae05a1908c9..37d308111ae 100644 --- a/orchestrator/pom.xml +++ b/orchestrator/pom.xml @@ -117,6 +117,12 @@ <scope>provided</scope> </dependency> <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>testutil</artifactId> + <version>${project.version}</version> + <scope>test</scope> + </dependency> + <dependency> <groupId>org.apache.curator</groupId> <artifactId>curator-test</artifactId> <scope>test</scope> 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 b2be4fe52ec..df124f2f690 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java @@ -75,15 +75,6 @@ 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. */ void suspendAll(HostName parentHostname, List<HostName> hostNames) 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 6577b4b96cc..f1f572621ce 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java @@ -1,40 +1,85 @@ // 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 the Orchestrator, e.g. timeout management. + * Context for an operation (or suboperation) of the Orchestrator that needs to pass through to the backend, + * e.g. timeout management and probing. * - * @author hakon + * @author hakonhall */ public class OrchestratorContext { - private static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(10); + 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 TIMEOUT_OVERHEAD = Duration.ofMillis(500); - private TimeBudget timeBudget; + private final Clock clock; + private final TimeBudget timeBudget; + private boolean probe; - public OrchestratorContext(Clock clock) { - this.timeBudget = TimeBudget.fromNow(clock, DEFAULT_TIMEOUT); + /** Create an OrchestratorContext for operations on multiple applications. */ + public static OrchestratorContext createContextForMultiAppOp(Clock clock) { + return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_BATCH_OP), true); } - /** Get the original timeout in seconds. */ - public long getOriginalTimeoutInSeconds() { - return timeBudget.originalTimeout().get().getSeconds(); + /** Create an OrchestratorContext for an operation on a single application. */ + public static OrchestratorContext createContextForSingleAppOp(Clock clock) { + return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_SINGLE_OP), true); } - /** - * 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); + 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() { + return new ClusterControllerClientTimeouts(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 OrchestratorContext to use within an application lock. */ + public OrchestratorContext createSubcontextWithinLock() { + // Move deadline towards past by a fixed amount to ensure there's time to process exceptions and + // access ZooKeeper before the lock times out. + TimeBudget subTimeBudget = timeBudget.withDeadline(timeBudget.deadline().get().minus(TIMEOUT_OVERHEAD)); + return new OrchestratorContext(clock, subTimeBudget, probe); + } + + /** Create an OrchestratorContext for an operation on a single application, but limited to current timeout. */ + public OrchestratorContext createSubcontextForSingleAppOp() { + 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; } - return shareOfRemaining * timeBudget.timeLeftOrThrow().get().toMillis() / 1000.0f; + return new OrchestratorContext( + clock, + TimeBudget.from(clock, now, Optional.of(Duration.between(now, deadline))), + probe); } } 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 ad8a35312e4..6811788ffb7 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java @@ -105,7 +105,9 @@ public class OrchestratorImpl implements Orchestrator { @Override public void setNodeStatus(HostName hostName, HostStatus status) throws OrchestrationException { ApplicationInstanceReference reference = getApplicationInstance(hostName).reference(); - try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly(reference)) { + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + try (MutableStatusRegistry statusRegistry = statusService + .lockApplicationInstance_forCurrentThreadOnly(reference, context.getTimeLeft())) { statusRegistry.setHostState(hostName, status); } } @@ -127,10 +129,10 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { final HostStatus currentHostState = statusRegistry.getHostStatus(hostName); if (HostStatus.NO_REMARKS == currentHostState) { @@ -139,7 +141,7 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(appInstance.reference()).getApplicationInstanceStatus(); if (appStatus == ApplicationInstanceStatus.NO_REMARKS) { - policy.releaseSuspensionGrant(context, appInstance, hostName, statusRegistry); + policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry); } } } @@ -148,7 +150,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(nodeGroup); + suspendGroup(OrchestratorContext.createContextForSingleAppOp(clock), nodeGroup); } @Override @@ -156,29 +158,32 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); NodeGroup nodeGroup = new NodeGroup(appInstance, hostName); - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { ApplicationApi applicationApi = new ApplicationApiImpl( nodeGroup, statusRegistry, clusterControllerClientFactory); - policy.acquirePermissionToRemove(context, applicationApi); + policy.acquirePermissionToRemove(context.createSubcontextWithinLock(), applicationApi); } } - // Public for testing purposes - @Override - public void suspendGroup(NodeGroup nodeGroup) throws HostStateChangeDeniedException, HostNameNotFoundException { + /** + * 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 { ApplicationInstanceReference applicationReference = nodeGroup.getApplicationReference(); - OrchestratorContext context = new OrchestratorContext(clock); try (MutableStatusRegistry hostStatusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( applicationReference, - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(applicationReference).getApplicationInstanceStatus(); if (appStatus == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { return; @@ -188,7 +193,7 @@ public class OrchestratorImpl implements Orchestrator { nodeGroup, hostStatusRegistry, clusterControllerClientFactory); - policy.grantSuspensionRequest(context, applicationApi); + policy.grantSuspensionRequest(context.createSubcontextWithinLock(), applicationApi); } } @@ -224,14 +229,12 @@ public class OrchestratorImpl implements Orchestrator { throw new BatchHostNameNotFoundException(parentHostname, hostNames, e); } + OrchestratorContext context = OrchestratorContext.createContextForMultiAppOp(clock); for (NodeGroup nodeGroup : nodeGroupsOrderedByApplication) { try { - suspendGroup(nodeGroup); + suspendGroup(context.createSubcontextForSingleAppOp(), 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); } @@ -301,12 +304,12 @@ public class OrchestratorImpl implements Orchestrator { private void setApplicationStatus(ApplicationId appId, ApplicationInstanceStatus status) throws ApplicationStateChangeDeniedException, ApplicationIdNotFoundException{ - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId, instanceLookupService); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appRef, - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { // Short-circuit if already in wanted state if (status == statusRegistry.getApplicationInstanceStatus()) return; @@ -321,7 +324,7 @@ public class OrchestratorImpl implements Orchestrator { // If the clustercontroller throws an error the nodes will be marked as allowed to be down // and be set back up on next resume invocation. - setClusterStateInController(context, application, ClusterControllerNodeState.MAINTENANCE); + setClusterStateInController(context.createSubcontextWithinLock(), application, ClusterControllerNodeState.MAINTENANCE); } statusRegistry.setApplicationInstanceStatus(status); 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 467b534f809..50ffe13b437 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,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.controller; +import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; @@ -15,23 +16,6 @@ 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<ClusterControllerJaxRsApi> clusterControllerApi; private final String clusterName; @@ -52,20 +36,22 @@ 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(); try { return clusterControllerApi.apply(api -> api.setNodeState( clusterName, storageNodeIndex, - context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), - stateRequest) - ); - } catch (IOException e) { + timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, + stateRequest), + timeouts); + } catch (IOException | UncheckedTimeoutException e) { String message = String.format( - "Giving up setting %s for storage node with index %d in cluster %s", + "Giving up setting %s for storage node with index %d in cluster %s: %s", stateRequest, storageNodeIndex, - clusterName); + clusterName, + e.getMessage()); throw new IOException(message, e); } @@ -79,16 +65,18 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ @Override public ClusterControllerStateResponse setApplicationState( OrchestratorContext context, - final ClusterControllerNodeState wantedState) throws IOException { - final ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); - final ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.FORCE); + 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(); try { return clusterControllerApi.apply(api -> api.setClusterState( clusterName, - context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), - stateRequest)); - } catch (IOException e) { + timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, + stateRequest), + timeouts); + } catch (IOException | UncheckedTimeoutException 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 new file mode 100644 index 00000000000..22f2440b408 --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java @@ -0,0 +1,74 @@ +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. + * + * <p>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.</p> + * + * @author hakonhall + */ +public class ClusterControllerClientTimeouts implements JaxRsTimeouts { + static final Duration CONNECT_TIMEOUT = Duration.ofMillis(100); + // Time reserved to guarantee that even though the server application honors a server timeout S, + // some time will pass before the server sees the timeout, and after it has returned. + static final Duration DOWNSTREAM_OVERHEAD = Duration.ofMillis(300); + // Minimum server-side timeout + static final Duration MIN_SERVER_TIMEOUT = Duration.ofMillis(100); + + private final TimeBudget timeBudget; + + /** + * 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 ones are down and/or not + * the master. + * + * @param timeBudget The time budget for a single logical call to the the Cluster Controller. + */ + public ClusterControllerClientTimeouts(TimeBudget timeBudget) { + this.timeBudget = timeBudget; + } + + @Override + public Duration getConnectTimeoutOrThrow() { + return CONNECT_TIMEOUT; + } + + @Override + public Duration getReadTimeoutOrThrow() { + Duration timeLeft = timeBudget.timeLeft().get(); + + // timeLeft = CONNECT_TIMEOUT + readTimeout + Duration readTimeout = timeLeft.minus(CONNECT_TIMEOUT); + + if (readTimeout.toMillis() <= 0) { + throw new UncheckedTimeoutException("Timed out after " + timeBudget.originalTimeout().get()); + } + + return readTimeout; + } + + public Duration getServerTimeoutOrThrow() { + // readTimeout = DOWNSTREAM_OVERHEAD + serverTimeout + Duration serverTimeout = getReadTimeoutOrThrow().minus(DOWNSTREAM_OVERHEAD); + + if (serverTimeout.toMillis() < MIN_SERVER_TIMEOUT.toMillis()) { + throw new UncheckedTimeoutException("Timed out after " + timeBudget.originalTimeout().get()); + } + + return serverTimeout; + } + +} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java new file mode 100644 index 00000000000..042eca0040e --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java @@ -0,0 +1,19 @@ +package com.yahoo.vespa.orchestrator.controller; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Error response from cluster controller. + * + * @author hakonhall + */ +public class ClusterControllerStateErrorResponse { + @JsonProperty("message") + public final String message; + + @JsonCreator + public ClusterControllerStateErrorResponse(@JsonProperty("message") String message) { + this.message = message; + } +} 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 5571eedeec6..33e74235862 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,9 +8,8 @@ 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 @@ -21,14 +20,12 @@ 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(CLUSTER_CONTROLLER_CONNECT_TIMEOUT_MS, CLUSTER_CONTROLLER_READ_TIMEOUT_MS)); + this(new JerseyJaxRsClientFactory()); } public RetryingClusterControllerClientFactory(JaxRsClientFactory jaxRsClientFactory) { @@ -36,19 +33,21 @@ public class RetryingClusterControllerClientFactory implements ClusterController } @Override - public ClusterControllerClient createClient(List<HostName> clusterControllers, - String clusterName) { - Set<HostName> clusterControllerSet = clusterControllers.stream().collect(Collectors.toSet()); - JaxRsStrategy<ClusterControllerJaxRsApi> 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); + public ClusterControllerClient createClient(List<HostName> clusterControllers, String clusterName) { + JaxRsStrategy<ClusterControllerJaxRsApi> 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. + // If there's only 1 CC, we'll try that one twice. + .setMaxIterations(clusterControllers.size() > 1 ? 1 : 2); 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 deleted file mode 100644 index 7459f0a6b11..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java +++ /dev/null @@ -1,54 +0,0 @@ -// 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<HostName> 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<ClusterControllerJaxRsApi> 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 ed506c82079..74b4b534acc 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<HostName> getHostNames() { - return hostNames.stream().collect(Collectors.toList()).stream().sorted().collect(Collectors.toList()); + return hostNames.stream().sorted().collect(Collectors.toList()); } public String toCommaSeparatedString() { diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java index 3e387012d2c..9900c8de752 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java @@ -12,10 +12,13 @@ import com.yahoo.vespa.orchestrator.OrchestratorContext; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClient; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory; import com.yahoo.vespa.orchestrator.controller.ClusterControllerNodeState; +import com.yahoo.vespa.orchestrator.controller.ClusterControllerStateErrorResponse; import com.yahoo.vespa.orchestrator.controller.ClusterControllerStateResponse; import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.policy.HostedVespaPolicy; +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Response; import java.io.IOException; import java.util.List; import java.util.Objects; @@ -75,6 +78,17 @@ public class StorageNodeImpl implements StorageNode { HostedVespaPolicy.CLUSTER_CONTROLLER_AVAILABLE_CONSTRAINT, "Failed to communicate with cluster controllers " + clusterControllers + ": " + e, e); + } catch (WebApplicationException e) { + Response webResponse = e.getResponse(); + // Response may contain detail message + ClusterControllerStateErrorResponse errorResponse = webResponse.readEntity(ClusterControllerStateErrorResponse.class); + String detail = errorResponse.message == null ? "" : ": " + errorResponse.message; + throw new HostStateChangeDeniedException( + hostName(), + HostedVespaPolicy.SET_NODE_STATE_CONSTRAINT, + "Failure from cluster controllers " + clusterControllers + " when setting node " + nodeIndex + + " in cluster " + clusterId + " to state " + wantedNodeState + detail, + e); } catch (UncheckedTimeoutException e) { throw new HostStateChangeDeniedException( hostName(), 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 c5ae553a98c..bd5eb6f3e29 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,6 +4,7 @@ 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; @@ -42,15 +43,11 @@ public class InMemoryStatusService implements StatusService { }; } - @Override - public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference) { - return lockApplicationInstance_forCurrentThreadOnly(applicationInstanceReference, 10); - } @Override public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - long timeoutSeconds) { + Duration timeout) { 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 c47be096242..76adef72b2b 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,6 +3,7 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; +import java.time.Duration; import java.util.Set; /** @@ -24,7 +25,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)}. + * {@link #lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference, Duration)}. */ ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference); @@ -52,12 +53,9 @@ 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, - long timeoutSeconds); + Duration timeout); /** * 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 deece6a4a65..7df29e038c1 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,21 +56,6 @@ 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<ApplicationInstanceReference> getAllSuspendedApplications() { try { @@ -93,13 +78,23 @@ 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, - long timeoutSeconds) { + Duration timeout) { String lockPath = applicationInstanceLock2Path(applicationInstanceReference); Lock lock = new Lock(lockPath, curator); - lock.acquire(Duration.ofSeconds(timeoutSeconds)); + lock.acquire(timeout); 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 76d9398c44e..80174d05a54 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java @@ -43,6 +43,8 @@ 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; @@ -245,6 +247,8 @@ 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( @@ -257,9 +261,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(DummyInstanceLookupService.TEST3_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST1_NODE_GROUP); + 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.verifyNoMoreInteractions(); } @@ -272,7 +276,7 @@ public class OrchestratorImplTest { DummyInstanceLookupService.TEST6_HOST_NAME, "some-constraint", "error message"); - doThrow(supensionFailure).when(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); + doThrow(supensionFailure).when(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); try { orchestrator.suspendAll( @@ -291,8 +295,8 @@ public class OrchestratorImplTest { } InOrder order = inOrder(orchestrator); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST3_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(any(), eq(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 228174a9b3d..35dda403aed 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,7 +6,8 @@ import com.yahoo.vespa.jaxrs.client.LocalPassThroughJaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; import org.junit.Test; -import static org.mockito.Matchers.anyFloat; +import java.time.Duration; + import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -28,7 +29,9 @@ public class ClusterControllerClientTest { final ClusterControllerNodeState wantedState = ClusterControllerNodeState.MAINTENANCE; OrchestratorContext context = mock(OrchestratorContext.class); - when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); + ClusterControllerClientTimeouts timeouts = mock(ClusterControllerClientTimeouts.class); + when(context.getClusterControllerTimeouts()).thenReturn(timeouts); + when(timeouts.getServerTimeoutOrThrow()).thenReturn(Duration.ofSeconds(1)); 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 new file mode 100644 index 00000000000..63b8e498f16 --- /dev/null +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java @@ -0,0 +1,91 @@ +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.DOWNSTREAM_OVERHEAD; +import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.MIN_SERVER_TIMEOUT; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +public class ClusterControllerClientTimeoutsTest { + // The minimum time that allows for a single RPC to CC. + private static final Duration MINIMUM_TIME_LEFT = CONNECT_TIMEOUT + .plus(DOWNSTREAM_OVERHEAD) + .plus(MIN_SERVER_TIMEOUT); + static { + assertEquals(Duration.ofMillis(500), MINIMUM_TIME_LEFT); + } + + 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(timeBudget); + } + + @Before + public void setUp() { + makeTimeouts(Duration.ofSeconds(3)); + } + + @Test + public void makesManyRequestsWithShortProcessingTime() { + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2900), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2600), timeouts.getServerTimeoutOrThrow()); + + clock.advance(Duration.ofMillis(100)); + + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2800), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2500), timeouts.getServerTimeoutOrThrow()); + + clock.advance(Duration.ofMillis(100)); + + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2700), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2400), timeouts.getServerTimeoutOrThrow()); + } + + @Test + public void alreadyTimedOut() { + clock.advance(Duration.ofSeconds(4)); + + try { + timeouts.getServerTimeoutOrThrow(); + fail(); + } catch (UncheckedTimeoutException e) { + assertEquals("Timed out after PT3S", 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("Timed out after PT3S", e.getMessage()); + } + } + + @Test + public void justEnoughTime() { + clock.advance(originalTimeout.minus(MINIMUM_TIME_LEFT)); + 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 new file mode 100644 index 00000000000..f47b43fa27b --- /dev/null +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java @@ -0,0 +1,54 @@ +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<HostName> 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<ClusterControllerStateRequest> requestCaptor = ArgumentCaptor.forClass(ClusterControllerStateRequest.class); + + verify(api, times(1)).setNodeState(eq(clusterName), eq(storageNode), eq(9.6f), requestCaptor.capture()); + ClusterControllerStateRequest request = requestCaptor.getValue(); + assertEquals(ClusterControllerStateRequest.Condition.SAFE, request.condition); + Map<String, Object> 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 deleted file mode 100644 index 4dabae14add..00000000000 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java +++ /dev/null @@ -1,116 +0,0 @@ -// 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<HostName> clusterControllers = Arrays.asList(); - - try { - clientFactory.createClient(clusterControllers, "clusterName"); - fail(); - } catch (IllegalArgumentException e) { - // As expected. - } - } - - @Test - public void testCreateClientWithSingleClusterControllerInstance() throws Exception { - final List<HostName> 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<HostName> clusterControllers = Arrays.asList(); - - try { - clientFactory.createClient(clusterControllers, "clusterName"); - fail(); - } catch (IllegalArgumentException e) { - // As expected. - } - } - - @Test - public void testCreateClientWithThreeClusterControllerInstances() throws Exception { - final List<HostName> 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 45ba862c8f1..a9b8127e7fe 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,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.resources; -import com.yahoo.jdisc.Timer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -32,6 +31,7 @@ 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,6 +41,7 @@ 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; @@ -74,7 +75,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))) + when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(eq(APPLICATION_INSTANCE_REFERENCE), any())) .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); when(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY.getHostStatus(any())) .thenReturn(HostStatus.NO_REMARKS); @@ -150,6 +151,11 @@ 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 2e914718e20..44847666670 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,6 +17,7 @@ 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; @@ -81,7 +82,8 @@ public class ZookeeperStatusServiceTest { @Test public void setting_host_state_is_idempotent() { try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, + Duration.ofSeconds(10))) { //shuffling to catch "clean database" failures for all cases. for (HostStatus hostStatus: shuffledList(HostStatus.values())) { @@ -105,11 +107,12 @@ public class ZookeeperStatusServiceTest { final CompletableFuture<Void> lockedSuccessfullyFuture; try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, + Duration.ofSeconds(10))) { lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> { try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2 - .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE)) + .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { } }); @@ -131,13 +134,13 @@ public class ZookeeperStatusServiceTest { ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { //must run in separate thread, since having 2 locks in the same thread fails CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> { try { zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE,1); + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(1)); fail("Both zookeeper host status services locked simultaneously for the same application instance"); } catch (RuntimeException e) { } @@ -211,7 +214,7 @@ public class ZookeeperStatusServiceTest { // Suspend try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } @@ -223,7 +226,7 @@ public class ZookeeperStatusServiceTest { // Resume try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.NO_REMARKS); } @@ -241,12 +244,12 @@ public class ZookeeperStatusServiceTest { assertThat(suspendedApps.size(), is(0)); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE2)) { + TestIds.APPLICATION_INSTANCE_REFERENCE2, Duration.ofSeconds(10))) { 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 a7963df0208..b3750440493 100644 --- a/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java +++ b/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java @@ -66,6 +66,23 @@ public class TimeBudget { }); } + /** Returns the time left, possibly negative if the deadline has passed. */ + public Optional<Duration> timeLeft() { + return timeout.map(timeout -> timeout.minus(timePassed())); + } + + /** Returns the time left as a new TimeBudget. */ + public TimeBudget timeLeftAsTimeBudget() { + Instant now = clock.instant(); + Optional<Instant> deadline = deadline(); + return new TimeBudget(clock, now, deadline.map(d -> Duration.between(now, d))); + } + + /** Returns a new TimeBudget with the same clock and start, but with this deadline. */ + public TimeBudget withDeadline(Instant deadline) { + return new TimeBudget(clock, start, Optional.of(Duration.between(start, deadline))); + } + private static Duration nonNegativeBetween(Instant start, Instant end) { return makeNonNegative(Duration.between(start, end)); } |