Do not rollback after failed suspendAll

2 files changed, 35 insertions, 60 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
index 27af80ea815..f05d87dacde 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
@@ -29,8 +29,6 @@ import com.yahoo.vespa.orchestrator.status.StatusService;
 import com.yahoo.vespa.service.monitor.ServiceMonitorStatus;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -44,8 +42,6 @@ import java.util.stream.Collectors;
  * @author smorgrav
  */
 public class OrchestratorImpl implements Orchestrator {
-
-
     private static final Logger log = Logger.getLogger(OrchestratorImpl.class.getName());
 
     private final Policy policy;
@@ -198,38 +194,16 @@ public class OrchestratorImpl implements Orchestrator {
             throw new BatchHostNameNotFoundException(parentHostname, hostNames, e);
         }
 
-        try {
-            for (NodeGroup nodeGroup : nodeGroupsOrderedByApplication) {
-                try {
-                    suspendGroup(nodeGroup);
-                } catch (HostStateChangeDeniedException e) {
-                    throw new BatchHostStateChangeDeniedException(parentHostname, nodeGroup, e);
-                } catch (HostNameNotFoundException e) {
-                    // Should never get here since since we would have received HostNameNotFoundException earlier.
-                    throw new BatchHostNameNotFoundException(parentHostname, hostNames, e);
-                } catch (RuntimeException e) {
-                    throw new BatchInternalErrorException(parentHostname, nodeGroup, e);
-                }
-            }
-        } catch (Exception e) {
-            // Rollback causes extra noise in a content clusters due to cluster version changes and calm-periods,
-            // so consider not doing a full rollback.
-            rollbackSuspendAll(nodeGroupsOrderedByApplication, e);
-            throw e;
-        }
-    }
-
-    private void rollbackSuspendAll(List<NodeGroup> orderedGroups, Exception exception) {
-        List<NodeGroup> reverseOrderedHostNames = new ArrayList<>(orderedGroups);
-        Collections.reverse(reverseOrderedHostNames);
-        for (NodeGroup nodeGroup : reverseOrderedHostNames) {
-            for (HostName hostName : nodeGroup.getHostNames()) {
-                try {
-                    resume(hostName);
-                } catch (HostStateChangeDeniedException | HostNameNotFoundException | RuntimeException e) {
-                    // We're forced to ignore these since we're already rolling back a suspension.
-                    exception.addSuppressed(e);
-                }
+        for (NodeGroup nodeGroup : nodeGroupsOrderedByApplication) {
+            try {
+                suspendGroup(nodeGroup);
+            } catch (HostStateChangeDeniedException e) {
+                throw new BatchHostStateChangeDeniedException(parentHostname, nodeGroup, e);
+            } catch (HostNameNotFoundException e) {
+                // Should never get here since since we would have received HostNameNotFoundException earlier.
+                throw new BatchHostNameNotFoundException(parentHostname, hostNames, e);
+            } catch (RuntimeException e) {
+                throw new BatchInternalErrorException(parentHostname, nodeGroup, e);
             }
         }
     }
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
index 8d6781d6d0c..11518bb4545 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
@@ -29,7 +29,6 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.spy;
@@ -227,11 +226,32 @@ public class OrchestratorImplTest {
     }
 
     @Test
-    public void rollbackWorks() throws Exception {
+    public void suspendAllWorks() throws Exception {
         // A spy is preferential because suspendAll() relies on delegating the hard work to suspend() and resume().
         OrchestratorImpl orchestrator = spy(this.orchestrator);
 
-        doNothing().when(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP);
+        orchestrator.suspendAll(
+                new HostName("parentHostname"),
+                Arrays.asList(
+                        DummyInstanceLookupService.TEST1_HOST_NAME,
+                        DummyInstanceLookupService.TEST3_HOST_NAME,
+                        DummyInstanceLookupService.TEST6_HOST_NAME));
+
+        // As of 2016-06-07 the order of the node groups are as follows:
+        //   TEST3: mediasearch:imagesearch:default
+        //   TEST6: tenant-id-3:application-instance-3:default
+        //   TEST1: test-tenant-id:application:instance
+        InOrder order = inOrder(orchestrator);
+        order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP);
+        order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP);
+        order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST1_NODE_GROUP);
+        order.verifyNoMoreInteractions();
+    }
+
+    @Test
+    public void whenSuspendAllFails() throws Exception {
+        // A spy is preferential because suspendAll() relies on delegating the hard work to suspend() and resume().
+        OrchestratorImpl orchestrator = spy(this.orchestrator);
 
         Throwable supensionFailure = new HostStateChangeDeniedException(
                 DummyInstanceLookupService.TEST6_HOST_NAME,
@@ -239,11 +259,6 @@ public class OrchestratorImplTest {
                 "error message");
         doThrow(supensionFailure).when(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP);
 
-        doThrow(new HostStateChangeDeniedException(DummyInstanceLookupService.TEST1_HOST_NAME, "foo1-constraint", "foo1-message"))
-                .when(orchestrator).resume(DummyInstanceLookupService.TEST1_HOST_NAME);
-        doNothing().when(orchestrator).resume(DummyInstanceLookupService.TEST6_HOST_NAME);
-        doNothing().when(orchestrator).resume(DummyInstanceLookupService.TEST3_HOST_NAME);
-
         try {
             orchestrator.suspendAll(
                     new HostName("parentHostname"),
@@ -253,30 +268,16 @@ public class OrchestratorImplTest {
                             DummyInstanceLookupService.TEST6_HOST_NAME));
             fail();
         } catch (BatchHostStateChangeDeniedException e) {
-            assertEquals(e.getSuppressed().length, 1);
-
             assertEquals("Failed to suspend NodeGroup{application=tenant-id-3:application-instance-3:prod:utopia-1:default, " +
-                    "hostNames=[test6.hostname.tld]} with parent host parentHostname: " +
+                            "hostNames=[test6.hostname.tld]} with parent host parentHostname: " +
                             "Changing the state of test6.hostname.tld would violate " +
-                            "some-constraint: error message; With suppressed throwable " +
-                            "com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException: " +
-                            "Changing the state of test1.hostname.tld " +
-                            "would violate foo1-constraint: foo1-message",
+                            "some-constraint: error message",
                     e.getMessage());
         }
 
         InOrder order = inOrder(orchestrator);
         order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP);
         order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP);
-
-        // As of 2016-06-07 the order of the node groups are as follows:
-        //   TEST3: mediasearch:imagesearch:default
-        //   TEST6: tenant-id-3:application-instance-3:default
-        //   TEST1: test-tenant-id:application:instance
-        // Which is reversed when rolling back
-        order.verify(orchestrator).resume(DummyInstanceLookupService.TEST1_HOST_NAME);
-        order.verify(orchestrator).resume(DummyInstanceLookupService.TEST6_HOST_NAME);
-        order.verify(orchestrator).resume(DummyInstanceLookupService.TEST3_HOST_NAME);
         order.verifyNoMoreInteractions();
     }