More robust cleanup of removed hosts for deployment groups

helios-services/src/main/java/com/spotify/helios/master/ZooKeeperMasterModel.java

@@ -516,7 +516,10 @@ private List<String> removedHosts(final List<String> currentHosts,
 
     // Calculate the freshly removed hosts (current - new) and add in any previously removed hosts
     // that haven't been undeployed yet.
-    return ImmutableList.copyOf(Sets.union(Sets.difference(ch, nh), prh));
+    final Set<String> rh = Sets.union(Sets.difference(ch, nh), prh);
+
+    // Finally, only include hosts that are still registered
+    return ImmutableList.copyOf(Sets.intersection(rh, ImmutableSet.copyOf(listHosts())));
   }
 
   @Override
@@ -539,12 +542,12 @@ public void updateDeploymentGroupHosts(final String groupName, final List<String
       final List<String> previouslyRemovedHosts = getHosts(
           client, Paths.statusDeploymentGroupRemovedHosts(groupName));
 
-      if (hosts.equals(curHosts)) {
+      final List<String> removedHosts = removedHosts(curHosts, hosts, previouslyRemovedHosts);
+
+      if (hosts.equals(curHosts) && removedHosts.equals(previouslyRemovedHosts)) {
         return;
       }
 
-      final List<String> removedHosts = removedHosts(curHosts, hosts, previouslyRemovedHosts);
-
       log.info("for deployment-group name={}, curHosts={}, new hosts={}, "
                + "previouslyRemovedHosts={}, derived removedHosts={}",
           groupName, curHosts, hosts, previouslyRemovedHosts, removedHosts);
@@ -660,18 +663,22 @@ private RollingUpdateOp getInitRollingUpdateOps(final DeploymentGroup deployment
 
     // give precedence to the updateHosts list so we don't end up in a state where we updated a host
     // and then removed the job from it (because of buggy logic in the calling method)
-    final List<String> actualHostsToUndeploy = new ArrayList<>(undeployHosts);
-    actualHostsToUndeploy.removeAll(updateHosts);
+    final List<String> updateHostsCopy = new ArrayList<>(updateHosts);
+    final List<String> undeployHostsCopy = new ArrayList<>(undeployHosts);
+    undeployHostsCopy.removeAll(updateHostsCopy);
 
-    // only try to deploy to hosts that are UP
-    final List<String> hostsToDeploy = updateHosts.stream()
+    // we only care about hosts that are UP
+    final List<String> upHostsToUndeploy = undeployHostsCopy.stream()
+        .filter(host -> checkHostUp(zooKeeperClient, host))
+        .collect(Collectors.toList());
+    final List<String> upHostsToDeploy = updateHostsCopy.stream()
         .filter(host -> checkHostUp(zooKeeperClient, host))
         .collect(Collectors.toList());
 
     rolloutTasks.addAll(RollingUndeployPlanner.of(deploymentGroup)
-                            .plan(actualHostsToUndeploy));
+                            .plan(upHostsToUndeploy));
     rolloutTasks.addAll(RollingUpdatePlanner.of(deploymentGroup)
-                            .plan(hostsToDeploy));
+                            .plan(upHostsToDeploy));
 
     log.info("generated rolloutTasks for deployment-group name={} "
              + "updateHosts={} undeployHosts={}: {}",

helios-services/src/test/java/com/spotify/helios/master/DeploymentGroupTest.java

@@ -151,6 +151,8 @@ public void testUpdateDeploymentGroupHosts() throws Exception {
     // Setup some hosts
     final String oldHost = "host1";
     final String newHost = "host2";
+    client.ensurePath(Paths.configHost(oldHost));
+    client.ensurePath(Paths.configHost(newHost));
     client.ensurePath(Paths.statusHostUp(oldHost));
     client.ensurePath(Paths.statusHostUp(newHost));