Task reaper should delete tasks with removed slots that were not yet assigned by nishanttotla · Pull Request #2557 · moby/swarmkit

nishanttotla · 2018-03-14T00:13:32Z

This PR addresses #2555.

cc @docker/core-swarmkit-maintainers

Signed-off-by: Nishant Totla nishanttotla@gmail.com

codecov · 2018-03-14T00:25:28Z

Codecov Report

Merging #2557 into master will increase coverage by 0.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2557      +/-   ##
==========================================
+ Coverage   61.74%   62.19%   +0.45%     
==========================================
  Files          51      134      +83     
  Lines        7112    21761   +14649     
==========================================
+ Hits         4391    13535    +9144     
- Misses       2264     6783    +4519     
- Partials      457     1443     +986

stevvooe · 2018-03-14T00:38:59Z

LGTM

anshulpundir

Can we also add unit-test for this ?

Also, I think this will introduce races where the task might get assigned to an agent as its being removed by the reaper.

anshulpundir · 2018-03-14T00:48:32Z

+		// haven't been assigned yet can be cleaned up right away
 		for _, t := range removeTasks {
-			if t.Status.State >= api.TaskStateCompleted {
+			if t.Status.State >= api.TaskStateCompleted || t.Status.State < api.TaskStateAssigned {


nit: I'd suggest making it:

t.Status.State < api.TaskStateAssigned || t.Status.State >= api.TaskStateCompleted

slightly better for reading

anshulpundir · 2018-03-14T00:49:14Z

+				// add tasks that are yet unassigned or have progressed beyond COMPLETE, with
+				// desired state REMOVE. These tasks are associated with slots that were removed
+				// as part of a service scale down or service removal.
+				if t.DesiredState == api.TaskStateRemove && (t.Status.State >= api.TaskStateCompleted || t.Status.State < api.TaskStateAssigned) {


same as above.

nishanttotla · 2018-03-14T01:07:19Z

@anshulpundir good point about races. Unless the agent doesn't try to run tasks that have desired state REMOVE, the race will exist.

As of now, I don't think the agent does this check cc @stevvooe

anshulpundir · 2018-03-14T02:15:18Z

+				// add tasks that are yet unassigned or have progressed beyond COMPLETE, with
+				// desired state REMOVE. These tasks are associated with slots that were removed
+				// as part of a service scale down or service removal.
+				if t.DesiredState == api.TaskStateRemove && (t.Status.State >= api.TaskStateCompleted || t.Status.State < api.TaskStateAssigned) {


Also, do we expect to get task update events for tasks which are marked REMOVE before they are assigned to the agent ? If a task is marked REMOVE, does the orchestrator even assign it to an agent ? @nishanttotla

Good point @anshulpundir. It would be fair to expect the orchestrator to not assign those tasks at all. Let me check this.

@anshulpundir so it seems like the dispatcher never checks desired state, only current state to decide to send down tasks: https://github.com/docker/swarmkit/blob/master/manager/dispatcher/dispatcher.go#L819

Does it make sense to check desired state here as well? Why should the dispatcher send a task to an agent if its desired state is past SHUTDOWN?

cc @stevvooe @cyli

From discussion IRL with @nishanttotla: It looks like the controller on the agent is responsible for looking at the desired state and acting on it (either by shutting down or not running at all). The dispatcher sends down all the tasks that are assigned to the agent, and any changes to those tasks. So if a task changes from DesiredState = RUNNING to DesiredState = SHUTDOWN it needs to send that update to the agent so that on the agent side the task gets shut down. If the dispatcher skips sending the update, the task will just keep running on the agent.

nishanttotla · 2018-04-04T21:49:37Z

Rebased on #2574 and added a test case. cc @cyli @anshulpundir @dperny

anshulpundir · 2018-04-04T23:37:51Z

+		Slot:         3,
+		DesiredState: api.TaskStateRemove,
+		Status: api.TaskStatus{
+			State: api.TaskStatePending,


Please also add a test for TaskStateNew

Will do. Regarding the comments below, I'll also rebase and add some more tests for the case of a running task reaper.

anshulpundir · 2018-04-04T23:38:21Z

LGTM after updating the tests for NEW state.

cyli

LGTM, but can we add a test for task reaper reaping REMOVEd tasks that are unassigned while the task reaper is running? (e.g. not on task reaper init).

anshulpundir · 2018-04-05T17:45:58Z

Nishant mentioned this yesterday. I think there are other tests in this file that don’t cover both the scenarios.

…

On Thu, Apr 5, 2018 at 10:37 AM Ying Li ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, but can we add a test for task reaper reaping REMOVEd tasks that are unassigned while the task reaper is running? (e.g. not on task reaper init). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2557 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AM7HYV0xuhowBeBbwsncYNNOCsroerRFks5tllY6gaJpZM4Sppa0> .

cyli · 2018-04-05T17:54:02Z

@anshulpundir I'm wondering if it'd be worth creating an exhaustive list of tasks in every combination of desired state + current state, and making sure the correct ones are reaped on start, and doing the same to create/update.

nishanttotla · 2018-04-05T18:09:08Z

@cyli exhaustive list can't hurt. It'll help systematize task reaper tests, and possibly catch bugs faster. I can take on that task in a separate PR.

…assigned Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

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 added the area/orchestration label Mar 14, 2018

anshulpundir reviewed Mar 14, 2018

View reviewed changes

nishanttotla mentioned this pull request Mar 28, 2018

Scheduler/TaskReaper: handle unassigned tasks marked for shutdown #2574

Merged

nishanttotla force-pushed the delete-tasks-that-havent-reached-agent branch 3 times, most recently from 09f9f94 to c61b8ed Compare April 4, 2018 21:10

nishanttotla force-pushed the delete-tasks-that-havent-reached-agent branch from c61b8ed to 1df0bf7 Compare April 4, 2018 21:48

anshulpundir reviewed Apr 4, 2018

View reviewed changes

anshulpundir requested a review from dperny April 4, 2018 23:38

cyli approved these changes Apr 5, 2018

View reviewed changes

Task reaper should delete tasks with removed slots that were not yet …

7f63825

…assigned Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the delete-tasks-that-havent-reached-agent branch from 1df0bf7 to 7f63825 Compare April 9, 2018 20:34

cyli merged commit ab856c7 into moby:master Apr 9, 2018

nishanttotla deleted the delete-tasks-that-havent-reached-agent branch April 9, 2018 22:21

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

stevvooe commented Mar 14, 2018

Uh oh!

anshulpundir 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishanttotla commented Mar 14, 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!

nishanttotla commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cyli left a comment

Choose a reason for hiding this comment

Uh oh!

anshulpundir commented Apr 5, 2018 via email

Uh oh!

cyli commented Apr 5, 2018

Uh oh!

nishanttotla commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nishanttotla commented Mar 14, 2018 •

edited

Loading

codecov Bot commented Mar 14, 2018 •

edited

Loading

anshulpundir left a comment •

edited

Loading

nishanttotla commented Mar 14, 2018 •

edited

Loading

anshulpundir commented Apr 4, 2018 •

edited

Loading