Account for IPReservations in IP pool utilization

calicoctl/calicoctl/commands/ipam/show.go

@@ -17,7 +17,6 @@ package ipam
 import (
 	"context"
 	"fmt"
-	"math"
 	"os"
 	"reflect"
 	"sort"
@@ -209,37 +208,41 @@ func ShowBlockUtilization(ctx context.Context, ipamClient ipam.Interface, showBl
 	}
 	t := table.NewWriter()
 	t.SetOutputMirror(os.Stdout)
-	t.AppendHeader(table.Row{"GROUPING", "CIDR", "IPS TOTAL", "IPS IN USE", "IPS FREE"})
+	t.AppendHeader(table.Row{"GROUPING", "CIDR", "IPS TOTAL", "IPS IN USE", "IPS RESERVED", "IPS FREE"})
 	t.SetColumnConfigs([]table.ColumnConfig{
 		{Name: "IPS TOTAL", Align: text.AlignRight},
 		{Name: "IPS IN USE", Align: text.AlignRight},
+		{Name: "IPS RESERVED", Align: text.AlignRight},
 		{Name: "IPS FREE", Align: text.AlignRight},
 	})
-	genRow := func(kind, cidr string, inUse, capacity float64) table.Row {
+	// IN USE counts allocated IPs and RESERVED counts IPs that an IPReservation
+	// covers; an IP allocated before it was reserved falls into both, so the
+	// percentages need not add up to 100.  FREE counts the IPs that are neither,
+	// which is why it comes from the library rather than being derived here.
+	genRow := func(kind, cidr string, capacity, inUse, reserved, free int) table.Row {
+		withPercentage := func(n int) string {
+			return fmt.Sprintf("%.5g (%.f%%)", float64(n), 100*float64(n)/float64(capacity))
+		}
 		return table.Row{
 			kind,
 			cidr,
-			fmt.Sprintf("%.5g", capacity),
-			// Note: the '+capacity/2' bits here give us rounding to the nearest
-			// integer, instead of rounding down, and so ensure that the two percentages
-			// add up to 100.
-			fmt.Sprintf("%.5g (%.f%%)", inUse, 100*inUse/capacity),
-			fmt.Sprintf("%.5g (%.f%%)", capacity-inUse, 100*(capacity-inUse)/capacity),
+			fmt.Sprintf("%.5g", float64(capacity)),
+			withPercentage(inUse),
+			withPercentage(reserved),
+			withPercentage(free),
 		}
 	}
 	for _, poolUse := range usage {
 		var blockRows []table.Row
-		var poolInUse float64
 		for _, blockUse := range poolUse.Blocks {
-			blockRows = append(blockRows, genRow("Block", blockUse.CIDR.String(), float64(blockUse.Capacity-blockUse.Available), float64(blockUse.Capacity)))
-			poolInUse += float64(blockUse.Capacity - blockUse.Available)
+			blockRows = append(blockRows, genRow("Block", blockUse.CIDR.String(),
+				blockUse.Capacity, blockUse.InUse, blockUse.Reserved, blockUse.Available))
 		}
-		ones, bits := poolUse.CIDR.Mask.Size()
-		poolCapacity := math.Pow(2, float64(bits-ones))
-		if ones > 0 {
+		if ones, _ := poolUse.CIDR.Mask.Size(); ones > 0 {
 			// Only show the IP Pool row for a real IP Pool and not for the orphaned
 			// block case.
-			t.AppendRow(genRow("IP Pool", poolUse.CIDR.String(), poolInUse, poolCapacity))
+			t.AppendRow(genRow("IP Pool", poolUse.CIDR.String(),
+				poolUse.Capacity, poolUse.InUse, poolUse.Reserved, poolUse.Available))
 		}
 		if showBlocks {
 			t.AppendRows(blockRows)

charts/calico/templates/calico-kube-controllers-rbac.yaml

@@ -121,6 +121,7 @@ rules:
       - ipreservations
     verbs:
       - list
+      - watch
   - apiGroups: ["projectcalico.org", "crd.projectcalico.org"]
     resources:
       - blockaffinities

design/ipam/ipam-core-library.md

@@ -20,6 +20,14 @@ here. A few methods carry design-relevant constraints worth calling out:
 
 - **`AutoAssign`** returns block-masked CIDRs, not `/32` (or `/128`). Callers narrow at the boundary. This is load-bearing for the CNI plugin's per-block route programming.
 - **`AssignIP`** enforces the target pool's `allowedUses` when `AssignIPArgs.IntendedUse` is non-empty: it fails if the pool containing the requested IP does not permit that use, mirroring the `filterPoolsByUse` filter `AutoAssign` applies. Callers that leave `IntendedUse` empty are exempt (back-compat). This closes a gap where a specific-IP request (e.g. the CNI `ipAddrs` annotation) could draw from a pool not sanctioned for its use.
+- **`GetUtilization`** reports `Capacity`, `InUse`, `Reserved` and `Available` per pool and per block. `InUse` (allocated) and `Reserved` (covered by an `IPReservation`) overlap when an
+  address was allocated before it was reserved, so `Capacity` is not their sum plus `Available`; `Available` counts addresses that are neither, and is the only one of the four that
+  answers "how many can still be handed out". Consumers must read it rather than deriving it. Pool-level counts span the whole pool CIDR, including space no block covers yet - a
+  reservation over unblocked space is still unassignable - so they are computed as a set operation (pool minus reservations minus blocks, via `go4.org/netipx`) rather than summed
+  from the blocks. Reservations may overlap and nest arbitrarily, which is why a set is needed and not a sum over CIDRs.
+- **`NumReservedIPsInCIDR`** is the pool-level reserved count on its own, for callers that already hold the `IPReservation`s and would rather not pay for a list of every allocation
+  block. kube-controllers uses it for `ipam_ippool_reserved` from syncer-fed reservations (see [ipam-gc](./ipam-gc.md#metrics)). It takes the resources, not CIDRs, so that a variant
+  can take a second kind of reserving resource without reshaping its callers.
 - **`ReleaseIPs`** takes `ReleaseOptions` with a sequence number; every release path must plumb it through (see [CAS retry and sequence numbers](#cas-retry-and-sequence-numbers)).
 - **`SetOwnerAttributes`** is KubeVirt-only and swaps owner attributes under preconditions, without releasing and re-allocating. Felix's live-migration monitor is the only non-CNI
   caller.
@@ -32,6 +40,10 @@ here. A few methods carry design-relevant constraints worth calling out:
 - Don't leak `crd.projectcalico.org/v1` types through new public APIs. The `lib/v3` -> `lib/internalapi` rename (https://github.com/projectcalico/calico/pull/11870) exists to keep
   that boundary clean.
 - `AutoAssign` returning block-masked CIDRs is load-bearing for the CNI plugin's routing. Don't quietly switch to `/32`.
+- Anything that makes an address unassignable has to be discounted by `GetUtilization` as well as by the allocation path, or the reporting surfaces over-count free addresses. The
+  two must be fed from the same set of reserved CIDRs: allocation and the per-block counts share the `addrFilter`, and the pool-level counts use the same CIDRs as a set.
+- There is one implementation of the reserved-set arithmetic, in [`reserved.go`](../../libcalico-go/lib/ipam/reserved.go). `GetUtilization` and `NumReservedIPsInCIDR` are both thin
+  callers of it. Don't grow a second copy in a consumer - a reporting surface that disagrees with `calicoctl ipam show` is worse than no surface.
 
 ## AutoAssign and host affinity
 

design/ipam/ipam-datastore.md

@@ -83,7 +83,8 @@ the block syncer plus a handle-side scan rather than a watch. See https://github
 
 Both stored as CRDs alongside the other IPAM resources. [`ipam_config.go`](../../libcalico-go/lib/backend/k8s/resources/ipam_config.go) wraps the singleton `IPAMConfig`. Storage
 shape only - field semantics, defaults, and `StrictAffinity` / `MaxBlocksPerHost` / `AutoAllocateBlocks` interactions live in
-[`ipam-core-library.md`](./ipam-core-library.md#ipamconfig). `IPReservation` is read at allocation time and converted into an ordinal filter; never participates in CAS.
+[`ipam-core-library.md`](./ipam-core-library.md#ipamconfig). `IPReservation` is read at allocation time and converted into an ordinal filter, and again by `GetUtilization` so that
+the reporting surfaces don't count reserved addresses as free. kube-controllers watches it on its syncer instead, to keep the read off the IPAM sync loop. Never participates in CAS.
 
 **Review notes**
 

design/ipam/ipam-gc.md

@@ -214,11 +214,23 @@ variants exist for backward compatibility.
 `updateMetrics` recomputes from scratch every sync - one walk over all blocks, no incremental state. The full-recompute *is* the consistency check; switching to incremental updates
 without a separate consistency check loses the protection.
 
+`ipam_ippool_reserved` is the exception to that walk: reservations make addresses unassignable without allocating them, and can cover pool space no block has been carved from, so the
+number isn't in the block state the controller tracks. `IPReservation` is therefore a fourth kind on the controller's syncer, cached by name in `reservations`, and
+`updateReservedMetrics` counts the covered addresses per pool with `ipam.NumReservedIPsInCIDR` (see [ipam-core-library](./ipam-core-library.md#public-api-surface)). The arithmetic is
+the library's, so the gauge agrees with `calicoctl ipam show`; the input is the syncer's, so the sync loop makes no datastore request for it. Being per-pool rather than per-node, the
+gauge is labelled `ippool` only, like `ipam_ippool_size`. It may overlap `ipam_allocations_in_use`, so usable capacity is
+`ipam_ippool_size - ipam_allocations_in_use - ipam_ippool_reserved` only when no reserved address is also allocated.
+
+Watching `IPReservation` needs `watch` in the kube-controllers ClusterRole, in the chart **and** in tigera/operator. With only `list` granted the List still succeeds and the syncer
+still reaches in-sync, so the symptom is a hot re-list of `IPReservation`s rather than a stalled controller - easy to miss in review, noisy in production.
+
 **Review notes**
 
 - `ipam_allocations_gc_candidates > 0` for extended periods is the canonical "GC is stuck" signal. Alert on it.
 - `ipam_allocations_gc_reclamations` rate is the canonical "we have a real leak somewhere" signal. Alert on it.
 - Don't switch `updateMetrics` to incremental updates without a separate consistency check. The current full-recompute is the consistency check.
+- A metric is not a licence to add a datastore request to the sync loop. The loop shares a goroutine with leak GC, and past overload has clogged it; new inputs belong on the syncer.
+  `ipam_ippool_reserved` was caught doing a LIST of every block per sync in review (https://github.com/projectcalico/calico/pull/13331).
 - The in-memory state maps must agree at all times. `assertConsistentState` in `ipam_test.go` is the canonical invariant check; any new map mutation needs a test that exercises it.
   The v3.32 memory-leak family (https://github.com/projectcalico/calico/pull/12277, /12286, /12287, /12288) all came from "added to one path, forgot another."
 

design/ipam/ipam-other-callers.md

@@ -23,10 +23,15 @@ migrate` parses the tunnel prefixes, and the CNI DEL path releases by both the h
 IPAM-meaningful subcommands; `configure` and `split` are admin-CRUD on `IPAMConfig` / `IPPool`. The `check` algorithm reuses the validity heuristics the GC applies, but exposed for
 manual review without a running controller.
 
+`show` prints IPS TOTAL / IN USE / RESERVED / FREE per pool and per block, taking all four straight from `GetUtilization` rather than deriving any of them: IN USE and RESERVED can
+cover the same address, so the columns need not sum to the total, and FREE is the only column that means "still assignable". See
+[ipam-core-library](./ipam-core-library.md#public-api-surface).
+
 **Review notes**
 
 - New tunnel handle prefixes need to be added to `calicoctl datastore migrate`, which rewrites tunnel handle IDs during node renames.
 - `check` and the GC share validity logic. If you change one, check the other doesn't drift.
+- The `show` columns are a reporting surface, not a derivation. If a new mechanism withholds addresses, it has to reach `GetUtilization` or `show` silently over-counts FREE.
 
 ## Node tunnel-address allocator
 

go.mod

@@ -92,6 +92,7 @@ require (
 	go.etcd.io/etcd/client/v2 v2.305.31
 	go.etcd.io/etcd/client/v3 v3.6.12
 	go.yaml.in/yaml/v3 v3.0.4
+	go4.org/netipx v0.0.0-20231129151722-fdeea329fbba
 	golang.org/x/mod v0.38.0
 	golang.org/x/net v0.57.0
 	golang.org/x/oauth2 v0.36.0

go.sum

@@ -890,6 +890,8 @@ go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ=
 go.yaml.in/yaml/v3 v3.0.3/go.mod h1:tBHosrYAkRZjRAOREWbDnBXUf08JOwYq++0QNwQiWzI=
 go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
 go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
+go4.org/netipx v0.0.0-20231129151722-fdeea329fbba h1:0b9z3AuHCjxk0x/opv64kcgZLBseWJUpBw5I82+2U4M=
+go4.org/netipx v0.0.0-20231129151722-fdeea329fbba/go.mod h1:PLyyIXexvUFg3Owu6p/WfdlivPbZJsZdgWZlrGope/Y=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=