Implement claiming addresses from other peers

[bboreham]

Addresses #1150

[tomwilkie]

My only suggestion is that you extend TestAllocatorFuzz for this operation, and perhaps add a simple smoke test too.

[rade]

I am somewhat worried about this causing excessive fragmentation. Say we had a running cluster, nicely segmented three-ways in the IP space. With lots of containers. Then we bounce it and the space allocation is different. Since we issue claims one-by-one, won't we end up with a ring containing as many tokes as there are containers?

I'm not suggesting we should necessarily attempt to fix this here, but might be worth leaving a comment in the code.

[bboreham]

Yes, I think your analysis is correct. With a more sophisticated request-management system, we could coalesce all contiguous requests into one, thus mostly removing the fragmentation.

There is another potential pitfall with the simple way requests are managed at present: if we do not hear back from the current owner we will hang the caller. And it's quite easy to get into a state where the current owner is actually shut down.

[rade]

We could grant larger ranges than requested. e.g. when receiving a request we grant a range s.t. we minimise the creation of tokens. There are downsides to that too obviously.

[tomwilkie]

Instead of hanging forever, could we detect when the current owner has gone
(from peers.go) and tell the user? Is that worth doing?

On Thu, Jul 16, 2015 at 10:45 AM, Bryan Boreham [email protected]
wrote:

Yes, I think your analysis is correct. With a more sophisticated
request-management system, we could coalesce all contiguous requests into
one, thus mostly removing the fragmentation.

There is another potential pitfall with the simple way requests are
managed at present: if we do not hear back from the current owner we will
hang the caller. And it's quite easy to get into a state where the current
owner is actually shut down.

—
Reply to this email directly or view it on GitHub
#1159 (comment).

[rade]

Instead of hanging forever, could we detect when the current owner has gone (from peers.go) and tell the user? Is that worth doing?

They could come back. All of IPAM makes that assumption, so it would be strange to do something different here.

[bboreham]

claim is a bit different - sometimes it sends back a nil answer meaning "I don't know yet but I'll try later" - so @tomwilkie's suggestion seems fair. More precisely, if the 'send-unicast' errors, then we would return to the caller while leaving the claim pending.

[bboreham]

Note that peers, on receipt of a request for space, typically donate half of it. This is rounded up, so a single IP works, but if we coalesced space then we'd need further work to make that behave as required. I think this would still be backwards-compatible - if you request 8 addresses from a back-version peer then you will get 4, so request another 4, get 2, and eventually satisfy the need.

Edit: just noticed you said grant larger ranges than requested, which makes much of the above irrelevant.

[bboreham]

Added fuzz test, smoke test and non-hanging. Granting larger ranges turned out to have subtleties so left for another day.

[tomwilkie]

Gee, thanks!

[rade]

Gee, thanks!

Really want your expert eyes on this.

[bboreham]

Just rebased against master and push-f'd

[tomwilkie]

I've given it a read through and can't spot anything obvious. Do you normally squash this kinda thing?

I'll think on corner cases overnight, unless there is a rush.

[rade]

Do you normally squash this kinda thing?

Can't see anything obviously squash-worthy.

[rade]

We are not seeing any coverage of Allocator.spaceRequestDenied. Is that expected?

[rade]

We are not seeing any coverage of Allocator.spaceRequestDenied. Is that expected?

False alarm. Problem turns out to be #1220.