IPAM driver #525
IPAM driver #525
Conversation
| config ipamapi.Config | ||
| allocator ipamapi.Allocator | ||
| // default address spaces are provided by ipam driver at registration time | ||
| defaultLocalAddressSpace, defaultGlobalAddressSpace string |
There was a problem hiding this comment.
minor:
May be we should just have single defaultAddressSpace field, and based on driver's scope we can initialize it at time of registration? This is assuming that IPAM driver operates in one mode at a time.
There was a problem hiding this comment.
Let me give you some context (sorry if redundant).
- The default address space is needed in case UI does not passes it (which will be always the case for first releases). We want the ipam driver to let us know which is the default addr space string.
- We need two default address spaces, one for local scoped networks (like for ex. bridge) and one to be used for global scoped networks (like for ex. overlay). Because on a docker host both kind of networks can co-exists.
There was a problem hiding this comment.
The default address space is needed in case UI does not passes it (which will be always the case for first releases). We want the ipam driver to let us know which is the default addr space string.
got it, that was my understanding too.
We need two default address spaces, one for local scoped networks (like for ex. bridge) and one to be used for global scoped networks (like for ex. overlay). Because on a docker host both kind of networks can co-exists.
I agree that libnetwork need to support both kind of networks, but does libnetwork expects the same IPAM driver to always work for local as well as global scoped networks? I ask as I don't think my driver would be able to serve both local and global scoped networks and it seems better to error out early that routing the request to driver. Would it be better to negotiate this as driver capability instead (like for NetworkDriver) and error out early if user tries to do this?
There was a problem hiding this comment.
@mapuri
Sorry for the late response:
It is not a requirement that a remote ipam driver must serve both local and global scope. As long as it is used in conjunction with the appropriate network driver.
It is instead a requirement for the Default IPAM to serve both local and global scoped net drivers simply because being the default builtin ipam driver, it has to serve both the inbuilt bridge and overlay drivers.
|
Don't forget to update the remote.md file with the changes to the remote driver API. |
|
@tomdee Thanks missed that. Will update it soon. |
| @@ -282,6 +328,17 @@ func (c *controller) addNetwork(n *network) error { | |||
| } | |||
| } | |||
|
|
|||
| c.Lock() | |||
| // Load the ipam driver if not done already | |||
| _, ok = c.ipams[n.ipamType] | |||
There was a problem hiding this comment.
Can we wrap the whole looking up an ipam driver in controller in a n.ipam() which will try to lookup ipam from c.ipams[]. If not found it will load the driver. This is a similar change I have made for network drivers. This makes the the whole driver resolution happen wherever we need to get hold of ipam driver and not just during the network creation time because we need to get hold of ipam in other nodes where the network wasn't created but just being used to connect.
There was a problem hiding this comment.
good idea. Will change it.
| } | ||
| } | ||
|
|
||
| if err := initIpams(c, c.globalStore); err != nil { |
There was a problem hiding this comment.
Same here. initIpamDrivers ?
- Add IPAM cotract and remote IPAM hooks - Add ipam registration in controller - Have default IPAM follow ipamapi contract Signed-off-by: Alessandro Boch <aboch@docker.com>
Signed-off-by: Alessandro Boch <aboch@docker.com>
|
@aboch can you please merge https://github.com/aboch/libnetwork_new/pull/6 in your branch and push the PR again (just cherry-pick b2cd7d5 for the docs). |
Signed-off-by: Madhu Venugopal <madhu@docker.com>
|
thanks @aboch for handling the API changes as suggested and updating the docs. |
|
LGTM |
|
👍 |
|
Niceeee 👍 |
This is work in progress for #489