diff options
author | Håkon Hallingstad <hakon@yahoo-inc.com> | 2017-06-15 16:59:25 +0200 |
---|---|---|
committer | Håkon Hallingstad <hakon@yahoo-inc.com> | 2017-06-15 16:59:25 +0200 |
commit | ceb06e7230ff8b1a7b23f7e9253c43db8028e94d (patch) | |
tree | 7125da1d9aaffee6de023e5187950be7381236c3 /orchestrator/src | |
parent | 9289a6bc9173f2aaaf9aa72c3d262ae2993a765d (diff) |
Do not rollback after failed suspendAll
Diffstat (limited to 'orchestrator/src')
-rw-r--r-- | orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java | 46 | ||||
-rw-r--r-- | orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java | 49 |
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(); } |