libnet/ipam: Various clean-ups #47727

akerouanton · 2024-04-17T11:50:42Z

- What I did

Consider reviewing per commit.

Un-embed mutex from addrSpace
Put addrSpace into a separate file
Remove dead functions DumpDatabase and NetworkRange
Split v4/v6 addrSpace
Introduce two new structs, PoolRequest and AllocatedPool, to represent the in/out values of RequestPool
Rewrote a few tests to make them more idiomatic
Remove the global vars used to store the local and global address pools
Move package libnet/ipam to libnet/ipams/defaultipam -- grouping all IPAM drivers under libnet/ipams

- How to verify it

Through tests.

robmry

Much neater, nice!

libnetwork/ipamapi/contract.go

libnetwork/internal/netiputil/netiputil.go

libnetwork/ipam/allocator.go

akerouanton · 2024-04-19T08:25:46Z

Ah, just realized one commit message was wrong. That's now fixed.

corhere

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.

corhere · 2024-04-25T22:00:30Z

libnetwork/ipamapi/contract.go

+ // 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


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?

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

if gws, ok := meta[netlabel.Gateway]; ok {

if ip, gwIP, err = net.ParseCIDR(gws); err != nil {

return nil, fmt.Errorf("failed to parse gateway address (%v) returned by ipam driver: %v", gws, err)

}

gwIP.IP = ip

}

moby/libnetwork/network.go

Lines 1619 to 1623 in 16b2c22

if gws, ok := d.Meta[netlabel.Gateway]; ok {

if d.Gateway, err = types.ParseCIDR(gws); err != nil {

return types.InvalidParameterErrorf("failed to parse gateway address (%v) returned by ipam driver: %v", gws, err)

}

}

So, I think it'd even be safe to replace it with an optional Gateway.

libnetwork/ipams/defaultipam/allocator.go

libnetwork/ipams/defaultipam/allocator_test.go

corhere · 2024-04-25T22:30:56Z

libnetwork/network.go

@@ -1508,21 +1509,27 @@ func (n *Network) ipamAllocate() error {
 return err
 }

