Implement allocating IPs from CIDR within bridge network #6101

LK4D4 · 2014-05-29T07:27:27Z

It is similar to openstack nova --fixed-cidr and allows to do things like this. And our network engineer very excited about this change :)
Also I've refactored ipallocator a little more.

LK4D4 · 2014-05-29T07:28:00Z

ping @shykes @unclejack @vieux @crosbymichael

LK4D4 · 2014-06-02T19:59:11Z

reping @unclejack @vieux @crosbymichael

GermanDZ · 2014-06-05T06:25:05Z

+1

subhraveti · 2014-06-08T23:15:47Z

+1 This is fantastic and nicely fits the Hadoop usecase. How hard would it be to extend this to specify the CIDR per bridge rather than globally (assuming #6155 is implemented)?

LK4D4 · 2014-06-09T04:15:54Z

@subhraveti ~ 4 hours with tests for "per-bridge" I think, it is mostly IP allocator feature.

LK4D4 · 2014-06-11T11:44:18Z

rebased
ping @unclejack @vieux @crosbymichael

tianon · 2014-06-19T18:11:29Z

I think this seems sane, since we're controlling IP address assignment, providing more control over how they get assigned is generally good, although the UI could end up getting kind of cumbersome to specify via command-line flags, but that's inevitable and a much larger discussion than what's proposed here. :)

LK4D4 · 2014-07-01T16:02:04Z

Also, benchmarks result:

benchmark              old ns/op     new ns/op     delta       
BenchmarkRequestIP     434104        275999        -36.42%     

benchmark              old allocs     new allocs     delta       
BenchmarkRequestIP     1278           771            -39.67%     

benchmark              old bytes     new bytes     delta       
BenchmarkRequestIP     31426         18648         -40.66%

fkautz · 2014-07-08T00:19:03Z

@LK4D4 Looks like there are some conflicts. Can you rebase this?

LK4D4 · 2014-07-08T04:16:26Z

@fkautz Rebased :) I hope that this rebase will bring PR a lucky charm for review.

fkautz · 2014-07-08T07:42:29Z

Assigning this to myself so that I can review it.

LK4D4 · 2014-07-08T07:49:09Z

@fkautz Cool, thank you!

fkautz · 2014-07-09T03:26:24Z

We will need some input from @shykes to make sure the changes are acceptable, since they change the UI. I've asked @tianon to add it to Thursday's meeting, contingent on @shykes being online.

Assuming he agrees with the UI approach, we'll also need additional documentation in docs. I have a few use cases to test out as well, I'll get back to you on how those go.

LK4D4 · 2014-07-09T04:21:24Z

It will be sixth meeting for this PR. I glad to see, that someone interested in this apart from me :)

shykes · 2014-07-10T23:32:31Z

docs/sources/reference/commandline/cli.md

@@ -54,6 +54,7 @@ expect an integer, and they can only be specified once.
      -b, --bridge=""                            Attach containers to a pre-existing network bridge
                                                   use 'none' to disable container networking
      --bip=""                                   Use this CIDR notation address for the network bridge's IP, not compatible with -b
+      --fixed-cidr=""                            IPv4 subnet for fixed IPs (ex: 10.20.0.0/16)


I think --ip= is more natural. The value can still use the CIDR notation of course.

@LK4D4 what about this comment ?

I would change the description to be more explicit like IPv4 subnet range for containers on thedocker0bridge

also - can you please add some documentation about how this new option interacts with or relates to the --bip option

We talked with @shykes on last meeting and he was okay with --fixed-cidr. This is more common in network engineers world.

@SvenDowideit This is not necessary docker0, it can be bridge specified by -b.

kewl :) if you can add this info into the docs, we're in a better place :)

shykes · 2014-07-10T23:43:21Z

+1 on the design, as long as it doesn't break links. Just a superficial note on the syntax (--ip feels simpler and more natural).

Sorry for being slow.

LK4D4 · 2014-07-11T04:22:50Z

@shykes I don't think that ip is more natural for this feature :) I expect that --ip sets one fixed IP within bridge range and possibly this will be useful feature. But here we specify not ip, but exactly the cidr.
--fixed-cidr notation already very popular in openstack nova.

fkautz · 2014-07-11T05:45:11Z

I agree, fixed CIDR is more descriptive. An IP should also be assignable
with /32.
On Jul 10, 2014 9:23 PM, "Alexandr Morozov" notifications@github.com
wrote:

@shykes https://github.com/shykes I don't think that ip is more natural
for this feature :) I expect that --ip sets one fixed IP within bridge
range and possibly this will be useful feature. But here we specify not ip,
but exactly the cidr.
--fixed-cidr notation already very popular in openstack nova.

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

