multi-pool: Support allocating from new IPAM pools on demand

[gandro]

This PR adds support for dynamically allocating IPs from a new pool. Previously, we required the user to specify in advance what pool the agent would allocate from. This however is very limiting, since it's often not known in advance what workload a particular node will run.

Therefore, this PR adds a mechanism where upon observing a allocation attempt for a new pool, we update the local CiliumNode CRD to request additional IPs for that pool. This then allows the allocation request to be successful upon retry. This retry mechanism is similar to how Cilium IPAM also works e.g. in ENI mode (where if the local node is out of IPs, we fail the CNI ADD request and expect kubelet to re-try again later).

This PR also implicitly adds support for allocating from pools without a "pre-allocate" value. This allows users to configure very small pools for specific workloads and assign those few IPs only if they are actually used.

ℹ️ Review per commit. The first few commits are mainly cleanup, the main logic is contained in the last two commits

For further context, see:

• Add support for allocating PodCIDRs from multiple IPAM pools #22762
• multi-pool: Determine IP pool based on ipam.cilium.io/ip-pool annotation #25511
• IPAM Multi-Pool Follow-ups #25470

CI and documentation will be added in a separate PR. See meta issue #25470

[gandro]

/test

[meyskens]

Code looks good to me, one tiny nit :)
Functionality I am not the best person for to ask so if a better expert has time to also review 😄

[meyskens]

Given this is a string might %q not be more cleat to the user? (default default=8) vs (default "default=8")

[gandro]

Thanks! Yes, I noticed this too, and I am torn on this. It's not really a string, it's a map. But option.NewNamedMapOptions does not allow me to define a default map, which is why I'm setting a default "string" value (which is later parsed as a map).

I've went back and forth between %s and %q. I ended up with %s, because other non-string types (like e.g. integers, durations and booleans) don't use quotes either:

cilium/Documentation/cmdref/cilium-agent.md

Line 17 in d137d83

--allocator-list-timeout duration Timeout for listing allocator state before exiting (default 3m0s)

[christarazi]

Another note here is that you don't need to explicitly mention the default here because flags does it for you, AFAIR.

[gandro]

Unfortunately, for some reason it doesn't here. See the generated docs, there is no duplicate default.

[squeed]

One thought occurs to me: is the owner a pod or the CNI container ID?

If it's a container ID, then that is unique for every CNI ADD (even retries), so we will have a churn issue. If it's a pod, then we're OK.

[gandro]

One thought occurs to me: is the owner a pod or the CNI container ID?

Good point. It's the pod:

cilium/plugins/cilium-cni/main.go

Line 121 in 2588462

podName := string(cniArgs.K8S_POD_NAMESPACE) + "/" + string(cniArgs.K8S_POD_NAME)

[gandro]

Addressed the feedback and rebased. No functional changes:

diff --git a/pkg/ipam/multipool.go b/pkg/ipam/multipool.go
index 18439b361e86..9b443dd9a2be 100644
--- a/pkg/ipam/multipool.go
+++ b/pkg/ipam/multipool.go
@@ -352,11 +352,11 @@ func neededIPCeil(numIP int, preAlloc int) int {
 // computeNeededIPsPerPoolLocked computes how many IPs we want to request from
 // the operator for each pool. The formula we use for each pool is basically
 //
-//       neededIPs = roundUp(inUseIPs + pendingIPs + preAllocIPs, preAlloc)
+//     neededIPs = roundUp(inUseIPs + pendingIPs + preAllocIPs, preAllocIPs)
 //
-//               inUseIPs              Number of IPs that are currently actively in use
-//               pendingIPs    Number of IPs that have been requested, but not yet assigned
-//               preAllocIPs   Minimum number of IPs that we want to pre-allocate as a buffer
+//           inUseIPs      Number of IPs that are currently actively in use
+//           pendingIPs    Number of IPs that have been requested, but not yet assigned
+//           preAllocIPs   Minimum number of IPs that we want to pre-allocate as a buffer
 //
 // Rounded up to the next multiple of preAllocIPs.
 func (m *multiPoolManager) computeNeededIPsPerPoolLocked() map[Pool]types.IPAMPoolDemand {
@@ -596,17 +596,11 @@ func (m *multiPoolManager) poolByFamilyLocked(poolName Pool, family Family) *pod
        return nil
 }
 
-func (m *multiPoolManager) allocateNext(owner string, poolName Pool, family Family, syncUpstream bool) (result *AllocationResult, err error) {
+func (m *multiPoolManager) allocateNext(owner string, poolName Pool, family Family, syncUpstream bool) (*AllocationResult, error) {
        m.mutex.Lock()
        defer m.mutex.Unlock()
 
        defer func() {
-               if err != nil {
-                       m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
-               } else {
-                       m.pendingIPsPerPool.markAsAllocated(poolName, owner, family)
-               }
-
                if syncUpstream {
                        m.k8sUpdater.TriggerWithReason("allocation of next IP")
                }
@@ -614,27 +608,25 @@ func (m *multiPoolManager) allocateNext(owner string, poolName Pool, family Fami
 
        pool := m.poolByFamilyLocked(poolName, family)
        if pool == nil {
+               m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
                return nil, fmt.Errorf("unable to allocate from pool %q (family %s): pool not (yet) available", poolName, family)
        }
 
        ip, err := pool.allocateNext()
        if err != nil {
+               m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
                return nil, err
        }
+
+       m.pendingIPsPerPool.markAsAllocated(poolName, owner, family)
        return &AllocationResult{IP: ip, IPPoolName: poolName}, nil
 }
 
-func (m *multiPoolManager) allocateIP(ip net.IP, owner string, poolName Pool, family Family, syncUpstream bool) (result *AllocationResult, err error) {
+func (m *multiPoolManager) allocateIP(ip net.IP, owner string, poolName Pool, family Family, syncUpstream bool) (*AllocationResult, error) {
        m.mutex.Lock()
        defer m.mutex.Unlock()
 
        defer func() {
-               if err != nil {
-                       m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
-               } else {
-                       m.pendingIPsPerPool.markAsAllocated(poolName, owner, family)
-               }
-
                if syncUpstream {
                        m.k8sUpdater.TriggerWithReason("allocation of specific IP")
                }
@@ -642,13 +634,17 @@ func (m *multiPoolManager) allocateIP(ip net.IP, owner string, poolName Pool, fa
 
        pool := m.poolByFamilyLocked(poolName, family)
        if pool == nil {
+               m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
                return nil, fmt.Errorf("unable to reserve IP %s from pool %q (family %s): pool not (yet) available", ip, poolName, family)
        }
 
-       err = pool.allocate(ip)
+       err := pool.allocate(ip)
        if err != nil {
+               m.pendingIPsPerPool.upsertPendingAllocation(poolName, owner, family)
                return nil, err
        }
+
+       m.pendingIPsPerPool.markAsAllocated(poolName, owner, family)
        return &AllocationResult{IP: ip, IPPoolName: poolName}, nil
 }

[gandro]

/test