Convert HostSuspensionResource to request handler

4 files changed, 197 insertions, 120 deletions

diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml
index fed0e617c79..da4a7dc4efc 100644
--- a/orchestrator/pom.xml
+++ b/orchestrator/pom.xml
@@ -134,6 +134,16 @@
       <artifactId>hamcrest-all</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.junit.jupiter</groupId>
+      <artifactId>junit-jupiter</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.junit.vintage</groupId>
+      <artifactId>junit-vintage-engine</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <build>
     <plugins>
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java
index 4cb22792237..ca720ec8b68 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java
@@ -1,21 +1,21 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.orchestrator.resources.hostsuspension;
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.orchestrator.resources;
 
 import com.google.common.util.concurrent.UncheckedTimeoutException;
-import com.yahoo.container.jaxrs.annotation.Component;
+import com.google.inject.Inject;
+import com.yahoo.container.jdisc.LoggingRequestHandler;
+import com.yahoo.jdisc.Response;
+import com.yahoo.restapi.JacksonJsonResponse;
+import com.yahoo.restapi.RestApi;
+import com.yahoo.restapi.RestApiException;
+import com.yahoo.restapi.RestApiRequestHandler;
 import com.yahoo.vespa.applicationmodel.HostName;
 import com.yahoo.vespa.orchestrator.BatchHostNameNotFoundException;
 import com.yahoo.vespa.orchestrator.BatchInternalErrorException;
 import com.yahoo.vespa.orchestrator.Orchestrator;
 import com.yahoo.vespa.orchestrator.policy.BatchHostStateChangeDeniedException;
-import com.yahoo.vespa.orchestrator.restapi.HostSuspensionApi;
 import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult;
 
-import javax.inject.Inject;
-import javax.ws.rs.Path;
-import javax.ws.rs.WebApplicationException;
-import javax.ws.rs.core.MediaType;
-import javax.ws.rs.core.Response;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -23,50 +23,59 @@ import java.util.stream.Collectors;
 
 /**
  * @author hakonhall
+ * @author bjorncs
  */
