Merge pull request #1033 from spotify/dxia/patch1

Fix NPE in HostsResource
spotify · Dec 6, 2016 · 0367d6f · 0367d6f
2 parents 5d3a200 + 91385cb
commit 0367d6f
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 17 deletions.
diff --git a/helios-services/src/main/java/com/spotify/helios/master/HostMatcher.java b/helios-services/src/main/java/com/spotify/helios/master/HostMatcher.java
@@ -65,6 +65,9 @@ public List<String> getMatchingHosts(final List<HostSelector> selectors) {
     for (final Map.Entry<String, Map<String, String>> entry : hostsAndLabels.entrySet()) {
       final String host = entry.getKey();
       final Map<String, String> hostLabels = entry.getValue();
+      if (hostLabels == null) {
+        continue;
+      }
 
       // every hostSelector in the group has to have a match in this host.
       // a match meaning the host has a label for that key and the value matches

diff --git a/helios-services/src/main/java/com/spotify/helios/master/MasterModel.java b/helios-services/src/main/java/com/spotify/helios/master/MasterModel.java
@@ -44,6 +44,11 @@ public interface MasterModel {
 
   HostStatus getHostStatus(String host);
 
+  /**
+   * Returns labels for {@code host}. Returns an empty map for hosts not found in the store.
+   */
+  Map<String, String> getHostLabels(String host);
+
   void addJob(Job job) throws JobExistsException;
 
   Job getJob(JobId jobId);

diff --git a/helios-services/src/main/java/com/spotify/helios/master/ZooKeeperMasterModel.java b/helios-services/src/main/java/com/spotify/helios/master/ZooKeeperMasterModel.java
@@ -1692,17 +1692,10 @@ public Deployment getDeployment(final String host, final JobId jobId) {
    */
   @Override
   public HostStatus getHostStatus(final String host) {
-    final Stat stat;
     final ZooKeeperClient client = provider.get("getHostStatus");
 
-    try {
-      stat = client.exists(Paths.configHostId(host));
-    } catch (KeeperException e) {
-      throw new HeliosRuntimeException("Failed to check host status", e);
-    }
-
-    if (stat == null) {
-      log.warn("Missing configuration for host {}", host);
+    if (!ZooKeeperRegistrarUtil.isHostRegistered(client, host)) {
+      log.warn("Host {} isn't registered in ZooKeeper.", host);
       return null;
     }
 
@@ -1725,6 +1718,18 @@ public HostStatus getHostStatus(final String host) {
         .build();
   }
 
+  @Override
+  public Map<String, String> getHostLabels(final String host) {
+    final ZooKeeperClient client = provider.get("getHostStatus");
+
+    if (!ZooKeeperRegistrarUtil.isHostRegistered(client, host)) {
+      return emptyMap();
+    }
+
+    final Map<String, String> labels = getLabels(client, host);
+    return labels == null ? emptyMap() : labels;
+  }
+
   private <T> T tryGetEntity(final ZooKeeperClient client, String path, TypeReference<T> type,
                              String name) {
     try {

diff --git a/helios-services/src/main/java/com/spotify/helios/master/resources/HostsResource.java b/helios-services/src/main/java/com/spotify/helios/master/resources/HostsResource.java
@@ -122,11 +122,7 @@ public List<String> list(@QueryParam("namePattern") final String namePattern,
           })
           .collect(Collectors.toList());
 
-      final Map<String, Map<String, String>> hostsAndLabels = hosts.stream()
-          .collect(Collectors.toMap(Function.identity(),
-                                    host -> model.getHostStatus(host).getLabels()
-                   )
-          );
+      final Map<String, Map<String, String>> hostsAndLabels = getLabels(hosts);
 
       final HostMatcher matcher = new HostMatcher(hostsAndLabels);
       hosts = matcher.getMatchingHosts(selectors);
@@ -361,4 +357,8 @@ public Optional<Deployment> jobGet(@PathParam("host") final String host,
     }
     return Optional.fromNullable(model.getDeployment(host, jobId));
   }
+
+  private Map<String, Map<String, String>> getLabels(final List<String> hosts) {
+    return hosts.stream().collect(Collectors.toMap(Function.identity(), model::getHostLabels));
+  }
 }
diff --git a/helios-services/src/main/java/com/spotify/helios/servicescommon/ZooKeeperRegistrarUtil.java b/helios-services/src/main/java/com/spotify/helios/servicescommon/ZooKeeperRegistrarUtil.java
@@ -30,6 +30,7 @@
 
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.KeeperException.NoNodeException;
+import org.apache.zookeeper.data.Stat;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -45,6 +46,15 @@ public class ZooKeeperRegistrarUtil {
 
   private static final Logger log = LoggerFactory.getLogger(ZooKeeperRegistrarUtil.class);
 
+  public static boolean isHostRegistered(final ZooKeeperClient client, final String host) {
+    try {
+      final Stat stat = client.exists(Paths.configHostId(host));
+      return stat != null;
+    } catch (KeeperException e) {
+      throw new HeliosRuntimeException("getting host " + host + " id failed", e);
+    }
+  }
+
   public static void registerHost(final ZooKeeperClient client, final String idPath,
                                   final String hostname, final String hostId)
       throws KeeperException {

diff --git a/helios-services/src/test/java/com/spotify/helios/master/resources/HostsResourceTest.java b/helios-services/src/test/java/com/spotify/helios/master/resources/HostsResourceTest.java
@@ -17,6 +17,7 @@
 
 package com.spotify.helios.master.resources;
 
+import static java.util.Collections.emptyMap;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
@@ -66,8 +67,8 @@ public void setUp() {
 
     final HostStatus.Builder statusBuilder = HostStatus.newBuilder()
         .setStatus(HostStatus.Status.UP)
-        .setJobs(Collections.emptyMap())
-        .setStatuses(Collections.emptyMap());
+        .setJobs(emptyMap())
+        .setStatuses(emptyMap());
 
     int i = 1;
     for (final String host : hosts) {
@@ -79,7 +80,7 @@ public void setUp() {
           .setLabels(labels)
           .build();
 
-      when(model.getHostStatus(host)).thenReturn(hostStatus);
+      when(model.getHostLabels(host)).thenReturn(labels);
     }
   }
 
@@ -109,6 +110,22 @@ public void listHostsSelectorFilter() {
                contains("host1.foo.example.com", "host2.foo.example.com"));
   }
 
+  @Test
+  public void listHostsSelectorFilterMissingStatus() {
+    when(model.getHostLabels(hosts.get(0))).thenReturn(emptyMap());
+    assertThat(resource.list(null, ImmutableList.of("site=foo")),
+               equalTo(hosts.subList(1, hosts.size())));
+
+    assertThat(resource.list(null, ImmutableList.of("site=bar")), empty());
+    assertThat(resource.list(null, ImmutableList.of("site!=foo")), empty());
+
+    assertThat(resource.list(null, ImmutableList.of("index in (1,2)")),
+               contains("host2.foo.example.com"));
+
+    assertThat(resource.list(null, ImmutableList.of("site=foo", "index in (1,2)")),
+               contains("host2.foo.example.com"));
+  }
+
   /** Test behavior when both a name pattern and selector list is specified */
   @Test
   public void listHostsNameAndSelectorFilter() {