-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) {


Why'd you remove the names of the return values? Those are good documentation!

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.

libnetwork/ipams/windowsipam/windowsipam_other.go

libnetwork/cnmallocator/drivers_ipam.go

libnetwork/controller.go

libnetwork/drvregistry/ipams_test.go

libnetwork/ipamutils/utils.go

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 <[email protected]>

`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 <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

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 <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

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 <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

This function is made a no-op on non-windows platform. Signed-off-by: Albin Kerouanton <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

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 <[email protected]>

Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton · 2024-04-26T20:42:30Z

Okay, let's merge this one and rebase #47768.

akerouanton added area/networking kind/refactor PR's that refactor, or clean-up code area/networking/ipam labels Apr 17, 2024

akerouanton added this to the 27.0.0 milestone Apr 17, 2024

akerouanton requested review from corhere, thaJeztah and robmry April 17, 2024 11:50

akerouanton self-assigned this Apr 17, 2024

akerouanton force-pushed the libnet-ipam-cleanup branch 3 times, most recently from 4d38248 to 64049ea Compare April 17, 2024 15:31

robmry reviewed Apr 17, 2024

View reviewed changes

libnetwork/ipamapi/contract.go Show resolved Hide resolved

libnetwork/internal/netiputil/netiputil.go Outdated Show resolved Hide resolved

libnetwork/ipam/allocator.go Outdated Show resolved Hide resolved

akerouanton force-pushed the libnet-ipam-cleanup branch 2 times, most recently from 11cd948 to a1f35fe Compare April 18, 2024 11:06

robmry approved these changes Apr 18, 2024

View reviewed changes

akerouanton force-pushed the libnet-ipam-cleanup branch from f405263 to f14d76a Compare April 19, 2024 08:25

corhere approved these changes Apr 25, 2024

View reviewed changes

akerouanton added 12 commits April 26, 2024 16:06

libnet/ipam: un-embed mutex from addrSpace

a047d4b

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 <[email protected]>

libnet/ipam: put addrSpace into a separate file

df88857

`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 <[email protected]>

libnet/ipam: remove dead DumpDatabase()

199c72c

Signed-off-by: Albin Kerouanton <[email protected]>

libnet/netutils: remove dead util NetworkRange

82aae0f

Signed-off-by: Albin Kerouanton <[email protected]>

integration-cli: createNetwork: add t.Helper()

1d5a12d

Signed-off-by: Albin Kerouanton <[email protected]>

libnet: move ipam pkg to ipam/defaultipam

29f2ca0

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 <[email protected]>

libnet/ipams/builtin: move to libnet/ipams

218394c

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 <[email protected]>

libnet/ipams/defaultipam: add a Register fn

8cec9f0

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 <[email protected]>

libnet/ipams/windowsipam: that driver knows its name

ae9e431

Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton added 5 commits April 26, 2024 17:28

libnet/ipams: Unconditionally call windowsipam.Register

eda4750

This function is made a no-op on non-windows platform. Signed-off-by: Albin Kerouanton <[email protected]>

libnet/ipams: register all drivers

3c97181

Signed-off-by: Albin Kerouanton <[email protected]>

libnet/ipams/defaultipam: move driver name to its pkg

f2387f3

Signed-off-by: Albin Kerouanton <[email protected]>

libnet/ipams/null: move driver name to its pkg

c5376e5

Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton force-pushed the libnet-ipam-cleanup branch from f14d76a to c5376e5 Compare April 26, 2024 15:29

akerouanton mentioned this pull request Apr 26, 2024

libnet/ipams/default: introduce a linear allocator #47768

Merged

robmry approved these changes Apr 26, 2024

View reviewed changes

akerouanton merged commit 48d769b into moby:master Apr 26, 2024
126 checks passed

akerouanton deleted the libnet-ipam-cleanup branch April 26, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnet/ipam: Various clean-ups #47727

libnet/ipam: Various clean-ups #47727

akerouanton commented Apr 17, 2024 •

edited

robmry left a comment

akerouanton commented Apr 19, 2024

corhere left a comment

corhere Apr 25, 2024

akerouanton Apr 26, 2024

corhere Apr 25, 2024

akerouanton Apr 26, 2024 •

edited

akerouanton commented Apr 26, 2024 •

edited

	if gws, ok := meta[netlabel.Gateway]; ok {
	if ip, gwIP, err = net.ParseCIDR(gws); err != nil {
	return nil, fmt.Errorf("failed to parse gateway address (%v) returned by ipam driver: %v", gws, err)
	}
	gwIP.IP = ip
	}

	if gws, ok := d.Meta[netlabel.Gateway]; ok {
	if d.Gateway, err = types.ParseCIDR(gws); err != nil {
	return types.InvalidParameterErrorf("failed to parse gateway address (%v) returned by ipam driver: %v", gws, err)
	}
	}

libnet/ipam: Various clean-ups #47727

libnet/ipam: Various clean-ups #47727

Conversation

akerouanton commented Apr 17, 2024 • edited

robmry left a comment

Choose a reason for hiding this comment

akerouanton commented Apr 19, 2024

corhere left a comment

Choose a reason for hiding this comment

corhere Apr 25, 2024

Choose a reason for hiding this comment

akerouanton Apr 26, 2024

Choose a reason for hiding this comment

corhere Apr 25, 2024

Choose a reason for hiding this comment

akerouanton Apr 26, 2024 • edited

Choose a reason for hiding this comment

akerouanton commented Apr 26, 2024 • edited

akerouanton commented Apr 17, 2024 •

edited

akerouanton Apr 26, 2024 •

edited

akerouanton commented Apr 26, 2024 •

edited