Implemented suggested changes to CapacityChecker.

3 files changed, 61 insertions, 60 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
index 02fcd494875..48f846d5e7f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
@@ -45,10 +45,8 @@ public class CapacityChecker {
                 .map(h -> nodeMap.get(h))
                 .collect(Collectors.toList());
         if (nodes.size() != hostnames.size()) {
-            List<String> notFoundNodes = hostnames.stream()
-                    .filter(h -> !nodes.stream()
-                            .map(Node::hostname).collect(Collectors.toSet()).contains(h))
-                    .collect(Collectors.toList());
+            Set<String> notFoundNodes = new HashSet<>(hostnames);
+            notFoundNodes.removeAll(nodes.stream().map(Node::hostname).collect(Collectors.toList()));
             throw new IllegalArgumentException(String.format("Host(s) not found: [ %s ]",
                     String.join(", ", notFoundNodes)));
         }
@@ -329,7 +327,7 @@ public class CapacityChecker {
     public static class HostRemovalFailure {
         public Optional<Node> host;
         public Optional<Node> tenant;
-        public AllocationFailureReasonList failureReasons;
+        public AllocationFailureReasonList allocationFailures;
 
         public static HostRemovalFailure none() {
             return new HostRemovalFailure(
@@ -345,10 +343,10 @@ public class CapacityChecker {
                     failureReasons);
         }
 
-        private HostRemovalFailure(Optional<Node> host, Optional<Node> tenant, AllocationFailureReasonList failureReasons) {
+        private HostRemovalFailure(Optional<Node> host, Optional<Node> tenant, AllocationFailureReasonList allocationFailures) {
             this.host = host;
             this.tenant = tenant;
-            this.failureReasons = failureReasons;
+            this.allocationFailures = allocationFailures;
         }
 
         @Override
@@ -361,8 +359,8 @@ public class CapacityChecker {
                     "\n\t\tTotal Reasons:    %s",
                     this.host.get().hostname(),
                     this.tenant.get().hostname(),
-                    this.failureReasons.singularReasonFailures().toString(),
-                    this.failureReasons.toString()
+                    this.allocationFailures.singularReasonFailures().toString(),
+                    this.allocationFailures.toString()
             );
         }
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/HostCapacityResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/HostCapacityResponse.java
index be8de0d4475..9f5af52cc08 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/HostCapacityResponse.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/HostCapacityResponse.java
@@ -34,36 +34,47 @@ public class HostCapacityResponse extends HttpResponse {
         Cursor root = slime.setObject();
 
         if (hostsJson != null) {
-            ObjectMapper om = new ObjectMapper();
-            String[] hostsArray;
-            try {
-                hostsArray = om.readValue(hostsJson, String[].class);
-            } catch (Exception e) {
-                throw new IllegalArgumentException(e.getMessage());
-            }
-            List<String> hostNames = Arrays.asList(hostsArray);
-            List<Node> hosts;
-            try {
-                hosts = capacityChecker.nodesFromHostnames(hostNames);
-            } catch (IllegalArgumentException e) {
-                throw new NotFoundException(e.getMessage());
-            }
-            var failure = capacityChecker.findHostRemovalFailure(hosts);
-            if (failure.isPresent() && failure.get().failureReason.failureReasons.size() == 0) {
-                root.setBool("removalPossible", false);
-                error(root, "Removing all hosts is trivially impossible.");
-            } else {
-                if (json) hostLossPossibleToSlime(root, failure, hosts);
-                else      hostLossPossibleToText(failure, hosts);
-            }
+            List<Node> hosts = parseHostList(hostsJson);
+            hostRemovalResponse(root, hosts);
         } else {
-            var failurePath = capacityChecker.worstCaseHostLossLeadingToFailure();
-            if (failurePath.isPresent()) {
-                if (json) zoneFailurePathToSlime(root, failurePath.get());
-                else      zoneFailurePathToText(failurePath.get());
-            } else {
-                error(root, "Node repository contained no hosts.");
-            }
+            zoneFailureReponse(root);
+        }
+    }
+
+    private List<Node> parseHostList(String hosts) {
+        ObjectMapper om = new ObjectMapper();
+        String[] hostsArray;
+        try {
+            hostsArray = om.readValue(hosts, String[].class);
+        } catch (Exception e) {
+            throw new IllegalArgumentException(e.getMessage());
+        }
+        List<String> hostNames = Arrays.asList(hostsArray);
+        try {
+            return capacityChecker.nodesFromHostnames(hostNames);
+        } catch (IllegalArgumentException e) {
+            throw new NotFoundException(e.getMessage());
+        }
+    }
+
+    private void hostRemovalResponse(Cursor root, List<Node> hosts) {
+        var failure = capacityChecker.findHostRemovalFailure(hosts);
+        if (failure.isPresent() && failure.get().failureReason.allocationFailures.size() == 0) {
+            root.setBool("removalPossible", false);
+            error(root, "Removing all hosts is trivially impossible.");
+        } else {
+            if (json) hostLossPossibleToSlime(root, failure, hosts);
+            else      hostLossPossibleToText(failure, hosts);
+        }
+    }
+
+    private void zoneFailureReponse(Cursor root) {
+        var failurePath = capacityChecker.worstCaseHostLossLeadingToFailure();
+        if (failurePath.isPresent()) {
+            if (json) zoneFailurePathToSlime(root, failurePath.get());
+            else      zoneFailurePathToText(failurePath.get());
+        } else {
+            error(root, "Node repository contained no hosts.");
         }
     }
 
@@ -101,11 +112,7 @@ public class HostCapacityResponse extends HttpResponse {
         var hosts = root.setArray("hostsToRemove");
         hostsToRemove.forEach(h -> hosts.addString(h.hostname()));
         CapacityChecker.AllocationHistory history = capacityChecker.allocationHistory;
-        if (failure.isEmpty()) {
-            root.setBool("removalPossible", true);
-        } else {
-            root.setBool("removalPossible", false);
-        }
+        root.setBool("removalPossible", failure.isEmpty());
         var arr = root.setArray("history");
         for (var entry : history.historyEntries) {
             var object = arr.addObject();
@@ -117,7 +124,7 @@ public class HostCapacityResponse extends HttpResponse {
         }
     }
 
-    public void zoneFailurePathToSlime(Cursor object, CapacityChecker.HostFailurePath failurePath) {
+    private void zoneFailurePathToSlime(Cursor object, CapacityChecker.HostFailurePath failurePath) {
         object.setLong("totalHosts", capacityChecker.getHosts().size());
         object.setLong("couldLoseHosts", failurePath.hostsCausingFailure.size());
         failurePath.failureReason.host.ifPresent(host ->
@@ -131,9 +138,9 @@ public class HostCapacityResponse extends HttpResponse {
             );
             var explanation = object.setObject("hostCandidateRejectionReasons");
             allocationFailureReasonListToSlime(explanation.setObject("singularReasonFailures"),
-                    failurePath.failureReason.failureReasons.singularReasonFailures());
+                    failurePath.failureReason.allocationFailures.singularReasonFailures());
             allocationFailureReasonListToSlime(explanation.setObject("totalFailures"),
-                    failurePath.failureReason.failureReasons);
+                    failurePath.failureReason.allocationFailures);
         });
         var details = object.setObject("details");
         hostLossPossibleToSlime(details, Optional.of(failurePath), failurePath.hostsCausingFailure);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTest.java
index 793789def2c..1f2112673d1 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTest.java
@@ -30,12 +30,8 @@ public class CapacityCheckerTest {
         tester.cleanRepository();
         tester.restoreNodeRepositoryFromJsonFile(Paths.get(path));
         var failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
-        if (failurePath.isPresent()) {
-//            System.out.println( tester.capacityChecker.allocationHistory );
-//            System.out.println( failurePath.get().failureReason );
-            System.out.println("Worst case host loss : " + failurePath.get().hostsCausingFailure.size());
-            assertTrue(tester.nodeRepository.getNodes(NodeType.host).containsAll(failurePath.get().hostsCausingFailure));
-        } else fail();
+        assertTrue(failurePath.isPresent());
+        assertTrue(tester.nodeRepository.getNodes(NodeType.host).containsAll(failurePath.get().hostsCausingFailure));
     }
 
     @Test
@@ -71,7 +67,7 @@ public class CapacityCheckerTest {
         failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("When there are multiple lacking resources, all failures are multipleReasonFailures",
                     failureReasons.size(), failureReasons.multipleReasonFailures().size());
             assertEquals(0, failureReasons.singularReasonFailures().size());
@@ -86,7 +82,7 @@ public class CapacityCheckerTest {
         var failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("All failures should be due to hosts having a lack of available ip addresses.",
                     failureReasons.singularReasonFailures().insufficientAvailableIps(), failureReasons.size());
         } else fail();
@@ -101,7 +97,7 @@ public class CapacityCheckerTest {
         var failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("All failures should be due to hosts lacking cpu cores.",
                     failureReasons.singularReasonFailures().insufficientVcpu(), failureReasons.size());
         } else fail();
@@ -112,7 +108,7 @@ public class CapacityCheckerTest {
         failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("All failures should be due to hosts lacking memory.",
                     failureReasons.singularReasonFailures().insufficientMemoryGb(), failureReasons.size());
         } else fail();
@@ -123,7 +119,7 @@ public class CapacityCheckerTest {
         failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("All failures should be due to hosts lacking disk space.",
                     failureReasons.singularReasonFailures().insufficientDiskGb(), failureReasons.size());
         } else fail();
@@ -135,7 +131,7 @@ public class CapacityCheckerTest {
         failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("All empty hosts should be invalid due to having incompatible disk speed.",
                     failureReasons.singularReasonFailures().incompatibleDiskSpeed(), emptyHostsWithSlowDisk);
         } else fail();
@@ -151,7 +147,7 @@ public class CapacityCheckerTest {
         var failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertEquals("With only one type of tenant, all failures should be due to violation of the parent host policy.",
                     failureReasons.singularReasonFailures().violatesParentHostPolicy(), failureReasons.size());
         } else fail();
@@ -162,7 +158,7 @@ public class CapacityCheckerTest {
         failurePath = tester.capacityChecker.worstCaseHostLossLeadingToFailure();
         assertTrue(failurePath.isPresent());
         if (failurePath.get().failureReason.tenant.isPresent()) {
-            var failureReasons = failurePath.get().failureReason.failureReasons;
+            var failureReasons = failurePath.get().failureReason.allocationFailures;
             assertNotEquals("Fewer distinct children than hosts should result in some parent host policy violations.",
                     failureReasons.size(), failureReasons.singularReasonFailures().violatesParentHostPolicy());
             assertNotEquals(0, failureReasons.singularReasonFailures().violatesParentHostPolicy());