[manager/orchestrator/reaper] Clean out the task reaper dirty set at the end of tick()

[anshulpundir]

Addresses the following from from #2672:

task reaper dirty set is not cleaned up in various different scenarios at the end of tick() and causes high CPU as each additional task causes tick() to be called.

For testing, we should get coverage for cases where the tasks are not expected to be removed from the task reaper dirty set e.g. service already deleted, size of dirty set 1000 but per slot retention limit not reached etc.

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "reaper" git@github.com:anshulpundir/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354429024
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[codecov]

Codecov Report

Merging #2675 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2675      +/-   ##
==========================================
+ Coverage   62.06%   62.18%   +0.11%     
==========================================
  Files         134      134              
  Lines       21740    21742       +2     
==========================================
+ Hits        13494    13520      +26     
+ Misses       6794     6775      -19     
+ Partials     1452     1447       -5

[dani-docker]

@anshulpundir
My only concern is that we might end up leaking tasks that matches the condition here
https://github.com/docker/swarmkit/blob/master/manager/orchestrator/taskreaper/task_reaper.go#L257-L266

One would assume that EventUpdateTask gets fired when the task state changes, I am not sure if this is the case, but the below commit comment makes it sounds like it does not, hence why the author removed the reset map logic.
Correction:
EventUpdateTask ends up in cleanup not in dirty map, so we need to handle this use case
2d575ef#diff-55a0275b4dead2bb67850f19207c7e8f

If this is the case, I suggest to delete all tasks from the dirty list except the ones that apply to the above corner case/condition

[anshulpundir]

My only concern is that we might end up leaking tasks that matches the condition here
https://github.com/docker/swarmkit/blob/master/manager/orchestrator/taskreaper/task_reaper.go#L257-L266

Assuming that my 'leak' you mean 'never be able to clean up', I don't think that we'll leak those tasks. They will be cleaned up whenever a new task gets created for that slot.

Having said that, I also think its OK to keep track of those tasks and not remove them from the dirty set, but it leads to a slightly inelegant implementation.

@dani-docker

[talex5]

I think EventUpdateTask needs updating to mark tasks as dirty.

[talex5]

I don't see anything that does it. The EventUpdateTask handler doesn't do anything with completed tasks unless they are also flagged for immediate removal.

[anshulpundir]

Historical tasks are only cleaned up on create events. I was going for as little changes as possible. At the same time, I don't see significant benefits from changing behavior to clean up during EventUpdateTask for tasks that have stopped running.

[talex5]

Is nil even a valid value for tr.dirty? Won't this just make it segfault on the next EventCreateTask?

[dani-docker]

maybe change it to tr.dirty = make(map[orchestrator.SlotTuple]struct{})

[anshulpundir]

yep!

[talex5]

Why is that a problem? We don't have to do anything until one of them terminates, and that will be handled (at least, if the previous comment was correct).

[anshulpundir]

Because tasks are only cleaned up on create events.

[talex5]

We could remove runningTasks completely, if we're not going to use it for anything.

[anshulpundir]

Running tasks can't be removed until they stop running.

[dani-docker]

Assuming that my 'leak' you mean 'never be able to clean up', I don't think that we'll leak those tasks. They will be cleaned up whenever a new task gets created for that slot.

Right, the task will eventually be cleaned up when a create task event comes in; but unlike other tasks that skips this condition and gets processed, here we might end up (temporary) with a number of tasks (+1) larger then taskHistory.

Having said that, I also think its OK to keep track of those tasks and not remove them from the dirty set, but it leads to a slightly inelegant implementation.

If we might end up reporting the wrong number of task history (> configured taskHistory), I think it's worth to keep track of those tasks

[dperny]

the indentation level changes here... this statement now executes for every task in historicTasks. Is this intended behavior?

[anshulpundir]

oversight on my part. Fixed!

[dperny]

nit: CVall

[dperny]

i force-pushed a fix for the typo, i'm gonna merge as soon as CI verifies i didn't accidentally the code.