Make IP allocator lazy #272

dominikh · 2013-03-30T22:38:41Z

Instead of allocating all possible IPs in advance, generate them as
needed. Released IPs will not be fed back to the channel but stored in
a linked list. Said linked list will be used once the initial
generator exhausted all possible IPs.

In the worst case (VMs using up the entire /8 space, then releasing
the entire /8 space), memory usage will be slightly higher than
before (overhead of the list). In the usual case (an equal amount of
VMs acquiring and releasing IPs), memory usage will be relatively
constant. In both cases, IPs that have never been acquired will not
occupy memory.

Due to required API changes (the IP allocator runs in a goroutine now
and thus cannot easily return errors), the functions that are
responsible for IP/Mask<->int conversions have been rewritten to be
error-free.

Fixes gh-231

titanous · 2013-03-30T23:03:56Z

network.go

Could be compacted to:

if ip, ok := <-alloc.queue; ok {

titanous · 2013-03-30T23:05:30Z

Does it make sense to prioritize reusing addresses over allocating new ones?

dominikh · 2013-03-30T23:07:01Z

Does it make sense to prioritize reusing addresses over allocating new ones?

Absolutely, yes.

shykes · 2013-03-30T23:42:29Z

Actually, no. It is better to allocate new addressed before using new ones.
This drastically decreases the frequency at which an IP is reused. In
theory this should not matter, but in a large distributed system it greatly
reduces the probability and gravity of sending traffic to the wrong
container.

This friendly tip brought to you courtesy of the dotCloud ops team /cc
@samalba @jpetazzo @kencochrane @chooper @shin-

On Sat, Mar 30, 2013 at 4:07 PM, Dominik Honnef [email protected]:

Does it make sense to prioritize reusing addresses over allocating new
ones?

Absolutely, yes.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15683545
.

titanous · 2013-03-30T23:45:19Z

@shykes Cool, I wasn't sure if that was the case. 📝

dominikh · 2013-03-30T23:46:43Z

If you allocate new addresses, however, you will slowly fill up the list of released IPs. In the case of an /8 network (see original issue), you will face continuously growing memory consumption for every VM you ran.

I do acknowledge the problem of reusing IPs, but not reusing them at all but yet storing them (which this PR does), is not acceptable either.

Options I can think of (not ops-approved, however) are starting to use old IPs once the list reaches a certain size, using them at random, or implementing a compact way of storing reusable IPs.

shykes · 2013-03-30T23:53:47Z

Here's what I have in mind:

Generator goes over address space mathematically (no storage consumption)
When generator reaches end of available space, it wraps back to beginning
All allocated IPs are stored in a map. Generator checks the map before
allocating. If the IP is in the map, the generator skips it

This way you only store as many IPs as you have allocated, regardless of
total available address space.

And we have to implement this map anyway for the "real port" feature.

On Sat, Mar 30, 2013 at 4:46 PM, Dominik Honnef [email protected]:

If you allocate new addresses, however, you will slowly fill up the list
of released IPs. In the case of an /8 network (see original issue), you
will face continuously growing memory consumption for every VM you ran.

I do acknowledge the problem of reusing IPs, but not reusing them at all
but yet storing them (which this PR does), is not acceptable either.

Options I can think of (not ops-approved, however) are starting to use old
IPs once the list reaches a certain size, using them at random, or
implementing a compact way of storing reusable IPs.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15683970
.

dominikh · 2013-03-30T23:59:11Z

That sounds reasonable.

Since you're referring to a "real port feature" that I'm not aware of I'll hold off implementing this since I don't know what kind of information you want in the map, or where the map is supposed to be managed.

shykes · 2013-03-31T00:05:18Z

See issue #273 for details. I'm thinking a simple map[int]struct{} would be
sufficient - we would use it as a set of currently allocated ports.

On Sat, Mar 30, 2013 at 4:59 PM, Dominik Honnef [email protected]:

That sounds reasonable.

Since you're referring to a "real port feature" that I'm not aware of I'll
hold off implementing this since I don't know what kind of information you
want in the map, or where the map is supposed to be managed.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15684110
.

shykes · 2013-03-31T00:06:20Z

The same goroutine which allocates IPs should probably also own access to
the map to avoid conflicts. Probably via 2 channels: one to allocate IPs,
one to release them, and a select on the 2.

On Sat, Mar 30, 2013 at 5:04 PM, Solomon Hykes
[email protected]:

See issue #273 for details. I'm thinking a simple map[int]struct{} would
be sufficient - we would use it as a set of currently allocated ports.

On Sat, Mar 30, 2013 at 4:59 PM, Dominik Honnef [email protected]:

That sounds reasonable.

Since you're referring to a "real port feature" that I'm not aware of
I'll hold off implementing this since I don't know what kind of information
you want in the map, or where the map is supposed to be managed.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15684110
.

dominikh · 2013-03-31T01:48:24Z

This should work as you intended.

titanous · 2013-03-31T01:51:19Z

network.go

I don't think this field should be exported. It will likely need a mutex if it's meant to be accessed by other goroutines.

titanous · 2013-03-31T01:58:55Z

There should be some tests that cover the allocation strategy.

dominikh · 2013-03-31T11:32:21Z

Done. I'm still not entirely sure about #273, but I'm sure someone else can make changes to the IP allocator as needed, based on this.

shykes · 2013-03-31T18:54:36Z

Thanks @dominikh. Reviewing.

dominikh · 2013-03-31T19:08:47Z

Fwiw, you might want to consider renaming the populate method. I don't think it's quite fitting anymore, considering what it's doing now.

titanous · 2013-03-31T19:10:03Z

@dominikh Please squash your commits before @shykes merges.

dominikh · 2013-03-31T19:12:50Z

@titanous Will do once he approves of the changes.

I suppose I'll just force push the feature branch and GitHub will do the right thing?

titanous · 2013-03-31T19:13:20Z

@dominikh Yep, the pull request will update when you force-push.

shykes · 2013-03-31T19:14:12Z

I agree, populate is no longer adequate. I guess Allocate() would work
better?

On Sun, Mar 31, 2013 at 12:08 PM, Dominik Honnef
[email protected]:

Fwiw, you might want to consider renaming the populate method. I don't
think it's quite fitting anymore, considering what it's doing now.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15696221
.

shykes · 2013-03-31T19:15:40Z

@dominikh sorry I need to go afk for a few more hours, but so far this looks like really good work. Thanks again.

dominikh · 2013-03-31T19:15:44Z

I was thinking of something like Start() maybe, but then there'd be the lack of a Stop(). Possibly Run()

dominikh · 2013-03-31T20:19:52Z

Or rather unexported versions of those. newIPAllocator should probably still be the only one calling it.

shykes · 2013-03-31T20:23:14Z

Agreed

On Sunday, March 31, 2013, Dominik Honnef wrote:

Or rather unexported versions of those. newIPAllocator should probably
still be the only one calling it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/272#issuecomment-15697293
.

shykes · 2013-04-01T02:30:17Z

Looks good! Let me know when you're ready for me to merge.

Instead of allocating all possible IPs in advance, generate them as needed. A loop will cycle through all possible IPs in sequential order, allocating them as needed and marking them as in use. Once the loop exhausts all IPs, it will wrap back to the beginning. IPs that are already in use will be skipped. When an IP is released, it will be cleared and be available for allocation again. Two decisions went into this design: 1) Minimize memory footprint by only allocating IPs that are actually in use 2) Minimize reuse of released IP addresses to avoid sending traffic to the wrong containers As a side effect, the functions for IP/Mask<->int conversion have been rewritten to never be able to fail in order to reduce the amount of error returns. Fixes gh-231

