diff options
author | HÃ¥kon Hallingstad <hakon.hallingstad@gmail.com> | 2019-11-18 09:45:44 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-11-18 09:45:44 +0100 |
commit | 9e419119abc9eaac1d0fd922a736d0dd47aa18c2 (patch) | |
tree | f312893255081a6c6101cd6e6bc9e3ac2d1c41de /node-admin/src | |
parent | 927de6064fa5e15f106fe21c2b6ab33195d857a8 (diff) |
Revert "Reduce timeouts for non-suspend ConfigServer REST API calls from node admin"
Diffstat (limited to 'node-admin/src')
4 files changed, 44 insertions, 82 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java index e6d719e66bb..9b496526804 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApi.java @@ -1,7 +1,6 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.configserver; -import java.time.Duration; import java.util.Optional; /** @@ -10,17 +9,6 @@ import java.util.Optional; * @author freva */ public interface ConfigServerApi extends AutoCloseable { - class Params { - private Optional<Duration> connectionTimeout; - - /** Set the socket connect and read timeouts. */ - public Params setConnectionTimeout(Duration connectionTimeout) { - this.connectionTimeout = Optional.of(connectionTimeout); - return this; - } - - public Optional<Duration> getConnectionTimeout() { return connectionTimeout; } - } <T> T get(String path, Class<T> wantedReturnType); @@ -28,8 +16,6 @@ public interface ConfigServerApi extends AutoCloseable { <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType); - <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType, Params params); - <T> T patch(String path, Object bodyJsonPojo, Class<T> wantedReturnType); <T> T delete(String path, Class<T> wantedReturnType); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java index 53c4bac5f6c..4dadcb359ea 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java @@ -135,14 +135,8 @@ public class ConfigServerApiImpl implements ConfigServerApi { @Override public <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType) { - return put(path, bodyJsonPojo, wantedReturnType, null); - } - - @Override - public <T> T put(String path, Optional<Object> bodyJsonPojo, Class<T> wantedReturnType, Params paramsOrNull) { return tryAllConfigServers(configServer -> { HttpPut put = new HttpPut(configServer.resolve(path)); - setRequestConfigOverride(paramsOrNull, put); setContentTypeToApplicationJson(put); if (bodyJsonPojo.isPresent()) { put.setEntity(new StringEntity(mapper.writeValueAsString(bodyJsonPojo.get()))); @@ -209,34 +203,23 @@ public class ConfigServerApiImpl implements ConfigServerApi { cm.setMaxTotal(200); // Increase max total connections to 200, which should be enough // Have experienced hang in socket read, which may have been because of - // system defaults, therefore set explicit timeouts. - RequestConfig defaultRequestConfig = RequestConfig.custom() - .setConnectionRequestTimeout(1_000) // connection from connection manager - .setConnectTimeout(10_000) // establishment of connection - .setSocketTimeout(10_000) // waiting for data + // system defaults, therefore set explicit timeouts. Set arbitrarily to + // 15s > 10s used by Orchestrator lock timeout. + int timeoutMs = 15_000; + RequestConfig requestBuilder = RequestConfig.custom() + .setConnectTimeout(timeoutMs) // establishment of connection + .setConnectionRequestTimeout(timeoutMs) // connection from connection manager + .setSocketTimeout(timeoutMs) // waiting for data .build(); return HttpClientBuilder.create() - .setDefaultRequestConfig(defaultRequestConfig) + .setDefaultRequestConfig(requestBuilder) .disableAutomaticRetries() .setUserAgent("node-admin") .setConnectionManager(cm) .build(); } - private static void setRequestConfigOverride(Params paramsOrNull, HttpRequestBase request) { - if (paramsOrNull == null) return; - - RequestConfig.Builder builder = RequestConfig.copy(request.getConfig()); - - paramsOrNull.getConnectionTimeout().ifPresent(connectionTimeout -> { - builder.setConnectTimeout((int) connectionTimeout.toMillis()); - builder.setSocketTimeout((int) connectionTimeout.toMillis()); - }); - - request.setConfig(builder.build()); - } - // Shuffle config server URIs to balance load private static List<URI> randomizeConfigServerUris(Collection<URI> configServerUris) { List<URI> shuffledConfigServerHosts = new ArrayList<>(configServerUris); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java index 20c0604b5dc..353abd64778 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java @@ -10,7 +10,6 @@ import com.yahoo.vespa.orchestrator.restapi.HostSuspensionApi; import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult; import com.yahoo.vespa.orchestrator.restapi.wire.UpdateHostResponse; -import java.time.Duration; import java.util.List; import java.util.Optional; @@ -20,15 +19,6 @@ import java.util.Optional; * @author dybis */ public class OrchestratorImpl implements Orchestrator { - // The server-side Orchestrator has an internal timeout of 10s. - // - // Note: A 409 has been observed to be returned after 33s in a case possibly involving - // zk leader election (which is unfortunate as it is difficult to differentiate between - // transient timeouts (do not allow suspend on timeout) and the config server being - // permanently down (allow suspend)). For now we'd like to investigate such long - // requests so keep the timeout low(er). - private static final Duration CONNECTION_TIMEOUT = Duration.ofSeconds(15); - // TODO: Find a way to avoid duplicating this (present in orchestrator's services.xml also). private static final String ORCHESTRATOR_PATH_PREFIX = "/orchestrator"; static final String ORCHESTRATOR_PATH_PREFIX_HOST_API @@ -46,8 +36,9 @@ public class OrchestratorImpl implements Orchestrator { public void suspend(final String hostName) { UpdateHostResponse response; try { - var params = new ConfigServerApi.Params().setConnectionTimeout(CONNECTION_TIMEOUT); - response = configServerApi.put(getSuspendPath(hostName), Optional.empty(), UpdateHostResponse.class, params); + response = configServerApi.put(getSuspendPath(hostName), + Optional.empty(), /* body */ + UpdateHostResponse.class); } catch (HttpException.NotFoundException n) { throw new OrchestratorNotFoundException("Failed to suspend " + hostName + ", host not found"); } catch (HttpException e) { @@ -67,11 +58,10 @@ public class OrchestratorImpl implements Orchestrator { public void suspend(String parentHostName, List<String> hostNames) { final BatchOperationResult batchOperationResult; try { - var params = new ConfigServerApi.Params().setConnectionTimeout(CONNECTION_TIMEOUT); - String hostnames = String.join("&hostname=", hostNames); + String params = String.join("&hostname=", hostNames); String url = String.format("%s/%s?hostname=%s", ORCHESTRATOR_PATH_PREFIX_HOST_SUSPENSION_API, - parentHostName, hostnames); - batchOperationResult = configServerApi.put(url, Optional.empty(), BatchOperationResult.class, params); + parentHostName, params); + batchOperationResult = configServerApi.put(url, Optional.empty(), BatchOperationResult.class); } catch (HttpException e) { throw new OrchestratorException("Failed to batch suspend for " + parentHostName + ": " + e.toString()); } catch (ConnectionException e) { diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImplTest.java index 936a7bb224d..e4c46c504be 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImplTest.java @@ -12,7 +12,6 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -30,10 +29,9 @@ public class OrchestratorImplTest { @Test public void testSuspendCall() { when(configServerApi.put( - eq(OrchestratorImpl.ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName+ "/suspended"), - eq(Optional.empty()), - eq(UpdateHostResponse.class), - any() + OrchestratorImpl.ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName+ "/suspended", + Optional.empty(), + UpdateHostResponse.class )).thenReturn(new UpdateHostResponse(hostName, null)); orchestrator.suspend(hostName); @@ -42,10 +40,9 @@ public class OrchestratorImplTest { @Test(expected=OrchestratorException.class) public void testSuspendCallWithFailureReason() { when(configServerApi.put( - eq(OrchestratorImpl.ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName+ "/suspended"), - eq(Optional.empty()), - eq(UpdateHostResponse.class), - any() + OrchestratorImpl.ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName+ "/suspended", + Optional.empty(), + UpdateHostResponse.class )).thenReturn(new UpdateHostResponse(hostName, new HostStateChangeDenialReason("hostname", "fail"))); orchestrator.suspend(hostName); @@ -53,16 +50,22 @@ public class OrchestratorImplTest { @Test(expected=OrchestratorNotFoundException.class) public void testSuspendCallWithNotFound() { - when(configServerApi.put(any(String.class), any(), any(), any())) - .thenThrow(new HttpException.NotFoundException("Not Found")); + when(configServerApi.put( + any(String.class), + any(), + any() + )).thenThrow(new HttpException.NotFoundException("Not Found")); orchestrator.suspend(hostName); } @Test(expected=RuntimeException.class) public void testSuspendCallWithSomeOtherException() { - when(configServerApi.put(any(String.class), any(), any(), any())) - .thenThrow(new RuntimeException("Some parameter was wrong")); + when(configServerApi.put( + any(String.class), + any(), + any() + )).thenThrow(new RuntimeException("Some parameter was wrong")); orchestrator.suspend(hostName); } @@ -100,8 +103,11 @@ public class OrchestratorImplTest { @Test(expected=RuntimeException.class) public void testResumeCallWithSomeOtherException() { - when(configServerApi.put(any(String.class), any(), any(), any())) - .thenThrow(new RuntimeException("Some parameter was wrong")); + when(configServerApi.put( + any(String.class), + any(), + any() + )).thenThrow(new RuntimeException("Some parameter was wrong")); orchestrator.suspend(hostName); } @@ -112,10 +118,9 @@ public class OrchestratorImplTest { List<String> hostNames = Arrays.asList("a1.host1.test.yahoo.com", "a2.host1.test.yahoo.com"); when(configServerApi.put( - eq("/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com"), - eq(Optional.empty()), - eq(BatchOperationResult.class), - any() + "/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com", + Optional.empty(), + BatchOperationResult.class )).thenReturn(BatchOperationResult.successResult()); orchestrator.suspend(parentHostName, hostNames); @@ -128,10 +133,9 @@ public class OrchestratorImplTest { String failureReason = "Failed to suspend"; when(configServerApi.put( - eq("/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com"), - eq(Optional.empty()), - eq(BatchOperationResult.class), - any() + "/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com", + Optional.empty(), + BatchOperationResult.class )).thenReturn(new BatchOperationResult(failureReason)); orchestrator.suspend(parentHostName, hostNames); @@ -144,10 +148,9 @@ public class OrchestratorImplTest { String exceptionMessage = "Exception: Something crashed!"; when(configServerApi.put( - eq("/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com"), - eq(Optional.empty()), - eq(BatchOperationResult.class), - any() + "/orchestrator/v1/suspensions/hosts/host1.test.yahoo.com?hostname=a1.host1.test.yahoo.com&hostname=a2.host1.test.yahoo.com", + Optional.empty(), + BatchOperationResult.class )).thenThrow(new RuntimeException(exceptionMessage)); orchestrator.suspend(parentHostName, hostNames); |