From 0ad7e2a305997e1aee7920f63384e9a27491d51f Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 26 Mar 2021 18:00:57 +0100 Subject: Convert HostSuspensionResource to request handler --- .../resources/HostSuspensionHandler.java | 81 +++++++++++++ .../hostsuspension/HostSuspensionResource.java | 72 ----------- .../resources/HostSuspensionHandlerTest.java | 113 +++++++++++++++++ .../resources/host/HostResourceTest.java | 133 ++++++--------------- 4 files changed, 233 insertions(+), 166 deletions(-) create mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java delete mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java create mode 100644 orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandlerTest.java (limited to 'orchestrator/src') diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java new file mode 100644 index 00000000000..ca720ec8b68 --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionHandler.java @@ -0,0 +1,81 @@ +// 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.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.wire.BatchOperationResult; + +import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.stream.Collectors; + +/** + * @author hakonhall + * @author bjorncs + */ +public class HostSuspensionHandler extends RestApiRequestHandler { + + private static final Logger log = Logger.getLogger(HostSuspensionHandler.class.getName()); + + private final Orchestrator orchestrator; + + @Inject + public HostSuspensionHandler(LoggingRequestHandler.Context context, Orchestrator orchestrator) { + super(context, HostSuspensionHandler::createRestApiDefinition); + this.orchestrator = orchestrator; + } + + 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 hostnamesAsStrings = context.queryParameters().getStringList("hostname"); + + HostName parentHostname = new HostName(parentHostnameString); + List 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 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 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 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 createRestApiException(e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR, e); + } + log.log(Level.FINE, "Suspended " + hostnames + " with parent " + parentHostname); + return BatchOperationResult.successResult(); + } + + 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/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java deleted file mode 100644 index 4cb22792237..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/hostsuspension/HostSuspensionResource.java +++ /dev/null @@ -1,72 +0,0 @@ -// 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; - -import com.google.common.util.concurrent.UncheckedTimeoutException; -import com.yahoo.container.jaxrs.annotation.Component; -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; -import java.util.stream.Collectors; - -/** - * @author hakonhall - */ -@Path("") -public class HostSuspensionResource implements HostSuspensionApi { - - private static final Logger log = Logger.getLogger(HostSuspensionResource.class.getName()); - - private final Orchestrator orchestrator; - - @Inject - public HostSuspensionResource(@Component Orchestrator orchestrator) { - this.orchestrator = orchestrator; - } - - @Override - public BatchOperationResult suspendAll(String parentHostnameString, List hostnamesAsStrings) { - HostName parentHostname = new HostName(parentHostnameString); - List 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); - } catch (UncheckedTimeoutException e) { - log.log(Level.FINE, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e); - throw createWebApplicationException(e.getMessage(), Response.Status.CONFLICT); - } 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); - } 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); - } - 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()); - } - -} 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 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 = @@ -168,21 +176,6 @@ public class HostResourceTest { assertEquals(hostName, response.hostname()); } - @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 { @@ -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()); - } - } } -- cgit v1.2.3