-@Path("")
-public class HostSuspensionResource implements HostSuspensionApi {
+public class HostSuspensionHandler extends RestApiRequestHandler<HostSuspensionHandler> {
 
-    private static final Logger log = Logger.getLogger(HostSuspensionResource.class.getName());
+    private static final Logger log = Logger.getLogger(HostSuspensionHandler.class.getName());
 
     private final Orchestrator orchestrator;
 
     @Inject
-    public HostSuspensionResource(@Component Orchestrator orchestrator) {
+    public HostSuspensionHandler(LoggingRequestHandler.Context context, Orchestrator orchestrator) {
+        super(context, HostSuspensionHandler::createRestApiDefinition);
         this.orchestrator = orchestrator;
     }
 
-    @Override
-    public BatchOperationResult suspendAll(String parentHostnameString, List<String> hostnamesAsStrings) {
+    private static RestApi createRestApiDefinition(HostSuspensionHandler self) {
+        return RestApi.builder()
+                .addRoute(RestApi.route("/orchestrator/v1/suspensions/hosts/{hostname}")
+                    .put(self::suspendAll))
+                .registerJacksonResponseEntity(BatchOperationResult.class)
+                .build();
+    }
+
+    private BatchOperationResult suspendAll(RestApi.RequestContext context) {
+        String parentHostnameString = context.pathParameters().getStringOrThrow("hostname");
+        List<String> hostnamesAsStrings = context.queryParameters().getStringList("hostname");
+
         HostName parentHostname = new HostName(parentHostnameString);
         List<HostName> hostnames = hostnamesAsStrings.stream().map(HostName::new).collect(Collectors.toList());
         try {
             orchestrator.suspendAll(parentHostname, hostnames);
         } catch (BatchHostStateChangeDeniedException e) {
             log.log(Level.FINE, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e);
-            throw createWebApplicationException(e.getMessage(), Response.Status.CONFLICT);
+            throw createRestApiException(e.getMessage(), Response.Status.CONFLICT, e);
         } catch (UncheckedTimeoutException e) {
             log.log(Level.FINE, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e);
-            throw createWebApplicationException(e.getMessage(), Response.Status.CONFLICT);
+            throw createRestApiException(e.getMessage(), Response.Status.CONFLICT, e);
         } catch (BatchHostNameNotFoundException e) {
             log.log(Level.FINE, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e);
             // Note that we're returning BAD_REQUEST instead of NOT_FOUND because the resource identified
             // by the URL path was found. It's one of the hostnames in the request it failed to find.
-            throw createWebApplicationException(e.getMessage(), Response.Status.BAD_REQUEST);
+            throw createRestApiException(e.getMessage(), Response.Status.BAD_REQUEST, e);
         } catch (BatchInternalErrorException e) {
             log.log(Level.FINE, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e);
-            throw createWebApplicationException(e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR);
+            throw createRestApiException(e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR, e);
         }
         log.log(Level.FINE, "Suspended " + hostnames + " with parent " + parentHostname);
         return BatchOperationResult.successResult();
     }
 
-    private WebApplicationException createWebApplicationException(String errorMessage, Response.Status status) {
-        return new WebApplicationException(
-                Response.status(status)
-                        .entity(new BatchOperationResult(errorMessage))
-                        .type(MediaType.APPLICATION_JSON_TYPE)
-                        .build());
+    private RestApiException createRestApiException(String errorMessage, int statusCode, Throwable cause) {
+        return new RestApiException(
+                new JacksonJsonResponse<>(statusCode, new BatchOperationResult(errorMessage), true),
+                errorMessage,
+                cause);
     }
-
 }
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandlerTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandlerTest.java
new file mode 100644
index 00000000000..9d413526037
--- /dev/null
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandlerTest.java
@@ -0,0 +1,113 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.orchestrator.resources;
+
+import com.google.common.util.concurrent.UncheckedTimeoutException;
+import com.yahoo.container.jdisc.HttpRequest;
+import com.yahoo.container.jdisc.HttpResponse;
+import com.yahoo.container.jdisc.LoggingRequestHandler;
+import com.yahoo.jdisc.test.MockMetric;
+import com.yahoo.test.json.JsonTestHelper;
+import com.yahoo.vespa.orchestrator.BatchHostNameNotFoundException;
+import com.yahoo.vespa.orchestrator.BatchInternalErrorException;
+import com.yahoo.vespa.orchestrator.Orchestrator;
+import com.yahoo.vespa.orchestrator.OrchestratorImpl;
+import com.yahoo.vespa.orchestrator.policy.BatchHostStateChangeDeniedException;
+import com.yahoo.vespa.orchestrator.resources.host.HostResourceTest;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.time.Clock;
+import java.time.Instant;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.stream.Collectors;
+
+import static com.yahoo.jdisc.http.HttpRequest.Method;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author hakonhall
+ * @author bjorncs
+ */
+class HostSuspensionHandlerTest {
+
+    private final Clock clock = mock(Clock.class);
+
+    @BeforeEach
+    public void setUp() {
+        when(clock.instant()).thenReturn(Instant.now());
+    }
+
+    @Test
+    void returns_200_on_success_batch() throws IOException {
+        HostSuspensionHandler handler = createHandler(HostResourceTest.createAlwaysAllowOrchestrator(clock));
+        HttpResponse response = executeSuspendAllRequest(handler, "parentHostname", List.of("hostname1", "hostname2"));
+        assertSuccess(response);
+    }
+
+    @Test
+    void returns_200_empty_batch() throws IOException {
+        HostSuspensionHandler handler = createHandler(HostResourceTest.createAlwaysAllowOrchestrator(clock));
+        HttpResponse response = executeSuspendAllRequest(handler, "parentHostname", List.of());
+        assertSuccess(response);
+    }
+
+    // Note: Missing host is 404 for a single-host, but 400 for multi-host (batch).
+    // This is so because the hostname is part of the URL path for single-host, while the
+    // hostnames are part of the request body for multi-host.
+    @Test
+    void returns_400_when_host_unknown_for_batch() {
+        HostSuspensionHandler handler = createHandler(HostResourceTest.createHostNotFoundOrchestrator(clock));
+        HttpResponse response = executeSuspendAllRequest(handler, "parentHostname", List.of("hostname1", "hostname2"));
+        assertEquals(400, response.getStatus());
+    }
+
+    @Test
+    void returns_409_when_request_rejected_by_policies_for_batch() {
+        OrchestratorImpl alwaysRejectResolver = HostResourceTest.createAlwaysRejectResolver(clock);
+        HostSuspensionHandler handler = createHandler(alwaysRejectResolver);
+        HttpResponse response = executeSuspendAllRequest(handler, "parentHostname", List.of("hostname1", "hostname2"));
+        assertEquals(409, response.getStatus());
+    }
+
+
+    @Test
+    void throws_409_on_suspendAll_timeout() throws BatchHostStateChangeDeniedException, BatchHostNameNotFoundException, BatchInternalErrorException {
+        Orchestrator orchestrator = mock(Orchestrator.class);
+        doThrow(new UncheckedTimeoutException("Timeout Message")).when(orchestrator).suspendAll(any(), any());
+        HostSuspensionHandler handler = createHandler(orchestrator);
+        HttpResponse response = executeSuspendAllRequest(handler, "parenthost", List.of("h1", "h2", "h3"));
+        assertEquals(409, response.getStatus());
+    }
+
+    private static HostSuspensionHandler createHandler(Orchestrator orchestrator) {
+        return new HostSuspensionHandler(
+                new LoggingRequestHandler.Context(Executors.newSingleThreadExecutor(), new MockMetric()),
+                orchestrator);
+    }
+
+    private static HttpResponse executeSuspendAllRequest(HostSuspensionHandler handler, String parentHostname, List<String> hostnames) {
+        StringBuilder uriBuilder = new StringBuilder("/orchestrator/v1/suspensions/hosts/").append(parentHostname);
+        if (!hostnames.isEmpty()) {
+            uriBuilder.append(hostnames.stream()
+                    .map(hostname -> "hostname=" + hostname)
+                    .collect(Collectors.joining("&", "?", "")));
+        }
+        HttpRequest request = HttpRequest.createTestRequest(uriBuilder.toString(), Method.PUT);
+        return handler.handle(request);
+    }
+
+    private static void assertSuccess(HttpResponse response) throws IOException {
+        assertEquals(200, response.getStatus());
+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+        response.render(out);
+        JsonTestHelper.assertJsonEquals(out.toString(), "{\"failure-reason\": null}");
+    }
+
+}
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/host/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/host/HostResourceTest.java
index b86dfb71e43..d056c3730fd 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/host/HostResourceTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/host/HostResourceTest.java
@@ -17,8 +17,6 @@ import com.yahoo.vespa.applicationmodel.ServiceType;
 import com.yahoo.vespa.applicationmodel.TenantId;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
-import com.yahoo.vespa.orchestrator.BatchHostNameNotFoundException;
-import com.yahoo.vespa.orchestrator.BatchInternalErrorException;
 import com.yahoo.vespa.orchestrator.DummyAntiServiceMonitor;
 import com.yahoo.vespa.orchestrator.Host;
 import com.yahoo.vespa.orchestrator.HostNameNotFoundException;
@@ -29,12 +27,9 @@ import com.yahoo.vespa.orchestrator.OrchestratorImpl;
 import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMock;
 import com.yahoo.vespa.orchestrator.model.ApplicationApi;
 import com.yahoo.vespa.orchestrator.model.ApplicationApiFactory;
-import com.yahoo.vespa.orchestrator.policy.BatchHostStateChangeDeniedException;
 import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException;
 import com.yahoo.vespa.orchestrator.policy.Policy;
 import com.yahoo.vespa.orchestrator.policy.SuspensionReasons;
-import com.yahoo.vespa.orchestrator.resources.hostsuspension.HostSuspensionResource;
-import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult;
 import com.yahoo.vespa.orchestrator.restapi.wire.GetHostResponse;
 import com.yahoo.vespa.orchestrator.restapi.wire.PatchHostRequest;
 import com.yahoo.vespa.orchestrator.restapi.wire.PatchHostResponse;
@@ -57,15 +52,12 @@ import javax.ws.rs.core.UriInfo;
 import java.net.URI;
 import java.time.Clock;
 import java.time.Instant;
-import java.util.Arrays;
 import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doThrow;
@@ -96,8 +88,6 @@ public class HostResourceTest {
                                 Set.of())));
     }
 
-    private final InMemoryFlagSource flagSource = new InMemoryFlagSource();
-
     private static final ServiceMonitor alwaysEmptyServiceMonitor = new ServiceMonitor() {
         private final ServiceModel emptyServiceModel = new ServiceModel(Map.of());
 
@@ -129,26 +119,8 @@ public class HostResourceTest {
         }
     }
 
-    private final OrchestratorImpl alwaysAllowOrchestrator = new OrchestratorImpl(
-            new AlwaysAllowPolicy(),
-            new ClusterControllerClientFactoryMock(),
-            EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
-            serviceMonitor,
-            SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
-            clock,
-            applicationApiFactory,
-            flagSource);
-
-    private final OrchestratorImpl hostNotFoundOrchestrator = new OrchestratorImpl(
-            new AlwaysAllowPolicy(),
-            new ClusterControllerClientFactoryMock(),
-            EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
-            alwaysEmptyServiceMonitor,
-            SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
-            clock,
-            applicationApiFactory,
-            flagSource);
-
+    private final OrchestratorImpl alwaysAllowOrchestrator = createAlwaysAllowOrchestrator(clock);
+    private final OrchestratorImpl hostNotFoundOrchestrator = createHostNotFoundOrchestrator(clock);
     private final UriInfo uriInfo = mock(UriInfo.class);
 
     @Before
@@ -156,6 +128,42 @@ public class HostResourceTest {
         when(clock.instant()).thenReturn(Instant.now());
     }
 
+    public static OrchestratorImpl createAlwaysAllowOrchestrator(Clock clock) {
+        return new OrchestratorImpl(
+                new AlwaysAllowPolicy(),
+                new ClusterControllerClientFactoryMock(),
+                EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
+                serviceMonitor,
+                SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
+                clock,
+                applicationApiFactory,
+                new InMemoryFlagSource());
+    }
+
+    public static OrchestratorImpl createHostNotFoundOrchestrator(Clock clock) {
+        return new OrchestratorImpl(
+                new AlwaysAllowPolicy(),
+                new ClusterControllerClientFactoryMock(),
+                EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
+                alwaysEmptyServiceMonitor,
+                SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
+                clock,
+                applicationApiFactory,
+                new InMemoryFlagSource());
+    }
+
+    public static OrchestratorImpl createAlwaysRejectResolver(Clock clock) {
+        return new OrchestratorImpl(
+                new HostResourceTest.AlwaysFailPolicy(),
+                new ClusterControllerClientFactoryMock(),
+                EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
+                serviceMonitor,
+                SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
+                clock,
+                applicationApiFactory,
+                new InMemoryFlagSource());
+    }
+
     @Test
     public void returns_200_on_success() {
         HostResource hostResource =
@@ -169,21 +177,6 @@ public class HostResourceTest {
     }
 
     @Test
-    public void returns_200_on_success_batch() {
-        HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysAllowOrchestrator);
-        BatchOperationResult response = hostSuspensionResource.suspendAll("parentHostname",
-                                                                          Arrays.asList("hostname1", "hostname2"));
-        assertTrue(response.success());
-    }
-
-    @Test
-    public void returns_200_empty_batch() {
-        HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysAllowOrchestrator);
-        BatchOperationResult response = hostSuspensionResource.suspendAll("parentHostname", List.of());
-        assertTrue(response.success());
-    }
-
-    @Test
     public void throws_404_when_host_unknown() {
         try {
             HostResource hostResource =
@@ -195,20 +188,6 @@ public class HostResourceTest {
         }
     }
 
-    // Note: Missing host is 404 for a single-host, but 400 for multi-host (batch).
-    // This is so because the hostname is part of the URL path for single-host, while the
-    // hostnames are part of the request body for multi-host.
-    @Test
-    public void throws_400_when_host_unknown_for_batch() {
-        try {
-            HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(hostNotFoundOrchestrator);
-            hostSuspensionResource.suspendAll("parentHostname", Arrays.asList("hostname1", "hostname2"));
-            fail();
-        } catch (WebApplicationException w) {
-            assertEquals(400, w.getResponse().getStatus());
-        }
-    }
-
     private static class AlwaysFailPolicy implements Policy {
         @Override
         public SuspensionReasons grantSuspensionRequest(OrchestratorContext context, ApplicationApi applicationApi) throws HostStateChangeDeniedException {
@@ -251,7 +230,7 @@ public class HostResourceTest {
                 SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
                 clock,
                 applicationApiFactory,
-                flagSource);
+                new InMemoryFlagSource());
 
         try {
             HostResource hostResource = new HostResource(alwaysRejectResolver, uriInfo);
@@ -262,27 +241,6 @@ public class HostResourceTest {
         }
     }
 
-    @Test
-    public void throws_409_when_request_rejected_by_policies_for_batch() {
-        final OrchestratorImpl alwaysRejectResolver = new OrchestratorImpl(
-                new AlwaysFailPolicy(),
-                new ClusterControllerClientFactoryMock(),
-                EVERY_HOST_IS_UP_HOST_STATUS_SERVICE,
-                serviceMonitor,
-                SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS,
-                clock,
-                applicationApiFactory,
-                flagSource);
-
-        try {
-            HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysRejectResolver);
-            hostSuspensionResource.suspendAll("parentHostname", Arrays.asList("hostname1", "hostname2"));
-            fail();
-        } catch (WebApplicationException w) {
-            assertEquals(409, w.getResponse().getStatus());
-        }
-    }
-
     @Test(expected = BadRequestException.class)
     public void patch_state_may_throw_bad_request() {
         Orchestrator orchestrator = mock(Orchestrator.class);
@@ -381,17 +339,4 @@ public class HostResourceTest {
         }
     }
 
-    @Test
-    public void throws_409_on_suspendAll_timeout() throws BatchHostStateChangeDeniedException, BatchHostNameNotFoundException, BatchInternalErrorException {
-        Orchestrator orchestrator = mock(Orchestrator.class);
-        doThrow(new UncheckedTimeoutException("Timeout Message")).when(orchestrator).suspendAll(any(), any());
-
-        try {
-            HostSuspensionResource resource = new HostSuspensionResource(orchestrator);
-            resource.suspendAll("parenthost", Arrays.asList("h1", "h2", "h3"));
-            fail();
-        } catch (WebApplicationException w) {
-            assertEquals(409, w.getResponse().getStatus());
-        }
-    }
 }