allocator: Fix events watch leak

[aaronlehmann]

The allocator starts a watch on events and never shuts it down. If this node loses leadership, events will continue to pile up in that queue forever, leading to memory exhaustion.

Fix this by giving the watch a well-defined lifecycle. Return it along with a cancel function from an init() method to make it clear it must be cancelled, instead of sticking the cancel function value in a struct where there is no clear responsibilty for calling it.

In the future, we might consider changing Watch to run a callback function, so shutting down the watch is mandatory (similar to what we do with store transactions).

cc @briantd @jakegdocker

[codecov]

Codecov Report

Merging #2215 into master will increase coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #2215      +/-   ##
==========================================
+ Coverage   60.03%   60.05%   +0.01%     
==========================================
  Files         124      124              
  Lines       20115    20122       +7     
==========================================
+ Hits        12077    12085       +8     
+ Misses       6680     6674       -6     
- Partials     1358     1363       +5

[rogaha]

👍 great catch @briantd! Thanks for working on a patch for it @aaronlehmann!

[ijc]

LGTM.

[cyli]

LGTM, with the caveat that I don't know the allocator code very well :) So please excuse the dumb question: this looks like it's creating a watch per allocActor? Granted there is only ever one right now, so it doesn't matter at the moment - but I was sure why there's a for loop - is the intention that there will be more in the future? If so in the future would we have to condense the watches again so that there is only one no matter how many allocActors there are?

[aaronlehmann]

It didn't make sense to me that the watch used to be shared between all actors. That wouldn't have worked once we had multiple actors, because they would have competed for events, so I moved the watch call to be per-actor.

[cyli]

Ah ok, so it's not like a pool of workers who grabs the first event, it's more of a fanout to all actors?

[aaronlehmann]

Correct. Other actors would be handling things other than network allocation. It's a bit of a weird design and we would probably change it anyway if we introduced other kinds of allocators.

[cyli]

Ok, that makes sense, thanks for explaining! 👍

[dongluochen]

LGTM

[ijc]

It didn't make sense to me that the watch used to be shared between all actors.

This was something I tripped over in one of my PoC attempts to refactor some of this stuff for CNI networking on top of #1965, I can confirm that it does not work well at all...