Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Retry failed tasks with new deploys #2269

Merged
merged 5 commits into from
Feb 23, 2022
Merged

Retry failed tasks with new deploys #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae13ac3
log obsolete request ids
Feb 22, 2022
41be52d
retry failed tasks even if they have a new deploy
Feb 22, 2022
f257650
add unit test for retrying tasks with new deploys
Feb 22, 2022
afd016c
tweak task state success check
Feb 22, 2022
dd71fbb
fix unit test
Feb 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions SingularityService/src/main/java/com/hubspot/singularity/scheduler/SingularityScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ private void handlePendingRequestsForDeployKey(
);

if (!isRequestActive(maybeRequest)) {
LOG.debug(
LOG.info(
"Pending request {} was obsolete (request {})",
requestId,
SingularityRequestWithState.getRequestState(maybeRequest)
Expand Down Expand Up @@ -475,7 +475,7 @@ private void handlePendingRequestsForDeployKey(
maybeRequestDeployState
)
) {
LOG.debug(
LOG.info(
"Pending request {} was obsolete (request {})",
pendingRequest,
SingularityRequestWithState.getRequestState(maybeRequest)
Expand Down Expand Up @@ -1014,12 +1014,28 @@ private Optional<PendingType> handleCompletedTaskWithStatistics(
);

if (!isDeployInUse(requestDeployState, taskId.getDeployId(), true)) {
LOG.debug(
"Task {} completed, but it didn't match active deploy state {} - ignoring",
taskId.getId(),
requestDeployState
);
return Optional.empty();
// failed tasks with remaining retries cannot short circuit here
// isFailed is false for lost and other weird task states
if (
!state.isSuccess() &&
(
deployStatistics.getNumSequentialRetries() <
request.getNumRetriesOnFailure().orElse(0)
)
) {
LOG.info(
"Task {} failed, but it didn't match active deploy state {} - may retry with current deploy state",
taskId.getId(),
requestDeployState
);
} else {
LOG.info(
"Task {} completed, but it didn't match active deploy state {} - ignoring",
taskId.getId(),
requestDeployState
);
return Optional.empty();
}
}

if (
Expand Down
92 changes: 92 additions & 0 deletions ...rityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,98 @@ public void testRetriesWithOverrides() {
Assertions.assertTrue(taskManager.getActiveTaskIds().isEmpty());
}

@Test
public void testRetriesWithNewDeploys() {
SingularityRequestBuilder bldr = new SingularityRequestBuilder(
requestId,
RequestType.RUN_ONCE
);
request = bldr.setNumRetriesOnFailure(Optional.of(2)).build();
saveRequest(request);

deployResource.deploy(
new SingularityDeployRequest(
new SingularityDeployBuilder(requestId, "d1")
.setCommand(Optional.of("cmd"))
.build(),
Optional.empty(),
Optional.empty()
),
singularityUser
);

scheduler.drainPendingQueue();
deployChecker.checkDeploys();
scheduler.drainPendingQueue();
resourceOffers();

Assertions.assertEquals(1, taskManager.getActiveTaskIds().size());

deployResource.deploy(
new SingularityDeployRequest(
new SingularityDeployBuilder(requestId, "d2")
.setCommand(Optional.of("cmd"))
.build(),
Optional.empty(),
Optional.empty()
),
singularityUser
);

statusUpdate(taskManager.getActiveTasks().get(0), TaskState.TASK_LOST);

scheduler.drainPendingQueue();
deployChecker.checkDeploys();
scheduler.drainPendingQueue();
resourceOffers();

Assertions.assertEquals(1, taskManager.getActiveTaskIds().size());
Assertions.assertEquals("d2", taskManager.getActiveTaskIds().get(0).getDeployId());

deployResource.deploy(
new SingularityDeployRequest(
new SingularityDeployBuilder(requestId, "d3")
.setCommand(Optional.of("cmd"))
.build(),
Optional.empty(),
Optional.empty()
),
singularityUser
);

statusUpdate(taskManager.getActiveTasks().get(0), TaskState.TASK_LOST);

scheduler.drainPendingQueue();
deployChecker.checkDeploys();
scheduler.drainPendingQueue();
resourceOffers();

Assertions.assertEquals(1, taskManager.getActiveTaskIds().size());
Assertions.assertEquals("d3", taskManager.getActiveTaskIds().get(0).getDeployId());

deployResource.deploy(
new SingularityDeployRequest(
new SingularityDeployBuilder(requestId, "d4")
.setCommand(Optional.of("cmd"))
.build(),
Optional.empty(),
Optional.empty()
),
singularityUser
);

statusUpdate(taskManager.getActiveTasks().get(0), TaskState.TASK_LOST);

scheduler.drainPendingQueue();
deployChecker.checkDeploys();
scheduler.drainPendingQueue();
resourceOffers();

// TODO - new deploys have new deploy statistics, so we lose track of the # of retries
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tasks could exceed numRetries if they deploy often enough, but cooldowns should keep the retries from becoming a problem.

Debated adding a new retryIgnoreDeploy field to requests or deploys, but figured that was overkill.

Assertions.assertEquals(1, taskManager.getActiveTaskIds().size());
Assertions.assertEquals("d4", taskManager.getActiveTaskIds().get(0).getDeployId());
}

/* @Test
public void testCooldownAfterSequentialFailures() {
initRequest();
Expand Down