Skip to content

Commit b2d5298

Browse files
ssalinasssalinas
committed
test and tweaks
1 parent 867e89d commit b2d5298

File tree

4 files changed

+156
-48
lines changed

4 files changed

+156
-48
lines changed

‎SingularityBase/src/main/java/com/hubspot/singularity/SingularityDeployProgress.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,14 +225,16 @@ public SingularityDeployProgress withPendingLbUpdate(
225225
return withPendingLbUpdate(
226226
loadBalancerUpdate,
227227
Collections.emptySet(),
228-
Collections.emptySet()
228+
Collections.emptySet(),
229+
false
229230
);
230231
}
231232

232233
public SingularityDeployProgress withPendingLbUpdate(
233234
SingularityLoadBalancerUpdate loadBalancerUpdate,
234235
Collection<SingularityTaskId> added,
235-
Collection<SingularityTaskId> removed
236+
Collection<SingularityTaskId> removed,
237+
boolean stepLaunchComplete
236238
) {
237239
Map<String, DeployProgressLbUpdateHolder> lbUpdateMap = new HashMap<>(lbUpdates);
238240
lbUpdateMap.put(
@@ -246,7 +248,7 @@ public SingularityDeployProgress withPendingLbUpdate(
246248
return new SingularityDeployProgress(
247249
targetActiveInstances,
248250
currentActiveInstances,
249-
false,
251+
stepLaunchComplete,
250252
failedDeployTasks,
251253
System.currentTimeMillis(),
252254
lbUpdateMap,

‎SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityDeployChecker.java

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,7 +1032,8 @@ private SingularityDeployResult checkDeployProgress(
10321032
deploy,
10331033
deployActiveTasks,
10341034
request,
1035-
updatePendingDeployRequest
1035+
updatePendingDeployRequest,
1036+
pendingDeploy.getDeployProgress()
10361037
);
10371038
}
10381039

@@ -1144,6 +1145,7 @@ private SingularityDeployResult checkAcceptanceOfDeployStep(
11441145
DeployState acceptanceHookDeployState = SingularityDeployAcceptanceManager.resultsToDeployState(
11451146
updatedProgress.getStepAcceptanceResults()
11461147
);
1148+
LOG.info("Acceptance checks had result {}", acceptanceHookDeployState);
11471149
if (deploy.getCanaryDeploySettings().isEnableCanaryDeploy()) {
11481150
if (acceptanceHookDeployState == DeployState.SUCCEEDED) {
11491151
if (deployActiveTasks.size() >= request.getInstancesSafe()) {
@@ -1180,37 +1182,10 @@ private SingularityDeployResult checkAcceptanceOfDeployStep(
11801182
acceptanceHookDeployState,
11811183
updatedProgress
11821184
);
1183-
if (
1184-
!deploy.getCanaryDeploySettings().isEnableCanaryDeploy() &&
1185-
request.isLoadBalanced() &&
1186-
configuration.getLoadBalancerUri() != null
1187-
) {
1188-
// Add previous tasks back to load balancer, since we previously took them out
1189-
if (pendingDeploy.getDeployProgress().getPendingLbUpdate().isPresent()) {
1190-
return checkLbRevertToActiveTasks(
1191-
request,
1192-
deploy,
1193-
pendingDeploy,
1194-
updatedProgress,
1195-
acceptanceHookDeployState,
1196-
deployActiveTasks,
1197-
otherActiveTasks
1198-
);
1199-
} else {
1200-
return enqueueLbRevertToActiveTasks(
1201-
request,
1202-
pendingDeploy,
1203-
updatedProgress,
1204-
deployActiveTasks,
1205-
otherActiveTasks
1206-
);
1207-
}
1208-
} else {
1209-
return new SingularityDeployResult(
1210-
acceptanceHookDeployState,
1211-
String.join(", ", updatedProgress.getAcceptanceResultMessageHistory())
1212-
);
1213-
}
1185+
return new SingularityDeployResult(
1186+
acceptanceHookDeployState,
1187+
String.join(", ", updatedProgress.getAcceptanceResultMessageHistory())
1188+
);
12141189
}
12151190
updatePendingDeploy(pendingDeploy, acceptanceHookDeployState, updatedProgress);
12161191
return new SingularityDeployResult(acceptanceHookDeployState);
@@ -1243,7 +1218,35 @@ private SingularityDeployResult checkAcceptanceOfDeployStep(
12431218
acceptanceHookDeployState,
12441219
updatedProgress
12451220
);
1246-
return new SingularityDeployResult(acceptanceHookDeployState);
1221+
if (
1222+
request.isLoadBalanced() && configuration.getLoadBalancerUri() != null
1223+
) {
1224+
// Add previous tasks back to load balancer, since we previously took them out
1225+
if (updatedProgress.getPendingLbUpdate().isPresent()) {
1226+
return checkLbRevertToActiveTasks(
1227+
request,
1228+
deploy,
1229+
pendingDeploy,
1230+
updatedProgress,
1231+
acceptanceHookDeployState,
1232+
deployActiveTasks,
1233+
otherActiveTasks
1234+
);
1235+
} else {
1236+
return enqueueLbRevertToActiveTasks(
1237+
request,
1238+
pendingDeploy,
1239+
updatedProgress,
1240+
deployActiveTasks,
1241+
otherActiveTasks
1242+
);
1243+
}
1244+
} else {
1245+
return new SingularityDeployResult(
1246+
acceptanceHookDeployState,
1247+
String.join(", ", updatedProgress.getAcceptanceResultMessageHistory())
1248+
);
1249+
}
12471250
}
12481251
}
12491252
case NONE:
@@ -1383,7 +1386,8 @@ private SingularityDeployResult enqueueLbRevertToActiveTasks(
13831386
SingularityDeployProgress deployProgress = updatedProgress.withPendingLbUpdate(
13841387
enqueueResult,
13851388
toAddBack,
1386-
Collections.emptyList()
1389+
deployActiveTasks,
1390+
true
13871391
);
13881392
updatePendingDeploy(pendingDeploy, DeployState.WAITING, deployProgress);
13891393
return new SingularityDeployResult(DeployState.WAITING);
@@ -1535,7 +1539,7 @@ private SingularityDeployResult enqueueAndProcessLbRequest(
15351539
// Save the lb enqueue + added/removed tasks for later
15361540
SingularityDeployProgress deployProgress = pendingDeploy
15371541
.getDeployProgress()
1538-
.withPendingLbUpdate(enqueueResult, deployActiveTasks, toRemoveFromLb);
1542+
.withPendingLbUpdate(enqueueResult, deployActiveTasks, toRemoveFromLb, false);
15391543
updatePendingDeploy(pendingDeploy, DeployState.WAITING, deployProgress);
15401544
maybeUpdatePendingRequest(pendingDeploy, deploy, request, updatePendingDeployRequest);
15411545
return new SingularityDeployResult(DeployState.WAITING);
@@ -1576,18 +1580,19 @@ private SingularityDeployResult processLbState(
15761580
pendingDeploy.getCurrentDeployState()
15771581
);
15781582
if (deployState == DeployState.SUCCEEDED) {
1579-
updatePendingDeploy(
1580-
pendingDeploy,
1581-
DeployState.WAITING,
1582-
deployProgress.withFinishedLbUpdate(lbUpdate, lbUpdateHolder)
1583+
SingularityDeployProgress updatedProgress = deployProgress.withFinishedLbUpdate(
1584+
lbUpdate,
1585+
lbUpdateHolder
15831586
);
1587+
updatePendingDeploy(pendingDeploy, DeployState.WAITING, updatedProgress);
15841588
// All tasks for current step are launched and in the LB if needed
15851589
return markStepLaunchFinished(
15861590
pendingDeploy,
15871591
deploy,
15881592
deployActiveTasks,
15891593
request,
1590-
updatePendingDeployRequest
1594+
updatePendingDeployRequest,
1595+
updatedProgress
15911596
);
15921597
} else if (deployState == DeployState.WAITING) {
15931598
updatePendingDeploy(
@@ -1596,7 +1601,8 @@ private SingularityDeployResult processLbState(
15961601
deployProgress.withPendingLbUpdate(
15971602
lbUpdate,
15981603
lbUpdateHolder.getAdded(),
1599-
lbUpdateHolder.getRemoved()
1604+
lbUpdateHolder.getRemoved(),
1605+
false
16001606
)
16011607
);
16021608
maybeUpdatePendingRequest(
@@ -1663,10 +1669,9 @@ private SingularityDeployResult markStepLaunchFinished(
16631669
SingularityDeploy deploy,
16641670
Collection<SingularityTaskId> deployActiveTasks,
16651671
SingularityRequest request,
1666-
Optional<SingularityUpdatePendingDeployRequest> updatePendingDeployRequest
1672+
Optional<SingularityUpdatePendingDeployRequest> updatePendingDeployRequest,
1673+
SingularityDeployProgress deployProgress
16671674
) {
1668-
SingularityDeployProgress deployProgress = pendingDeploy.getDeployProgress();
1669-
16701675
if (updatePendingDeployRequest.isPresent()) {
16711676
maybeUpdatePendingRequest(
16721677
pendingDeploy,

‎SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularityDeployAcceptanceTest.java

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,23 @@
11
package com.hubspot.singularity.scheduler;
22

33
import com.google.inject.Inject;
4+
import com.hubspot.baragon.models.BaragonRequestState;
45
import com.hubspot.singularity.CanaryDeploySettings;
56
import com.hubspot.singularity.DeployAcceptanceMode;
67
import com.hubspot.singularity.DeployAcceptanceResult;
78
import com.hubspot.singularity.DeployAcceptanceState;
9+
import com.hubspot.singularity.DeployProgressLbUpdateHolder;
810
import com.hubspot.singularity.DeployState;
911
import com.hubspot.singularity.SingularityDeployBuilder;
1012
import com.hubspot.singularity.SingularityDeployProgress;
1113
import com.hubspot.singularity.SingularityDeployResult;
14+
import com.hubspot.singularity.SingularityPendingDeploy;
1215
import com.hubspot.singularity.SingularityRequest;
1316
import com.hubspot.singularity.SingularityTask;
1417
import com.hubspot.singularity.SingularityTaskId;
1518
import com.hubspot.singularity.api.SingularityDeployRequest;
1619
import com.hubspot.singularity.hooks.DeployAcceptanceHook;
20+
import java.util.Collections;
1721
import java.util.Optional;
1822
import java.util.Set;
1923
import org.apache.mesos.v1.Protos.TaskState;
@@ -275,4 +279,101 @@ public void testDeployWithAcceptanceConditions() {
275279
deployManager.getDeployResult(requestId, secondDeployId).get().getDeployState()
276280
);
277281
}
282+
283+
@Test
284+
public void testLbRevertsAfterFailedAcceptanceStepOnNonCanary() {
285+
NoopDeployAcceptanceHook hook = (NoopDeployAcceptanceHook) acceptanceHooks
286+
.iterator()
287+
.next();
288+
hook.setNextResult(
289+
new DeployAcceptanceResult(DeployAcceptanceState.FAILED, "ruh-roh")
290+
);
291+
initLoadBalancedRequest();
292+
initFirstDeploy();
293+
SingularityTask firstTask = launchTask(
294+
request,
295+
firstDeploy,
296+
1,
297+
TaskState.TASK_RUNNING
298+
);
299+
SingularityDeployBuilder builder = new SingularityDeployBuilder(
300+
requestId,
301+
secondDeployId
302+
);
303+
builder
304+
.setCommand(Optional.of("sleep 1"))
305+
.setCanaryDeploySettings(
306+
CanaryDeploySettings
307+
.newbuilder()
308+
.setAcceptanceMode(DeployAcceptanceMode.CHECKS)
309+
.setEnableCanaryDeploy(false)
310+
.build()
311+
)
312+
.setServiceBasePath(Optional.of("/basepath"))
313+
.setLoadBalancerGroups(Optional.of(Collections.singleton("group")));
314+
deployResource.deploy(
315+
new SingularityDeployRequest(builder.build(), Optional.of(false), Optional.empty()),
316+
singularityUser
317+
);
318+
deployChecker.checkDeploys();
319+
scheduler.drainPendingQueue();
320+
Assertions.assertEquals(1, taskManager.getPendingTaskIds().size());
321+
resourceOffers();
322+
323+
Assertions.assertEquals(
324+
1,
325+
taskManager.getActiveTaskIdsForDeploy(requestId, secondDeployId).size()
326+
);
327+
SingularityTaskId firstNewTaskId = taskManager
328+
.getActiveTaskIdsForDeploy(requestId, secondDeployId)
329+
.get(0);
330+
statusUpdate(taskManager.getTask(firstNewTaskId).get(), TaskState.TASK_RUNNING);
331+
332+
deployChecker.checkDeploys();
333+
SingularityPendingDeploy pendingDeploy = deployManager
334+
.getPendingDeploy(requestId)
335+
.get();
336+
Assertions.assertEquals(DeployState.WAITING, pendingDeploy.getCurrentDeployState());
337+
testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING);
338+
339+
deployChecker.checkDeploys();
340+
341+
pendingDeploy = deployManager.getPendingDeploy(requestId).get();
342+
Assertions.assertEquals(DeployState.WAITING, pendingDeploy.getCurrentDeployState());
343+
testingLbClient.setNextBaragonRequestState(BaragonRequestState.SUCCESS);
344+
345+
deployChecker.checkDeploys();
346+
347+
// Acceptance checks fail
348+
testingLbClient.setNextBaragonRequestState(BaragonRequestState.WAITING);
349+
deployChecker.checkDeploys();
350+
pendingDeploy = deployManager.getPendingDeploy(requestId).get();
351+
Assertions.assertEquals(DeployState.WAITING, pendingDeploy.getCurrentDeployState());
352+
Assertions.assertEquals(
353+
DeployAcceptanceState.FAILED,
354+
pendingDeploy
355+
.getDeployProgress()
356+
.getStepAcceptanceResults()
357+
.entrySet()
358+
.iterator()
359+
.next()
360+
.getValue()
361+
);
362+
SingularityDeployProgress deployProgress = pendingDeploy.getDeployProgress();
363+
DeployProgressLbUpdateHolder lbUpdateHolder = deployProgress
364+
.getLbUpdates()
365+
.get(
366+
deployProgress.getPendingLbUpdate().get().getLoadBalancerRequestId().toString()
367+
);
368+
Assertions.assertTrue(lbUpdateHolder.getAdded().contains(firstTask.getTaskId()));
369+
Assertions.assertTrue(lbUpdateHolder.getRemoved().contains(firstNewTaskId));
370+
371+
testingLbClient.setNextBaragonRequestState(BaragonRequestState.SUCCESS);
372+
deployChecker.checkDeploys();
373+
SingularityDeployResult deployResult = deployManager
374+
.getDeployResult(requestId, secondDeployId)
375+
.get();
376+
Assertions.assertEquals(DeployState.FAILED, deployResult.getDeployState());
377+
Assertions.assertTrue(deployResult.getMessage().get().contains("ruh-roh"));
378+
}
278379
}

‎SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularityTestModule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public SingularityTestModule(
112112
Logger hsLogger = context.getLogger("com.hubspot");
113113
hsLogger.setLevel(
114114
Level.toLevel(
115-
System.getProperty("singularity.test.log.level.for.com.hubspot", "WARN")
115+
System.getProperty("singularity.test.log.level.for.com.hubspot", "DEBUG")
116116
)
117117
);
118118

0 commit comments

Comments
 (0)