From c3708915c4964e5c8dfaf6e3333a4d8a09e71ae9 Mon Sep 17 00:00:00 2001 From: Haakon Dybdahl Date: Thu, 23 Jun 2016 08:03:04 +0200 Subject: Code review based improvements. --- .../java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java | 2 +- .../com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java | 4 ++-- .../vespa/hosted/node/admin/NodeAdminStateUpdater.java | 11 +---------- .../java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java | 4 ++-- .../com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java | 11 ++++++----- .../vespa/hosted/node/admin/restapi/RestApiHandler.java | 14 +++++++++++--- .../yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java | 10 +++++----- .../vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java | 3 +-- .../hosted/node/admin/integrationTests/NodeAdminMock.java | 2 +- .../node/admin/integrationTests/RunInContainerTest.java | 2 +- 10 files changed, 31 insertions(+), 32 deletions(-) (limited to 'node-admin') diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java index 147f7cdab9f..863a8f47965 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java @@ -11,7 +11,7 @@ import java.util.Set; */ public interface NodeAdmin { - void setState(final List containersToRun); + void refreshContainersToRun(final List containersToRun); boolean freezeAndCheckIfAllFrozen(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java index ce59c19ec5a..f9e131e3d6c 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java @@ -48,7 +48,7 @@ public class NodeAdminImpl implements NodeAdmin { this.nodeAgentFactory = nodeAgentFactory; } - public void setState(final List containersToRun) { + public void refreshContainersToRun(final List containersToRun) { final List existingContainers = docker.getAllManagedContainers(); synchronizeLocalContainerState(containersToRun, existingContainers); @@ -153,7 +153,7 @@ public class NodeAdminImpl implements NodeAdmin { .map(spec -> spec.hostname) .collect(Collectors.toSet()); final Set obsoleteAgentHostNames = diff(nodeAgents.keySet(), nodeHostNames); - obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).terminate()); + obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).stop()); nodeSpecContainerPairs.forEach(nodeSpecContainerPair -> { final Optional nodeSpec = nodeSpecContainerPair.getFirst(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java index 97bcdfa724a..191da411f02 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java @@ -1,31 +1,22 @@ // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin; -import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; -import com.yahoo.vespa.applicationmodel.HostName; -import com.yahoo.vespa.hosted.node.admin.docker.Docker; import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository; -import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepositoryImpl; import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator; -import com.yahoo.vespa.hosted.node.admin.orchestrator.OrchestratorImpl; -import com.yahoo.vespa.hosted.node.admin.util.Environment; import java.util.ArrayList; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.function.Function; import java.util.logging.Level; import java.util.logging.Logger; import static com.yahoo.vespa.hosted.node.admin.NodeAdminStateUpdater.State.RESUMED; import static com.yahoo.vespa.hosted.node.admin.NodeAdminStateUpdater.State.SUSPENDED; import static java.util.concurrent.TimeUnit.MILLISECONDS; -import static java.util.concurrent.TimeUnit.SECONDS; /** * Pulls information from node repository and forwards containers to run to node admin. @@ -113,7 +104,7 @@ public class NodeAdminStateUpdater extends AbstractComponent { return; } try { - nodeAdmin.setState(containersToRun); + nodeAdmin.refreshContainersToRun(containersToRun); } catch (Throwable t) { log.log(Level.WARNING, "Failed updating node admin: ", t); return; diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java index eac1b228378..4d5e19e4bce 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java @@ -11,7 +11,7 @@ package com.yahoo.vespa.hosted.node.admin; public interface NodeAgent { enum Command {UPDATE_FROM_NODE_REPO, FREEZE, UNFREEZE} - enum State {WAITING, WORKING, DIRTY, FROZEN, TERMINATED} + enum State {WAITING, WORKING, FROZEN, TERMINATED} /** * Signals to the agent that it should update the node specification and container state and maintain wanted state. @@ -40,5 +40,5 @@ public interface NodeAgent { * Cleans up any resources the agent owns, such as threads, connections etc. Cleanup is synchronous; when this * method returns, no more actions will be taken by the agent. */ - void terminate(); + void stop(); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java index d4af21900c3..aa7e76780ab 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java @@ -105,18 +105,18 @@ public class NodeAgentImpl implements NodeAgent { logger.log(LogLevel.INFO, logPrefix + "Scheduling start of NodeAgent"); synchronized (monitor) { if (state == State.TERMINATED) { - throw new IllegalStateException("Cannot re-start a stopped node agent"); + throw new IllegalStateException("Cannot re-start a stopped (terminated) node agent"); } } thread.start(); } @Override - public void terminate() { + public void stop() { logger.log(LogLevel.INFO, logPrefix + "Scheduling stop of NodeAgent"); synchronized (monitor) { if (state == State.TERMINATED) { - throw new IllegalStateException("Cannot stop an already stopped node agent"); + throw new IllegalStateException("Cannot stop an already stopped (terminated) node agent"); } wantedState = State.TERMINATED; } @@ -400,7 +400,7 @@ public class NodeAgentImpl implements NodeAgent { } } } catch (InterruptedException e) { - throw new RuntimeException(e); + logger.severe("NodeAgent thread interrupted. Ignoring this: " + e.getMessage()); } } @@ -429,7 +429,8 @@ public class NodeAgentImpl implements NodeAgent { new IllegalStateException(String.format("Node '%s' missing from node repository.", hostname))); final Optional existingContainer = docker.getContainer(hostname); synchronizeLocalContainerState(nodeSpec, existingContainer); - } catch (Exception e) { + } catch (Throwable e) { + // TODO: We should probably halt the VM at this stage so the containers survive and NodeAdmin get into a fresh state. logger.log(LogLevel.ERROR, logPrefix + "Unhandled exception.", e); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java index 06fb9f63148..aea9905ca5c 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java @@ -1,5 +1,7 @@ package com.yahoo.vespa.hosted.node.admin.restapi; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.container.jdisc.LoggingRequestHandler; @@ -29,6 +31,7 @@ import static com.yahoo.jdisc.http.HttpRequest.Method.PUT; public class RestApiHandler extends LoggingRequestHandler{ private final NodeAdminStateUpdater refresher; + private final static ObjectMapper objectMapper = new ObjectMapper(); public RestApiHandler(Executor executor, AccessLog accessLog, ComponentsProvider componentsProvider) { super(executor, accessLog); @@ -48,7 +51,11 @@ public class RestApiHandler extends LoggingRequestHandler{ } private HttpResponse handleGet(HttpRequest request) { - return new SimpleResponse(200, refresher.getDebugPage()); + String path = request.getUri().getPath(); + if (path.endsWith("info")) { + return new SimpleResponse(200, refresher.getDebugPage()); + } + return new SimpleResponse(400, "unknown path" + path); } private HttpResponse handlePut(HttpRequest request) { @@ -77,8 +84,9 @@ public class RestApiHandler extends LoggingRequestHandler{ public SimpleResponse(int code, String message) { super(code); - // TODO: Use some library to build json as this easily fails - this.jsonMessage = "{ \"jsonMessage\":\"" + message + "\"}"; + ObjectNode objectNode = objectMapper.createObjectNode(); + objectNode.put("jsonMessage", message); + this.jsonMessage = objectNode.toString(); } @Override diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java index f3bfeb47e9f..737dd25092a 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java @@ -77,30 +77,30 @@ public class NodeAdminImplTest { inOrder.verify(nodeAgentFactory).apply(hostName); inOrder.verify(nodeAgent1).start(); inOrder.verify(nodeAgent1).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO); - inOrder.verify(nodeAgent1, never()).terminate(); + inOrder.verify(nodeAgent1, never()).stop(); nodeAdmin.synchronizeLocalContainerState(asList(nodeSpec), asList(existingContainer)); inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class)); inOrder.verify(nodeAgent1, never()).start(); inOrder.verify(nodeAgent1).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO); - inOrder.verify(nodeAgent1, never()).terminate(); + inOrder.verify(nodeAgent1, never()).stop(); nodeAdmin.synchronizeLocalContainerState(Collections.emptyList(), asList(existingContainer)); inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class)); inOrder.verify(nodeAgent1, never()).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO); - verify(nodeAgent1).terminate(); + verify(nodeAgent1).stop(); nodeAdmin.synchronizeLocalContainerState(asList(nodeSpec), asList(existingContainer)); inOrder.verify(nodeAgentFactory).apply(hostName); inOrder.verify(nodeAgent2).start(); inOrder.verify(nodeAgent2).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO); - inOrder.verify(nodeAgent2, never()).terminate(); + inOrder.verify(nodeAgent2, never()).stop(); nodeAdmin.synchronizeLocalContainerState(Collections.emptyList(), Collections.emptyList()); inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class)); inOrder.verify(nodeAgent2, never()).start(); inOrder.verify(nodeAgent2, never()).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO); - inOrder.verify(nodeAgent2).terminate(); + inOrder.verify(nodeAgent2).stop(); verifyNoMoreInteractions(nodeAgent1); verifyNoMoreInteractions(nodeAgent2); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java index f9d9ffdf0be..2b8517ab33b 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java @@ -6,7 +6,6 @@ import com.yahoo.vespa.hosted.node.admin.docker.ContainerName; import com.yahoo.vespa.hosted.node.admin.integrationTests.OrchestratorMock; import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository; import com.yahoo.vespa.hosted.node.admin.noderepository.NodeState; -import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator; import org.junit.Test; import org.mockito.stubbing.Answer; @@ -45,7 +44,7 @@ public class NodeAdminStateUpdaterTest { throw new RuleBaseException("This exception is expected, and should show up in the log."); } return null; - }).when(nodeAdmin).setState(anyList()); + }).when(nodeAdmin).refreshContainersToRun(anyList()); final List containersToRun = new ArrayList<>(); containersToRun.add(createSample()); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java index 4043195ea5e..0f042162de3 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java @@ -27,7 +27,7 @@ public class NodeAdminMock implements NodeAdmin { private Object monitor = new Object(); @Override - public void setState(List containersToRun) { + public void refreshContainersToRun(List containersToRun) { synchronized (monitor) { hostnames.clear(); containersToRun.forEach(container -> hostnames.add(container.hostname)); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java index b704e4624b7..21862f4598f 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java @@ -107,7 +107,7 @@ public class RunInContainerTest { waitForJdiscContainerToServe(); assertThat(doPutCall("resume"), is(true)); assertThat(doPutCall("suspend"), is(false)); - assertThat(doGetInfoCall(), is("{ \"jsonMessage\":\"isRunningUpdates is false. NodeAdmin: " + + assertThat(doGetInfoCall(), is("{\"jsonMessage\":\"isRunningUpdates is false. NodeAdmin: " + "Unfreeze called while in state false " + "Freeze called while in state false\"}")); } -- cgit v1.2.3