Make IP allocator lazy

Add support for --network=netns:/proc/pid/ns/net

…twork [19.03 backport] Re-align proxy commit with libnetwork vendor

…aster-9c83848fc95a200c0c69a34cdeeb1f18a090c13a [master] sync to upstream master 9c83848

titanous reviewed Mar 30, 2013
View reviewed changes

network.go Outdated

Copy link

Contributor

titanous Mar 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be compacted to:

if ip, ok := <-alloc.queue; ok {

titanous reviewed Mar 31, 2013
View reviewed changes

shykes pushed a commit that referenced this pull request Apr 1, 2013

Merge pull request #272 from dominikh/improve/lazy-ip-allocator

29b7ecb

Make IP allocator lazy

shykes merged commit 29b7ecb into moby:master Apr 1, 2013

jpetazzo mentioned this pull request Apr 5, 2013

Choose a container's public port, if available #273

Closed

rhvgoyal pushed a commit to rhvgoyal/moby that referenced this pull request Sep 19, 2017

Merge pull request moby#272 from runcom/netns-1.13

790e958

Add support for --network=netns:/proc/pid/ns/net

tiborvass pushed a commit to tiborvass/docker that referenced this pull request Jun 12, 2019

Merge pull request moby#272 from thaJeztah/19.03_backport_align_libne…

a6905fa

…twork [19.03 backport] Re-align proxy commit with libnetwork vendor

dperny pushed a commit to dperny/docker that referenced this pull request Oct 18, 2021

Merge pull request moby#272 from GordonTheTurtle/sync-with-upstream-m…

2c28b9e

…aster-9c83848fc95a200c0c69a34cdeeb1f18a090c13a [master] sync to upstream master 9c83848

Make IP allocator lazy #272

Make IP allocator lazy #272

Uh oh!

Conversation

dominikh commented Mar 30, 2013

Uh oh!

titanous Mar 30, 2013

Choose a reason for hiding this comment

Uh oh!

titanous commented Mar 30, 2013

Uh oh!

dominikh commented Mar 30, 2013

Uh oh!

shykes commented Mar 30, 2013

Uh oh!

titanous commented Mar 30, 2013

Uh oh!

dominikh commented Mar 30, 2013

Uh oh!

shykes commented Mar 30, 2013

Uh oh!

dominikh commented Mar 30, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

titanous Mar 31, 2013

Choose a reason for hiding this comment

Uh oh!

titanous commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

titanous commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

titanous commented Mar 31, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

dominikh commented Mar 31, 2013

Uh oh!

shykes commented Mar 31, 2013

Uh oh!

shykes commented Apr 1, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants