diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2021-04-27 12:47:42 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-04-27 12:47:42 +0200 |
commit | 33b310ebd7fc46564ff6d9c15dcc0a559f7e34ff (patch) | |
tree | 696e03e557c9ee33995b1aca5120ac74556264b2 | |
parent | 98adc735ed9ab9acb956bbd138f072b761c60c01 (diff) | |
parent | 29234349cc188aa8f7fc948b3208d83a87f461e3 (diff) |
Merge pull request #17611 from vespa-engine/jonmv/reapply-apache-client
Jonmv/reapply apache client
26 files changed, 346 insertions, 200 deletions
diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java index d467402f79f..2e5e445d63a 100644 --- a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java +++ b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java @@ -9,11 +9,12 @@ import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpEntity; import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.ParseException; +import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.HttpEntities; import org.apache.hc.core5.net.URIBuilder; import java.io.IOException; -import java.io.InputStream; import java.io.UncheckedIOException; import java.net.URI; import java.net.URISyntaxException; @@ -21,7 +22,6 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; -import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; import java.util.function.Function; @@ -104,7 +104,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override - public RequestBuilder send(HostStrategy hosts, Method method) { + public ConfigServerClient.RequestBuilder send(HostStrategy hosts, Method method) { return new RequestBuilder(hosts, method); } @@ -114,10 +114,11 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { private final Method method; private final HostStrategy hosts; private final URIBuilder uriBuilder = new URIBuilder(); + private final List<String> pathSegments = new ArrayList<>(); private HttpEntity entity; private RequestConfig config = ConfigServerClient.defaultRequestConfig; - private BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler = ConfigServerClient::throwOnError; - private Consumer<IOException> catcher = ConfigServerClient::retryAll; + private ResponseVerifier verifier = ConfigServerClient.throwOnError; + private Consumer<IOException> catcher = ConfigServerClient.retryAll; private RequestBuilder(HostStrategy hosts, Method method) { if ( ! hosts.iterator().hasNext()) @@ -129,7 +130,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { @Override public RequestBuilder at(List<String> pathSegments) { - uriBuilder.setPathSegments(requireNonNull(pathSegments)); + this.pathSegments.addAll(pathSegments); return this; } @@ -145,12 +146,23 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override + public ConfigServerClient.RequestBuilder emptyParameters(List<String> keys) { + for (String key : keys) + uriBuilder.setParameter(key, null); + + return this; + } + + @Override public RequestBuilder parameters(List<String> pairs) { if (pairs.size() % 2 != 0) throw new IllegalArgumentException("Must supply parameter key/values in pairs"); - for (int i = 0; i < pairs.size(); ) - uriBuilder.setParameter(pairs.get(i++), pairs.get(i++)); + for (int i = 0; i < pairs.size(); ) { + String key = pairs.get(i++), value = pairs.get(i++); + if (value != null) + uriBuilder.setParameter(key, value); + } return this; } @@ -175,56 +187,54 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override - public RequestBuilder handling(BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler) { - this.handler = requireNonNull(handler); + public RequestBuilder throwing(ResponseVerifier verifier) { + this.verifier = requireNonNull(verifier); return this; } @Override - public <T> T read(Function<byte[], T> mapper) { - return mapIfSuccess(input -> { - try (input) { - return mapper.apply(input.readAllBytes()); + public String read() { + return handle((response, __) -> { + try (response) { + return response.getEntity() == null ? "" : EntityUtils.toString(response.getEntity()); } - catch (IOException e) { - throw new RetryException(e); + catch (ParseException e) { + throw new IllegalStateException(e); // This isn't actually thrown by apache >_< + } + }); + } + + @Override + public <T> T read(Function<byte[], T> mapper) { + return handle((response, __) -> { + try (response) { + return mapper.apply(response.getEntity() == null ? new byte[0] : EntityUtils.toByteArray(response.getEntity())); } }); } @Override public void discard() throws UncheckedIOException, ResponseException { - mapIfSuccess(input -> { - try (input) { + handle((response, __) -> { + try (response) { return null; } - catch (IOException e) { - throw new RetryException(e); - } }); } @Override - public InputStream stream() throws UncheckedIOException, ResponseException { - return mapIfSuccess(input -> input); + public HttpInputStream stream() throws UncheckedIOException, ResponseException { + return handle((response, __) -> new HttpInputStream(response)); } - /** Returns the mapped body, if successful, retrying any IOException. The caller must close the body stream. */ - private <T> T mapIfSuccess(Function<InputStream, T> mapper) { + @Override + public <T> T handle(ResponseHandler<T> handler) { + uriBuilder.setPathSegments(pathSegments); return execute(this, (response, request) -> { try { - handler.accept(response, request); // This throws on unacceptable responses. - - InputStream body = response.getEntity() != null ? response.getEntity().getContent() - : InputStream.nullInputStream(); - return mapper.apply(new ForwardingInputStream(body) { - @Override - public void close() throws IOException { - super.close(); - response.close(); - } - }); + verifier.verify(response, request); // This throws on unacceptable responses. + return handler.handle(response, request); } catch (IOException | RuntimeException | Error e) { try { @@ -247,6 +257,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } + @SuppressWarnings("unchecked") private static <T extends Throwable> void sneakyThrow(Throwable t) throws T { throw (T) t; diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java index c0f72378c1b..c92acd7cd0b 100644 --- a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java +++ b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java @@ -7,10 +7,10 @@ import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.HttpEntity; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; -import org.apache.hc.core5.http.ParseException; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.util.Timeout; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.io.UncheckedIOException; @@ -20,18 +20,18 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.IntStream; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.toUnmodifiableList; /** * @author jonmv */ -public interface ConfigServerClient extends AutoCloseable { +public interface ConfigServerClient extends Closeable { RequestConfig defaultRequestConfig = RequestConfig.custom() .setConnectionRequestTimeout(Timeout.ofSeconds(5)) @@ -40,20 +40,12 @@ public interface ConfigServerClient extends AutoCloseable { .build(); /** Wraps with a {@link RetryException} and rethrows. */ - static void retryAll(IOException e) { + Consumer<IOException> retryAll = (e) -> { throw new RetryException(e); - } + }; /** Throws a a {@link RetryException} if {@code statusCode == 503}, or a {@link ResponseException} unless {@code 200 <= statusCode < 300}. */ - static void throwOnError(ClassicHttpResponse response, ClassicHttpRequest request) { - if (response.getCode() < HttpStatus.SC_OK || response.getCode() >= HttpStatus.SC_REDIRECTION) { - ResponseException e = ResponseException.of(response, request); - if (response.getCode() == HttpStatus.SC_SERVICE_UNAVAILABLE) - throw new RetryException(e); - - throw e; - } - } + ResponseVerifier throwOnError = new DefaultResponseVerifier() { }; /** Reads the response body, throwing an {@link UncheckedIOException} if this fails, or {@code null} if there is none. */ static byte[] getBytes(ClassicHttpResponse response) { @@ -71,10 +63,10 @@ public interface ConfigServerClient extends AutoCloseable { /** Builder for a request against a given set of hosts, using this config server client. */ interface RequestBuilder { - /** Sets the request path. */ + /** Appends to the request path. */ default RequestBuilder at(String... pathSegments) { return at(List.of(pathSegments)); } - /** Sets the request path. */ + /** Appends to the request path. */ RequestBuilder at(List<String> pathSegments); /** Sets the request body as UTF-8 application/json. */ @@ -83,12 +75,20 @@ public interface ConfigServerClient extends AutoCloseable { /** Sets the request body. */ RequestBuilder body(HttpEntity entity); - /** Sets the parameter key/values for the request. Number of arguments must be even. */ + /** Sets query parameters without a value, like {@code ?debug&recursive}. */ + default RequestBuilder emptyParameters(String... keys) { + return emptyParameters(Arrays.asList(keys)); + } + + /** Sets query parameters without a value, like {@code ?debug&recursive}. */ + RequestBuilder emptyParameters(List<String> keys); + + /** Sets the parameter key/values for the request. Number of arguments must be even. Null values are omitted. */ default RequestBuilder parameters(String... pairs) { return parameters(Arrays.asList(pairs)); } - /** Sets the parameter key/values for the request. Number of arguments must be even. */ + /** Sets the parameter key/values for the request. Number of arguments must be even. Null values are omitted. */ RequestBuilder parameters(List<String> pairs); /** Overrides the default socket read timeout of the request. {@code Duration.ZERO} gives infinite timeout. */ @@ -109,7 +109,10 @@ public interface ConfigServerClient extends AutoCloseable { * Sets the (error) response handler for this request. The default is {@link #throwOnError}. * When the handler returns normally, the response is treated as a success, and passed on to a response mapper. */ - RequestBuilder handling(BiConsumer<ClassicHttpResponse, ClassicHttpRequest> handler); + RequestBuilder throwing(ResponseVerifier handler); + + /** Reads the response as a {@link String}, or throws if unsuccessful. */ + String read(); /** Reads and maps the response, or throws if unsuccessful. */ <T> T read(Function<byte[], T> mapper); @@ -118,10 +121,88 @@ public interface ConfigServerClient extends AutoCloseable { void discard(); /** Returns the raw response input stream, or throws if unsuccessful. The caller must close the returned stream. */ - InputStream stream(); + HttpInputStream stream(); + + /** Uses the response and request, if successful, to generate a mapped response. */ + <T> T handle(ResponseHandler<T> handler); + + } + + + class HttpInputStream extends ForwardingInputStream { + + private final ClassicHttpResponse response; + + protected HttpInputStream(ClassicHttpResponse response) throws IOException { + super(response.getEntity() != null ? response.getEntity().getContent() + : InputStream.nullInputStream()); + this.response = response; + } + + public int statusCode() { return response.getCode(); } + + public String contentType() { return response.getEntity().getContentType(); } + + @Override + public void close() throws IOException { + super.close(); + response.close(); + } + + } + + + /** Reads a successful response and request to compute a result. */ + @FunctionalInterface + interface ResponseHandler<T> { + + /** Called with successful responses, as per {@link ResponseVerifier}. The caller must close the response. */ + T handle(ClassicHttpResponse response, ClassicHttpRequest request) throws IOException; } + + /** Verifies a response, throwing on error responses, possibly indicating retries. */ + @FunctionalInterface + interface ResponseVerifier { + + /** Whether this status code means the response is an error response. */ + default boolean isError(int statusCode) { + return statusCode < HttpStatus.SC_OK || HttpStatus.SC_REDIRECTION <= statusCode; + } + + /** Whether this status code means we should retry. Has no effect if this is not also an error. */ + default boolean shouldRetry(int statusCode) { + return statusCode == HttpStatus.SC_SERVICE_UNAVAILABLE; + } + + /** Verifies the given response, consuming it and throwing if it is an error; or leaving it otherwise. */ + default void verify(ClassicHttpResponse response, ClassicHttpRequest request) throws IOException { + if (isError(response.getCode())) { + try (response) { + byte[] body = response.getEntity() == null ? new byte[0] : EntityUtils.toByteArray(response.getEntity()); + RuntimeException exception = toException(response.getCode(), body, request); + throw shouldRetry(response.getCode()) ? new RetryException(exception) : exception; + } + } + } + + /** Throws the appropriate exception, for the given status code and body. */ + RuntimeException toException(int statusCode, byte[] body, ClassicHttpRequest request); + + } + + + interface DefaultResponseVerifier extends ResponseVerifier { + + @Override + default RuntimeException toException(int statusCode, byte[] body, ClassicHttpRequest request) { + return new ResponseException(request + " failed with status " + statusCode + " and body '" + new String(body, UTF_8) + "'"); + } + + } + + /** What host(s) to try for a request, in what order. A host may be specified multiple times, for retries. */ @FunctionalInterface interface HostStrategy extends Iterable<URI> { @@ -147,6 +228,7 @@ public interface ConfigServerClient extends AutoCloseable { } + /** Exception wrapper that signals retries should be attempted. */ final class RetryException extends RuntimeException { @@ -160,25 +242,12 @@ public interface ConfigServerClient extends AutoCloseable { } + /** An exception due to server error, a bad request, or similar, which resulted in a non-OK HTTP response. */ class ResponseException extends RuntimeException { - public ResponseException(String message, Throwable cause) { - super(message, cause); - } - - public static ResponseException of(ClassicHttpResponse response, ClassicHttpRequest request) { - String detail; - Throwable thrown = null; - try { - detail = request.getEntity() == null ? " and no body" - : " and body '" + EntityUtils.toString(request.getEntity()) + "'"; - } - catch (IOException | ParseException e) { - detail = ". Reading body failed"; - thrown = e; - } - return new ResponseException(request + " failed with status " + response.getCode() + detail, thrown); + public ResponseException(String message) { + super(message); } } diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/HttpConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/HttpConfigServerClient.java index c5b07eceaf5..0fae2e734d8 100644 --- a/configserver-client/src/main/java/ai/vespa/hosted/client/HttpConfigServerClient.java +++ b/configserver-client/src/main/java/ai/vespa/hosted/client/HttpConfigServerClient.java @@ -13,6 +13,8 @@ import org.apache.hc.core5.http.io.SocketConfig; import org.apache.hc.core5.util.TimeValue; import org.apache.hc.core5.util.Timeout; +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLSession; import java.io.IOException; import java.util.Collection; import java.util.Set; @@ -50,11 +52,14 @@ public class HttpConfigServerClient extends AbstractConfigServerClient { manager.setValidateAfterInactivity(TimeValue.ofSeconds(10)); return manager; }, - new AthenzIdentityVerifier(Set.copyOf(serverIdentities)), + new AthenzIdentityVerifier(Set.copyOf(serverIdentities)) { + @Override public boolean verify(String hostname, SSLSession session) { + return super.verify(hostname, session) || "localhost".equals(hostname); + } + }, false) .disableAutomaticRetries() .setUserAgent(userAgent) - .setDefaultRequestConfig(defaultRequestConfig) .build(); } diff --git a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java index 3b46501a4bf..b2f17f43f5c 100644 --- a/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java +++ b/configserver-client/src/test/java/ai/vespa/hosted/client/HttpConfigServerClientTest.java @@ -70,14 +70,15 @@ class HttpConfigServerClientTest { server.resetRequests(); // Successful attempt returns. - server.stubFor(get("/root/boot")) + server.stubFor(get("/root/boot/toot")) .setResponse(okJson("{}").build()); assertEquals("{}", client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10), Method.GET) .at("root", "boot") + .at("toot") .read(String::new)); - server.verify(1, getRequestedFor(urlEqualTo("/root/boot"))); + server.verify(1, getRequestedFor(urlEqualTo("/root/boot/toot"))); server.verify(1, anyRequestedFor(anyUrl())); server.resetRequests(); @@ -88,10 +89,9 @@ class HttpConfigServerClientTest { () -> client.send(HostStrategy.repeating(URI.create("http://localhost:" + server.port()), 10), Method.GET) .read(String::new)); - assertEquals("GET / failed with status 409 and no body", thrown.getMessage()); + assertEquals("GET / failed with status 409 and body 'hi'", thrown.getMessage()); server.verify(1, getRequestedFor(urlEqualTo("/"))); server.verify(1, anyRequestedFor(anyUrl())); - } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServerException.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServerException.java index 4240b0d9fa6..d651eda7139 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServerException.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServerException.java @@ -1,38 +1,32 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.configserver; -import java.net.URI; -import java.util.Objects; +import com.yahoo.slime.Inspector; +import com.yahoo.slime.SlimeUtils; +import org.apache.hc.core5.http.ClassicHttpRequest; + +import java.util.stream.Stream; /** - * @author Tony Vaagenes + * An exception due to server error, a bad request, or similar. + * + * @author jonmv */ public class ConfigServerException extends RuntimeException { - private final URI serverUri; - private final ErrorCode errorCode; - private final String serverMessage; + private final ErrorCode code; + private final String message; - public ConfigServerException(URI serverUri, String context, String serverMessage, ErrorCode errorCode, Throwable cause) { - super(context + ": " + serverMessage, cause); - this.serverUri = Objects.requireNonNull(serverUri); - this.errorCode = Objects.requireNonNull(errorCode); - this.serverMessage = Objects.requireNonNull(serverMessage); + public ConfigServerException(ErrorCode code, String message, String context) { + super(context + ": " + message); + this.code = code; + this.message = message; } - public ErrorCode getErrorCode() { - return errorCode; - } + public ErrorCode code() { return code; } - public URI getServerUri() { - return serverUri; - } + public String message() { return message; } - public String getServerMessage() { - return serverMessage; - } - - // TODO: Copied from Vespa. Expose these in Vespa and use them here public enum ErrorCode { APPLICATION_LOCK_FAILURE, BAD_REQUEST, @@ -46,7 +40,18 @@ public class ConfigServerException extends RuntimeException { UNKNOWN_VESPA_VERSION, PARENT_HOST_NOT_READY, CERTIFICATE_NOT_READY, - LOAD_BALANCER_NOT_READY + LOAD_BALANCER_NOT_READY, + INCOMPLETE_RESPONSE + } + + public static ConfigServerException readException(byte[] body, String context) { + Inspector root = SlimeUtils.jsonToSlime(body).get(); + String codeName = root.field("error-code").asString(); + ErrorCode code = Stream.of(ErrorCode.values()) + .filter(value -> value.name().equals(codeName)) + .findAny().orElse(ErrorCode.INCOMPLETE_RESPONSE); + String message = root.field("message").valid() ? root.field("message").asString() : "(no message)"; + return new ConfigServerException(code, message, context); } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java index 05bdf3c3412..d2f19f4df9f 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/LoadBalancer.java @@ -18,11 +18,11 @@ public class LoadBalancer { private final String id; private final ApplicationId application; private final ClusterSpec.Id cluster; - private final HostName hostname; + private final Optional<HostName> hostname; private final State state; private final Optional<String> dnsZone; - public LoadBalancer(String id, ApplicationId application, ClusterSpec.Id cluster, HostName hostname, State state, + public LoadBalancer(String id, ApplicationId application, ClusterSpec.Id cluster, Optional<HostName> hostname, State state, Optional<String> dnsZone) { this.id = Objects.requireNonNull(id, "id must be non-null"); this.application = Objects.requireNonNull(application, "application must be non-null"); @@ -44,7 +44,7 @@ public class LoadBalancer { return cluster; } - public HostName hostname() { + public Optional<HostName> hostname() { return hostname; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index 51adeb5c2b9..0458a64c5a9 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -228,7 +228,7 @@ public class InternalStepRunner implements StepRunner { // Retry certain failures for up to one hour. Optional<RunStatus> result = startTime.isBefore(controller.clock().instant().minus(Duration.ofHours(1))) ? Optional.of(deploymentFailed) : Optional.empty(); - switch (e.getErrorCode()) { + switch (e.code()) { case CERTIFICATE_NOT_READY: logger.log("Waiting for certificate to become ready on config server: New application, or old one has expired"); if (startTime.plus(timeouts.endpointCertificate()).isBefore(controller.clock().instant())) { @@ -238,15 +238,15 @@ public class InternalStepRunner implements StepRunner { return result; case ACTIVATION_CONFLICT: case APPLICATION_LOCK_FAILURE: - logger.log("Deployment failed with possibly transient error " + e.getErrorCode() + + logger.log("Deployment failed with possibly transient error " + e.code() + ", will retry: " + e.getMessage()); return result; case LOAD_BALANCER_NOT_READY: case PARENT_HOST_NOT_READY: - logger.log(e.getServerMessage()); + logger.log(e.message()); return result; case OUT_OF_CAPACITY: - logger.log(e.getServerMessage()); + logger.log(e.message()); return controller.system().isCd() && startTime.plus(timeouts.capacity()).isAfter(controller.clock().instant()) ? Optional.empty() : Optional.of(outOfCapacity); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index 0517bfef5e5..994dc877182 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -202,15 +202,15 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler { return ErrorResponse.badRequest(Exceptions.toMessageString(e)); } catch (ConfigServerException e) { - switch (e.getErrorCode()) { + switch (e.code()) { case NOT_FOUND: return ErrorResponse.notFoundError(Exceptions.toMessageString(e)); case ACTIVATION_CONFLICT: - return new ErrorResponse(CONFLICT, e.getErrorCode().name(), Exceptions.toMessageString(e)); + return new ErrorResponse(CONFLICT, e.code().name(), Exceptions.toMessageString(e)); case INTERNAL_SERVER_ERROR: return ErrorResponse.internalServerError(Exceptions.toMessageString(e)); default: - return new ErrorResponse(BAD_REQUEST, e.getErrorCode().name(), Exceptions.toMessageString(e)); + return new ErrorResponse(BAD_REQUEST, e.code().name(), Exceptions.toMessageString(e)); } } catch (RuntimeException e) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java index 0356e11ae36..898b2531460 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java @@ -188,9 +188,10 @@ public class RoutingPolicies { private void storePoliciesOf(LoadBalancerAllocation allocation, @SuppressWarnings("unused") Lock lock) { var policies = new LinkedHashMap<>(get(allocation.deployment.applicationId())); for (LoadBalancer loadBalancer : allocation.loadBalancers) { + if (loadBalancer.hostname().isEmpty()) continue; var policyId = new RoutingPolicyId(loadBalancer.application(), loadBalancer.cluster(), allocation.deployment.zoneId()); var existingPolicy = policies.get(policyId); - var newPolicy = new RoutingPolicy(policyId, loadBalancer.hostname(), loadBalancer.dnsZone(), + var newPolicy = new RoutingPolicy(policyId, loadBalancer.hostname().get(), loadBalancer.dnsZone(), allocation.endpointIdsOf(loadBalancer), new Status(isActive(loadBalancer), GlobalRouting.DEFAULT_STATUS)); // Preserve global routing status for existing policy diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java index 4a4159180b5..976cdb5c674 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java @@ -275,11 +275,9 @@ public class DeploymentContext { /** Fail current deployment in given job */ public DeploymentContext outOfCapacity(JobType type) { return failDeployment(type, - new ConfigServerException(URI.create("https://config.server"), - "Failed to deploy application", + new ConfigServerException(ConfigServerException.ErrorCode.OUT_OF_CAPACITY, "Out of capacity", - ConfigServerException.ErrorCode.OUT_OF_CAPACITY, - new RuntimeException("Out of capacity from test code"))); + "Failed to deploy application")); } /** Fail current deployment in given job */ diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java index 6bd7feb8d96..14244d7bdda 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java @@ -99,11 +99,9 @@ public class InternalStepRunnerTest { @Test public void retriesDeploymentForOneHour() { - RuntimeException exception = new ConfigServerException(URI.create("https://server"), - "test failure", + RuntimeException exception = new ConfigServerException(ConfigServerException.ErrorCode.APPLICATION_LOCK_FAILURE, "Exception to retry", - ConfigServerException.ErrorCode.APPLICATION_LOCK_FAILURE, - new RuntimeException("Retry me")); + "test failure"); tester.configServer().throwOnNextPrepare(exception); tester.jobs().deploy(app.instanceId(), JobType.devUsEast1, Optional.empty(), applicationPackage()); assertEquals(unfinished, tester.jobs().last(app.instanceId(), JobType.devUsEast1).get().stepStatuses().get(Step.deployReal)); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java index 8d60a55a1c3..9e15f2ec788 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java @@ -30,7 +30,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.ContainerE import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Log; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; -import com.yahoo.vespa.hosted.controller.api.integration.configserver.NotFoundException; import com.yahoo.vespa.hosted.controller.api.integration.configserver.PrepareResponse; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ProxyResponse; import com.yahoo.vespa.hosted.controller.api.integration.configserver.QuotaUsage; @@ -391,7 +390,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer putLoadBalancers(id.zoneId(), List.of(new LoadBalancer(UUID.randomUUID().toString(), id.applicationId(), cluster, - HostName.from("lb-0--" + id.applicationId().serializedForm() + "--" + id.zoneId().toString()), + Optional.of(HostName.from("lb-0--" + id.applicationId().serializedForm() + "--" + id.zoneId().toString())), LoadBalancer.State.active, Optional.of("dns-zone-1")))); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemRoutingPolicyMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemRoutingPolicyMaintainerTest.java index d7440a706ea..1eadae18668 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemRoutingPolicyMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/SystemRoutingPolicyMaintainerTest.java @@ -35,7 +35,7 @@ public class SystemRoutingPolicyMaintainerTest { tester.configServer().putLoadBalancers(zone, List.of(new LoadBalancer("lb1", SystemApplication.configServer.id(), ClusterSpec.Id.from("config"), - HostName.from("lb1.example.com"), + Optional.of(HostName.from("lb1.example.com")), LoadBalancer.State.active, Optional.of("dns-zone-1")))); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index abea055dde8..0137ea7eeba 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -1127,7 +1127,7 @@ public class ApplicationApiTest extends ControllerContainerTest { 400); ConfigServerMock configServer = tester.serviceRegistry().configServerMock(); - configServer.throwOnNextPrepare(new ConfigServerException(new URI("server-url"), "Failed to prepare application", "Invalid application package", ConfigServerException.ErrorCode.INVALID_APPLICATION_PACKAGE, null)); + configServer.throwOnNextPrepare(new ConfigServerException(ConfigServerException.ErrorCode.INVALID_APPLICATION_PACKAGE, "Failed to prepare application", "Invalid application package")); // GET non-existent application package tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/package", GET).userIdentity(HOSTED_VESPA_OPERATOR), diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java index f574d6bc3f1..1be7f16e85f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java @@ -81,7 +81,7 @@ public class JobControllerApiHandlerHelperTest { tester.triggerJobs(); // us-east-3 eats the deployment failure and fails before deployment, while us-west-1 fails after. - tester.configServer().throwOnNextPrepare(new ConfigServerException(URI.create("url"), "Failed to deploy application", "ERROR!", INVALID_APPLICATION_PACKAGE, null)); + tester.configServer().throwOnNextPrepare(new ConfigServerException(INVALID_APPLICATION_PACKAGE, "ERROR!", "Failed to deploy application")); tester.runner().run(); assertEquals(deploymentFailed, tester.jobs().last(app.instanceId(), productionUsEast3).get().status()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java index e96af475216..d03dec06753 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java @@ -416,7 +416,7 @@ public class RoutingPoliciesTest { var loadBalancer = new LoadBalancer("LB-0-Z-" + zone1.value(), context.instanceId(), ClusterSpec.Id.from("c0"), - newHostname, + Optional.of(newHostname), LoadBalancer.State.active, Optional.of("dns-zone-1")); tester.controllerTester().configServer().putLoadBalancers(zone1, List.of(loadBalancer)); @@ -705,7 +705,7 @@ public class RoutingPoliciesTest { new LoadBalancer("LB-" + i + "-Z-" + zone.value(), application, ClusterSpec.Id.from("c" + i), - lbHostname, + Optional.of(lbHostname), LoadBalancer.State.active, Optional.of("dns-zone-1"))); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java index 6f7b7c4d57d..a6bb57bef29 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java @@ -5,6 +5,7 @@ import com.yahoo.vespa.hosted.provision.maintenance.LoadBalancerExpirer; import java.time.Instant; import java.util.Objects; +import java.util.Optional; /** * Represents a load balancer for an application's cluster. This is immutable. @@ -14,15 +15,18 @@ import java.util.Objects; public class LoadBalancer { private final LoadBalancerId id; - private final LoadBalancerInstance instance; + private final Optional<LoadBalancerInstance> instance; private final State state; private final Instant changedAt; - public LoadBalancer(LoadBalancerId id, LoadBalancerInstance instance, State state, Instant changedAt) { + public LoadBalancer(LoadBalancerId id, Optional<LoadBalancerInstance> instance, State state, Instant changedAt) { this.id = Objects.requireNonNull(id, "id must be non-null"); this.instance = Objects.requireNonNull(instance, "instance must be non-null"); this.state = Objects.requireNonNull(state, "state must be non-null"); this.changedAt = Objects.requireNonNull(changedAt, "changedAt must be non-null"); + if (state == State.active && instance.isEmpty()) { + throw new IllegalArgumentException("Load balancer instance is required in state " + state); + } } /** An identifier for this load balancer. The ID is unique inside the zone */ @@ -31,7 +35,7 @@ public class LoadBalancer { } /** The instance associated with this */ - public LoadBalancerInstance instance() { + public Optional<LoadBalancerInstance> instance() { return instance; } @@ -58,7 +62,7 @@ public class LoadBalancer { } /** Returns a copy of this with instance set to given instance */ - public LoadBalancer with(LoadBalancerInstance instance) { + public LoadBalancer with(Optional<LoadBalancerInstance> instance) { return new LoadBalancer(id, instance, state, changedAt); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java index 2ef12177eaf..9665d8872de 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java @@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancerSpec; import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; import java.time.Duration; +import java.time.Instant; import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; @@ -56,10 +57,10 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer { /** Move reserved load balancer that have expired to inactive */ private void expireReserved() { - var expiry = nodeRepository().clock().instant().minus(reservedExpiry); - patchLoadBalancers(lb -> lb.state() == State.reserved && - lb.changedAt().isBefore(expiry), - lb -> db.writeLoadBalancer(lb.with(State.inactive, nodeRepository().clock().instant()))); + Instant now = nodeRepository().clock().instant(); + Instant expiry = now.minus(reservedExpiry); + patchLoadBalancers(lb -> lb.state() == State.reserved && lb.changedAt().isBefore(expiry), + lb -> db.writeLoadBalancer(lb.with(State.inactive, now))); } /** Deprovision inactive load balancers that have expired */ @@ -95,13 +96,14 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer { var failed = new ArrayList<LoadBalancerId>(); var lastException = new AtomicReference<Exception>(); patchLoadBalancers(lb -> lb.state() == State.inactive, lb -> { + if (lb.instance().isEmpty()) return; var allocatedNodes = allocatedNodes(lb.id()).stream().map(Node::hostname).collect(Collectors.toSet()); - var reals = new LinkedHashSet<>(lb.instance().reals()); + var reals = new LinkedHashSet<>(lb.instance().get().reals()); // Remove any real no longer allocated to this application reals.removeIf(real -> !allocatedNodes.contains(real.hostname().value())); try { service.create(new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals), true); - db.writeLoadBalancer(lb.with(lb.instance().withReals(reals))); + db.writeLoadBalancer(lb.with(lb.instance().map(instance -> instance.withReals(reals)))); } catch (Exception e) { failed.add(lb.id()); lastException.set(e); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java index dddc535b36a..4cd01167293 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java @@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancers; import java.util.Comparator; import java.util.LinkedHashSet; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.TreeSet; @@ -70,6 +71,7 @@ public class NodeAcl { loadBalancers.list(allocation.owner()).asList() .stream() .map(LoadBalancer::instance) + .flatMap(Optional::stream) .map(LoadBalancerInstance::networks) .forEach(trustedNetworks::addAll); }); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java index 66172521d4c..04c8f012b8b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java @@ -18,6 +18,7 @@ import java.io.UncheckedIOException; import java.time.Instant; import java.util.LinkedHashSet; import java.util.Optional; +import java.util.Set; import java.util.function.Function; /** @@ -50,21 +51,22 @@ public class LoadBalancerSerializer { Cursor root = slime.setObject(); root.setString(idField, loadBalancer.id().serializedForm()); - root.setString(hostnameField, loadBalancer.instance().hostname().toString()); + // TODO(mpolden): Stop writing this field for empty instance after 2021-06-01 + root.setString(hostnameField, loadBalancer.instance().map(instance -> instance.hostname().value()).orElse("")); root.setString(stateField, asString(loadBalancer.state())); root.setLong(changedAtField, loadBalancer.changedAt().toEpochMilli()); - loadBalancer.instance().dnsZone().ifPresent(dnsZone -> root.setString(dnsZoneField, dnsZone.id())); + loadBalancer.instance().flatMap(LoadBalancerInstance::dnsZone).ifPresent(dnsZone -> root.setString(dnsZoneField, dnsZone.id())); Cursor portArray = root.setArray(portsField); - loadBalancer.instance().ports().forEach(portArray::addLong); + loadBalancer.instance().ifPresent(instance -> instance.ports().forEach(portArray::addLong)); Cursor networkArray = root.setArray(networksField); - loadBalancer.instance().networks().forEach(networkArray::addString); + loadBalancer.instance().ifPresent(instance -> instance.networks().forEach(networkArray::addString)); Cursor realArray = root.setArray(realsField); - loadBalancer.instance().reals().forEach(real -> { + loadBalancer.instance().ifPresent(instance -> instance.reals().forEach(real -> { Cursor realObject = realArray.addObject(); realObject.setString(hostnameField, real.hostname().value()); realObject.setString(ipAddressField, real.ipAddress()); realObject.setLong(portField, real.port()); - }); + })); try { return SlimeUtils.toJsonBytes(slime); } catch (IOException e) { @@ -75,7 +77,7 @@ public class LoadBalancerSerializer { public static LoadBalancer fromJson(byte[] data) { Cursor object = SlimeUtils.jsonToSlime(data).get(); - var reals = new LinkedHashSet<Real>(); + Set<Real> reals = new LinkedHashSet<>(); object.field(realsField).traverse((ArrayTraverser) (i, realObject) -> { reals.add(new Real(HostName.from(realObject.field(hostnameField).asString()), realObject.field(ipAddressField).asString(), @@ -83,20 +85,19 @@ public class LoadBalancerSerializer { }); - var ports = new LinkedHashSet<Integer>(); + Set<Integer> ports = new LinkedHashSet<>(); object.field(portsField).traverse((ArrayTraverser) (i, port) -> ports.add((int) port.asLong())); - var networks = new LinkedHashSet<String>(); + Set<String> networks = new LinkedHashSet<>(); object.field(networksField).traverse((ArrayTraverser) (i, network) -> networks.add(network.asString())); + Optional<HostName> hostname = optionalString(object.field(hostnameField), Function.identity()).filter(s -> !s.isEmpty()).map(HostName::from); + Optional<DnsZone> dnsZone = optionalString(object.field(dnsZoneField), DnsZone::new); + Optional<LoadBalancerInstance> instance = hostname.map(h -> new LoadBalancerInstance(h, dnsZone, ports, + networks, reals)); + return new LoadBalancer(LoadBalancerId.fromSerializedForm(object.field(idField).asString()), - new LoadBalancerInstance( - HostName.from(object.field(hostnameField).asString()), - optionalString(object.field(dnsZoneField), DnsZone::new), - ports, - networks, - reals - ), + instance, stateFromString(object.field(stateField).asString()), Instant.ofEpochMilli(object.field(changedAtField).asLong())); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java index 09d19300c59..f53eb189ec1 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java @@ -170,18 +170,35 @@ public class LoadBalancerProvisioner { Instant now = nodeRepository.clock().instant(); Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id); if (loadBalancer.isEmpty() && activate) return; // Nothing to activate as this load balancer was never prepared - LoadBalancerInstance instance = provisionInstance(id, realsOf(nodes), loadBalancer); + + Set<Real> reals = realsOf(nodes); + Optional<LoadBalancerInstance> instance = provisionInstance(id, reals, loadBalancer); LoadBalancer newLoadBalancer; if (loadBalancer.isEmpty()) { newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now); } else { - var newState = activate ? LoadBalancer.State.active : loadBalancer.get().state(); - newLoadBalancer = loadBalancer.get().with(instance).with(newState, now); + LoadBalancer.State state = activate && instance.isPresent() + ? LoadBalancer.State.active + : loadBalancer.get().state(); + newLoadBalancer = loadBalancer.get().with(instance).with(state, now); if (loadBalancer.get().state() != newLoadBalancer.state()) { log.log(Level.FINE, "Moving " + newLoadBalancer.id() + " to state " + newLoadBalancer.state()); } } - db.writeLoadBalancers(List.of(newLoadBalancer), transaction.nested()); + + if (activate) { + db.writeLoadBalancers(List.of(newLoadBalancer), transaction.nested()); + } else { + // Always store load balancer so that LoadBalancerExpirer can expire partially provisioned load balancers + db.writeLoadBalancer(newLoadBalancer); + } + + // Signal that load balancer is not ready yet + if (instance.isEmpty()) { + throw new LoadBalancerServiceException("Could not (re)configure " + id + ", targeting: " + + reals + ". The operation will be retried on next deployment", + null); + } } private void provision(ApplicationTransaction transaction, ClusterSpec.Id clusterId, NodeList nodes) { @@ -189,17 +206,18 @@ public class LoadBalancerProvisioner { } /** Provision or reconfigure a load balancer instance, if necessary */ - private LoadBalancerInstance provisionInstance(LoadBalancerId id, Set<Real> reals, - Optional<LoadBalancer> currentLoadBalancer) { + private Optional<LoadBalancerInstance> provisionInstance(LoadBalancerId id, Set<Real> reals, + Optional<LoadBalancer> currentLoadBalancer) { if (hasReals(currentLoadBalancer, reals)) return currentLoadBalancer.get().instance(); log.log(Level.FINE, "Creating " + id + ", targeting: " + reals); try { - return service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals), - allowEmptyReals(currentLoadBalancer)); + return Optional.of(service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals), + allowEmptyReals(currentLoadBalancer))); } catch (Exception e) { - throw new LoadBalancerServiceException("Failed to (re)configure " + id + ", targeting: " + - reals + ". The operation will be retried on next deployment", e); + log.log(Level.WARNING, "Could not (re)configure " + id + ", targeting: " + + reals + ". The operation will be retried on next deployment", e); } + return Optional.empty(); } /** Returns the nodes allocated to the given load balanced cluster */ @@ -246,7 +264,8 @@ public class LoadBalancerProvisioner { /** Returns whether load balancer has given reals */ private static boolean hasReals(Optional<LoadBalancer> loadBalancer, Set<Real> reals) { if (loadBalancer.isEmpty()) return false; - return loadBalancer.get().instance().reals().equals(reals); + if (loadBalancer.get().instance().isEmpty()) return false; + return loadBalancer.get().instance().get().reals().equals(reals); } /** Returns whether to allow given load balancer to have no reals */ @@ -273,5 +292,4 @@ public class LoadBalancerProvisioner { return cluster.combinedId().orElse(cluster.id()); } - } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java index 1ef449555d9..bcd0af4e121 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/LoadBalancersResponse.java @@ -9,6 +9,7 @@ import com.yahoo.slime.JsonFormat; import com.yahoo.slime.Slime; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; +import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerList; import com.yahoo.vespa.hosted.provision.node.filter.ApplicationFilter; @@ -65,21 +66,23 @@ public class LoadBalancersResponse extends HttpResponse { lbObject.setString("tenant", lb.id().application().tenant().value()); lbObject.setString("instance", lb.id().application().instance().value()); lbObject.setString("cluster", lb.id().cluster().value()); - lbObject.setString("hostname", lb.instance().hostname().value()); - lb.instance().dnsZone().ifPresent(dnsZone -> lbObject.setString("dnsZone", dnsZone.id())); + lb.instance().ifPresent(instance -> lbObject.setString("hostname", instance.hostname().value())); + lb.instance().flatMap(LoadBalancerInstance::dnsZone).ifPresent(dnsZone -> lbObject.setString("dnsZone", dnsZone.id())); Cursor networkArray = lbObject.setArray("networks"); - lb.instance().networks().forEach(networkArray::addString); + lb.instance().ifPresent(instance -> instance.networks().forEach(networkArray::addString)); Cursor portArray = lbObject.setArray("ports"); - lb.instance().ports().forEach(portArray::addLong); + lb.instance().ifPresent(instance -> instance.ports().forEach(portArray::addLong)); Cursor realArray = lbObject.setArray("reals"); - lb.instance().reals().forEach(real -> { - Cursor realObject = realArray.addObject(); - realObject.setString("hostname", real.hostname().value()); - realObject.setString("ipAddress", real.ipAddress()); - realObject.setLong("port", real.port()); + lb.instance().ifPresent(instance -> { + instance.reals().forEach(real -> { + Cursor realObject = realArray.addObject(); + realObject.setString("hostname", real.hostname().value()); + realObject.setString("ipAddress", real.ipAddress()); + realObject.setLong("port", real.port()); + }); }); }); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java index fec6e40fd35..4d19c2c1d41 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java @@ -63,7 +63,7 @@ public class LoadBalancerExpirerTest { tester.nodeRepository().nodes().setReady(tester.nodeRepository().nodes().list(Node.State.dirty).asList(), Agent.system, getClass().getSimpleName()); expirer.maintain(); assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals()); - assertEquals(Set.of(), loadBalancers.get().get(lb1).instance().reals()); + assertEquals(Set.of(), loadBalancers.get().get(lb1).instance().get().reals()); // Expirer defers removal of load balancer until expiration time passes expirer.maintain(); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java index bcb78844666..51d6f7fb2fb 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java @@ -30,7 +30,7 @@ public class LoadBalancerSerializerTest { "application1", "default"), ClusterSpec.Id.from("qrs")), - new LoadBalancerInstance( + Optional.of(new LoadBalancerInstance( HostName.from("lb-host"), Optional.of(new DnsZone("zone-id-1")), ImmutableSet.of(4080, 4443), @@ -40,19 +40,19 @@ public class LoadBalancerSerializerTest { 4080), new Real(HostName.from("real-2"), "127.0.0.2", - 4080))), + 4080)))), LoadBalancer.State.active, now); var serialized = LoadBalancerSerializer.fromJson(LoadBalancerSerializer.toJson(loadBalancer)); assertEquals(loadBalancer.id(), serialized.id()); - assertEquals(loadBalancer.instance().hostname(), serialized.instance().hostname()); - assertEquals(loadBalancer.instance().dnsZone(), serialized.instance().dnsZone()); - assertEquals(loadBalancer.instance().ports(), serialized.instance().ports()); - assertEquals(loadBalancer.instance().networks(), serialized.instance().networks()); + assertEquals(loadBalancer.instance().get().hostname(), serialized.instance().get().hostname()); + assertEquals(loadBalancer.instance().get().dnsZone(), serialized.instance().get().dnsZone()); + assertEquals(loadBalancer.instance().get().ports(), serialized.instance().get().ports()); + assertEquals(loadBalancer.instance().get().networks(), serialized.instance().get().networks()); assertEquals(loadBalancer.state(), serialized.state()); assertEquals(loadBalancer.changedAt().truncatedTo(MILLIS), serialized.changedAt()); - assertEquals(loadBalancer.instance().reals(), serialized.instance().reals()); + assertEquals(loadBalancer.instance().get().reals(), serialized.instance().get().reals()); } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java index b7e671a7b76..744322c9edc 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java @@ -180,7 +180,7 @@ public class AclProvisioningTest { // Load balancer is allocated to application var loadBalancers = tester.nodeRepository().loadBalancers().list(application); assertEquals(1, loadBalancers.asList().size()); - var lbNetworks = loadBalancers.asList().get(0).instance().networks(); + var lbNetworks = loadBalancers.asList().get(0).instance().get().networks(); assertEquals(2, lbNetworks.size()); // ACL for nodes with allocation trust their respective load balancer networks, if any diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java index a3780db4789..eb3bdff484d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java @@ -10,6 +10,7 @@ import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.HostSpec; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; +import com.yahoo.config.provision.exception.LoadBalancerServiceException; import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; @@ -59,7 +60,7 @@ public class LoadBalancerProvisionerTest { clusterRequest(ClusterSpec.Type.container, containerCluster1), clusterRequest(ClusterSpec.Type.content, contentCluster)); assertEquals(1, lbApp1.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbApp1.get().get(0).instance().reals().size()); + assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbApp1.get().get(0).instance().get().reals().size()); tester.activate(app1, nodes); tester.activate(app2, prepare(app2, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")))); assertEquals(1, lbApp2.get().size()); @@ -67,11 +68,11 @@ public class LoadBalancerProvisionerTest { // Reals are configured after activation assertEquals(app1, lbApp1.get().get(0).id().application()); assertEquals(containerCluster1, lbApp1.get().get(0).id().cluster()); - assertEquals(Collections.singleton(4443), lbApp1.get().get(0).instance().ports()); - assertEquals("127.0.0.1", get(lbApp1.get().get(0).instance().reals(), 0).ipAddress()); - assertEquals(4443, get(lbApp1.get().get(0).instance().reals(), 0).port()); - assertEquals("127.0.0.2", get(lbApp1.get().get(0).instance().reals(), 1).ipAddress()); - assertEquals(4443, get(lbApp1.get().get(0).instance().reals(), 1).port()); + assertEquals(Collections.singleton(4443), lbApp1.get().get(0).instance().get().ports()); + assertEquals("127.0.0.1", get(lbApp1.get().get(0).instance().get().reals(), 0).ipAddress()); + assertEquals(4443, get(lbApp1.get().get(0).instance().get().reals(), 0).port()); + assertEquals("127.0.0.2", get(lbApp1.get().get(0).instance().get().reals(), 1).ipAddress()); + assertEquals(4443, get(lbApp1.get().get(0).instance().get().reals(), 1).port()); // A container is failed Supplier<List<Node>> containers = () -> tester.getNodes(app1).container().asList(); @@ -83,13 +84,13 @@ public class LoadBalancerProvisionerTest { clusterRequest(ClusterSpec.Type.container, containerCluster1), clusterRequest(ClusterSpec.Type.content, contentCluster))); LoadBalancer loadBalancer = tester.nodeRepository().loadBalancers().list(app1).asList().get(0); - assertEquals(2, loadBalancer.instance().reals().size()); - assertTrue("Failed node is removed", loadBalancer.instance().reals().stream() + assertEquals(2, loadBalancer.instance().get().reals().size()); + assertTrue("Failed node is removed", loadBalancer.instance().get().reals().stream() .map(Real::hostname) .map(HostName::value) .noneMatch(hostname -> hostname.equals(toFail.hostname()))); - assertEquals(containers.get().get(0).hostname(), get(loadBalancer.instance().reals(), 0).hostname().value()); - assertEquals(containers.get().get(1).hostname(), get(loadBalancer.instance().reals(), 1).hostname().value()); + assertEquals(containers.get().get(0).hostname(), get(loadBalancer.instance().get().reals(), 0).hostname().value()); + assertEquals(containers.get().get(1).hostname(), get(loadBalancer.instance().get().reals(), 1).hostname().value()); assertSame("State is unchanged", LoadBalancer.State.active, loadBalancer.state()); // Add another container cluster to first app @@ -110,6 +111,7 @@ public class LoadBalancerProvisionerTest { .collect(Collectors.toList()); List<HostName> reals = lbApp1.get().stream() .map(LoadBalancer::instance) + .flatMap(Optional::stream) .map(LoadBalancerInstance::reals) .flatMap(Collection::stream) .map(Real::hostname) @@ -152,7 +154,7 @@ public class LoadBalancerProvisionerTest { .findFirst() .orElseThrow()); - // Next redeploy does not create a new load balancer instance + // Next redeploy does not create a new load balancer instance because reals are unchanged tester.loadBalancerService().throwOnCreate(true); tester.activate(app1, prepare(app1, clusterRequest(ClusterSpec.Type.container, containerCluster1), @@ -208,7 +210,7 @@ public class LoadBalancerProvisionerTest { var combinedId = ClusterSpec.Id.from("container1"); var nodes = prepare(app1, clusterRequest(ClusterSpec.Type.combined, ClusterSpec.Id.from("content1"), Optional.of(combinedId))); assertEquals(1, lbs.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().reals().size()); + assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().get().reals().size()); tester.activate(app1, nodes); assertSame(LoadBalancer.State.active, lbs.get().get(0).state()); assertEquals(combinedId, lbs.get().get(0).id().cluster()); @@ -222,7 +224,7 @@ public class LoadBalancerProvisionerTest { var nodes = prepare(configServerApp, Capacity.fromRequiredNodeType(NodeType.config), clusterRequest(ClusterSpec.Type.admin, cluster)); assertEquals(1, lbs.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().reals().size()); + assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().get().reals().size()); tester.activate(configServerApp, nodes); assertSame(LoadBalancer.State.active, lbs.get().get(0).state()); assertEquals(cluster, lbs.get().get(0).id().cluster()); @@ -236,7 +238,7 @@ public class LoadBalancerProvisionerTest { var nodes = prepare(controllerApp, Capacity.fromRequiredNodeType(NodeType.controller), clusterRequest(ClusterSpec.Type.container, cluster)); assertEquals(1, lbs.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().reals().size()); + assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().get().reals().size()); tester.activate(controllerApp, nodes); assertSame(LoadBalancer.State.active, lbs.get().get(0).state()); assertEquals(cluster, lbs.get().get(0).id().cluster()); @@ -253,7 +255,7 @@ public class LoadBalancerProvisionerTest { // instance1 is deployed tester.activate(instance1, prepare(instance1, clusterRequest(ClusterSpec.Type.container, devCluster))); - // instance2 clashes because cluster name matches instance1 + // instance2 clashes because instance name matches instance1 cluster name try { prepare(instance2, clusterRequest(ClusterSpec.Type.container, defaultCluster)); fail("Expected exception"); @@ -263,10 +265,38 @@ public class LoadBalancerProvisionerTest { // instance2 changes cluster name and does not clash tester.activate(instance2, prepare(instance2, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")))); - // instance3 clashes because instance name matches instance2 cluster + // instance3 does not clash tester.activate(instance3, prepare(instance3, clusterRequest(ClusterSpec.Type.container, defaultCluster))); } + @Test + public void provisioning_load_balancer_fails_initially() { + Supplier<List<LoadBalancer>> lbs = () -> tester.nodeRepository().loadBalancers().list(app1).asList(); + ClusterSpec.Id cluster = ClusterSpec.Id.from("qrs1"); + + // Provisioning load balancer fails on deployment + tester.loadBalancerService().throwOnCreate(true); + try { + prepare(app1, clusterRequest(ClusterSpec.Type.container, cluster)); + fail("Expected exception"); + } catch (LoadBalancerServiceException ignored) {} + List<LoadBalancer> loadBalancers = lbs.get(); + assertEquals(1, loadBalancers.size()); + assertSame(LoadBalancer.State.reserved, loadBalancers.get(0).state()); + assertTrue("Load balancer has no instance", loadBalancers.get(0).instance().isEmpty()); + + // Next deployment succeeds + tester.loadBalancerService().throwOnCreate(false); + Set<HostSpec> nodes = prepare(app1, clusterRequest(ClusterSpec.Type.container, cluster)); + loadBalancers = lbs.get(); + assertSame(LoadBalancer.State.reserved, loadBalancers.get(0).state()); + assertTrue("Load balancer has instance", loadBalancers.get(0).instance().isPresent()); + tester.activate(app1, nodes); + loadBalancers = lbs.get(); + assertSame(LoadBalancer.State.active, loadBalancers.get(0).state()); + assertTrue("Load balancer has instance", loadBalancers.get(0).instance().isPresent()); + } + private void dirtyNodesOf(ApplicationId application) { tester.nodeRepository().nodes().deallocate(tester.nodeRepository().nodes().list().owner(application).asList(), Agent.system, this.getClass().getSimpleName()); } |