Improve suspension denied reason

6 files changed, 64 insertions, 59 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApi.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApi.java
index e9bb4984c2e..65c45c8df76 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApi.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApi.java
@@ -26,7 +26,6 @@ public interface ClusterApi {
     Optional<StorageNode> storageNodeInGroup();
     Optional<StorageNode> upStorageNodeInGroup();
 
-    String servicesDownAndNotInGroupDescription();
-    String nodesAllowedToBeDownNotInGroupDescription();
+    String downDescription();
 
 }
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApiImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApiImpl.java
index b747d8c2e22..24f56eac85d 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApiImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ClusterApiImpl.java
@@ -131,25 +131,56 @@ class ClusterApiImpl implements ClusterApi {
         return numberOfServicesDown * 100 / (serviceCluster.serviceInstances().size() + missingServices);
     }
 
+    /**
+     * A description of the hosts outside the group that are allowed to be down,
+     * and a description of the services outside the group and outside of the allowed services
+     * that are down.
+     */
     @Override
-    public String servicesDownAndNotInGroupDescription() {
-        // Sort these for readability and testing stability
-        return Stream
-                .concat(servicesDownAndNotInGroup.stream().map(ServiceInstance::toString).sorted(),
-                        missingServices > 0 ? Stream.of(descriptionOfMissingServices) : Stream.of())
-                .collect(Collectors.toList())
-                .toString();
-    }
+    public String downDescription() {
+        StringBuilder description = new StringBuilder();
 
-    @Override
-    public String nodesAllowedToBeDownNotInGroupDescription() {
-        return servicesNotInGroup.stream()
+        Set<HostName> suspended = servicesNotInGroup.stream()
                 .map(ServiceInstance::hostName)
                 .filter(hostName -> hostStatus(hostName).isSuspended())
-                .sorted()
-                .distinct()
-                .collect(Collectors.toList())
-                .toString();
+                .collect(Collectors.toSet());
+
+        if (suspended.size() > 0) {
+            description.append(" ");
+
+            final int nodeLimit = 3;
+            description.append("Suspended hosts: ");
+            description.append(suspended.stream().sorted().distinct().limit(nodeLimit).collect(Collectors.toList()).toString());
+            if (suspended.size() > nodeLimit) {
+                description.append(", and " + (suspended.size() - nodeLimit) + " more");
+            }
+            description.append(".");
+        }
+
+        Set<ServiceInstance> downElsewhere = servicesDownAndNotInGroup.stream()
+                .filter(serviceInstance -> !suspended.contains(serviceInstance.hostName()))
+                .collect(Collectors.toSet());
+
+        final int downElsewhereTotal = downElsewhere.size() + missingServices;
+        if (downElsewhereTotal > 0) {
+            description.append(" ");
+
+            final int serviceLimit = 2; // services info is verbose
+            description.append("Services down on resumed hosts: ");
+            description.append(Stream.concat(
+                    downElsewhere.stream().map(ServiceInstance::toString).sorted(),
+                    missingServices > 0 ? Stream.of(descriptionOfMissingServices) : Stream.of())
+                    .limit(serviceLimit)
+                    .collect(Collectors.toList())
+                    .toString());
+
+            if (downElsewhereTotal > serviceLimit) {
+                description.append(", and " + (downElsewhereTotal - serviceLimit) + " more");
+            }
+            description.append(".");
+        }
+
+        return description.toString();
     }
 
     private Optional<StorageNode> storageNodeInGroup(Predicate<ServiceInstance> storageServicePredicate) {
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostStateChangeDeniedException.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostStateChangeDeniedException.java
index e13cf17d420..a6b3cbc87dc 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostStateChangeDeniedException.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostStateChangeDeniedException.java
@@ -2,55 +2,43 @@
 package com.yahoo.vespa.orchestrator.policy;
 
 import com.yahoo.vespa.applicationmodel.HostName;
-import com.yahoo.vespa.applicationmodel.ServiceType;
 import com.yahoo.vespa.orchestrator.OrchestrationException;
 import com.yahoo.vespa.orchestrator.model.NodeGroup;
 
-import java.util.Optional;
-
 /**
  * @author bakksjo
  */
 public class HostStateChangeDeniedException extends OrchestrationException {
     private final String constraintName;
-    private final Optional<ServiceType> serviceType;
 
     public HostStateChangeDeniedException(HostName hostName, String constraintName, String message) {
         this(hostName, constraintName, message, null);
     }
 
     public HostStateChangeDeniedException(HostName hostName, String constraintName, String message, Exception e) {
-        this(hostName.s(), constraintName, Optional.empty(), message, e);
+        this(hostName.s(), constraintName, message, e);
     }
 
     public HostStateChangeDeniedException(NodeGroup nodeGroup, String constraintName, String message) {
-        this(nodeGroup.toCommaSeparatedString(), constraintName, Optional.empty(), message, null);
+        this(nodeGroup.toCommaSeparatedString(), constraintName, message, null);
     }
 
     private HostStateChangeDeniedException(String nodes,
                                            String constraintName,
-                                           Optional<ServiceType> serviceType,
                                            String message,
                                            Throwable cause) {
-        super(createMessage(nodes, constraintName, serviceType, message), cause);
+        super(createMessage(nodes, constraintName, message), cause);
         this.constraintName = constraintName;
-        this.serviceType = serviceType;
     }
 
     private static String createMessage(String nodes,
                                         String constraintName,
-                                        Optional<ServiceType> serviceType,
                                         String message) {
         return "Changing the state of " + nodes + " would violate " + constraintName
-                + (serviceType.isPresent() ? " for service type " + serviceType.get() : "")
                 + ": " + message;
     }
 
     public String getConstraintName() {
         return constraintName;
     }
-
-    public Optional<ServiceType> getServiceType() {
-        return serviceType;
-    }
 }
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicy.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicy.java
index 1e895d0e757..ccb0bb57186 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicy.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicy.java
@@ -26,13 +26,11 @@ public class HostedVespaClusterPolicy implements ClusterPolicy {
         throw new HostStateChangeDeniedException(
                 clusterApi.getNodeGroup(),
                 ENOUGH_SERVICES_UP_CONSTRAINT,
-                "Suspension percentage for service type " + clusterApi.serviceType()
+                "Suspension for service type " + clusterApi.serviceType()
                         + " would increase from " + clusterApi.percentageOfServicesDown()
                         + "% to " + clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown()
                         + "%, over the limit of " + percentageOfServicesAllowedToBeDown + "%."
-                        + " These instances may be down: " + clusterApi.servicesDownAndNotInGroupDescription()
-                        + " and these hosts are allowed to be down: "
-                        + clusterApi.nodesAllowedToBeDownNotInGroupDescription());
+                        + clusterApi.downDescription());
     }
 
     @Override
@@ -56,9 +54,7 @@ public class HostedVespaClusterPolicy implements ClusterPolicy {
                 "Down percentage for service type " + clusterApi.serviceType()
                         + " would increase to " + clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown()
                         + "%, over the limit of " + percentageOfServicesAllowedToBeDown + "%."
-                        + " These instances may be down: " + clusterApi.servicesDownAndNotInGroupDescription()
-                        + " and these hosts are allowed to be down: "
-                        + clusterApi.nodesAllowedToBeDownNotInGroupDescription());
+                        + clusterApi.downDescription());
     }
 
     // Non-private for testing purposes
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java
index 62925dc003e..a5cb5cfa630 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java
@@ -21,7 +21,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
@@ -81,15 +80,10 @@ public class ClusterApiImplTest {
 
         assertEquals("{ clusterId=cluster, serviceType=service-type }", clusterApi.clusterInfo());
         assertFalse(clusterApi.isStorageCluster());
-        assertEquals("[ServiceInstance{configId=service-2, hostName=host2, serviceStatus=" +
-                        "ServiceStatusInfo{status=DOWN, since=Optional.empty, lastChecked=Optional.empty}}, "
-                        + "ServiceInstance{configId=service-3, hostName=host3, serviceStatus=" +
-                        "ServiceStatusInfo{status=UP, since=Optional.empty, lastChecked=Optional.empty}}, "
-                        + "ServiceInstance{configId=service-4, hostName=host4, serviceStatus=" +
-                        "ServiceStatusInfo{status=DOWN, since=Optional.empty, lastChecked=Optional.empty}}]",
-                clusterApi.servicesDownAndNotInGroupDescription());
-        assertEquals("[host3, host4]",
-                clusterApi.nodesAllowedToBeDownNotInGroupDescription());
+        assertEquals(" Suspended hosts: [host3, host4]. Services down on resumed hosts: [" +
+                        "ServiceInstance{configId=service-2, hostName=host2, serviceStatus=" +
+                        "ServiceStatusInfo{status=DOWN, since=Optional.empty, lastChecked=Optional.empty}}].",
+                clusterApi.downDescription());
         assertEquals(60, clusterApi.percentageOfServicesDown());
         assertEquals(80, clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown());
     }
