summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorBjørn Christian Seime <bjorncs@verizonmedia.com>2020-11-06 11:28:42 +0100
committerBjørn Christian Seime <bjorncs@verizonmedia.com>2020-11-09 16:03:48 +0100
commit6a419fc0d2bed5e49f89990ef4bc647cf50d1f08 (patch)
tree7cf1abd9884af3690ba4a4253b29ea12acca92e0
parent14a69660ed619b5ea5a3c483f9e90d72d3a5e17d (diff)
Rewrite ConfigConvergenceChecker to use Apache instead of Jersey client
The effective exception handling for some confiserver APIs may have changed as WebApplicationException leaked out with old Jersey-based implementation. Connections will now be reused for a short duration.
-rw-r--r--configserver/pom.xml5
-rw-r--r--configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java156
-rw-r--r--configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java17
-rw-r--r--configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java19
4 files changed, 105 insertions, 92 deletions
diff --git a/configserver/pom.xml b/configserver/pom.xml
index 8cd1b4b4254..43187038f97 100644
--- a/configserver/pom.xml
+++ b/configserver/pom.xml
@@ -220,6 +220,11 @@
<scope>test</scope>
</dependency>
<dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
<groupId>com.yahoo.vespa</groupId>
<artifactId>http-utils</artifactId>
<version>${project.version}</version>
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java
index 6b316c06b54..e89b9685a06 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java
@@ -1,27 +1,28 @@
// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
package com.yahoo.vespa.config.server.application;
-import ai.vespa.util.http.VespaClientBuilderFactory;
+import ai.vespa.util.http.VespaHttpClientBuilder;
import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.inject.Inject;
import com.yahoo.component.AbstractComponent;
import com.yahoo.config.model.api.HostInfo;
import com.yahoo.config.model.api.PortInfo;
import com.yahoo.config.model.api.ServiceInfo;
-import java.util.logging.Level;
+import com.yahoo.log.LogLevel;
import com.yahoo.slime.Cursor;
import com.yahoo.vespa.config.server.http.JSONResponse;
-import org.glassfish.jersey.client.ClientProperties;
-import org.glassfish.jersey.client.proxy.WebResourceFactory;
-
-import javax.ws.rs.GET;
-import javax.ws.rs.Path;
-import javax.ws.rs.ProcessingException;
-import javax.ws.rs.client.Client;
-import javax.ws.rs.client.ClientRequestFilter;
-import javax.ws.rs.client.WebTarget;
-import javax.ws.rs.core.HttpHeaders;
+import org.apache.http.HttpStatus;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.net.URI;
+import java.net.URISyntaxException;
import java.time.Duration;
import java.util.ArrayList;
import java.util.LinkedHashMap;
@@ -29,6 +30,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.TimeUnit;
import java.util.logging.Logger;
import java.util.stream.Collectors;
@@ -42,12 +44,11 @@ import static com.yahoo.config.model.api.container.ContainerServiceType.QRSERVER
*
* @author Ulf Lilleengen
* @author hmusum
+ * @author bjorncs
*/
public class ConfigConvergenceChecker extends AbstractComponent {
private static final Logger log = Logger.getLogger(ConfigConvergenceChecker.class.getName());
- private static final String statePath = "/state/v1/";
- private static final String configSubPath = "config";
private final static Set<String> serviceTypesToCheck = Set.of(
CONTAINER.serviceName,
QRSERVER.serviceName,
@@ -58,16 +59,12 @@ public class ConfigConvergenceChecker extends AbstractComponent {
"distributor"
);
- private final StateApiFactory stateApiFactory;
- private final VespaClientBuilderFactory clientBuilderFactory = new VespaClientBuilderFactory();
+ private final CloseableHttpClient httpClient;
+ private final ObjectMapper jsonMapper = new ObjectMapper();
@Inject
public ConfigConvergenceChecker() {
- this(ConfigConvergenceChecker::createStateApi);
- }
-
- public ConfigConvergenceChecker(StateApiFactory stateApiFactory) {
- this.stateApiFactory = stateApiFactory;
+ this.httpClient = createHttpClient();
}
/** Fetches the active config generation for all services in the given application. */
@@ -99,7 +96,7 @@ public class ConfigConvergenceChecker extends AbstractComponent {
long currentGeneration = getServiceGeneration(URI.create("http://" + hostAndPortToCheck), timeout);
boolean converged = currentGeneration >= wantedGeneration;
return ServiceResponse.createOkResponse(requestUrl, hostAndPortToCheck, wantedGeneration, currentGeneration, converged);
- } catch (ProcessingException e) { // e.g. if we cannot connect to the service to find generation
+ } catch (NonSuccessStatusCodeException | IOException e) { // e.g. if we cannot connect to the service to find generation
return ServiceResponse.createNotFoundResponse(requestUrl, hostAndPortToCheck, wantedGeneration, e.getMessage());
} catch (Exception e) {
return ServiceResponse.createErrorResponse(requestUrl, hostAndPortToCheck, wantedGeneration, e.getMessage());
@@ -108,46 +105,47 @@ public class ConfigConvergenceChecker extends AbstractComponent {
@Override
public void deconstruct() {
- clientBuilderFactory.close();
- }
-
- @Path(statePath)
- public interface StateApi {
- @Path(configSubPath)
- @GET
- JsonNode config();
- }
-
- public interface StateApiFactory {
- StateApi createStateApi(Client client, URI serviceUri);
+ try {
+ httpClient.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
}
/** Gets service generation for a list of services (in parallel). */
private Map<ServiceInfo, Long> getServiceGenerations(List<ServiceInfo> services, Duration timeout) {
return services.parallelStream()
- .collect(Collectors.toMap(service -> service,
- service -> {
- try {
- return getServiceGeneration(URI.create("http://" + service.getHostName()
- + ":" + getStatePort(service).get()), timeout);
- }
- catch (ProcessingException e) { // Cannot connect to service to determine service generation
- return -1L;
- }
- },
- (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); },
- LinkedHashMap::new
- ));
+ .collect(Collectors.toMap(
+ service -> service,
+ service -> {
+ try {
+ return getServiceGeneration(URI.create("http://" + service.getHostName()
+ + ":" + getStatePort(service).get()), timeout);
+ } catch (IOException | NonSuccessStatusCodeException e) {
+ return -1L;
+ }
+ },
+ (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); },
+ LinkedHashMap::new
+ ));
}
/** Get service generation of service at given URL */
- private long getServiceGeneration(URI serviceUrl, Duration timeout) {
- Client client = createClient(timeout);
- try {
- StateApi state = stateApiFactory.createStateApi(client, serviceUrl);
- return generationFromContainerState(state.config());
- } finally {
- client.close();
+ private long getServiceGeneration(URI serviceUrl, Duration timeout) throws IOException, NonSuccessStatusCodeException {
+ HttpGet request = new HttpGet(createApiUri(serviceUrl));
+ request.setConfig(createRequestConfig(timeout));
+ try (CloseableHttpResponse response = httpClient.execute(request)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ if (statusCode != HttpStatus.SC_OK) throw new NonSuccessStatusCodeException(statusCode);
+ if (response.getEntity() == null) throw new IOException("Response has no content");
+ JsonNode jsonContent = jsonMapper.readTree(response.getEntity().getContent());
+ return generationFromContainerState(jsonContent);
+ } catch (Exception e) {
+ log.log(
+ LogLevel.DEBUG,
+ e,
+ () -> String.format("Failed to retrieve service config generation for '%s': %s", serviceUrl, e.getMessage()));
+ throw e;
}
}
@@ -166,16 +164,6 @@ public class ConfigConvergenceChecker extends AbstractComponent {
return false;
}
- private Client createClient(Duration timeout) {
- return clientBuilderFactory.newBuilder()
- .register(
- (ClientRequestFilter) ctx ->
- ctx.getHeaders().put(HttpHeaders.USER_AGENT, List.of("config-convergence-checker")))
- .property(ClientProperties.CONNECT_TIMEOUT, (int) timeout.toMillis())
- .property(ClientProperties.READ_TIMEOUT, (int) timeout.toMillis())
- .build();
- }
-
private static Optional<Integer> getStatePort(ServiceInfo service) {
return service.getPorts().stream()
.filter(port -> port.getTags().contains("state"))
@@ -187,9 +175,43 @@ public class ConfigConvergenceChecker extends AbstractComponent {
return state.get("config").get("generation").asLong(-1);
}
- private static StateApi createStateApi(Client client, URI uri) {
- WebTarget target = client.target(uri);
- return WebResourceFactory.newResource(StateApi.class, target);
+ private static URI createApiUri(URI serviceUrl) {
+ try {
+ return new URIBuilder(serviceUrl)
+ .setPath("/state/v1/config")
+ .build();
+ } catch (URISyntaxException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static CloseableHttpClient createHttpClient() {
+ return VespaHttpClientBuilder
+ .create()
+ .setUserAgent("config-convergence-checker")
+ .setConnectionTimeToLive(20, TimeUnit.SECONDS)
+ .setMaxConnPerRoute(4)
+ .setMaxConnTotal(100)
+ .setDefaultRequestConfig(createRequestConfig(Duration.ofSeconds(10)))
+ .build();
+ }
+
+ private static RequestConfig createRequestConfig(Duration timeout) {
+ int timeoutMillis = (int)timeout.toMillis();
+ return RequestConfig.custom()
+ .setConnectionRequestTimeout(timeoutMillis)
+ .setConnectTimeout(timeoutMillis)
+ .setSocketTimeout(timeoutMillis)
+ .build();
+ }
+
+ private static class NonSuccessStatusCodeException extends Exception {
+ final int statusCode;
+
+ NonSuccessStatusCodeException(int statusCode) {
+ super("Expected status code 200, got " + statusCode);
+ this.statusCode = statusCode;
+ }
}
private static class ServiceListResponse extends JSONResponse {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java
index 6aeb774d2b0..1630e965771 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java
@@ -2,12 +2,12 @@
package com.yahoo.vespa.config.server.application;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
+import com.yahoo.component.Version;
import com.yahoo.config.model.api.Model;
import com.yahoo.config.provision.ApplicationId;
import com.yahoo.config.provision.ApplicationName;
import com.yahoo.config.provision.InstanceName;
import com.yahoo.config.provision.TenantName;
-import com.yahoo.component.Version;
import com.yahoo.container.jdisc.HttpResponse;
import com.yahoo.slime.Slime;
import com.yahoo.slime.SlimeUtils;
@@ -31,8 +31,8 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.okJson;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
/**
* @author Ulf Lilleengen
@@ -184,10 +184,15 @@ public class ConfigConvergenceCheckerTest {
.withBody("response too slow")));
HttpResponse response = checker.getServiceConfigGenerationResponse(application, hostAndPort(service), requestUrl, Duration.ofMillis(1));
// Message contained in a SocketTimeoutException may differ across platforms, so we do a partial match of the response here
- assertResponse((responseBody) -> assertTrue("Response matches", responseBody.startsWith(
- "{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) +
- "\",\"wantedGeneration\":3,\"error\":\"java.net.SocketTimeoutException") &&
- responseBody.endsWith("\"}")), 404, response);
+ assertResponse(
+ responseBody ->
+ assertThat(responseBody)
+ .startsWith("{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) +
+ "\",\"wantedGeneration\":3,\"error\":\"")
+ .contains("timed out")
+ .endsWith("\"}"),
+ 404,
+ response);
}
private URI testServer() {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
index 7276091fed0..06a455954ac 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
@@ -1,7 +1,6 @@
// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
package com.yahoo.vespa.config.server.http.v2;
-import com.fasterxml.jackson.databind.ObjectMapper;
import com.yahoo.cloud.config.ConfigserverConfig;
import com.yahoo.component.Version;
import com.yahoo.config.model.api.ModelFactory;
@@ -18,7 +17,6 @@ import com.yahoo.vespa.config.server.MockLogRetriever;
import com.yahoo.vespa.config.server.MockProvisioner;
import com.yahoo.vespa.config.server.MockTesterClient;
import com.yahoo.vespa.config.server.TestComponentRegistry;
-import com.yahoo.vespa.config.server.application.ConfigConvergenceChecker;
import com.yahoo.vespa.config.server.application.HttpProxy;
import com.yahoo.vespa.config.server.application.OrchestratorMock;
import com.yahoo.vespa.config.server.deploy.DeployTester;
@@ -37,12 +35,10 @@ import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
-import javax.ws.rs.client.Client;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
-import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.List;
@@ -425,21 +421,6 @@ public class ApplicationHandlerTest {
return createApplicationHandler().handle(HttpRequest.createTestRequest(restartUrl, GET));
}
- private static class MockStateApiFactory implements ConfigConvergenceChecker.StateApiFactory {
- boolean createdApi = false;
- @Override
- public ConfigConvergenceChecker.StateApi createStateApi(Client client, URI serviceUri) {
- createdApi = true;
- return () -> {
- try {
- return new ObjectMapper().readTree("{\"config\":{\"generation\":1}}");
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- };
- }
- }
-
private ApplicationHandler createApplicationHandler() {
return createApplicationHandler(applicationRepository);
}