Skip to content

Issue #18: IP Allocator rework - #26

Merged
mavenugo merged 1 commit into
moby:masterfrom
aboch:ipam
Apr 9, 2015
Merged

Issue #18: IP Allocator rework#26
mavenugo merged 1 commit into
moby:masterfrom
aboch:ipam

Conversation

@aboch

@aboch aboch commented Mar 10, 2015

Copy link
Copy Markdown
Contributor
  • Ported network utility functions and their tests in libnetwork:
    docker/daemon/networkdriver/utilg.go => libnetwork/utils.go
    docker/daemon/networkdriver/network_test.go => libnetwork/utils_test.go
  • Moved well-known error messages from network/driver/network.go to
    libnetwork/network.go
  • Decouple IPAM API from its implementation:
    Added IPAM contract in libnetwork, libnetwork/ipam_contract.go.
    It defines the API the IPAM plugin needs to implement. And expose
    them to libnetwork clients, proxying the API calls to the respective
    implementation in the IPAM plugin.
  • Moved ip_allocator and test code to libnetwork, just added code for
    registering with libnetwork, as a first step to create a separate
    plugin without breaking existing libnetwork functionality

Closes #18.

Signed-off-by: Alessandro Boch aboch@socketplane.io

@LK4D4

LK4D4 commented Mar 10, 2015

Copy link
Copy Markdown
Contributor

Nice! But golint failed.
Also can we do this in less global way without register-deregister stuff, like package ipam with interface and implementations?

@aboch

aboch commented Mar 10, 2015

Copy link
Copy Markdown
Contributor Author

Yeah, I just resubmitted after taking care of golint comments. I really love golint...
There is a discussion in having the IPAM service being swappable as well the same way the network plugin is (bridge driver for ex.).
I am not sure I get the concern about the registering logic. I think it is very flexible and would allow with minimal changes to support multiple IPAM services and select the desired one when launching the system.

@aboch

aboch commented Mar 10, 2015

Copy link
Copy Markdown
Contributor Author

To rectify my last comment, regarding the multiple IPAM plugins, that was just an example about the flexibility of the register. Not that there is actually an idea of supporting them at the moment.

@LK4D4

LK4D4 commented Mar 10, 2015

Copy link
Copy Markdown
Contributor

@aboch I think that register is not flexible at all :( I think that having one allocator struct per network driver is much more flexible. Also probably have two allocators in system not bad too.
Having global state in library is sorta bad behavior. Sometime you can't avoid that, but libcontainer is not that case.

@aboch

aboch commented Mar 10, 2015

Copy link
Copy Markdown
Contributor Author

During the design discussions happened so far, we converged toward the decision of having the IPAM and network services be provided by two separate entities. Though they will be tied to a common API contract specified by libnetwork.
I understand your concerns on the states in the plugin. I would say those are architectural concerns (and on those I will let @mrjana articulate) and are not related to using or not a registering logic.

If we take this separate IPAM plugin requirement, I see the registering logic a natural implementation.
I am still not clear why the registering is not flexible, please let me know what I am missing, I am very open to changes.

@LK4D4

LK4D4 commented Mar 10, 2015

Copy link
Copy Markdown
Contributor

Having IPAM and network driver as separate structures is easy too I think. We already have global driver and allocator in docker and this is not very exciting.
Just such design is not go-way and inconvenient to use. Golang is about datastructures, and package(such big package as libnetwork in particular) is bad example of datastructure.
Sorry for being so annoying, I just like go way so much and I like that you can read code easily. Global states break readability at all and I felt it on my own skin with current networkdriver in docker.

@icecrime

Copy link
Copy Markdown

Cool! Will review.

@icecrime

Copy link
Copy Markdown

@LK4D4 Were your concerns addressed?

@LK4D4

LK4D4 commented Mar 11, 2015

Copy link
Copy Markdown
Contributor

@icecrime Nope :) But I don't think that my concerns mean something in your kingdom.

@aboch

aboch commented Mar 11, 2015

Copy link
Copy Markdown
Contributor Author

@LK4D4 In the latest push I moved the allocatedIPs map and lock as members of allocator struct. That was a mistake they were global, missed that.

@icecrime

Copy link
Copy Markdown

@LK4D4 Of course your concerns matter :)

Comment thread ipam/ipam_proxy.go Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd rather have the bridge hold the IPAM instance rather than a global variable. Any particular reason to do it this way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the same reason why network driver registers with libnetwork: IPAM is on his way to become a separate pluggable service, same for network bridge, they will be on the same level.
There will be a network daemon (which will integrate libnetwork) in charge of orchestrating the network related configurations/functioning for the registered services.
The communication between the driver and ipam plugins will probably not be achieved via the daemon/libnetwork, but directly. Still, the network driver needs to get from the network daemon
the information on how to reach the ipam plugin and the IPAM interface API so that it knows how to talk to IPAM service.
Also, when somebody pushes the ipam database configuration, the net daemon has to have an handle of IPAM service, the same way it holds an handle to the network driver.

As such, when the daemon code will come in, I am expecting that both libnetwork global variables (drivers in libnetwork/driver.go and ipam in libnetwork/ipam_proxy.go) will move out of libnetwork into the daemon code.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we change it when we get there? Today there is no reason to make it a global variable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I will stick to the changes needed to address the reported Issue. Without overloading the diffs with unrelated changes.

- Move ipallocator package into libnetwork
- Also ported network utility functions and their tests in libnetwork:
  docker/daemon/networkdriver/utilg.go => libnetwork/utils.go
  docker/daemon/networkdriver/network_test.go => libnetwork/utils_test.go
- Changed drivers/setup_device.go and setup_ipv4.go to reuse functions in
  utils.go, instead of redefining internally.
- Modified utils to use vishvananda/netlink instead of libcontainer/netlink

Signed-off-by: Alessandro Boch <aboch@socketplane.io>
@dave-tucker

Copy link
Copy Markdown
Contributor

LGTM, nice work @aboch

@mavenugo

mavenugo commented Apr 9, 2015

Copy link
Copy Markdown
Contributor

LGTM.

mavenugo added a commit that referenced this pull request Apr 9, 2015
Issue #18: IP Allocator rework
@mavenugo
mavenugo merged commit 672ced7 into moby:master Apr 9, 2015
@aboch
aboch deleted the ipam branch April 9, 2015 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Break dependency on ipallocator

5 participants