LK4D4 · 2014-07-16T05:06:35Z

ping @shykes
What are you think about --fixed-cidr now?

fkautz · 2014-07-17T16:51:44Z

--fixed-cidr only provides half of the story. I think --fixed-cidr should not set the subnet, only the ip range allocaiton. If we are following openstack's approach, we should use --fixed-range to set the subnet separately from the allocation range. These two combined allow for a great degree of flexibility.

[edit] --fixed-cidr works as i was hoping, no change is necessary.

tianon · 2014-07-17T20:02:38Z

Just to keep the conversation together:

https://botbot.me/freenode/docker-dev/msg/18199269/

<shykes> I'm ok to merge 6101

fkautz · 2014-07-17T23:26:10Z

Had a talk with @LK4D4. The subnet isn't touched, so we should be good to go. Just need to add some documentation.

jessfraz · 2014-09-15T16:31:54Z

LGTM

jessfraz · 2014-09-15T17:21:28Z

I think just needs docs review from @SvenDowideit

SvenDowideit · 2014-09-16T00:42:01Z

docs/sources/articles/networking.md


 *  `--bip=CIDR` — supply a specific IP address and netmask for the
    `docker0` bridge, using standard CIDR notation like
    `192.168.1.5/24`.

+ *  `--fixed-cidr=CIDR` — restrict the IP range from the `docker0` subnet,
+    using the standard CIDR notation like `172.167.1.0/28`. This range must
+    be nested in bridge subnet(docker0 or setted by `--bridge`). For example


space before (

put docker0 into docker0 code quotes

s/setted by/set using/

SvenDowideit · 2014-09-16T00:45:07Z

Docs LGTM - @jamtur01 @fredlf @ostezer

While I like having examples, giving the users an idea of situations when they might use an option, I can't really think of one that is interesting - its the kind of thing an admin will use because they know they need it.

LK4D4 · 2014-09-16T06:47:32Z

Okay, now we have bunch of lgtms here :) We need the one who brave enough to merge it or close it forever. ping @crosbymichael

ghost · 2014-09-16T09:34:22Z

docs/sources/articles/networking.md


 *  `--bip=CIDR` — supply a specific IP address and netmask for the
    `docker0` bridge, using standard CIDR notation like
    `192.168.1.5/24`.

+ *  `--fixed-cidr=CIDR` — restrict the IP range from the `docker0` subnet,
+    using the standard CIDR notation like `172.167.1.0/28`. This range must
+    be nested in bridge subnet (`docker0` or set using `--bridge`). For example


@LK4D4 Docs mostly LGTM. However, I find "[..] nested in bridge subnet" to be bit confusing.
Do you think there can be another way to put it so it becomes simpler and easier to understand?

hmmm, "must be subset of bridge IP range"? I'm not very good at English still, so almost any english sentence is equally cryptic for me :)

"must be subset of bridge IP range" sounds fine.

May I humbly suggest: "IPv4 range for fixed IPs (ex: 10.20.0.0/16); this range must be a subset of the bridge IP range"

@LK4D4 use @md5 's text - its good, and we can re-write it later if needed

crosbymichael · 2014-09-17T00:54:41Z

I don't have time to test right now but if someone else reviews and tests I'm good with this

jessfraz · 2014-09-17T01:00:23Z

maybe @tiborvass or @unclejack tomorrow?

LK4D4 · 2014-09-17T07:39:34Z

Fixed article text.

unclejack · 2014-09-17T08:55:27Z

LGTM

The docs need to be approved as well. Please don't merge until @fredlf @jamtur01 and @ostezer have approved the docs and the help text.

LK4D4 · 2014-09-17T20:36:40Z

pint @fredlf @ostezer
Guys, review docs pls.

fredlf · 2014-09-18T00:23:38Z

docs/sources/articles/networking.md

-supports its default route.  Both are configurable at server startup:
+Docker configures `docker0` with an IP address, netmask and IP
+allocation range. The host machine can both receive and send packets to
+containers connected to the bridge, and gives it an MTU — the *maximum


It's not clear to me what the pronoun "it" is referring to. The bridge? docker0? Might help to break it into two sentences and get rid of the pronoun.

I'm actually don't follow. docker0 and "it" are in different sentences and docker0 and bridge are synonyms anyway. Could you provide exact text for me?

Docker-DCO-1.1-Signed-off-by: Frederick F. Kautz IV <fkautz@alumni.cmu.edu> (github: fkautz) Signed-off-by: Alexandr Morozov <lk4d4@docker.com>

LK4D4 · 2014-09-18T04:50:16Z

