Merge pull request #2263 from yahoo/freva/do-not-throw-runtime-exception-on-non-200-response

Do not throw RuntimeException on non-200 response

6 files changed, 39 insertions, 38 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
index 35582c1bc2e..c3755fc8d5b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
@@ -21,7 +21,6 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import java.util.stream.Collectors;
 
 /**
@@ -33,10 +32,10 @@ public class NodeRepositoryImpl implements NodeRepository {
     private final int port;
     private final ConfigServerHttpRequestExecutor requestExecutor;
 
-    public NodeRepositoryImpl(Set<String> configServerHosts, int configPort, String baseHostName) {
+    public NodeRepositoryImpl(ConfigServerHttpRequestExecutor requestExecutor, int configPort, String baseHostName) {
         this.baseHostName = baseHostName;
         this.port = configPort;
-        this.requestExecutor = ConfigServerHttpRequestExecutor.create(configServerHosts);
+        this.requestExecutor = requestExecutor;
     }
 
     @Override
@@ -154,21 +153,25 @@ public class NodeRepositoryImpl implements NodeRepository {
 
     @Override
     public void markAsDirty(String hostName) {
-        NodeMessageResponse response = requestExecutor.put(
-                "/nodes/v2/state/dirty/" + hostName,
-                port,
-                Optional.empty(), /* body */
-                NodeMessageResponse.class);
-        NODE_ADMIN_LOGGER.info(response.message);
+        markNodeToState(hostName, Node.State.dirty);
     }
 
     @Override
     public void markAsReady(final String hostName) {
+        markNodeToState(hostName, Node.State.ready);
+    }
+
+    private void markNodeToState(String hostName, Node.State state) {
         NodeMessageResponse response = requestExecutor.put(
-                "/nodes/v2/state/ready/" + hostName,
+                "/nodes/v2/state/" + state + "/" + hostName,
                 port,
                 Optional.empty(), /* body */
                 NodeMessageResponse.class);
         NODE_ADMIN_LOGGER.info(response.message);
+
+        if (response.errorCode == null || response.errorCode.isEmpty()) {
+            return;
+        }
+        throw new RuntimeException("Unexpected message " + response.message + " " + response.errorCode);
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/bindings/NodeMessageResponse.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/bindings/NodeMessageResponse.java
index 929cf62ce74..8dd293013f3 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/bindings/NodeMessageResponse.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/bindings/NodeMessageResponse.java
@@ -13,4 +13,6 @@ import com.fasterxml.jackson.annotation.JsonProperty;
 public class NodeMessageResponse {
     @JsonProperty("message")
     public String message;
+    @JsonProperty("error-code")
+    public String errorCode;
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
index 5d227b26eb4..e9e5e15712d 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
@@ -62,8 +62,9 @@ public class ComponentsProviderImpl implements ComponentsProvider {
             throw new IllegalStateException("Environment setting for config servers missing or empty.");
         }
 
-        Orchestrator orchestrator = new OrchestratorImpl(ConfigServerHttpRequestExecutor.create(configServerHosts));
-        NodeRepository nodeRepository = new NodeRepositoryImpl(configServerHosts, WEB_SERVICE_PORT, baseHostName);
+        ConfigServerHttpRequestExecutor requestExecutor = ConfigServerHttpRequestExecutor.create(configServerHosts);
+        Orchestrator orchestrator = new OrchestratorImpl(requestExecutor);
+        NodeRepository nodeRepository = new NodeRepositoryImpl(requestExecutor, WEB_SERVICE_PORT, baseHostName);
         DockerOperations dockerOperations = new DockerOperationsImpl(docker, environment, metricReceiver);
 
         Optional<StorageMaintainer> storageMaintainer = isRunningLocally ?
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
index 239829bc094..87eb80c1837 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
@@ -96,12 +96,7 @@ public class ConfigServerHttpRequestExecutor {
                     if (response.getStatusLine().getStatusCode() == Response.Status.NOT_FOUND.getStatusCode()) {
                         throw new NotFoundException("Not found returned from " + configServer);
                     }
-                    if (response.getStatusLine().getStatusCode() != Response.Status.OK.getStatusCode()) {
-                        String entity = read(response.getEntity());
-                        NODE_ADMIN_LOGGER.info("Non-200 HTTP response code received:\n" + entity);
-                        throw new RuntimeException("Did not get response code 200, but " + response.getStatusLine().getStatusCode() +
-                                entity);
-                    }
+
                     try {
                         return mapper.readValue(response.getEntity().getContent(), wantedReturnType);
                     } catch (IOException e) {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
index 1e027bdd4a6..5c6717075dc 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
@@ -2,12 +2,12 @@
 
 package com.yahoo.vespa.hosted.node.admin.noderepository;
 
-import com.google.common.collect.Sets;
 import com.yahoo.application.Networking;
 import com.yahoo.application.container.JDisc;
 import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
 import com.yahoo.vespa.hosted.dockerapi.DockerImage;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAttributes;
+import com.yahoo.vespa.hosted.node.admin.util.ConfigServerHttpRequestExecutor;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.testutils.ContainerConfig;
 
@@ -18,9 +18,9 @@ import org.junit.Test;
 import java.io.IOException;
 import java.net.ServerSocket;
 import java.time.Instant;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
-import java.util.Set;
 
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertFalse;
@@ -36,7 +36,8 @@ import static org.junit.Assert.fail;
 public class NodeRepositoryImplTest {
     private JDisc container;
     private int port;
-    private final Set<String> configServerHosts = Sets.newHashSet("127.0.0.1");
+    private final ConfigServerHttpRequestExecutor requestExecutor = ConfigServerHttpRequestExecutor.create(
+            Collections.singleton("127.0.0.1"));
 
 
     private int findRandomOpenPort() throws IOException {
@@ -62,7 +63,7 @@ public class NodeRepositoryImplTest {
 
     private void waitForJdiscContainerToServe() throws InterruptedException {
         Instant start = Instant.now();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(Sets.newHashSet("127.0.0.1"), port, "foobar");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "foobar");
         while (Instant.now().minusSeconds(120).isBefore(start)) {
             try {
                 nodeRepositoryApi.getContainersToRun();
@@ -84,7 +85,7 @@ public class NodeRepositoryImplTest {
     @Test
     public void testGetContainersToRunAPi() throws IOException, InterruptedException {
         waitForJdiscContainerToServe();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         final List<ContainerNodeSpec> containersToRun = nodeRepositoryApi.getContainersToRun();
         assertThat(containersToRun.size(), is(1));
         final ContainerNodeSpec nodeSpec = containersToRun.get(0);
@@ -101,7 +102,7 @@ public class NodeRepositoryImplTest {
     @Test
     public void testGetContainer() throws InterruptedException, IOException {
         waitForJdiscContainerToServe();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         String hostname = "host4.yahoo.com";
         Optional<ContainerNodeSpec> nodeSpec = nodeRepositoryApi.getContainerNodeSpec(hostname);
         assertThat(nodeSpec.isPresent(), is(true));
@@ -111,7 +112,7 @@ public class NodeRepositoryImplTest {
     @Test
     public void testGetContainerForNonExistingNode() throws InterruptedException, IOException {
         waitForJdiscContainerToServe();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         String hostname = "host-that-does-not-exist";
         Optional<ContainerNodeSpec> nodeSpec = nodeRepositoryApi.getContainerNodeSpec(hostname);
         assertFalse(nodeSpec.isPresent());
@@ -120,7 +121,7 @@ public class NodeRepositoryImplTest {
     @Test
     public void testUpdateNodeAttributes() throws InterruptedException, IOException {
         waitForJdiscContainerToServe();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         String hostname = "host4.yahoo.com";
         nodeRepositoryApi.updateNodeAttributes(
                 hostname,
@@ -133,7 +134,7 @@ public class NodeRepositoryImplTest {
     @Test(expected = RuntimeException.class)
     public void testUpdateNodeAttributesWithBadValue() throws InterruptedException, IOException {
         waitForJdiscContainerToServe();
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         String hostname = "host4.yahoo.com";
         nodeRepositoryApi.updateNodeAttributes(
                 hostname,
@@ -145,7 +146,7 @@ public class NodeRepositoryImplTest {
 
     @Test
     public void testMarkAsReady() throws InterruptedException, IOException {
-        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(configServerHosts, port, "dockerhost4");
+        NodeRepository nodeRepositoryApi = new NodeRepositoryImpl(requestExecutor, port, "dockerhost4");
         waitForJdiscContainerToServe();
 
         nodeRepositoryApi.markAsReady("host55.yahoo.com");
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
index cc1db1eb3c7..7d6a95d8dc8 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
@@ -5,13 +5,11 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.yahoo.collections.ArraySet;
 import org.apache.http.HttpEntity;
-import org.apache.http.HttpResponse;
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.junit.Test;
-import org.mockito.stubbing.Answer;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -22,6 +20,7 @@ import java.util.Set;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
 import static org.hamcrest.junit.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
@@ -40,6 +39,8 @@ public class ConfigServerHttpRequestExecutorTest {
     public static class TestPojo {
         @JsonProperty("foo")
         public String foo;
+        @JsonProperty("error-code")
+        public Integer errorCode;
     }
 
     private final StringBuilder mockLog = new StringBuilder();
@@ -47,7 +48,7 @@ public class ConfigServerHttpRequestExecutorTest {
 
     private CloseableHttpClient createClientMock() throws IOException {
         CloseableHttpClient httpMock = mock(CloseableHttpClient.class);
-        when(httpMock.execute(any())).thenAnswer((Answer<HttpResponse>) invocationOnMock -> {
+        when(httpMock.execute(any())).thenAnswer(invocationOnMock -> {
             HttpGet get = (HttpGet) invocationOnMock.getArguments()[0];
             mockLog.append(get.getMethod()).append(" ").append(get.getURI()).append("  ");
             CloseableHttpResponse response = mock(CloseableHttpResponse.class);
@@ -57,7 +58,8 @@ public class ConfigServerHttpRequestExecutorTest {
             if (mockReturnCode == 100000) throw new RuntimeException("FAIL");
             HttpEntity entity = mock(HttpEntity.class);
             when(response.getEntity()).thenReturn(entity);
-            InputStream stream = new ByteArrayInputStream("{ \"foo\":\"bar\", \"no\":3}".getBytes(StandardCharsets.UTF_8));
+            String returnMessage = "{\"foo\":\"bar\", \"no\":3, \"error-code\": " + mockReturnCode + "}";
+            InputStream stream = new ByteArrayInputStream(returnMessage.getBytes(StandardCharsets.UTF_8));
             when(entity.getContent()).thenReturn(stream);
             return response;
         });
@@ -84,12 +86,9 @@ public class ConfigServerHttpRequestExecutorTest {
         // Server is returning 400, no retries.
         mockReturnCode = 400;
         ConfigServerHttpRequestExecutor executor = new ConfigServerHttpRequestExecutor(configServers, createClientMock());
-        try {
-            executor.get("/path", 666, TestPojo.class);
-            fail("Expected failure");
-        } catch (Exception e) {
-            // ignore
-        }
+
+        TestPojo testPojo = executor.get("/path", 666, TestPojo.class);
+        assertEquals(testPojo.errorCode.intValue(), mockReturnCode);
         assertLogStringContainsGETForAHost();
     }