Scheduler/TaskReaper: handle unassigned tasks marked for shutdown by anshulpundir · Pull Request #2574 · moby/swarmkit

anshulpundir · 2018-03-27T22:41:41Z

scheduler: changes to ignore unassigned tasks.
task reaper: changes to cleanup unassigned tasks in terminal state.

nishanttotla · 2018-03-27T22:50:33Z


+		// Also ignore tasks that have not yet been assigned but desired state is beyond TaskStateRunning
+		// This can happen if you update, delete or scale down a service before its tasks were assigned.
+		if t.Status.State == api.TaskStatePending && t.DesiredState > api.TaskStateRunning {


Why is this comparison not t.Status.State <= api.TaskStatePending

The < is already handled in the condition above @nishanttotla

@anshulpundir sorry maybe I'm missing something, but for case api.EventUpdateTask:, I don't see a specific condition about this.

My bad, please ignore my comments. I got confused between two changes.

codecov · 2018-03-27T22:54:00Z

Codecov Report

Merging #2574 into master will decrease coverage by 0.71%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2574      +/-   ##
==========================================
- Coverage   62.26%   61.55%   -0.72%     
==========================================
  Files         134      134              
  Lines       21736    21805      +69     
==========================================
- Hits        13535    13422     -113     
- Misses       6754     6935     +181     
- Partials     1447     1448       +1

nishanttotla · 2018-03-28T21:43:44Z

+
+				// Also clean up tasks which have not yet been assigned but have been
+				// marked for shutdown (likley because of a service update).
+				if t.Status.State < api.TaskStateAssigned && t.DesiredState == api.TaskStateShutdown {


Based on IRL discussion with @anshulpundir, we think there can be a race condition here (see also related discussion in #2557):

I'm unable to come up with an exact scenario, but it's something to think about cc @dperny @cyli. If y'all think this looks good, we can merge it.

UPDATE: Based on another IRL discussion with @anshulpundir, we concluded that this change looks fine, in conjunction with the change in manager/scheduler/scheduler.go. The main idea is that as long as current state is <= PENDING, the task will not have reached the agent. Hence, it is safe to more aggressively remove it in the task reaper. After this change, we think it is also safe to merge #2557.

cyli

LGTM

cyli

Actually apologies for being so mercurial, but I have 3 questions on further examination:

For task reaper, do we also need to find pending tasks that are in desired state shutdown when the task reaper first starts up? I wasn't entirely sure from reading that moby issue, but do the tasks get stuck permanently in pending/shutdown, and if so, would they get stuck there forever if an update was lost due to a change in leadership?
Would it be possible to add tests for reaping these cases?
Would it make sense to also add a test for the case where a scheduler won't schedule a task if it's pending but the desired state is > running?

anshulpundir · 2018-03-28T23:58:52Z

For task reaper, do we also need to find pending tasks that are in desired state shutdown? I wasn't entirely sure from reading that moby issue, but do the tasks get stuck permanently in pending/shutdown, and if so, would they get stuck there forever if an update was lost due to a change in leadership?

I think they get stuck regardless since the reaper doesn't consider tasks with current state < assigned.

Would it be possible to add tests for reaping these cases?
Would it make sense to also add a test for the case where a scheduler won't schedule a task if it's pending but the desired state is > running?

yup, updating the PR!

cyli · 2018-03-29T00:04:19Z

I think they get stuck regardless since the reaper doesn't consider tasks with current state < assigned.

@anshulpundir Er sorry, I asked that question backwards. I meant do we need to add a similar check for current state < assigned and desired state == shutdown to https://github.com/anshulpundir/swarmkit/blob/b1bbd23bf33f90d643016a54f31bfcb4eaaa9c6e/manager/orchestrator/taskreaper/task_reaper.go#L72? Because if the task reaper gets the update that a pending task should shutdown, but then dies because of a leader change, and the next leader starts up, it may not get the update event. So would that pending/shutdown task get stuck forever?

anshulpundir · 2018-03-29T00:05:21Z

Ahh yes, I meant to update that. But yes, thx for pointing that out! @cyli

… running. Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

nishanttotla · 2018-03-29T16:38:57Z

Good point @cyli. On that note, we might want to think about modularizing some code in the task reaper, since we've been having to put every new condition in two places.

anshulpundir · 2018-03-30T22:59:52Z

ping @cyli @nishanttotla for review.

nishanttotla · 2018-04-02T18:26:00Z


 	// Set both task states to RUNNING.
 	updatedTask1 := observedTask1.Copy()
-	updatedTask1.DesiredState = api.TaskStateRunning


@anshulpundir sorry I don't follow why this is removed?

because desired state is api.TaskStateRunning when a task is created.

Okay, makes sense.

nishanttotla · 2018-04-02T19:02:00Z

+	// Normally task2/task3 should get assigned first since its a preassigned task.
+	assignment3 := watchAssignment(t, watch)
+	assert.Equal(t, assignment3.ID, "task1")
+	assert.Equal(t, assignment3.NodeID, "node1")


Should we also assert here that task2/task3 are still in the same state?

cyli

Apologies @anshulpundir, just one question on task history and pending/shutdown tasks.

cyli · 2018-04-03T00:09:12Z

+		if err != nil {
+			log.G(ctx).WithError(err).Error("failed to find tasks with desired state SHUTDOWN in task reaper init")
+		}
+		for _, t := range shutdownTasks {


Non-blocking optimization: I was wondering if it'd make sense to move this loop outside of the view? If there are a lot of shutdown tasks, this could block whatever lock the view function is holding.

also a great point.

cyli · 2018-04-03T00:11:21Z

+		}
+		for _, t := range shutdownTasks {
+			if t.Status.State < api.TaskStateAssigned {
+				tr.cleanup = append(tr.cleanup, t.ID)


Would this cleanup right away, if these are added here? If there are any orphaned or remove tasks, line 126 calls tr.tick(), which cleans up all tasks in tr.cleanup immediately.

Perhaps this loop should be moved to below the orphaned and removed tasks check? Would it make sense to add a check for this to make sure that these tasks are handled through the regular history process?

Nice catch. I'll fix this. I think I can also add a unit-test for this.

cyli · 2018-04-03T00:18:43Z

+
+		testutils.Expect(t, watch, state.EventCommit{})
+
+		// Set the task state for the restarted task PENDING to simulate allocation.


Non-blocking: To simulate what actually happens, should this be set to pending before the force update, so that the task is already in pending state when it is shut down? If not, why must it occur here, instead of before the force update?

…n assigned but have been marked for shutdown. Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

nishanttotla · 2018-04-04T16:25:48Z

+				// This check is important to ignore tasks which are running or need to be running,
+				// but to delete tasks which are either past running,
+				// or have not reached running but need to be shutdown (because of a service update, for example).
+				if t.DesiredState == api.TaskStateRunning && t.Status.State <= api.TaskStateRunning {


I'm not sure this is really needed. Don't new tasks always start with desired state running?

I think so: https://github.com/docker/swarmkit/blob/master/api/objects.proto#L230

Not sure why you think this is not needed. We need to shutdown tasks with desired state SHUTDOWN and actual state < ASSIGNED.

This condition is to skip tasks which are not those covered by the condition above. LMK if this is not clear and we can discuss IRL. @nishanttotla

Oh oops, I missed the change from || to &&, because previously it would count a task with desired state of SHUTDOWN and current state of PENDING to be running - thanks for fixing.

Oops, I missed it too. This is needed, sorry for the sliip.

cyli

LGTM, thanks @anshulpundir

Relevant changes: - moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership - moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown - moby/swarmkit#2561 Avoid predefined error log - moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned - moby/swarmkit#2587 [fips] Agent reports FIPS status - moby/swarmkit#2603 Fix manager/state/store.timedMutex Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Relevant changes: - moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership - moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown - moby/swarmkit#2561 Avoid predefined error log - moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned - moby/swarmkit#2587 [fips] Agent reports FIPS status - moby/swarmkit#2603 Fix manager/state/store.timedMutex Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 333b2f28fef4ba857905e7263e7b9bbbf7c522fc Component: engine

Relevant changes: - moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership - moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown - moby/swarmkit#2561 Avoid predefined error log - moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned - moby/swarmkit#2587 [fips] Agent reports FIPS status - moby/swarmkit#2603 Fix manager/state/store.timedMutex Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 333b2f28fef4ba857905e7263e7b9bbbf7c522fc) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

nishanttotla reviewed Mar 27, 2018

View reviewed changes

anshulpundir changed the title ~~[manager/scheduler] Ignore unassigned tasks with desired state beyond running~~ Handle unassigned tasks marked for shutdown. Mar 27, 2018

nishanttotla reviewed Mar 28, 2018

View reviewed changes

cyli approved these changes Mar 28, 2018

View reviewed changes

cyli reviewed Mar 28, 2018

View reviewed changes

[manager/scheduler] Ignore unassigned tasks with desired state beyond…

e3fcf6d

… running. Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir changed the title ~~Handle unassigned tasks marked for shutdown.~~ Handle unassigned tasks marked for shutdown Mar 30, 2018

anshulpundir force-pushed the sched branch from b1bbd23 to efec412 Compare March 30, 2018 21:20

anshulpundir changed the title ~~Handle unassigned tasks marked for shutdown~~ Scheduler/TaskReaper: handle unassigned tasks marked for shutdown Mar 30, 2018

anshulpundir force-pushed the sched branch from efec412 to 5826637 Compare March 30, 2018 21:30

nishanttotla reviewed Apr 2, 2018

View reviewed changes

nishanttotla approved these changes Apr 2, 2018

View reviewed changes

cyli reviewed Apr 3, 2018

View reviewed changes

[manager/orchestrator/taskreaper] Cleanup tasks that have not yet bee…

88064b5

…n assigned but have been marked for shutdown. Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir force-pushed the sched branch from 5826637 to 88064b5 Compare April 3, 2018 23:43

nishanttotla reviewed Apr 4, 2018

View reviewed changes

cyli approved these changes Apr 4, 2018

View reviewed changes

nishanttotla merged commit de4c028 into moby:master Apr 4, 2018

anshulpundir deleted the sched branch April 4, 2018 21:08

nishanttotla mentioned this pull request Apr 4, 2018

Task reaper should delete tasks with removed slots that were not yet assigned #2557

Merged

thaJeztah mentioned this pull request Apr 17, 2018

Bump SwarmKit to 9c2aa152c3054371b833483a7ddad8d15052ec4f moby/moby#36880

Merged

thaJeztah mentioned this pull request Apr 19, 2018

[18.03] update swarmkit docker-archive/docker-ce#521

Closed

This was referenced May 1, 2018

[18.03] Cherry picking scheduler and task reaper fixes, plus allocator logging. #2616

Merged

[bump_v17.06] Adding Task State Remove and other fixes #2623

Merged

This was referenced Jul 17, 2018

Revert "[manager/scheduler] Ignore unassigned tasks with desired state beyond running." [WIP] #2710

Closed

Services in global mode do not reschedule when stopped #2705

Closed


		testutils.Expect(t, watch, state.EventCommit{})

		// Set the task state for the restarted task PENDING to simulate allocation.

Conversation

anshulpundir commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli left a comment

Choose a reason for hiding this comment

Uh oh!

cyli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anshulpundir commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyli commented Mar 29, 2018

Uh oh!

anshulpundir commented Mar 29, 2018

Uh oh!

nishanttotla commented Mar 29, 2018

Uh oh!

anshulpundir commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anshulpundir commented Mar 27, 2018 •

edited

Loading

codecov Bot commented Mar 27, 2018 •

edited

Loading

cyli left a comment •

edited

Loading

anshulpundir commented Mar 28, 2018 •

edited

Loading