@@ -110,9 +104,9 @@ public class ClusterApiImplTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(),
-                    containsString("Changing the state of cfg1 would violate enough-services-up: Suspension percentage " +
-                            "for service type configserver would increase from 33% to 66%, over the limit of 10%. " +
-                            "These instances may be down: [1 missing config server] and these hosts are allowed to be down: []"));
+                    containsString("Changing the state of cfg1 would violate enough-services-up: " +
+                            "Suspension for service type configserver would increase from 33% to 66%, " +
+                            "over the limit of 10%. Services down on resumed hosts: [1 missing config server]."));
         }
     }
 
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicyTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicyTest.java
index d834034c9a8..4462e886d1b 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicyTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaClusterPolicyTest.java
@@ -101,8 +101,7 @@ public class HostedVespaClusterPolicyTest {
         when(clusterApi.serviceType()).thenReturn(new ServiceType("service-type"));
         when(clusterApi.percentageOfServicesDown()).thenReturn(5);
         when(clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown()).thenReturn(percentageOfServicesDownIfGroupIsAllowedToBeDown);
-        when(clusterApi.servicesDownAndNotInGroupDescription()).thenReturn("services-down-and-not-in-group");
-        when(clusterApi.nodesAllowedToBeDownNotInGroupDescription()).thenReturn("allowed-to-be-down");
+        when(clusterApi.downDescription()).thenReturn(" Down description");
 
         NodeGroup nodeGroup = mock(NodeGroup.class);
         when(clusterApi.getNodeGroup()).thenReturn(nodeGroup);
@@ -116,11 +115,9 @@ public class HostedVespaClusterPolicyTest {
             }
         } catch (HostStateChangeDeniedException e) {
             if (!expectSuccess) {
-                assertEquals("Changing the state of node-group would violate enough-services-up: "
-                        + "Suspension percentage for service type service-type would increase from "
-                        + "5% to 13%, over the limit of 10%. These instances may be down: "
-                        + "services-down-and-not-in-group and these hosts are allowed to be down: "
-                        + "allowed-to-be-down", e.getMessage());
+                assertEquals("Changing the state of node-group would violate enough-services-up: " +
+                        "Suspension for service type service-type would increase from 5% to 13%, " +
+                        "over the limit of 10%. Down description", e.getMessage());
                 assertEquals("enough-services-up", e.getConstraintName());
             }
         }