libnet/ipam: Various clean-ups #47727
Conversation
4d38248 to
64049ea
Compare
11cd948 to
a1f35fe
Compare
f405263 to
f14d76a
Compare
|
Ah, just realized one commit message was wrong. That's now fixed. |
corhere
left a comment
There was a problem hiding this comment.
What an improvement! It's amazing how much we were held back by cnmallocator living in the Swarmkit repo.
The meat of the changes look good. All I really have are aesthetic quibbles; nothing that couldn't be ignored, or dealt with in a follow-up PR.
| // Meta represents a list of extra IP addresses automatically reserved | ||
| // during the pool allocation. These are generally keyed by well-known | ||
| // strings defined in the netlabel package. | ||
| Meta map[string]string |
There was a problem hiding this comment.
If the values are always IP addresses, why are they stringly-typed? Or is the doc comment misleadingly describing how the default IPAM driver happens to populate a generic bag of data with implementation-defined semantics?
There was a problem hiding this comment.
Yeah, I'll update this comment -- it's misleading.
I'm also wondering how useful it is. No builtin driver set any Meta, only remote drivers might do that. And it's used only in two places:
moby/libnetwork/cnmallocator/networkallocator.go
Lines 905 to 910 in 16b2c22
Lines 1619 to 1623 in 16b2c22
So, I think it'd even be safe to replace it with an optional Gateway.
| } | ||
|
|
||
| func (n *Network) requestPoolHelper(ipam ipamapi.Ipam, addressSpace, requestedPool, requestedSubPool string, options map[string]string, v6 bool) (poolID string, pool *net.IPNet, meta map[string]string, err error) { | ||
| func (n *Network) requestPoolHelper(ipam ipamapi.Ipam, addressSpace, requestedPool, requestedSubPool string, options map[string]string, v6 bool) (string, *net.IPNet, map[string]string, error) { |
There was a problem hiding this comment.
Why'd you remove the names of the return values? Those are good documentation!
There was a problem hiding this comment.
I think it made a lot of sense when they were assigned from the call to RequestPool. Now that everything lives in alloc, I feel like it's too easy to return the wrong thing.
Anyway, I think it doesn't matter much. As discussed on Tuesday, this method will go away in the next PR.
Embedding `sync.Mutex` into a struct is considered a bad practice as it makes the mutex methods part of the embedding struct's API. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
`addrSpace` methods are currently scattered in two different files. As upcoming work will rewrite some of these methods, better put them into a separate file. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Address spaces are a continuum of addresses that can be used for a specific purpose (ie. 'local' for unmanaged containers, 'global for Swarm). v4 and v6 addresses aren't of the same size -- hence combining them into a single address space doesn't form a continuum. Better set them apart into two different address spaces. Also, the upcoming rewrite of `addrSpace` will benefit from that split. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The `RequestPool` method has many args and named returns. This makes the code hard to follow at times. This commit adds one struct, `PoolRequest`, to replace these args, and one struct, `AllocatedPool`, to replace these named returns. Both structs' fields are properly documented to better define their semantics, and their relationship with address allocation. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Prior to this change, daemon's `default-address-pools` param would be passed to `SetDefaultIPAddressPool()` to set a global var named `defaultAddressPool`. This var would then be retrieved during the `default` IPAM driver registration. Both steps were executed in close succession during libnet's controller initialization. This change removes the global var and just pass the user-defined `default-address-pools` to the `default` driver's `Register` fn. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
All drivers except the default ipam driver are stored in ipams/. Since `default` isn't a valid Go pkg name, this package is renamed to `defaultipam`, following `windowsipam` example. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Packages in libnet/ipams are drivers, except builtin -- it's used to register drivers. Move files one level up and delete this pkg. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
All drivers except the default have a Register function. Before this change, default's registration was handled by another package. Move this logic into the driver pkg. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This function is made a no-op on non-windows platform. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Prior to this change, cnmallocator would call `ConfigGlobalScopeDefaultNetworks` right before initializing its IPAM drivers. This function was mutating some global state used during drivers init. This change just remove the global state, and adds an arg to ipams.Register and defaultipam.Register to pass the global pools by arguments instead. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
f14d76a to
c5376e5
Compare
|
Okay, let's merge this one and rebase #47768. |
- What I did
Consider reviewing per commit.
addrSpaceaddrSpaceinto a separate fileDumpDatabaseandNetworkRangeaddrSpacePoolRequestandAllocatedPool, to represent the in/out values ofRequestPoollibnet/ipamtolibnet/ipams/defaultipam-- grouping all IPAM drivers underlibnet/ipams- How to verify it
Through tests.