From 21f0a02ffc6e19e14b1b0be87a2f73b643e05c07 Mon Sep 17 00:00:00 2001 From: Peter Teixeira Date: Thu, 4 May 2017 09:31:21 -0400 Subject: [PATCH 1/3] Add skip lb removal flag to DeleteRequestRequest Add a flag that can be set on the request deletion request to avoid removing the basepath associated with the service from the load balancer. The suggested use case is when the application associated with the service is moving out of Singularity, but will still be running and should remain load-balanced. --- .../SingularityRequestCleanup.java | 22 ++++++++-- .../api/SingularityDeleteRequestRequest.java | 11 ++++- .../singularity/data/RequestManager.java | 4 +- .../singularity/helpers/RequestHelper.java | 3 +- .../resources/RequestResource.java | 10 +++-- .../scheduler/SingularityCleaner.java | 6 ++- .../singularity/SingularityHistoryTest.java | 2 +- .../scheduler/HistoryPersisterTest.java | 10 ++--- .../scheduler/SingularitySchedulerTest.java | 42 +++++++++++++++++-- .../SingularitySchedulerTestBase.java | 8 ++++ 10 files changed, 95 insertions(+), 23 deletions(-) diff --git a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestCleanup.java b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestCleanup.java index 6ad1fbad9b..6155b2299e 100644 --- a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestCleanup.java +++ b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestCleanup.java @@ -9,6 +9,7 @@ public class SingularityRequestCleanup { private final Optional user; private final RequestCleanupType cleanupType; private final Optional killTasks; + private final Optional removeFromLoadBalancer; private final Optional skipHealthchecks; private final Optional deployId; private final long timestamp; @@ -18,16 +19,24 @@ public class SingularityRequestCleanup { private final Optional runShellCommandBeforeKill; @JsonCreator - public SingularityRequestCleanup(@JsonProperty("user") Optional user, @JsonProperty("cleanupType") RequestCleanupType cleanupType, @JsonProperty("timestamp") long timestamp, - @JsonProperty("killTasks") Optional killTasks, @JsonProperty("requestId") String requestId, @JsonProperty("deployId") Optional deployId, - @JsonProperty("skipHealthchecks") Optional skipHealthchecks, @JsonProperty("message") Optional message, @JsonProperty("actionId") Optional actionId, - @JsonProperty("runShellCommandBeforeKill") Optional runShellCommandBeforeKill) { + public SingularityRequestCleanup(@JsonProperty("user") Optional user, + @JsonProperty("cleanupType") RequestCleanupType cleanupType, + @JsonProperty("timestamp") long timestamp, + @JsonProperty("killTasks") Optional killTasks, + @JsonProperty("removeFromLoadBalancer") Optional removeFromLoadBalancer, + @JsonProperty("requestId") String requestId, + @JsonProperty("deployId") Optional deployId, + @JsonProperty("skipHealthchecks") Optional skipHealthchecks, + @JsonProperty("message") Optional message, + @JsonProperty("actionId") Optional actionId, + @JsonProperty("runShellCommandBeforeKill") Optional runShellCommandBeforeKill) { this.user = user; this.cleanupType = cleanupType; this.timestamp = timestamp; this.requestId = requestId; this.deployId = deployId; this.killTasks = killTasks; + this.removeFromLoadBalancer = removeFromLoadBalancer; this.skipHealthchecks = skipHealthchecks; this.actionId = actionId; this.message = message; @@ -42,6 +51,10 @@ public Optional getKillTasks() { return killTasks; } + public Optional getRemoveFromLoadBalancer() { + return removeFromLoadBalancer; + } + public String getRequestId() { return requestId; } @@ -80,6 +93,7 @@ public String toString() { "user=" + user + ", cleanupType=" + cleanupType + ", killTasks=" + killTasks + + ", removeFromLoadBalancer=" + removeFromLoadBalancer + ", skipHealthchecks=" + skipHealthchecks + ", deployId=" + deployId + ", timestamp=" + timestamp + diff --git a/SingularityBase/src/main/java/com/hubspot/singularity/api/SingularityDeleteRequestRequest.java b/SingularityBase/src/main/java/com/hubspot/singularity/api/SingularityDeleteRequestRequest.java index 0fba9bcb21..3d8c426768 100644 --- a/SingularityBase/src/main/java/com/hubspot/singularity/api/SingularityDeleteRequestRequest.java +++ b/SingularityBase/src/main/java/com/hubspot/singularity/api/SingularityDeleteRequestRequest.java @@ -9,12 +9,15 @@ public class SingularityDeleteRequestRequest { private final Optional message; private final Optional actionId; + private final Optional deleteFromLoadBalancer; @JsonCreator public SingularityDeleteRequestRequest(@JsonProperty("message") Optional message, - @JsonProperty("actionId") Optional actionId) { + @JsonProperty("actionId") Optional actionId, + @JsonProperty("deleteFromLoadBalancer") Optional deleteFromLoadBalancer) { this.message = message; this.actionId = actionId; + this.deleteFromLoadBalancer = deleteFromLoadBalancer; } @ApiModelProperty(required=false, value="A message to show to users about why this action was taken") @@ -27,11 +30,17 @@ public Optional getActionId() { return actionId; } + @ApiModelProperty(required = false, value = "Should the service associated with the request be removed from the load balancer") + public Optional getDeleteFromLoadBalancer() { + return deleteFromLoadBalancer; + } + @Override public String toString() { return "SingularityDeleteRequestRequest{" + "message=" + message + ", actionId=" + actionId + + ", deleteFromLoadBalancer=" + deleteFromLoadBalancer + '}'; } } diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java b/SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java index 04618e3333..2e191f6b2a 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java @@ -398,11 +398,11 @@ public Optional getRequest(String requestId, boolea return getData(getRequestPath(requestId), requestTranscoder); } - public void startDeletingRequest(SingularityRequest request, Optional user, Optional actionId, Optional message) { + public void startDeletingRequest(SingularityRequest request, Optional removeFromLoadBalancer, Optional user, Optional actionId, Optional message) { final long now = System.currentTimeMillis(); // delete it no matter if the delete request already exists. - createCleanupRequest(new SingularityRequestCleanup(user, RequestCleanupType.DELETING, now, Optional.of(Boolean.TRUE), request.getId(), Optional. absent(), + createCleanupRequest(new SingularityRequestCleanup(user, RequestCleanupType.DELETING, now, Optional.of(Boolean.TRUE), removeFromLoadBalancer, request.getId(), Optional. absent(), Optional. absent(), message, actionId, Optional.absent())); markDeleting(request, System.currentTimeMillis(), user, message); diff --git a/SingularityService/src/main/java/com/hubspot/singularity/helpers/RequestHelper.java b/SingularityService/src/main/java/com/hubspot/singularity/helpers/RequestHelper.java index 57b08f5bdb..b4429747fd 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/helpers/RequestHelper.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/helpers/RequestHelper.java @@ -97,9 +97,10 @@ private void checkReschedule(SingularityRequest newRequest, Optional actionId = maybeBounceRequest.get().getActionId().or(Optional.of(UUID.randomUUID().toString())); + Optional removeFromLoadBalancer = Optional.absent(); SingularityCreateResult createResult = requestManager.createCleanupRequest( new SingularityRequestCleanup(user, maybeBounceRequest.get().getIncremental().or(true) ? RequestCleanupType.INCREMENTAL_BOUNCE : RequestCleanupType.BOUNCE, - System.currentTimeMillis(), Optional. absent(), newRequest.getId(), Optional.of(maybeDeployId.get()), skipHealthchecks, message, actionId, maybeBounceRequest.get().getRunShellCommandBeforeKill())); + System.currentTimeMillis(), Optional. absent(), removeFromLoadBalancer, newRequest.getId(), Optional.of(maybeDeployId.get()), skipHealthchecks, message, actionId, maybeBounceRequest.get().getRunShellCommandBeforeKill())); if (createResult != SingularityCreateResult.EXISTED) { requestManager.bounce(newRequest, System.currentTimeMillis(), user, Optional.of("Bouncing due to bounce after scale")); diff --git a/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java b/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java index c1199f7a46..4cad009d5b 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java @@ -235,10 +235,11 @@ public SingularityRequestParent bounce(String requestId, Optional removeFromLoadBalancer = Optional.absent(); requestManager.createCleanupRequest( new SingularityRequestCleanup(JavaUtils.getUserEmail(user), isIncrementalBounce ? RequestCleanupType.INCREMENTAL_BOUNCE : RequestCleanupType.BOUNCE, - System.currentTimeMillis(), Optional. absent(), requestId, Optional.of(deployId), skipHealthchecks, message, actionId, runBeforeKill)); + System.currentTimeMillis(), Optional. absent(), removeFromLoadBalancer, requestId, Optional.of(deployId), skipHealthchecks, message, actionId, runBeforeKill)); requestManager.bounce(requestWithState.getRequest(), System.currentTimeMillis(), JavaUtils.getUserEmail(user), message); @@ -378,9 +379,10 @@ public SingularityRequestParent pause(String requestId, Optional removeFromLoadBalancer = Optional.absent(); SingularityCreateResult result = requestManager.createCleanupRequest(new SingularityRequestCleanup(JavaUtils.getUserEmail(user), - RequestCleanupType.PAUSING, now, killTasks, requestId, Optional. absent(), Optional. absent(), message, actionId, runBeforeKill)); + RequestCleanupType.PAUSING, now, killTasks, removeFromLoadBalancer, requestId, Optional. absent(), Optional. absent(), message, actionId, runBeforeKill)); checkConflict(result == SingularityCreateResult.CREATED, "%s is already pausing - try again soon", requestId, result); @@ -617,13 +619,15 @@ public SingularityRequest deleteRequest(String requestId, Optional message = Optional.absent(); Optional actionId = Optional.absent(); + Optional deleteFromLoadBalancer = Optional.absent(); if (deleteRequest.isPresent()) { actionId = deleteRequest.get().getActionId(); message = deleteRequest.get().getMessage(); + deleteFromLoadBalancer = deleteRequest.get().getDeleteFromLoadBalancer(); } - requestManager.startDeletingRequest(request, JavaUtils.getUserEmail(user), actionId, message); + requestManager.startDeletingRequest(request, deleteFromLoadBalancer, JavaUtils.getUserEmail(user), actionId, message); mailer.sendRequestRemovedMail(request, JavaUtils.getUserEmail(user), message); diff --git a/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java b/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java index 9a719fa8e0..5fa3c19a3e 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java @@ -366,7 +366,9 @@ private void drainRequestCleanupQueue() { } else { Optional maybeHistory = requestHistoryHelper.getLastHistory(requestId); if (maybeHistory.isPresent()) { - if (maybeHistory.get().getRequest().isLoadBalanced() && configuration.isDeleteRemovedRequestsFromLoadBalancer()) { + if (maybeHistory.get().getRequest().isLoadBalanced() + && configuration.isDeleteRemovedRequestsFromLoadBalancer() + && requestCleanup.getRemoveFromLoadBalancer().or(true)) { createLbCleanupRequest(requestId, matchingActiveTaskIds); } requestManager.markDeleted(maybeHistory.get().getRequest(), start, requestCleanup.getUser(), requestCleanup.getMessage()); @@ -659,7 +661,7 @@ private void cleanupRequestIfNoRemainingTasks(SingularityTaskCleanup cleanupTask requestManager.createCleanupRequest( new SingularityRequestCleanup( cleanupTask.getUser(), RequestCleanupType.DELETING, System.currentTimeMillis(), - Optional.of(Boolean.TRUE), requestId, Optional. absent(), + Optional.of(Boolean.TRUE), Optional.absent(), requestId, Optional. absent(), Optional. absent(), cleanupTask.getMessage(), Optional. absent(), Optional.absent())); } } diff --git a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java index 02d03d83d9..8db54e3e40 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java @@ -491,7 +491,7 @@ public void testMessage() { } requestResource.scale(requestId, new SingularityScaleRequest(Optional.of(2), Optional. absent(), Optional. absent(), Optional. absent(), Optional.of(msg), Optional.absent(), Optional.absent())); - requestResource.deleteRequest(requestId, Optional.of(new SingularityDeleteRequestRequest(Optional.of("a msg"), Optional. absent()))); + requestResource.deleteRequest(requestId, Optional.of(new SingularityDeleteRequestRequest(Optional.of("a msg"), Optional. absent(), Optional.absent()))); cleaner.drainCleanupQueue(); diff --git a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/HistoryPersisterTest.java b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/HistoryPersisterTest.java index 2b8b91b24e..5d8c2c0245 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/HistoryPersisterTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/HistoryPersisterTest.java @@ -48,7 +48,7 @@ public void testRequestAgePurging() { Assert.assertTrue(!requestManager.getRequestHistory(requestId).isEmpty()); - requestManager.startDeletingRequest(request, user, Optional. absent(), Optional. absent()); + requestManager.startDeletingRequest(request, Optional.absent(), user, Optional. absent(), Optional. absent()); requestManager.deleteHistoryParent(requestId); @@ -79,17 +79,17 @@ public void testRequestCountPurging() { configuration.setMaxRequestsWithHistoryInZkWhenNoDatabase(Optional.of(2)); configuration.setDeleteStaleRequestsFromZkWhenNoDatabaseAfterHours(7); - requestManager.startDeletingRequest(requestOne, user, Optional.absent(), Optional.absent()); + requestManager.startDeletingRequest(requestOne, Optional.absent(), user, Optional.absent(), Optional.absent()); requestManager.deleteHistoryParent(requestOne.getId()); requestManager.activate(requestOne, RequestHistoryType.CREATED, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(4), Optional. absent(), Optional. absent()); requestManager.cooldown(requestOne, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(3)); - requestManager.startDeletingRequest(requestTwo, user, Optional.absent(), Optional.absent()); + requestManager.startDeletingRequest(requestTwo, Optional.absent(), user, Optional.absent(), Optional.absent()); requestManager.deleteHistoryParent(requestTwo.getId()); requestManager.activate(requestTwo, RequestHistoryType.CREATED, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(4), Optional. absent(), Optional. absent()); requestManager.cooldown(requestTwo, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(3)); - requestManager.startDeletingRequest(requestThree, user, Optional.absent(), Optional.absent()); + requestManager.startDeletingRequest(requestThree, Optional.absent(), user, Optional.absent(), Optional.absent()); requestManager.deleteHistoryParent(requestThree.getId()); requestManager.activate(requestThree, RequestHistoryType.CREATED, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(4), Optional. absent(), Optional. absent()); requestManager.cooldown(requestThree, System.currentTimeMillis() - TimeUnit.HOURS.toMillis(3)); @@ -198,7 +198,7 @@ public void testPurgingDoesntApplyIfDatabasePresent() { initRequest(); initFirstDeploy(); - requestManager.startDeletingRequest(request, user, Optional. absent(), Optional. absent()); + requestManager.startDeletingRequest(request, Optional.absent(), user, Optional. absent(), Optional. absent()); requestManager.deleteHistoryParent(requestId); diff --git a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java index 782c61f719..3e359b06bb 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java @@ -878,6 +878,39 @@ public void testLbCleanupDoesNotRemoveBeforeAdd() { Assert.assertTrue(taskManager.getCleanupTaskIds().contains(taskOne.getTaskId())); } + @Test + public void testLbCleanupSkippedOnSkipRemoveFlag() { + configuration.setDeleteRemovedRequestsFromLoadBalancer(true); + initLoadBalancedRequest(); + initLoadBalancedDeploy(); + + boolean removeFromLoadBalancer = false; + SingularityDeleteRequestRequest deleteRequest = new SingularityDeleteRequestRequest(Optional.absent(), Optional.absent(), Optional.of(removeFromLoadBalancer)); + + requestResource.deleteRequest(requestId, Optional.of(deleteRequest)); + + testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING); + + Assert.assertFalse("Should be a request cleanup request", requestManager.getCleanupRequests().isEmpty()); + cleaner.drainCleanupQueue(); + Assert.assertTrue("Should not be a load balancer cleanup request", requestManager.getLbCleanupRequests().isEmpty()); + } + + @Test + public void testLbCleanupOccursOnRequestDelete() { + configuration.setDeleteRemovedRequestsFromLoadBalancer(true); + initLoadBalancedRequest(); + initLoadBalancedDeploy(); + + requestResource.deleteRequest(requestId, Optional.absent()); + + testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING); + + Assert.assertFalse("Should be a request cleanup request", requestManager.getCleanupRequests().isEmpty()); + cleaner.drainCleanupQueue(); + Assert.assertFalse("Should be a request to cleanup the load balancer", requestManager.getLbCleanupRequestIds().isEmpty()); + } + @Test public void testReconciliation() { Assert.assertTrue(!taskReconciliation.isReconciliationRunning()); @@ -960,7 +993,7 @@ public void badPauseExpires() { initRequest(); requestManager.createCleanupRequest(new SingularityRequestCleanup(Optional.absent(), RequestCleanupType.PAUSING, System.currentTimeMillis(), - Optional.absent(), requestId, Optional.absent(), Optional. absent(), Optional.absent(), Optional.absent(), Optional.absent())); + Optional.absent(), Optional.absent(), requestId, Optional.absent(), Optional. absent(), Optional.absent(), Optional.absent(), Optional.absent())); cleaner.drainCleanupQueue(); @@ -1142,7 +1175,7 @@ public void testIncrementalBounce() { SingularityTask taskTwo = startSeparatePlacementTask(firstDeploy, 2); requestManager.createCleanupRequest(new SingularityRequestCleanup(user, RequestCleanupType.INCREMENTAL_BOUNCE, System.currentTimeMillis(), - Optional.absent(), requestId, Optional.of(firstDeployId), Optional. absent(), Optional.absent(), Optional.absent(), Optional.absent())); + Optional.absent(), Optional.absent(), requestId, Optional.of(firstDeployId), Optional. absent(), Optional.absent(), Optional.absent(), Optional.absent())); Assert.assertTrue(requestManager.cleanupRequestExists(requestId)); @@ -1426,7 +1459,7 @@ public void testRemovedRequestData() { deployChecker.checkDeploys(); Assert.assertEquals(DeployState.WAITING, deployManager.getPendingDeploys().get(0).getCurrentDeployState()); - requestManager.startDeletingRequest(request, Optional.absent(), Optional.absent(), Optional.absent()); + requestManager.startDeletingRequest(request, Optional.absent(), Optional.absent(), Optional.absent(), Optional.absent()); requestManager.markDeleted(request, now, Optional.absent(), Optional.absent()); deployChecker.checkDeploys(); SingularityDeployResult deployResult = deployManager.getDeployResult(requestId, firstDeployId).get(); @@ -1440,7 +1473,7 @@ public void testDeletingState() { Assert.assertEquals(RequestState.ACTIVE, requestManager.getRequest(requestId).get().getState()); Assert.assertEquals(1, requestManager.getRequestHistory(requestId).size()); - requestManager.startDeletingRequest(request, Optional.absent(), Optional.absent(), Optional.of("the cake is a lie")); + requestManager.startDeletingRequest(request, Optional.absent(), Optional.absent(), Optional.absent(), Optional.of("the cake is a lie")); Assert.assertEquals(RequestState.DELETING, requestManager.getRequest(requestId).get().getState()); Assert.assertEquals(2, requestManager.getRequestHistory(requestId).size()); @@ -1846,6 +1879,7 @@ public void testScaleWithBounceDoesNotLaunchExtraInstances() { Assert.assertEquals(5, taskManager.getPendingTaskIds().size()); } + @Test public void testAcceptOffersWithRoleForRequestWithRole() { SingularityRequestBuilder bldr = new SingularityRequestBuilder(requestId, RequestType.ON_DEMAND); bldr.setRequiredRole(Optional.of("test-role")); diff --git a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTestBase.java b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTestBase.java index d47bbf02bd..1928d67a31 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTestBase.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTestBase.java @@ -528,6 +528,14 @@ protected void initHCDeploy() { firstDeploy = initAndFinishDeploy(request, new SingularityDeployBuilder(request.getId(), firstDeployId).setCommand(Optional.of("sleep 100")).setHealthcheck(Optional.of(new HealthcheckOptionsBuilder("http://uri").build()))); } + protected void initLoadBalancedDeploy() { + SingularityDeployBuilder builder = new SingularityDeployBuilder(requestId, firstDeployId) + .setCommand(Optional.of("sleep 100")) + .setServiceBasePath(Optional.of("/basepath")) + .setLoadBalancerGroups(Optional.of(Collections.singleton("test"))); + firstDeploy = initAndFinishDeploy(request, builder); + } + protected SingularityDeploy initAndFinishDeploy(SingularityRequest request, String deployId) { return initAndFinishDeploy(request, new SingularityDeployBuilder(request.getId(), deployId).setCommand(Optional.of("sleep 100"))); } From 1c5440d7d1646e3fa2265dc8b3373d506995ceed Mon Sep 17 00:00:00 2001 From: Peter Teixeira Date: Fri, 5 May 2017 08:57:11 -0400 Subject: [PATCH 2/3] Correctly propagate remove request When a request has active tasks, first the tasks are removed and the request cleanup request is destroyed. Once all the tasks are removed, then a new request cleanup request is submitted. I had missed this case, so the `removeFromLoadBalancer` flag was set on the first request, but not on the second, when it would actually take affect. This correctly propagates the `removeFromLoadBalancer` flag across both requests. --- .../singularity/SingularityTaskCleanup.java | 20 ++++++++++++++++++- .../resources/RequestResource.java | 3 +-- .../scheduler/SingularityCleaner.java | 4 ++-- .../scheduler/SingularitySchedulerTest.java | 20 +++++++++++++++---- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityTaskCleanup.java b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityTaskCleanup.java index 2b6c8724b4..012f035e82 100644 --- a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityTaskCleanup.java +++ b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityTaskCleanup.java @@ -13,11 +13,14 @@ public class SingularityTaskCleanup { private final Optional message; private final Optional actionId; private final Optional runBeforeKillId; + private final Optional removeFromLoadBalancer; @JsonCreator public SingularityTaskCleanup(@JsonProperty("user") Optional user, @JsonProperty("cleanupType") TaskCleanupType cleanupType, @JsonProperty("timestamp") long timestamp, @JsonProperty("taskId") SingularityTaskId taskId, @JsonProperty("message") Optional message, @JsonProperty("actionId") Optional actionId, - @JsonProperty("runBeforeKillId") Optional runBeforeKillId) { + @JsonProperty("runBeforeKillId") Optional runBeforeKillId, + @JsonProperty("removeFromLoadBalancer") Optional removeFromLoadBalancer + ) { this.user = user; this.cleanupType = cleanupType; this.timestamp = timestamp; @@ -25,6 +28,17 @@ public SingularityTaskCleanup(@JsonProperty("user") Optional user, @Json this.message = message; this.actionId = actionId; this.runBeforeKillId = runBeforeKillId; + this.removeFromLoadBalancer = removeFromLoadBalancer; + } + + public SingularityTaskCleanup(Optional user, + TaskCleanupType cleanupType, + long timestamp, + SingularityTaskId taskId, + Optional message, + Optional actionId, + Optional runBeforeKillId) { + this(user, cleanupType, timestamp, taskId, message, actionId, runBeforeKillId, Optional.absent()); } public Optional getActionId() { @@ -55,6 +69,10 @@ public Optional getRunBeforeKillId() { return runBeforeKillId; } + public Optional getRemoveFromLoadBalancer() { + return removeFromLoadBalancer; + } + @Override public String toString() { return "SingularityTaskCleanup{" + diff --git a/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java b/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java index 4cad009d5b..2755c1f51a 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java @@ -235,11 +235,10 @@ public SingularityRequestParent bounce(String requestId, Optional removeFromLoadBalancer = Optional.absent(); requestManager.createCleanupRequest( new SingularityRequestCleanup(JavaUtils.getUserEmail(user), isIncrementalBounce ? RequestCleanupType.INCREMENTAL_BOUNCE : RequestCleanupType.BOUNCE, - System.currentTimeMillis(), Optional. absent(), removeFromLoadBalancer, requestId, Optional.of(deployId), skipHealthchecks, message, actionId, runBeforeKill)); + System.currentTimeMillis(), Optional. absent(), Optional.absent(), requestId, Optional.of(deployId), skipHealthchecks, message, actionId, runBeforeKill)); requestManager.bounce(requestWithState.getRequest(), System.currentTimeMillis(), JavaUtils.getUserEmail(user), message); diff --git a/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java b/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java index 5fa3c19a3e..bb35d308fd 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityCleaner.java @@ -497,7 +497,7 @@ private void delete(SingularityRequestCleanup requestCleanup, Iterable absent(), + Optional.of(Boolean.TRUE), cleanupTask.getRemoveFromLoadBalancer(), requestId, Optional. absent(), Optional. absent(), cleanupTask.getMessage(), Optional. absent(), Optional.absent())); } } diff --git a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java index 3e359b06bb..3ff7c5bafc 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java @@ -883,6 +883,7 @@ public void testLbCleanupSkippedOnSkipRemoveFlag() { configuration.setDeleteRemovedRequestsFromLoadBalancer(true); initLoadBalancedRequest(); initLoadBalancedDeploy(); + startTask(firstDeploy); boolean removeFromLoadBalancer = false; SingularityDeleteRequestRequest deleteRequest = new SingularityDeleteRequestRequest(Optional.absent(), Optional.absent(), Optional.of(removeFromLoadBalancer)); @@ -891,9 +892,14 @@ public void testLbCleanupSkippedOnSkipRemoveFlag() { testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING); - Assert.assertFalse("Should be a request cleanup request", requestManager.getCleanupRequests().isEmpty()); + Assert.assertFalse("Tasks should get cleaned up", requestManager.getCleanupRequests().isEmpty()); cleaner.drainCleanupQueue(); - Assert.assertTrue("Should not be a load balancer cleanup request", requestManager.getLbCleanupRequests().isEmpty()); + killKilledTasks(); + + Assert.assertFalse("The request should get cleaned up", requestManager.getCleanupRequests().isEmpty()); + cleaner.drainCleanupQueue(); + + Assert.assertTrue("The request should not be removed from the load balancer", requestManager.getLbCleanupRequestIds().isEmpty()); } @Test @@ -901,14 +907,20 @@ public void testLbCleanupOccursOnRequestDelete() { configuration.setDeleteRemovedRequestsFromLoadBalancer(true); initLoadBalancedRequest(); initLoadBalancedDeploy(); + startTask(firstDeploy); requestResource.deleteRequest(requestId, Optional.absent()); testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING); - Assert.assertFalse("Should be a request cleanup request", requestManager.getCleanupRequests().isEmpty()); + Assert.assertFalse("Tasks should get cleaned up", requestManager.getCleanupRequests().isEmpty()); cleaner.drainCleanupQueue(); - Assert.assertFalse("Should be a request to cleanup the load balancer", requestManager.getLbCleanupRequestIds().isEmpty()); + killKilledTasks(); + + Assert.assertFalse("The request should get cleaned up", requestManager.getCleanupRequests().isEmpty()); + cleaner.drainCleanupQueue(); + + Assert.assertFalse("The request should get removed from the load balancer", requestManager.getLbCleanupRequestIds().isEmpty()); } @Test From bc42354e9fe1a521d2a77258039ec73084abe907 Mon Sep 17 00:00:00 2001 From: Peter Teixeira Date: Fri, 5 May 2017 09:38:26 -0400 Subject: [PATCH 3/3] Add flag to remove modal to delete from lb Add a flag to the remove request modal to delete the request from the load balancer. --- SingularityUI/app/actions/api/requests.es6 | 4 +-- .../common/modalButtons/RemoveButton.jsx | 9 +++++- .../common/modalButtons/RemoveModal.jsx | 30 ++++++++++++++----- .../header/RequestActionButtons.jsx | 5 ++-- 4 files changed, 35 insertions(+), 13 deletions(-) diff --git a/SingularityUI/app/actions/api/requests.es6 b/SingularityUI/app/actions/api/requests.es6 index dbf6bed74a..3514c7a59a 100644 --- a/SingularityUI/app/actions/api/requests.es6 +++ b/SingularityUI/app/actions/api/requests.es6 @@ -39,9 +39,9 @@ export const SaveRequest = buildJsonApiAction( export const RemoveRequest = buildJsonApiAction( 'REMOVE_REQUEST', 'DELETE', - (requestId, { message }) => ({ + (requestId, { message, deleteFromLoadBalancer }) => ({ url: `/requests/request/${requestId}`, - body: { message } + body: { message, deleteFromLoadBalancer } }) ); diff --git a/SingularityUI/app/components/common/modalButtons/RemoveButton.jsx b/SingularityUI/app/components/common/modalButtons/RemoveButton.jsx index c9c524422e..bc3d70079f 100644 --- a/SingularityUI/app/components/common/modalButtons/RemoveButton.jsx +++ b/SingularityUI/app/components/common/modalButtons/RemoveButton.jsx @@ -17,6 +17,7 @@ const removeTooltip = ( export default class RemoveButton extends Component { static propTypes = { requestId: PropTypes.string.isRequired, + loadBalanced: PropTypes.bool, loadBalancerData: PropTypes.object, children: PropTypes.node, then: PropTypes.func @@ -36,7 +37,13 @@ export default class RemoveButton extends Component { return ( {getClickComponent(this)} - + ); } diff --git a/SingularityUI/app/components/common/modalButtons/RemoveModal.jsx b/SingularityUI/app/components/common/modalButtons/RemoveModal.jsx index 234d8923df..fb7ce9cf51 100644 --- a/SingularityUI/app/components/common/modalButtons/RemoveModal.jsx +++ b/SingularityUI/app/components/common/modalButtons/RemoveModal.jsx @@ -5,9 +5,29 @@ import { RemoveRequest } from '../../../actions/api/requests'; import FormModal from '../modal/FormModal'; +const controls = (loadBalanced) => { + const formElements = []; + formElements.push({ + name: 'message', + type: FormModal.INPUT_TYPES.STRING, + label: 'Message (optional)' + }); + if (loadBalanced) { + formElements.push({ + name: 'deleteFromLoadBalancer', + type: FormModal.INPUT_TYPES.BOOLEAN, + label: 'Remove from load balancer', + defaultValue: true + }); + } + + return formElements; +}; + class RemoveModal extends Component { static propTypes = { requestId: PropTypes.string.isRequired, + loadBalanced: PropTypes.bool, loadBalancerData: PropTypes.object, removeRequest: PropTypes.func.isRequired }; @@ -19,7 +39,7 @@ class RemoveModal extends Component { render() { const loadBalancerWarning = (
-

Removing this request will also remove the following settings from the load balancer

+

If you remove this request from the load balancer, the following settings will also be removed:

{JSON.stringify(this.props.loadBalancerData, null, 2)}
); @@ -31,13 +51,7 @@ class RemoveModal extends Component { action="Remove Request" onConfirm={this.props.removeRequest} buttonStyle="danger" - formElements={[ - { - name: 'message', - type: FormModal.INPUT_TYPES.STRING, - label: 'Message (optional)' - } - ]}> + formElements={controls(this.props.loadBalanced)}>

Are you sure you want to remove this request?

{this.props.requestId}

If not paused, removing this request will kill all active and scheduled tasks and tasks for it will not run again unless it is reposted to Singularity.

diff --git a/SingularityUI/app/components/requestDetail/header/RequestActionButtons.jsx b/SingularityUI/app/components/requestDetail/header/RequestActionButtons.jsx index f8d6be3fdb..7b34d90bad 100644 --- a/SingularityUI/app/components/requestDetail/header/RequestActionButtons.jsx +++ b/SingularityUI/app/components/requestDetail/header/RequestActionButtons.jsx @@ -8,7 +8,7 @@ import { Link } from 'react-router'; import JSONButton from '../../common/JSONButton'; import { FetchRequest } from '../../../actions/api/requests'; -import { +import { FetchActiveTasksForRequest, FetchRequestHistory } from '../../../actions/api/history'; @@ -161,8 +161,9 @@ const RequestActionButtons = ({requestParent, fetchRequest, fetchRequestHistory, } const removeButton = ( -