Disallow cfg suspension based solely on being down

3 files changed, 28 insertions, 19 deletions

diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorTest.java
index 83c902ed981..f885d5301de 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorTest.java
@@ -102,7 +102,7 @@ public class OrchestratorTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(), containsString("Changing the state of cfg2 would violate enough-services-up"));
-            assertThat(e.getMessage(), containsString("Suspended hosts: [cfg1]"));
+            assertThat(e.getMessage(), containsString("[cfg1] are suspended."));
         }
 
         // cfg1 is removed from the application
@@ -114,7 +114,7 @@ public class OrchestratorTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(), containsString("Changing the state of cfg2 would violate enough-services-up"));
-            assertThat(e.getMessage(), containsString("Services down on resumed hosts: [1 missing config server]"));
+            assertThat(e.getMessage(), containsString("[1 missing config server] are down."));
         }
 
         // cfg1 is reprovisioned, added to the node repo, and activated
@@ -129,7 +129,7 @@ public class OrchestratorTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(), containsString("Changing the state of cfg1 would violate enough-services-up"));
-            assertThat(e.getMessage(), containsString("Suspended hosts: [cfg2]"));
+            assertThat(e.getMessage(), containsString("[cfg2] are suspended"));
         }
 
         // etc (should be the same as for cfg1)
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 da8591c6631..3ea739b385e 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
@@ -95,11 +95,11 @@ public class ClusterApiImplTest {
 
         assertEquals("{ clusterId=cluster, serviceType=service-type }", clusterApi.clusterInfo());
         assertFalse(clusterApi.isStorageCluster());
-        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}}].",
+        assertEquals(" [host3, host4] are suspended. [ServiceInstance{configId=service-2, hostName=host2, " +
+                     "serviceStatus=ServiceStatusInfo{status=DOWN, since=Optional.empty, lastChecked=Optional.empty}}] " +
+                     "are down.",
                 clusterApi.downDescription());
-        assertEquals(60, clusterApi.percentageOfServicesDown());
+        assertEquals(60, clusterApi.percentageOfServicesDownOutsideGroup());
         assertEquals(80, clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown());
     }
 
@@ -181,8 +181,8 @@ public class ClusterApiImplTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(),
-                    containsString("Suspension of service with type 'configserver' not allowed: 33% are suspended already. " +
-                            "Services down on resumed hosts: [1 missing config server]."));
+                    containsString("Changing the state of cfg1 would violate enough-services-up: 33% of the config " +
+                                   "servers are down or suspended already: [1 missing config server] are down."));
         }
     }
 
@@ -201,18 +201,25 @@ public class ClusterApiImplTest {
             fail();
         } catch (HostStateChangeDeniedException e) {
             assertThat(e.getMessage(),
-                    containsString("Suspension of service with type 'hostadmin' not allowed: 33% are suspended already. " +
-                            "Services down on resumed hosts: [1 missing config server host]."));
+                    containsString("Changing the state of cfg1 would violate enough-services-up: 33% of the config " +
+                                   "server hosts are down or suspended already: [1 missing config server host] are down."));
         }
     }
 
     @Test
-    public void testCfg1SuspendsIfDownWithMissingCfg3() throws HostStateChangeDeniedException {
+    public void testCfg1DoesNotSuspendIfDownWithMissingCfg3() throws HostStateChangeDeniedException {
         ClusterApiImpl clusterApi = makeCfg1ClusterApi(ServiceStatus.DOWN, ServiceStatus.UP);
 
         HostedVespaClusterPolicy policy = new HostedVespaClusterPolicy(flagSource, zone);
 
-        policy.verifyGroupGoingDownIsFine(clusterApi);
+        try {
+            policy.verifyGroupGoingDownIsFine(clusterApi);
+            fail();
+        } catch (HostStateChangeDeniedException e) {
+            assertThat(e.getMessage(),
+                       containsString("Changing the state of cfg1 would violate enough-services-up: 33% of the config " +
+                                      "servers are down or suspended already: [1 missing config server] are down."));
+        }
     }
 
     @Test
@@ -320,7 +327,7 @@ public class ClusterApiImplTest {
                 clock);
 
         assertEquals(expectedNoServicesInGroupIsUp.map(SuspensionReasons::getMessagesInOrder),
-                     clusterApi.reasonsForNoServicesInGroupIsUp().map(SuspensionReasons::getMessagesInOrder));
+                     clusterApi.allServicesDown().map(SuspensionReasons::getMessagesInOrder));
         assertEquals(expectedNoServicesOutsideGroupIsDown, clusterApi.noServicesOutsideGroupIsDown());
     }
 
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 303dabebba8..8f47051621f 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
@@ -131,13 +131,14 @@ public class HostedVespaClusterPolicyTest {
                                             int percentageOfServicesDownIfGroupIsAllowedToBeDown,
                                             boolean expectSuccess) throws HostStateChangeDeniedException {
         when(clusterApi.noServicesOutsideGroupIsDown()).thenReturn(noServicesOutsideGroupIsDown);
-        when(clusterApi.reasonsForNoServicesInGroupIsUp()).thenReturn(noServicesInGroupIsUp);
+        when(clusterApi.allServicesDown()).thenReturn(noServicesInGroupIsUp);
         when(clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown()).thenReturn(20);
         doReturn(ConcurrentSuspensionLimitForCluster.TEN_PERCENT).when(policy).getConcurrentSuspensionLimit(clusterApi);
 
         when(applicationApi.applicationId()).thenReturn(ApplicationId.fromSerializedForm("a:b:c"));
         when(clusterApi.serviceType()).thenReturn(new ServiceType("service-type"));
-        when(clusterApi.percentageOfServicesDown()).thenReturn(5);
+        when(clusterApi.serviceDescription(true)).thenReturn("services of {service-type,cluster-id}");
+        when(clusterApi.percentageOfServicesDownOutsideGroup()).thenReturn(5);
         when(clusterApi.percentageOfServicesDownIfGroupIsAllowedToBeDown()).thenReturn(percentageOfServicesDownIfGroupIsAllowedToBeDown);
         when(clusterApi.downDescription()).thenReturn(" Down description");
 
@@ -152,9 +153,10 @@ public class HostedVespaClusterPolicyTest {
             }
         } catch (HostStateChangeDeniedException e) {
             if (!expectSuccess) {
-                assertEquals("Changing the state of node-group would violate enough-services-up: " +
-                        "Suspension of service with type 'service-type' would increase from 5% to 13%, " +
-                        "over the limit of 10%. Down description", e.getMessage());
+                assertEquals("Changing the state of node-group would violate enough-services-up: The percentage of downed " +
+                             "or suspended services of {service-type,cluster-id} would increase from 5% to 13% (limit is 10%): " +
+                             "Down description",
+                             e.getMessage());
                 assertEquals("enough-services-up", e.getConstraintName());
             }
         }