Retry proxy request with single target

6 files changed, 55 insertions, 47 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
index e93e4f3529f..49a23a8337b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ConfigServerRestExecutorImpl.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.proxy;
 
+import ai.vespa.util.http.retry.Sleeper;
 import com.google.inject.Inject;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.jdisc.http.HttpRequest.Method;
@@ -53,9 +54,12 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C
     private static final Logger LOG = Logger.getLogger(ConfigServerRestExecutorImpl.class.getName());
     private static final Duration PROXY_REQUEST_TIMEOUT = Duration.ofSeconds(10);
     private static final Duration PING_REQUEST_TIMEOUT = Duration.ofMillis(500);
+    private static final Duration SINGLE_TARGET_WAIT = Duration.ofSeconds(2);
+    private static final int SINGLE_TARGET_RETRIES = 3;
     private static final Set<String> HEADERS_TO_COPY = Set.of("X-HTTP-Method-Override", "Content-Type");
 
     private final CloseableHttpClient client;
+    private final Sleeper sleeper;
 
     @Inject
     public ConfigServerRestExecutorImpl(ZoneRegistry zoneRegistry, ServiceIdentityProvider sslContextProvider) {
@@ -66,22 +70,31 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C
 
         this.client = createHttpClient(config, sslContextProvider,
                 new ControllerOrConfigserverHostnameVerifier(zoneRegistry));
+        this.sleeper = new Sleeper.Default();
     }
 
     @Override
     public ProxyResponse handle(ProxyRequest proxyRequest) throws ProxyException {
-        // Make a local copy of the list as we want to manipulate it in case of ping problems.
-        List<URI> allServers = new ArrayList<>(proxyRequest.getTargets());
+        List<URI> targets = new ArrayList<>(proxyRequest.getTargets());
 
         StringBuilder errorBuilder = new StringBuilder();
-        if (queueFirstServerIfDown(allServers)) {
+        boolean singleTarget = targets.size() == 1;
+        if (singleTarget) {
+            for (int i = 0; i < SINGLE_TARGET_RETRIES - 1; i++) {
+                targets.add(targets.get(0));
+            }
+        } else if (queueFirstServerIfDown(targets)) {
             errorBuilder.append("Change ordering due to failed ping.");
         }
-        for (URI uri : allServers) {
-            Optional<ProxyResponse> proxyResponse = proxyCall(uri, proxyRequest, errorBuilder);
+
+        for (URI url : targets) {
+            Optional<ProxyResponse> proxyResponse = proxyCall(url, proxyRequest, errorBuilder);
             if (proxyResponse.isPresent()) {
                 return proxyResponse.get();
             }
+            if (singleTarget) {
+                sleeper.sleep(SINGLE_TARGET_WAIT);
+            }
         }
 
         throw new ProxyException(ErrorResponse.internalServerError("Failed talking to config servers: "
@@ -185,6 +198,7 @@ public class ConfigServerRestExecutorImpl extends AbstractComponent implements C
                 .setConnectionRequestTimeout((int) PING_REQUEST_TIMEOUT.toMillis())
                 .setSocketTimeout((int) PING_REQUEST_TIMEOUT.toMillis()).build();
         httpGet.setConfig(config);
+
         try (CloseableHttpResponse response = client.execute(httpGet)) {
             if (response.getStatusLine().getStatusCode() == 200) {
                 return false;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
index f398683567b..a6314df9581 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequest.java
@@ -28,33 +28,21 @@ public class ProxyRequest {
     private final List<URI> targets;
     private final String targetPath;
 
-    /**
-     * The constructor calls exception if the request is invalid.
-     *
-     * @param request the request from the jdisc framework.
-     * @param targets list of targets this request should be proxied to (targets are tried once in order until a response is returned).
-     * @param targetPath the path to proxy to.
-     * @throws ProxyException on errors
-     */
-    public ProxyRequest(HttpRequest request, List<URI> targets, String targetPath) throws ProxyException {
-        this(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(), request.getData(),
-             targets, targetPath);
-    }
-
-    ProxyRequest(Method method, URI requestUri, Map<String, List<String>> headers, InputStream body,
-                 List<URI> targets, String targetPath) throws ProxyException {
-        Objects.requireNonNull(requestUri, "Request must be non-null");
-        if (!requestUri.getPath().endsWith(targetPath))
-            throw new ProxyException(ErrorResponse.badRequest(String.format(
-                    "Request path '%s' does not end with proxy path '%s'", requestUri.getPath(), targetPath)));
-
+    ProxyRequest(Method method, URI url, Map<String, List<String>> headers, InputStream body, List<URI> targets,
+                 String path) {
+        Objects.requireNonNull(url);
+        if (!url.getPath().endsWith(path)) {
+            throw new IllegalArgumentException(String.format("Request path '%s' does not end with proxy path '%s'", url.getPath(), path));
+        }
+        if (targets.isEmpty()) {
+            throw new IllegalArgumentException("targets must be non-empty");
+        }
         this.method = Objects.requireNonNull(method);
-        this.requestUri = Objects.requireNonNull(requestUri);
+        this.requestUri = Objects.requireNonNull(url);
         this.headers = Objects.requireNonNull(headers);
         this.requestData = body;
-
         this.targets = List.copyOf(targets);
-        this.targetPath = targetPath.startsWith("/") ? targetPath : "/" + targetPath;
+        this.targetPath = path.startsWith("/") ? path : "/" + path;
     }
 
 
@@ -100,4 +88,16 @@ public class ProxyRequest {
         return "[targets: " + targets + " request: " + targetPath + "]";
     }
 
+    /** Create a proxy request that tries all given targets in order */
+    public static ProxyRequest tryAll(List<URI> targets, String path, HttpRequest request) {
+        return new ProxyRequest(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(),
+                                request.getData(), targets, path);
+    }
+
+    /** Create a proxy request that repeatedly tries a single target */
+    public static ProxyRequest tryOne(URI target, String path, HttpRequest request) {
+        return new ProxyRequest(request.getMethod(), request.getUri(), request.getJDiscRequest().headers(),
+                                request.getData(), List.of(target), path);
+    }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
index 6ffdea93a1c..a1a7dab2955 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/configserver/ConfigServerApiHandler.java
@@ -96,7 +96,7 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler {
         }
 
         try {
-            return proxy.handle(new ProxyRequest(request, List.of(getEndpoint(zoneId)), cfgPath));
+            return proxy.handle(ProxyRequest.tryOne(getEndpoint(zoneId), cfgPath, request));
         } catch (ProxyException e) {
             throw new RuntimeException(e);
         }
@@ -126,4 +126,5 @@ public class ConfigServerApiHandler extends AuditLoggingRequestHandler {
     private URI getEndpoint(ZoneId zoneId) {
         return CONTROLLER_ZONE.equals(zoneId) ? CONTROLLER_URI : zoneRegistry.getConfigServerVipUri(zoneId);
     }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
index a127a44efb2..f1bb4ba9268 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/zone/v2/ZoneApiHandler.java
@@ -20,8 +20,6 @@ import com.yahoo.vespa.hosted.controller.proxy.ProxyException;
 import com.yahoo.vespa.hosted.controller.proxy.ProxyRequest;
 import com.yahoo.yolean.Exceptions;
 
-import java.net.URI;
-import java.util.List;
 import java.util.logging.Level;
 
 /**
@@ -85,7 +83,7 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler {
             throw new IllegalArgumentException("No such zone: " + zoneId.value());
         }
         try {
-            return proxy.handle(new ProxyRequest(request, getConfigserverEndpoints(zoneId), path.getRest()));
+            return proxy.handle(proxyRequest(zoneId, path.getRest(), request));
         } catch (ProxyException e) {
             throw new RuntimeException(e);
         }
@@ -114,13 +112,12 @@ public class ZoneApiHandler extends AuditLoggingRequestHandler {
         return ErrorResponse.notFoundError("Nothing at " + path);
     }
 
-    private List<URI> getConfigserverEndpoints(ZoneId zoneId) {
+    private ProxyRequest proxyRequest(ZoneId zoneId, String path, HttpRequest request) {
         // TODO: Use config server VIP for all zones that have one
         if (zoneId.region().value().startsWith("aws-") || zoneId.region().value().contains("-aws-")) {
-            return List.of(zoneRegistry.getConfigServerVipUri(zoneId));
-        } else {
-            return zoneRegistry.getConfigServerUris(zoneId);
+            return ProxyRequest.tryOne(zoneRegistry.getConfigServerVipUri(zoneId), path, request);
         }
+        return ProxyRequest.tryAll(zoneRegistry.getConfigServerUris(zoneId), path, request);
     }
 
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
index d8373cb8928..15ec138354d 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyRequestTest.java
@@ -21,12 +21,6 @@ public class ProxyRequestTest {
     public final ExpectedException exception = ExpectedException.none();
 
     @Test
-    public void testEmpty() throws Exception {
-        exception.expectMessage("Request must be non-null");
-        new ProxyRequest(HttpRequest.Method.GET, null, Map.of(), null, List.of(), "/zone/v2");
-    }
-
-    @Test
     public void testBadUri() throws Exception {
         exception.expectMessage("Request path '/path' does not end with proxy path '/zone/v2/'");
         testRequest("http://domain.tld/path", "/zone/v2/");
@@ -67,8 +61,9 @@ public class ProxyRequestTest {
         }
     }
 
-    private static ProxyRequest testRequest(String url, String pathPrefix) throws ProxyException {
-        return new ProxyRequest(
-                HttpRequest.Method.GET, URI.create(url), Map.of(), null, List.of(), pathPrefix);
+    private static ProxyRequest testRequest(String url, String pathPrefix) {
+        return new ProxyRequest(HttpRequest.Method.GET, URI.create(url), Map.of(), null,
+                                List.of(URI.create("http://example.com")), pathPrefix);
     }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
index 0aac59321b5..539d4a6dd75 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/proxy/ProxyResponseTest.java
@@ -20,7 +20,7 @@ public class ProxyResponseTest {
     @Test
     public void testRewriteUrl() throws Exception {
         ProxyRequest request = new ProxyRequest(HttpRequest.Method.GET, URI.create("http://domain.tld/zone/v2/dev/us-north-1/configserver"),
-                Map.of(), null, List.of(), "configserver");
+                Map.of(), null, List.of(URI.create("http://example.com")), "configserver");
         ProxyResponse proxyResponse = new ProxyResponse(
                 request,
                 "response link is http://configserver:1234/bla/bla/",
@@ -38,7 +38,7 @@ public class ProxyResponseTest {
     @Test
     public void testRewriteSecureUrl() throws Exception {
         ProxyRequest request = new ProxyRequest(HttpRequest.Method.GET, URI.create("https://domain.tld/zone/v2/prod/eu-south-3/configserver"),
-                Map.of(), null, List.of(), "configserver");
+                Map.of(), null, List.of(URI.create("http://example.com")), "configserver");
         ProxyResponse proxyResponse = new ProxyResponse(
                 request,
                 "response link is http://configserver:1234/bla/bla/",
@@ -52,4 +52,5 @@ public class ProxyResponseTest {
         String document = new String(outputStream.toByteArray(), StandardCharsets.UTF_8);
         assertEquals("response link is https://domain.tld/zone/v2/prod/eu-south-3/bla/bla/", document);
     }
+
 }