allocator: Less aggressive retry

[aaronlehmann]

Instead of retrying unallocated tasks, services, and networks every time data changes in the store, limit these retries to every 5 minutes.

When a repeated attempt to allocate one of these objects fails, log it at the debug log level, to reduce noise in the logs.

cc @alexmavr @yongtang @aboch

[codecov]

Codecov Report

Merging #2021 into master will increase coverage by 0.05%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2021      +/-   ##
==========================================
+ Coverage   53.66%   53.71%   +0.05%     
==========================================
  Files         109      109              
  Lines       18991    19008      +17     
==========================================
+ Hits        10191    10210      +19     
+ Misses       7578     7564      -14     
- Partials     1222     1234      +12

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4762bc...513d028. Read the comment docs.

[aboch]

Couldn't doTaskAlloc(ctx,ev) retrieve pendingTasks on its own via ctx.nc.pendingTasks ?

[aboch]

If working on the nc retrieved from the context is equivalent, would it make sense to write this method as

func (a *Allocator) procTasksNetwork(ctx context.Context, onRetryInterval bool) {
    nc := a.netCtx
    quiet := false
    toAllocate := nc.pendingTasks
    if onRetryInterval {
        toAllocate = nc.unallocatedTasks
        quiet = true
    }
...

[aboch]

Logic looks good to me.
I just have a couple of comments about the functions' prototype.

[aaronlehmann]

Updated, thanks

[aboch]

This line should go below the if block, after which we know what toAllocate points to

[aaronlehmann]

Updated

[aboch]

Looks good to me

[aluzzardi]

Do we want to handle the potentially impossible case in which we don't get a commit?

e.g. we receive a commit (and turns out that it free'ed up an IP address), we're above the 5 minutes limit so we don't try, and no other commit comes after so we don't allocate the task.

[aaronlehmann]

I think that's a very good point. I had considered this but didn't want to add too much complexity, especially because I think this should be backported. Do you think it's a good idea to add a timer that triggers after 5 minutes if no commits happen during that interval?

[aluzzardi]

I think it's such a rare case that we may not need to bother ...

I guess it depends if the fix would be extremely tiny?

Can this simply be another switch case with a time.After?

[aluzzardi]

Or a timer that we reset every time we receive a commit

[aluzzardi]

Or maybe we shouldn't bother :)

This is going to be so rare that the code to handle this case this may be buggy and we'll never notice

[aaronlehmann]

Yeah, let's not bother. I liked the suggestion of adding a time.After in the select, but then I realized this would start a timer every time we receive an event, and that timer wouldn't be reaped until it fired 5 minutes later. It could be a big waste of resources. The right way to do it would be to use an actual time.Timer and reset it whenever there's activity, but that's easier to mess up, and I think you're right that it's a very unusual case. Even without addressing that case, this PR is still making things a lot better without adding too much risk.

[aluzzardi]

LGTM