libnetwork/ipam: refactor all the things #44968

corhere · 2023-02-09T23:58:32Z

- What I did

Simplified the implementation of package ipam by:
- removing the affordances for half-baked and otherwise unused functionality,
- overhauling the internal data model without datastore concerns, and
- moving code around to better follow the Single Responsibility Principle.
Migrated the implementation to use net/netip types internally, with thoughtfully-designed address manipulation utilities

I took great care to keep the behaviour as close to that of the original code as possible, including the not-so-great error messages.

- How I did it
128-bit arithmetic and lots of iterative refactoring.

I avoided rearranging functions within—or moving them across—source files for ease of review. I recommend using the split view when reviewing.

- How to verify it
The existing unit tests are fairly comprehensive, and have done a good job of catching mistakes during development.

- Description for the changelog

N/A; refactor with no significant user-visible differences

- A picture of a cute animal (not mandatory but encouraged)

corhere · 2023-02-10T00:00:25Z

cc: @akerouanton hopefully this'll be a cleaner state to rebase #43033 onto.

libnetwork/ipam/allocator.go

The two-phase commit dance serves no purpose with the current IPAM allocator implementation. There are no fallible operations between the call to aSpace.updatePoolDBOnAdd() and invoking the returned closure. Allocate the subnet in the address space immediately when called and get rid of the closure return. Signed-off-by: Cory Snider <csnider@mirantis.com>

Only two address spaces are supported: LocalDefault and GlobalDefault. Support for non-default address spaces in the IPAM Allocator is vestigial, from a time when IPAM state was stored in a persistent shared datastore. There is no way to create non-default address spaces through the IPAM API so there is no need to retain code to support the use of such address spaces. Drop all pretense that more address spaces can exist, to the extent that the IPAM API allows. Signed-off-by: Cory Snider <csnider@mirantis.com>

There is only one call site, in (*Allocator).RequestPool. The logic is tightly coupled between the caller and callee, and having them separate makes it harder to reason about. Signed-off-by: Cory Snider <csnider@mirantis.com>

The address spaces are orthogonal. There is no shared state between them logically so there is no reason for them to share any in-memory data structures. addrSpace is responsible for allocating subnets and addresses, while Allocator is responsible for implementing the IPAM API. Lower all the implementation details of allocation into addrSpace. There is no longer a need to include the name of the address space in the map keys for subnets now that each addrSpace holds its own state independently from other addrSpaces. Remove the AddressSpace field from the struct used for map keys within an addrSpace so that an addrSpace does not need to know its own name. Pool allocations were encoded in a tree structure, using parent references and reference counters. This structure affords for pools subdivided an arbitrary number of times to be modeled, in theory. In practice, the max depth is only two: master pools and sub-pools. The allocator data model has also been heavily influenced by the requirements and limitations of Datastore persistence, which are no longer applicable. Address allocations are always associated with a master pool. Sub-pools only serve to restrict the range of addresses within the master pool which could be allocated from. Model pool allocations within an address space as a two-level hierarchy of maps. Signed-off-by: Cory Snider <csnider@mirantis.com>

The netip types can be used as map keys, unlike net.IP and friends, which is a very useful property to have for this application. Signed-off-by: Cory Snider <csnider@mirantis.com>

Prior to moby#44968, libnetwork would happily accept a ChildSubnet with a bigger mask than its parent subnet. In such case, it was producing IP addresses based on the parent subnet, and the child subnet was not allocated from the address pool. Previous commit disallow such bad IPAM config for API >= 1.44. However, for older API versions, we still accept these bad values and instead automatically fix it for the user. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Prior to moby#44968, libnetwork would happily accept a ChildSubnet with a bigger mask than its parent subnet. In such case, it was producing IP addresses based on the parent subnet, and the child subnet was not allocated from the address pool. Previous commit returns a warning message if a an invalid IPAM config is detected. This commit automatically fixes an invalid ChildSubnet, either coming from the API, or from a stored network, if one is detected. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Prior to moby#44968, libnetwork would happily accept a ChildSubnet with a bigger mask than its parent subnet. In such case, it was producing IP addresses based on the parent subnet, and the child subnet was not allocated from the address pool. This commit automatically fixes invalid ChildSubnet for networks stored in libnetwork's datastore. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Prior to moby#44968, libnetwork would happily accept a ChildSubnet with a bigger mask than its parent subnet. In such case, it was producing IP addresses based on the parent subnet, and the child subnet was not allocated from the address pool. This commit automatically fixes invalid ChildSubnet for networks stored in libnetwork's datastore. Signed-off-by: Albin Kerouanton <albinker@gmail.com> (cherry picked from commit 3e8af08) Signed-off-by: Albin Kerouanton <albinker@gmail.com>

corhere added area/networking kind/refactor PR's that refactor, or clean-up code labels Feb 9, 2023

corhere added this to the v-next milestone Feb 9, 2023

corhere requested review from tianon, cpuguy83, neersighted and thaJeztah February 9, 2023 23:58

corhere force-pushed the libnet/ipam-cleanup branch from 86628d4 to 6341c6e Compare February 10, 2023 18:38

cpuguy83 requested changes Feb 10, 2023

View reviewed changes

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

corhere force-pushed the libnet/ipam-cleanup branch from 6341c6e to 721a309 Compare February 16, 2023 23:29

cpuguy83 approved these changes Feb 23, 2023

View reviewed changes

corhere added 5 commits February 23, 2023 18:09

libnet/ipam: inline parsePoolRequest function

8273db2

There is only one call site, in (*Allocator).RequestPool. The logic is tightly coupled between the caller and callee, and having them separate makes it harder to reason about. Signed-off-by: Cory Snider <csnider@mirantis.com>

libnet/ipam: use netip types internally

3c59ef2

The netip types can be used as map keys, unlike net.IP and friends, which is a very useful property to have for this application. Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere force-pushed the libnet/ipam-cleanup branch from 721a309 to 3c59ef2 Compare February 23, 2023 23:10

neersighted approved these changes Mar 2, 2023

View reviewed changes

corhere merged commit e321570 into moby:master Mar 2, 2023

corhere deleted the libnet/ipam-cleanup branch March 2, 2023 20:28

akerouanton mentioned this pull request Jun 15, 2023

Validate IPAM config before handing it over to libnetwork #45759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnetwork/ipam: refactor all the things #44968

libnetwork/ipam: refactor all the things #44968

corhere commented Feb 9, 2023

corhere commented Feb 10, 2023

libnetwork/ipam: refactor all the things #44968

libnetwork/ipam: refactor all the things #44968

Conversation

corhere commented Feb 9, 2023

corhere commented Feb 10, 2023