@fredlf Thanks for review. Could you help me with that big sentence about MTU? I'm not very experienced English language user :/

SvenDowideit · 2014-09-19T00:29:47Z

how about something like

The host machine can both receive and send packets to
containers connected to the bridge.
 The *maximum transmission unit* (MTU) or largest packet length that the interface will
allow will be copied from the Docker host's default route interface.

I've ignored the 1500 bytes - do we need it?

fredlf · 2014-09-19T00:42:11Z

LGTM. We'll fix the docs later.

LK4D4 · 2014-09-19T07:40:07Z

ping @unclejack
can we merge this now?

DwayneBull · 2014-09-22T09:04:59Z

Will this fix docker changing the container ip on container or deamon restart?

LK4D4 · 2014-09-22T09:08:50Z

@DwayneBull Nope

DwayneBull · 2014-09-22T09:43:47Z

@LK4D4 I assume you could get round it with something like --fixed-cidr="172.168.0.1/32"

LK4D4 · 2014-09-22T11:37:45Z

@DwayneBull Problem that this is daemon-wide option. There is another PR for per-container IP control.

unclejack · 2014-09-22T11:50:02Z

@jamtur01 Is it OK if we merge this PR with the current docs?

jamtur01 · 2014-09-22T14:07:46Z

LGTM

Implement allocating IPs from CIDR within bridge network

LK4D4 added the /project/doc label May 29, 2014

LK4D4 mentioned this pull request Jun 20, 2014

Docker gives out IP addresses already in use #6561

Closed

erikh added Distribution labels Jun 26, 2014

LK4D4 mentioned this pull request Jun 27, 2014

Let users specify custom network bridge for individual containers #6155

Closed

phemmer mentioned this pull request Jul 3, 2014

Extend run config to let users specify an IP #5829

Closed

fkautz added the /system/networking label Jul 8, 2014

fkautz self-assigned this Jul 8, 2014

shykes reviewed Jul 10, 2014
View reviewed changes

SvenDowideit reviewed Sep 16, 2014
View reviewed changes

ghost reviewed Sep 16, 2014
View reviewed changes

fredlf reviewed Sep 18, 2014
View reviewed changes

Adding additional documentation for fixed-cidr networking

3bb12d3

Docker-DCO-1.1-Signed-off-by: Frederick F. Kautz IV <fkautz@alumni.cmu.edu> (github: fkautz) Signed-off-by: Alexandr Morozov <lk4d4@docker.com>

fredlf added the /project/docs/needs edit label Sep 19, 2014

unclejack added a commit that referenced this pull request Sep 22, 2014

Merge pull request #6101 from LK4D4/ip_range_#4986

9fb34ae

Implement allocating IPs from CIDR within bridge network

unclejack merged commit 9fb34ae into moby:master Sep 22, 2014

Implement allocating IPs from CIDR within bridge network #6101

Implement allocating IPs from CIDR within bridge network #6101

Conversation

LK4D4 commented May 29, 2014

LK4D4 commented May 29, 2014

LK4D4 commented Jun 2, 2014

GermanDZ commented Jun 5, 2014

subhraveti commented Jun 8, 2014

LK4D4 commented Jun 9, 2014

LK4D4 commented Jun 11, 2014

tianon commented Jun 19, 2014

LK4D4 commented Jul 1, 2014

fkautz commented Jul 8, 2014

LK4D4 commented Jul 8, 2014

fkautz commented Jul 8, 2014

LK4D4 commented Jul 8, 2014

fkautz commented Jul 9, 2014

LK4D4 commented Jul 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shykes commented Jul 10, 2014

LK4D4 commented Jul 11, 2014

fkautz commented Jul 11, 2014

LK4D4 commented Jul 16, 2014

fkautz commented Jul 17, 2014

tianon commented Jul 17, 2014

fkautz commented Jul 17, 2014

jessfraz commented Sep 15, 2014

jessfraz commented Sep 15, 2014

Choose a reason for hiding this comment

SvenDowideit commented Sep 16, 2014

LK4D4 commented Sep 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Sep 17, 2014

jessfraz commented Sep 17, 2014

LK4D4 commented Sep 17, 2014

unclejack commented Sep 17, 2014

LK4D4 commented Sep 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented Sep 18, 2014

SvenDowideit commented Sep 19, 2014

fredlf commented Sep 19, 2014

LK4D4 commented Sep 19, 2014

DwayneBull commented Sep 22, 2014

LK4D4 commented Sep 22, 2014

DwayneBull commented Sep 22, 2014

LK4D4 commented Sep 22, 2014

unclejack commented Sep 22, 2014

jamtur01 commented Sep 22, 2014