From 44c0313dc7a9e354a4d5b435c370abc6cad737ea Mon Sep 17 00:00:00 2001 From: Kristina Solovyova Date: Tue, 1 Sep 2026 19:04:54 +0400 Subject: [PATCH] fix: detect compute-blocked drive growth, split per-cause warnings, fix infeasibility binding --- cmd/weka-capacity/autofulldrives_test.go | 37 + cmd/weka-capacity/plan.go | 5 + doc/operator/deployment/act-as-daemonset.md | 104 ++- internal/capacityplanner/autofulldrives.go | 119 ++- .../capacityplanner/autofulldrives_compute.go | 65 +- .../capacityplanner/autofulldrives_fit.go | 13 +- .../capacityplanner/autofulldrives_test.go | 850 +++++++++++++++++- .../autofulldrives_warnings.go | 74 +- .../autofulldrives_warnings_test.go | 97 +- internal/capacityplanner/compute_layout.go | 51 +- .../capacityplanner/compute_layout_test.go | 28 +- internal/capacityplanner/infeasibility.go | 73 +- internal/capacityplanner/inventory/collect.go | 77 +- internal/capacityplanner/nodecapacity.go | 5 + internal/capacityplanner/planner.go | 35 +- .../wekaclient/client_reconciler_loop.go | 6 +- .../wekacluster/funcs_fd_planning.go | 4 +- .../wekacluster/funcs_fd_planning_test.go | 241 ++++- .../controllers/wekacluster/planner_events.go | 25 +- .../wekacluster/reconciler_loop.go | 11 +- .../controllers/wekacontainer/controller.go | 2 +- pkg/go-steps-engine | 2 +- 22 files changed, 1708 insertions(+), 216 deletions(-) diff --git a/cmd/weka-capacity/autofulldrives_test.go b/cmd/weka-capacity/autofulldrives_test.go index 13834372d..5bd519326 100644 --- a/cmd/weka-capacity/autofulldrives_test.go +++ b/cmd/weka-capacity/autofulldrives_test.go @@ -996,3 +996,40 @@ func mapKeys(m map[string]any) []string { sort.Strings(out) return out } + +// TestAutoFullDrivesNodeRows_ComputeDeletingNote covers the NOTE for the compute-blocked deferral. The +// contract for the column is that any row with used < avail explains the gap, and this cause reaches it on +// both paths: a create that never happens, and a growth held at the container's current size. Without the +// case the row shows a bare gap while WARNINGS names the node. +func TestAutoFullDrivesNodeRows_ComputeDeletingNote(t *testing.T) { + nodeInv := []capacityplanner.NodeCapacity{ + // create deferred: no container of ours yet, nothing planned. + {NodeName: "n-blk-create", FDValue: "n-blk-create", DriveCapacitiesGiB: []int{3840, 3840}, + HasDeletingComputeContainer: true}, + // growth deferred: holds 1 of its 3 drives, no Grow entry written this pass. + {NodeName: "n-blk-grow", FDValue: "n-blk-grow", OwnDriveCapacitiesGiB: []int{3840}, + DriveCapacitiesGiB: []int{3840, 3840}, HasDeletingComputeContainer: true}, + } + existing := []capacityplanner.ExistingContainer{ + {Name: "c-blk-grow", Node: "n-blk-grow", TlcGiB: 3840, NumCores: 1, NumDrives: 1}, + } + plan := &capacityplanner.CapacityPlan{} + + byNode := map[string]autoFullDrivesNodeRow{} + for _, r := range autoFullDrivesNodeRows(nodeInv, existing, plan) { + byNode[r.Node] = r + } + + for _, node := range []string{"n-blk-create", "n-blk-grow"} { + row, ok := byNode[node] + if !ok { + t.Fatalf("%s: missing row", node) + } + if row.DrivesUsed >= row.DrivesAvail { + t.Fatalf("%s: fixture must leave a gap to explain, got used=%d avail=%d", node, row.DrivesUsed, row.DrivesAvail) + } + if !strings.Contains(row.Note, "compute container being deleted") { + t.Errorf("%s: Note = %q, want it to name the deleting compute container", node, row.Note) + } + } +} diff --git a/cmd/weka-capacity/plan.go b/cmd/weka-capacity/plan.go index 1e4eee645..11ab6258c 100644 --- a/cmd/weka-capacity/plan.go +++ b/cmd/weka-capacity/plan.go @@ -695,6 +695,11 @@ func autoFullDrivesNodeRows(nodeInv []capacityplanner.NodeCapacity, existing []c row.Note = note(capacityplanner.WarningKindTransient, "drive container being deleted") case existingByNode[n.NodeName].Unscheduled: row.Note = note(capacityplanner.WarningKindTransient, "pod has not been scheduled yet") + // Not gated on nodeStateNotPlanned: this defers a growth as well as a create, and a grown-but- + // deferred node still renders its existing (smaller) container rather than "not planned". Ordered + // after Unscheduled to match the walk, which resolves that skip first. + case n.HasDeletingComputeContainer: + row.Note = note(capacityplanner.WarningKindTransient, "compute container being deleted") case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): row.Note = "drives held back by the numDrives pin — see WARNINGS" } diff --git a/doc/operator/deployment/act-as-daemonset.md b/doc/operator/deployment/act-as-daemonset.md index da1e3d8ed..7f50761b2 100644 --- a/doc/operator/deployment/act-as-daemonset.md +++ b/doc/operator/deployment/act-as-daemonset.md @@ -307,8 +307,9 @@ demands are found in this order: 1. **A new compute container on a node that has none yet**, if any compute-eligible node is free. This is preferred because it disturbs nothing already running. 2. **Growing an existing compute container in place**, for whatever new containers cannot carry. Each - container can grow up to `capacityPlannerConstraints.maxCoresPerContainer` and only as far as its - node's own spare CPU, hugepages and memory allow. + container can grow up to `capacityPlannerConstraints.maxCoresPerContainer` — or up to a pinned + `computeCores`, whichever is lower — and only as far as its node's own spare CPU, hugepages and + memory allow. The two combine: the operator uses the least in-place growth that lets the rest fit on free nodes, so a free node too small to absorb the whole shortfall is still used for as much as it can take. Only if both @@ -518,10 +519,19 @@ than absorbing a subset. The infeasibility report names the offending nodes — not just the first — with the binding dimension (physical CPU, hugepages, or memory) and the needed-versus-available figures for each, so one `kubectl describe wekacluster` tells you the story. The message spells out up to ten nodes and then -appends `(+N more)`; the structured report behind it always carries every one. +appends `(+N more)`; the structured report behind it always carries every one. On the **growth** path +specifically, the message adds a clause explaining that the missing headroom may be held by this +cluster's own compute container on that node — since compute reservations only ever rise, a compute +container placed earlier can end up holding room a later drive-container growth needs. See +[Relaxing a pin in stages can strand capacity](#relaxing-a-pin-in-stages-can-strand-capacity). Remedies: +- **Delete the co-located compute container**, when the report names it as the cause of a growth + failure — the operator re-places it once the drive container has grown into the freed headroom. If + weka refuses the deactivation because active compute would drop too low, add compute capacity + elsewhere first. Only applies to a growth blocked this way, not to a fresh create. See + [Relaxing a pin in stages can strand capacity](#relaxing-a-pin-in-stages-can-strand-capacity). - **Free resources on the node** — evict or resize whatever else is holding its CPU, hugepages or memory. - **Pin `driveCores` lower.** This is usually the right answer: drives are decoupled from cores, so @@ -660,6 +670,46 @@ count, cores, or the number of containers, no matter what changes on the cluster current `numDrives`/cores; removal is handled by the normal container-deactivation flows, not by this mode's sizing logic. +### Relaxing a pin in stages can strand capacity + +**Relax `driveCores` and `numDrives` together, in one patch.** Relaxing them in separate patches can +strand a node permanently: a compute container is sized against what its co-located drive container +needs **right now**, and compute cores and hugepages only ever rise (see +[Compute sizing](#compute-sizing)) — so one placed or grown while the drive container is still pinned +small takes headroom that never comes back, and the drive container's later growth then fails +`AutoFullDrivesInfeasible` for good, even though every reconcile along the way was locally correct. +Dropping both pins at once leaves no intermediate reconcile: the drive walk charges the container's +fully-grown footprint against the node before compute is ever sized against the remainder. + +For example, on a node with 60,000 MiB of hugepages and 6 signed drives: a drive container pinned at +`numDrives: 4, driveCores: 2` (4 drives / 2 cores / 3,728 MiB) shares the node with no compute container +yet. Dropping only the `driveCores` pin grows it to 4/4/6,656 MiB and, in the same reconcile, sizes a +*new* compute container against that 4-drive state — as large as the remaining headroom allows (17 +cores / 52,088 MiB, leaving 1,256 MiB free). Dropping the `numDrives` pin next asks the drive container +to grow again, to the node's full 6 signed drives — which needs 9,984 MiB — but only 1,256 MiB is free, +and nothing ever shrinks to make room. The plan is `AutoFullDrivesInfeasible` from then on. + +**Recovery:** delete the `WekaContainer` for this cluster's compute container on the affected node — +one node at a time — and let the operator re-place it. The drive walk runs before compute sizing +within a single reconcile, so with the compute container gone the drive container grows first and the +replacement is sized against what is left over. + +Weka gates the deactivation on how much **active** compute the cluster keeps: if removing this +container would leave too few compute processes for the buckets already allocated, weka refuses it and +the `WekaContainer` sits in `Deleting` while the operator retries. Whether it refuses depends on the +bucket count fixed when the cluster started IO, so it varies between otherwise identical clusters. When +it does refuse, add compute capacity elsewhere first, then retry: + +- **a new compute container on a spare compute-eligible node** — widen `roleNodeSelector.compute` to a + node that has none. A new container gets a new pod, so its cores are active as soon as it joins. +- **or grow an existing compute container and recreate its pod.** Growth alone is not enough: raising + cores changes the spec but not the running pod, so the extra cores are not active until the pod is + recreated (see [Pod-restart caveat](#pod-restart-caveat)). + +The node whose drive growth is blocked is **deferred**, not failed, while its compute container is +being deleted — so the rest of the fleet keeps growing and compute keeps being planned meanwhile, +which is what makes the added capacity available at all. + ### Pod-restart caveat Raising an existing container's core count changes the `WekaContainer` **spec**, but it does **not** @@ -750,7 +800,10 @@ surplus drives stranded on purpose, reported as a Normal event. This applies to the modes where `numDrives` sizes the containers directly — explicit container counts, and `numDrives` + `driveCapacity` — not to the daemonset mode, where `numDrives` is a pin the planner -owns and a change is carried by [Expand-only reconciliation](#expand-only-reconciliation) instead. +owns and a change is carried by [Expand-only reconciliation](#expand-only-reconciliation) instead. In +the daemonset mode, relaxing that pin in stages — rather than in the same patch as `driveCores` — can +carry only partially, and permanently so; see +[Relaxing a pin in stages can strand capacity](#relaxing-a-pin-in-stages-can-strand-capacity). Raising `numDrives` propagates to the drive containers that already exist. Their drive count, cores and hugepages are rewritten together, and the change is **increase-only**: lowering `numDrives` leaves @@ -851,17 +904,24 @@ Every reason below lands on the **`WekaCluster`** except `UnschedulableDriveCont **`WekaContainer`** — so `kubectl describe wekacluster ` alone will not show them. Check `kubectl describe wekacontainer ` when you need per-container detail. -Events are throttled per reason, on the reason alone — the message is not part of the key. A repeat -within the window is dropped rather than re-posted. The advisories that describe a **converged** state -use a 15-minute window, because a permanently compute-limited cluster is healthy and re-posting a -Warning every minute forever trips alerting. The three fleet-wide aggregates — `DrivesStranded`, -`PlacementDeferred`, `NodeIneligible` — instead use 3 minutes: each names every affected node in one -message, and because the key ignores that message, the window also bounds how long a node that joins -the set *after* the last event stays unreported. - -Planner warnings are split into **one reason per cause**, so +Each planner warning has a **kind**, which selects its reason, and within a reason a bounded +**cause**, which says which of that reason's conditions this particular event describes. Events are +throttled on reason plus cause — not on the full message, which for the fleet-wide aggregates below +varies with the affected node set and is deliberately not part of the key. A repeat of the same +reason *and* cause within the window is dropped; a *different* cause under the same reason is posted +at once, since it is a different key. The advisories that describe a **converged** state use a +15-minute window, because a permanently compute-limited cluster is healthy and re-posting a Warning +every minute forever trips alerting. The three fleet-wide aggregates — `DrivesStranded`, +`PlacementDeferred`, `NodeIneligible` — instead use 3 minutes: each names every node hit by one +cause in one message. The node set is not part of the key, so a node joining or leaving that set +under a cause that already fired still waits out the window before it is named — deliberately: +keying on the set too would make a node flapping `Ready`/`NotReady` re-fire the event on every flip, +which is the worst moment for extra event volume. That is added latency, not lost information — +whenever the event does fire, it names the complete current set. + +Planner warnings are split into **one reason per kind**, so `kubectl get events --field-selector reason=AutoFullDrivesInfeasible` isolates the actionable ones -without matching message text, and the causes that are not problems are Normal rather than Warning. +without matching message text, and the kinds that are not problems are Normal rather than Warning. | Reason | Type | Object | Throttle | When it fires | |--------|------|--------|----------|---------------| @@ -869,11 +929,11 @@ without matching message text, and the causes that are not problems are Normal r | `AutoFullDrivesGrowthDetected` | Normal | Cluster | 1 min | Growth was **applied** to ≥1 existing drive container — `numDrives`/cores actually written to the spec. It names each container, its node, and its new drives/cores, and says when a pod recreation is owed. It does not report growth the planner proposed but did not commit. | | `AutoFullDrivesGrowthDeferred` | Warning | Cluster | 15 min | Growth was planned but **none** of it could be applied because an update failed. The operator retries on the next reconcile, but a later plan may no longer offer the same growth (node headroom changes as pods schedule). | | `AutoFullDrivesDrivesStranded` | Normal | Cluster | 3 min | A pinned `numDrives` leaves signed drives unused. One aggregated message covering the whole fleet, listing each node as *used of signed*. **Expected** whenever the pin is in force — it is Normal precisely because you asked for it. Raise or drop `numDrives` to use them. | -| `AutoFullDrivesPlacementDeferred` | Normal | Cluster | 3 min | Placement is waiting this pass. Usually one aggregated message covering every node affected, for either of two per-node causes: a node still hosts a this-cluster drive container that is being deleted, or an existing container's growth is deferred because its pod is not yet scheduled. It also carries the fleet-wide case, where *every* signed drive is still held by containers being deleted so planning cannot start at all — there the drives are signed, merely not released yet. Clears itself. | -| `AutoFullDrivesNodeIneligible` | Normal | Cluster | 3 min | A node matching the drive-role selector is cordoned, `NotReady`, or carries an untolerated taint, so it gets no **new** container. Normal rather than Warning because on its own it costs nothing — the plan proceeds on the remaining nodes, and if the loss actually matters the plan goes infeasible and `AutoFullDrivesInfeasible` says so. All ineligible nodes arrive in one message, each with its own reason (e.g. `cordoned`) — so a node cordoned after the last event is reported within one window rather than waiting out a long one. Anything already running there keeps running and still grows. See [Troubleshooting](#troubleshooting). | +| `AutoFullDrivesPlacementDeferred` | Normal | Cluster | 3 min | Placement is waiting this pass, for one of four causes, each its own message and its own throttle key so one cause firing never silences another: an existing container's growth is deferred because its pod is not yet scheduled; a node still hosts a this-cluster drive container that is being deleted; a node hosts a this-cluster compute container that is being deleted and holds what the pending placement needs — a create as readily as a growth, and the message names the binding dimension (cores, hugepages or memory) only when every node hit by this cause is short of the same one; or, fleet-wide, *every* signed drive is still held by containers being deleted so planning cannot start at all — there the drives are signed, merely not released yet. A pass can therefore emit up to three of these events for the per-node causes, each naming every node hit by that one cause, plus the fleet-wide one as a fourth. Clears itself. | +| `AutoFullDrivesNodeIneligible` | Normal | Cluster | 3 min | A node matching the drive-role selector is cordoned, `NotReady`, or carries an untolerated taint, so it gets no **new** container. Normal rather than Warning because on its own it costs nothing — the plan proceeds on the remaining nodes, and if the loss actually matters the plan goes infeasible and `AutoFullDrivesInfeasible` says so. All currently-ineligible nodes arrive in one message, each still carrying its own reason inline (e.g. `cordoned`), but the throttle cause is the *set* of distinct reasons present this pass — so a node going `NotReady` changes the cause and is reported at once even while a separately cordoned node's event is still inside its own window, instead of one reason masking the other. Anything already running there keeps running and still grows. See [Troubleshooting](#troubleshooting). | | `AutoFullDrivesComputeLayout` | Warning | Cluster | 15 min | Every compute-sizing advisory from the shared compute layout step, joined into one message per pass. | -| `AutoFullDrivesWarning` | Warning | Cluster | 15 min | Fallback only: a planner warning whose cause has no dedicated reason yet. | -| `AutoFullDrivesInfeasible` | Warning | Cluster | 1 min | The plan can't proceed and **nothing is created**. Causes: a node that cannot fit a container sized for all its drives (named, with the binding dimension and needed-vs-available), `driveCores` pinned above a node's drive count, `numDrives` pinned above a node's signed count, or not enough compute capacity for the ratio. The message names the binding reason and suggested fixes. | +| `AutoFullDrivesWarning` | Warning | Cluster | 15 min | Fallback only: a planner warning whose kind has no dedicated reason yet. | +| `AutoFullDrivesInfeasible` | Warning | Cluster | 1 min | The plan can't proceed and **nothing is created**. Triggers: a node that cannot fit a container sized for all its drives (named, with the binding dimension and needed-vs-available), `driveCores` pinned above a node's drive count, `numDrives` pinned above a node's signed count, not enough compute capacity for the ratio, or a growth blocked by headroom a co-located compute container holds (see [Relaxing a pin in stages can strand capacity](#relaxing-a-pin-in-stages-can-strand-capacity)). The message names the binding reason and, for a growth blocked this way, the remedy. The full remedy catalog travels in the structured report rather than the event — `weka-capacity plan` renders it. | | `AutoFullDrivesNoSignedDrives` | Normal | Cluster | 1 min | No node matching the drive-role selector has a signed, non-blocked full drive yet. Planning is deferred; sign drives and the operator picks them up on its own. Drives held by a container being deleted are *not* this case — see `AutoFullDrivesPlacementDeferred`. | | `UnschedulableDriveContainer` | Warning | **Container** | none | A node-pinned drive container **whose pod never bound** was deleted, so its capacity can be re-placed, after the scheduler had been reporting `PodScheduled=False`/`Reason=Unschedulable` for longer than the GC timeout. The message carries the scheduler's own explanation. A pod still `Pending` for another reason (e.g. a slow DKMS build) is left alone. See [Troubleshooting](#troubleshooting). | | `UnschedulableComputeContainer` | Warning | **Container** | none | The same, for a compute container. See [Troubleshooting](#troubleshooting). | @@ -933,11 +993,13 @@ fails earlier, on the CRD's [both-or-neither rule](#the-both-or-neither-rule), w message. See [Changing sizing mode on a live cluster](#changing-sizing-mode-on-a-live-cluster). **`AutoFullDrivesInfeasible` fires and nothing is created.** -The message names the binding cause. The four to expect: a node that cannot fit a container sized for +The message names the binding cause. The five to expect: a node that cannot fit a container sized for all of its drives ([details and remedies](#when-a-node-cannot-fit-its-drives)); `driveCores` pinned above a node's drive count; `numDrives` pinned above a node's signed count -([details](#numdrives-as-a-per-node-override)); or not enough compute capacity for the -[ratio](#compute-sizing). Remember that **one** bad node is enough to block the whole cluster. +([details](#numdrives-as-a-per-node-override)); not enough compute capacity for the +[ratio](#compute-sizing); or a drive-container growth blocked by headroom a co-located compute +container holds ([details](#relaxing-a-pin-in-stages-can-strand-capacity)). Remember that **one** bad +node is enough to block the whole cluster. **A newly added node isn't getting a drive container.** Confirm the node matches the drive-role `nodeSelector` and has signed full drives. Reconciliation is diff --git a/internal/capacityplanner/autofulldrives.go b/internal/capacityplanner/autofulldrives.go index e7cd44a51..bd1e45adc 100644 --- a/internal/capacityplanner/autofulldrives.go +++ b/internal/capacityplanner/autofulldrives.go @@ -17,6 +17,9 @@ type autoNode struct { nc NodeCapacity // existing is this cluster's drive container on the node, or nil when there is none yet. existing *ExistingContainer + // ownCompute is whether this cluster also runs a compute container here. Only the growth-hazard + // diagnostic needs it: its remedy names a container to delete, so it may not be offered otherwise. + ownCompute bool // free is the node's unallocated drives; all is own+free — both descending, which is what makes a // numDrives pin take the largest drives. free []int @@ -25,20 +28,29 @@ type autoNode struct { // autoFullDrivesNodes indexes the inventory into name-sorted autoNodes, so Create/Grow/Warnings ordering is // deterministic and a fleet always plans the same way twice. -func autoFullDrivesNodes(existingDrives []ExistingContainer, inventory []NodeCapacity) []autoNode { +func autoFullDrivesNodes( + existingDrives []ExistingContainer, existingCompute []ExistingComputeContainer, inventory []NodeCapacity, +) []autoNode { byNode := make(map[string]*ExistingContainer, len(existingDrives)) for i := range existingDrives { if existingDrives[i].Node != "" { byNode[existingDrives[i].Node] = &existingDrives[i] } } + computeByNode := make(map[string]bool, len(existingCompute)) + for _, ec := range existingCompute { + if ec.Node != "" { + computeByNode[ec.Node] = true + } + } nodes := make([]autoNode, 0, len(inventory)) for i := range inventory { nc := inventory[i] nodes = append(nodes, autoNode{ - nc: nc, - existing: byNode[nc.NodeName], - free: SortDriveCapacitiesDesc(nc.DriveCapacitiesGiB), + nc: nc, + existing: byNode[nc.NodeName], + ownCompute: computeByNode[nc.NodeName], + free: SortDriveCapacitiesDesc(nc.DriveCapacitiesGiB), all: SortDriveCapacitiesDesc( append(append([]int(nil), nc.OwnDriveCapacitiesGiB...), nc.DriveCapacitiesGiB...)), }) @@ -59,7 +71,7 @@ func PlanAutoFullDrives( computeNodes map[string]bool, cons *CapacityConstraints, ) CapacityPlan { - nodes := autoFullDrivesNodes(existingDrives, inventory) + nodes := autoFullDrivesNodes(existingDrives, existingCompute, inventory) plan, totals, remaining := planAutoFullDrivesDrives(desired, nodes, cons) // Set above the feasibility gate so an infeasible plan can still report the core demand its claimed drives @@ -109,10 +121,13 @@ func planAutoFullDrivesDrives( // same condition (a cordoned node, an unscheduled pod) commonly hits several nodes in one pass. var stranded []strandedNode var failures []autoFitFailure - var ineligible []string // "h1-2-a (cordoned)" - var ineligibleDrives int // free signed drives on those nodes, for formatIneligibleWarning's total - var deferred []string // nodes whose existing container's pod is unscheduled - var deleting []string // nodes with HasDeletingDriveContainer + var ineligible []string // "h1-2-a (cordoned)" + var ineligibleDrives int // free signed drives on those nodes, for formatIneligibleWarning's total + ineligibleReasons := map[string]bool{} // distinct IneligibleReason values seen, for the NodeIneligible Cause + var deferred []string // nodes whose existing container's pod is unscheduled + var deleting []string // nodes with HasDeletingDriveContainer + var computeBlocked []string // nodes whose fit failed because HasDeletingComputeContainer holds what it needs + computeBlockedBindings := map[string]bool{} // distinct fit.binding values among them, for the warning's wording // Called on both exits. The infeasibility check below returns from inside the walk, and a condition // already collected is still true of the plan that return carries — the CLI's per-node NOTE column points @@ -122,10 +137,24 @@ func planAutoFullDrivesDrives( plan.Warnings = append(plan.Warnings, formatStrandedWarning(stranded, desired.NumDrives)) } if len(ineligible) > 0 { - plan.Warnings = append(plan.Warnings, formatIneligibleWarning(ineligible, ineligibleDrives)) + reasons := make([]string, 0, len(ineligibleReasons)) + for r := range ineligibleReasons { + reasons = append(reasons, r) + } + sort.Strings(reasons) + plan.Warnings = append(plan.Warnings, formatIneligibleWarning(ineligible, ineligibleDrives, reasons)) } - if len(deferred) > 0 || len(deleting) > 0 { - plan.Warnings = append(plan.Warnings, formatPlacementDeferredWarning(deferred, deleting)) + if len(deferred) > 0 || len(deleting) > 0 || len(computeBlocked) > 0 { + // Name the blocked dimension only when every node agrees on it, the same rule autoNodeFitInfeasible + // uses for Binding — one node's cause must not stand for the rest. + binding := "" + if len(computeBlockedBindings) == 1 { + for b := range computeBlockedBindings { + binding = b + } + } + plan.Warnings = append(plan.Warnings, + formatPlacementDeferredWarning(deferred, deleting, computeBlocked, binding)...) } } @@ -155,6 +184,7 @@ func planAutoFullDrivesDrives( totals.tlcGiBAvailable += sumInts(drives) ineligible = append(ineligible, fmt.Sprintf("%s (%s)", name, n.nc.IneligibleReason)) ineligibleDrives += len(drives) + ineligibleReasons[n.nc.IneligibleReason] = true continue } // The one per-node skip that is a skip rather than an infeasibility: it clears itself, so failing @@ -186,17 +216,49 @@ func planAutoFullDrivesDrives( to := autoFootprint{cores: max(cur.cores, np.cores), drives: max(cur.drives, np.numDrives())} newTlcGiB := max(curTlcGiB, np.tlcGiB()) - // An unscheduled container's cores charge at their frozen value here, matching that its Grow entry - // never gets written below — the ratcheted `to.cores` is what growth would apply, not what is - // actually taken while the pod sits unscheduled. + unscheduled := n.existing != nil && n.existing.Unscheduled + + // The fit runs before the totals because what this node charges depends on whether the walk will skip + // it below, and only the fit can tell. Never for an unscheduled node: that one is skipped either way, + // and fitting it could only manufacture an infeasibility. + var fit autoFitResult + if !unscheduled { + fit = autoNodeFit(&n.nc, cur, to, cons) + } + // !unscheduled because that node never ran a fit: its zero-valued result reads as a failure, which + // would pull it into the compute-blocked charging convention instead of the unscheduled one. + blockedByDeletingCompute := !unscheduled && !fit.ok && n.nc.HasDeletingComputeContainer + + // A node that either skip below leaves alone charges the cores it is actually running: its Grow entry + // never gets written, so the ratcheted `to.cores` is what growth would apply, not what is taken. + // Charging the target sizes compute against cores that do not exist, which can flip the plan + // infeasible — and an infeasible plan applies nothing, including the growth the other nodes earned. chargedCores := to.cores - if n.existing != nil && n.existing.Unscheduled { + if unscheduled || blockedByDeletingCompute { chargedCores = cur.cores } - totals.drivesTaken += to.drives + // Drives and TLC freeze for a compute-blocked node only; an unscheduled one keeps charging its planned + // figure (TestPlanAutoFullDrives_UnscheduledDriveContainer_ComputeCountsPlannedNotFrozenCapacity pins + // that). The difference is causal, not cosmetic: tlcGiBTaken sizes compute hugepages, so charging + // capacity this pass will not create raises compute demand on the very node whose growth compute is + // already blocking. An unscheduled pod is only waiting on the scheduler, and pre-sizing compute for the + // drives it will bring costs nothing. + chargedDrives, chargedTlcGiB := to.drives, newTlcGiB + if blockedByDeletingCompute { + // cur.drives is the count the container holds, but ExistingContainer.TlcGiB is structurally 0 on an + // auto-full-drives container — the mode is defined by driveCapacity and containerCapacity both being + // unset, which is all DriveContainerCapacities reads. Its capacity comes from the node's own-drive + // split instead, the same fallback the CLI's NODES table uses, and is 0 on the create path. + chargedDrives, chargedTlcGiB = cur.drives, 0 + if n.existing != nil { + chargedTlcGiB = sumInts(n.nc.OwnDriveCapacitiesGiB) + } + } + + totals.drivesTaken += chargedDrives totals.drivesAvailable += len(drives) - totals.tlcGiBTaken += newTlcGiB + totals.tlcGiBTaken += chargedTlcGiB totals.tlcGiBAvailable += sumInts(drives) totals.driveCoresTaken += chargedCores @@ -208,21 +270,30 @@ func planAutoFullDrivesDrives( } // An unscheduled pod holds no node resources to grow into, and raising its spec would only make it - // harder to schedule. Skipped after the totals so the fleet accounting still reflects its drives, and - // before the fit so it cannot manufacture an infeasibility. - if n.existing != nil && n.existing.Unscheduled { + // harder to schedule. Skipped after the totals so the fleet accounting still reflects its drives. + if unscheduled { deferred = append(deferred, name) continue } - fit := autoNodeFit(&n.nc, cur, to, cons) if !fit.ok { - kind := "create" + // A deleting compute container on this node still holds the hugepages the fit needs, but that + // clears itself once the deletion lands — and failing the plan here is exactly what would stop + // compute from ever being re-planned, which is the capacity weka needs before it will let the + // deactivation through (see PlanAutoFullDrives's infeasibility gate). Deferred, not infeasible, + // same as the deleting-drive-container skip above. + if blockedByDeletingCompute { + computeBlocked = append(computeBlocked, name) + computeBlockedBindings[fit.binding] = true + continue + } + kind := fitKindCreate if n.existing != nil { - kind = "growth" + kind = fitKindGrowth } failures = append(failures, autoFitFailure{ node: name, kind: kind, numDrives: to.drives, toCores: to.cores, fit: fit, + ownCompute: n.ownCompute, }) continue } diff --git a/internal/capacityplanner/autofulldrives_compute.go b/internal/capacityplanner/autofulldrives_compute.go index 113415188..064274ba5 100644 --- a/internal/capacityplanner/autofulldrives_compute.go +++ b/internal/capacityplanner/autofulldrives_compute.go @@ -139,24 +139,25 @@ func planComputeAutoFullDrives(in *autoComputeInput, plan *CapacityPlan) { // Prefer new containers, top up with in-place growth: the least growth that lets the rest fit on free // nodes. probe reports whether a given growTake is coverable, deriving the layout for the target it // leaves; it has no side effects, so it is safe to call for candidates that are never committed. - probe := func(growTake int) (computeProbe, string) { + probe := func(growTake int) (computeProbe, string, string) { // Growth alone closes it, so there is nothing to derive — and deriving against a zero target would // resurrect the phantom-container case guarded above. growTake==0 is excluded: a zero deficit there // still owes the derivation its floor containers. if deficit-growTake <= 0 && growTake > 0 { - return computeProbe{growthAlone: true}, "" + return computeProbe{growthAlone: true}, "", "" } // specCount is hard 0: a pinned computeContainers means the cluster is not in this mode at all. - count, cores, infeasible, warnings := deriveComputeLayout( + count, cores, infeasible, binding, warnings := deriveComputeLayout( 0, in.desired.ComputeCores, deficit-growTake, floor, in.cons.MaxCoresPerContainer, coreHeadroom, nodeHugepagesMiB, hugepagesFor, ) - return computeProbe{count: count, cores: cores, warnings: warnings}, infeasible + return computeProbe{count: count, cores: cores, warnings: warnings}, infeasible, binding } // unaidedReason is the growTake==0 attempt's own explanation (usually the binding resource, often compute - // hugepages) for why new containers alone do not fit, and leads the infeasibility below. - best, unaidedReason := probe(0) + // hugepages) for why new containers alone do not fit, and leads the infeasibility below; unaidedBinding is + // its structured classification, carried through so the caller never re-parses the English reason. + best, unaidedReason, unaidedBinding := probe(0) growTake := 0 if unaidedReason != "" { // The search space is [1, hi]: hi is where growth alone would close the deficit, or all the growth @@ -172,7 +173,7 @@ func planComputeAutoFullDrives(in *autoComputeInput, plan *CapacityPlan) { // len(kept)*MaxCoresPerContainer and every probe re-derives the whole layout. for lo := 1; lo <= hi; { mid := lo + (hi-lo)/2 - if p, infeasible := probe(mid); infeasible == "" { + if p, infeasible, _ := probe(mid); infeasible == "" { best, growTake, found = p, mid, true hi = mid - 1 // a smaller growTake may also cover it — keep the least } else { @@ -185,7 +186,7 @@ func planComputeAutoFullDrives(in *autoComputeInput, plan *CapacityPlan) { // container count — rises as it shrinks. Coverability can therefore hold on an interval and fail // above it, which a binary search would walk away from. Scan. for g := 1; g <= hi; g++ { - if p, infeasible := probe(g); infeasible == "" { + if p, infeasible, _ := probe(g); infeasible == "" { best, growTake, found = p, g, true break } @@ -199,10 +200,14 @@ func planComputeAutoFullDrives(in *autoComputeInput, plan *CapacityPlan) { "; growing the %d existing compute container(s) in place offers only %d more core(s), "+ "which does not close the %d-core shortfall", len(kept), growTotal, deficit) } + reason += drainingComputeClause(in) setInfeasible(plan, &InfeasibilityReport{ - Reason: reason, - Pool: "compute", - Fixes: fixesAutoFullDrivesCompute(in.cons), + Reason: reason, + Pool: "compute", + Binding: unaidedBinding, + // ShortfallGiB stays 0: the deficit here is in cores or MiB-hugepages, never GiB, and + // converting either into GiB would invent a number this report never measured. + Fixes: fixesAutoFullDrivesCompute(in.cons), }) return } @@ -283,10 +288,30 @@ func autoKeptCompute(in *autoComputeInput) (kept []autoComputeEntry, pinned map[ return kept, pinned, keptCores } -// autoComputeGrowHeadroom is how many extra data cores ec's node can absorb in place, bounded by -// MaxCoresPerContainer and the node's remaining CPU/hugepages/memory. All bounds are deltas: the container's -// current footprint is already charged against remaining. The hugepages bound has no closed form, so the -// candidate size walks down until the delta fits. +// drainingComputeClause names nodes whose compute headroom is depressed by a compute container of this +// cluster that is pending deletion, for appending to a shortfall report. Wording only: the planner cannot +// tell whether that reservation returning would actually close the gap, so the shortfall stays infeasible — +// but which of "wait" or "intervene" is right turns on this fact, and nothing else in the report carries it. +func drainingComputeClause(in *autoComputeInput) string { + var nodes []string + for node, nc := range in.remaining { + if nc.HasDeletingComputeContainer { + nodes = append(nodes, node) + } + } + if len(nodes) == 0 { + return "" + } + sort.Strings(nodes) + return fmt.Sprintf( + "; a compute container of this cluster is still being deleted on %s, so this shortfall may clear on "+ + "its own once that deletion lands", listNodes(nodes)) +} + +// autoComputeGrowHeadroom is how many extra data cores ec's node can absorb in place, bounded by a pinned +// computeCores, MaxCoresPerContainer and the node's remaining CPU/hugepages/memory. All bounds are deltas: +// the container's current footprint is already charged against remaining. The hugepages bound has no closed +// form, so the candidate size walks down until the delta fits. func autoComputeGrowHeadroom(ec *ExistingComputeContainer, nc *NodeCapacity, keptCount int, in *autoComputeInput) int { // includeBase=false throughout: the management core and memory base are already reserved by the running // container, so only the per-core increments are charged. @@ -294,6 +319,12 @@ func autoComputeGrowHeadroom(ec *ExistingComputeContainer, nc *NodeCapacity, kep if in.cons.MaxCoresPerContainer > 0 { maxCores = min(maxCores, in.cons.MaxCoresPerContainer) } + // A pinned computeCores is every compute container's exact size, which deriveComputeLayout honors for new + // containers; growth stops there too, so a shortfall new containers cannot place is reported rather than + // absorbed by an oversized survivor. + if in.desired.ComputeCores > 0 { + maxCores = min(maxCores, in.desired.ComputeCores) + } if in.cons.MemoryPerCoreMiB > 0 { maxCores = min(maxCores, ec.NumCores+nc.AvailableMemoryMiB/in.cons.MemoryPerCoreMiB) } @@ -442,8 +473,8 @@ func autoPlaceNewCompute( setInfeasible(plan, &InfeasibilityReport{ Reason: fmt.Sprintf( "compute: cannot place %d new compute container(s) to cover the %d-core shortfall — "+ - "only %d free fitting compute node(s) (each holds up to %d cores + %d MiB hugepages)", - count, shortfall, len(candidates), cores, perContainerHP), + "only %d free fitting compute node(s) (each holds up to %d cores + %d MiB hugepages)%s", + count, shortfall, len(candidates), cores, perContainerHP, drainingComputeClause(in)), Pool: "compute", Binding: "cores", Fixes: fixesAutoFullDrivesCompute(in.cons), diff --git a/internal/capacityplanner/autofulldrives_fit.go b/internal/capacityplanner/autofulldrives_fit.go index ea4c5cc44..203319808 100644 --- a/internal/capacityplanner/autofulldrives_fit.go +++ b/internal/capacityplanner/autofulldrives_fit.go @@ -12,6 +12,14 @@ const ( bindingMemory = "memory" ) +// fitKindCreate and fitKindGrowth classify an autoFitFailure: a create failure has no existing container to +// grow from, a growth failure does. Constants, not string literals, so a typo can't silently disable the +// growth-hazard diagnostic that keys off this value. +const ( + fitKindCreate = "create" + fitKindGrowth = "growth" +) + // autoFootprint is what a drive container is on a node: the cores it runs and the drives it holds. The zero // value means "nothing there yet", which is the create path. type autoFootprint struct { @@ -102,8 +110,11 @@ func chargeFit(nc *NodeCapacity, cost autoFitCost) { // whole walk so the infeasibility names every offender rather than the first one reached. type autoFitFailure struct { node string - kind string // "create" | "growth" + kind string // fitKindCreate | fitKindGrowth numDrives int toCores int fit autoFitResult + // ownCompute is whether this cluster runs a compute container on the node, which is what makes the + // growth-hazard diagnostic applicable — its remedy is to delete that container. + ownCompute bool } diff --git a/internal/capacityplanner/autofulldrives_test.go b/internal/capacityplanner/autofulldrives_test.go index dd0ead6b8..7c55ccd54 100644 --- a/internal/capacityplanner/autofulldrives_test.go +++ b/internal/capacityplanner/autofulldrives_test.go @@ -676,6 +676,99 @@ func TestPlanAutoFullDrives_ComputeLayout_HugepagesBound_MoreSmallerContainers(t } } +// TestPlanAutoFullDrives_ComputeLayout_HugepagesBound_Infeasible_ReportsBinding reproduces the live bug +// (weka-capacity dry-run): an auto-full-drives compute-layout infeasibility that is entirely a hugepages +// shortfall must carry Binding="hugepages" in the structured report, not the empty string, or a consumer +// of InfeasibilityReport sees no binding and no shortfall and concludes nothing is wrong. +func TestPlanAutoFullDrives_ComputeLayout_HugepagesBound_Infeasible_ReportsBinding(t *testing.T) { + cons := testCons() + cons.FullDrivesComputeToDriveCoreRatio = 0 + const bigFree = 1 << 28 + + var inv []NodeCapacity + for i := 1; i <= 6; i++ { + inv = append(inv, NodeCapacity{ + NodeName: "d" + itoa(i), FDValue: "d" + itoa(i), + DriveCapacitiesGiB: uniformDrives(15, 100), TlcGiB: 1500, + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }) + } + // 8 compute-only nodes, ample cores (64) so cores never bind; hugepages split 6-high/2-low so the + // per-container bar (3000 MiB/core * 12 cores = 36000 MiB) is met by only 6 of the 8 nodes, mirroring + // the observed "only 6 of 8 nodes meet the ... bar" shape. + for i := 1; i <= 6; i++ { + inv = append(inv, tightNode("c"+itoa(i), 0, 64, 40000)) + } + for i := 7; i <= 8; i++ { + inv = append(inv, tightNode("c"+itoa(i), 0, 64, 10000)) + } + computeNodes := computeNodeSet("c1", "c2", "c3", "c4", "c5", "c6", "c7", "c8") + desired := AutoFullDrivesDesired{DriveCores: 15} + + plan := PlanAutoFullDrives(desired, nil, nil, inv, computeNodes, cons) + + if plan.Infeasible == "" { + t.Fatalf("expected infeasible (hugepages bar met by only 6 of 8 compute nodes), got feasible plan: %+v", plan) + } + if !strings.Contains(plan.Infeasible, "hugepages insufficient for") || !strings.Contains(plan.Infeasible, "only 6 of 8 nodes") { + t.Fatalf("Infeasible = %q, want it to name the hugepages shortfall and the 6-of-8 node count", plan.Infeasible) + } + if plan.Infeasibility.Pool != "compute" { + t.Errorf("Infeasibility.Pool = %q, want %q", plan.Infeasibility.Pool, "compute") + } + if plan.Infeasibility.Binding != bindingHugepages { + t.Errorf("Infeasibility.Binding = %q, want %q", plan.Infeasibility.Binding, bindingHugepages) + } + // The shortfall here is in cores/MiB, never GiB; ShortfallGiB must stay 0 rather than invent a + // lossy MiB->GiB conversion. + if plan.Infeasibility.ShortfallGiB != 0 { + t.Errorf("Infeasibility.ShortfallGiB = %d, want 0 (hugepages shortfall is not GiB-quantifiable here)", plan.Infeasibility.ShortfallGiB) + } +} + +// TestPlanAutoFullDrives_ComputeLayout_CoresBound_Infeasible_ReportsBinding is the cores-bound sibling of +// the hugepages test above: compute nodes have ample hugepages but too few cores, so Binding must read +// "cores", not "hugepages" or "". +func TestPlanAutoFullDrives_ComputeLayout_CoresBound_Infeasible_ReportsBinding(t *testing.T) { + cons := testCons() + cons.FullDrivesComputeToDriveCoreRatio = 0 + const bigFree = 1 << 28 + + var inv []NodeCapacity + for i := 1; i <= 6; i++ { + inv = append(inv, NodeCapacity{ + NodeName: "d" + itoa(i), FDValue: "d" + itoa(i), + DriveCapacitiesGiB: uniformDrives(15, 100), TlcGiB: 1500, + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }) + } + // 8 compute-only nodes with only 5 allocatable CPUs each (4 data cores after the non-HT base charge) + // but effectively unlimited hugepages, so cores alone bind the layout. + for i := 1; i <= 8; i++ { + inv = append(inv, tightNode("c"+itoa(i), 0, 5, bigFree)) + } + computeNodes := computeNodeSet("c1", "c2", "c3", "c4", "c5", "c6", "c7", "c8") + desired := AutoFullDrivesDesired{DriveCores: 15} + + plan := PlanAutoFullDrives(desired, nil, nil, inv, computeNodes, cons) + + if plan.Infeasible == "" { + t.Fatalf("expected infeasible (4 data cores/node can't cover the 90-core compute:drive requirement), got feasible plan: %+v", plan) + } + if !strings.Contains(plan.Infeasible, "hugepages are sufficient") { + t.Fatalf("Infeasible = %q, want it to attribute the failure to cores alone", plan.Infeasible) + } + if plan.Infeasibility.Pool != "compute" { + t.Errorf("Infeasibility.Pool = %q, want %q", plan.Infeasibility.Pool, "compute") + } + if plan.Infeasibility.Binding != bindingCores { + t.Errorf("Infeasibility.Binding = %q, want %q", plan.Infeasibility.Binding, bindingCores) + } + if plan.Infeasibility.ShortfallGiB != 0 { + t.Errorf("Infeasibility.ShortfallGiB = %d, want 0 (core shortfall is not GiB-quantifiable here)", plan.Infeasibility.ShortfallGiB) + } +} + // TestPlanAutoFullDrives_Infeasible_ReportsFullClaimAndNoCompute: an infeasible plan still reports what it // would have claimed, so an operator can see the size of what they are being denied, and carries no compute // layout at all. @@ -2779,6 +2872,15 @@ func TestPlanAutoFullDrives_OneNodeCannotFitFailsWholePlan(t *testing.T) { if u := plan.Infeasibility.RejectedNodes[0].Unit; u == "" { t.Fatal("RejectedNodes must carry a Unit so renderers do not print a CPU count as GiB") } + // n-short has no existing container: this is a create-only failure, so the message must not carry the + // growth-hazard clause (there is no drive container yet for a compute container to have blocked) and the + // fix catalog must not lead with the delete-compute tip. + if strings.Contains(plan.Infeasible, "compute container") { + t.Fatalf("create-only failure must not mention a blocking compute container, got %q", plan.Infeasible) + } + if hasFix(plan.Infeasibility.Fixes, "delete this cluster's compute container") { + t.Fatalf("create-only failure must not suggest deleting a compute container, got %+v", plan.Infeasibility.Fixes) + } } func TestPlanAutoFullDrives_EveryNonFittingNodeIsNamed(t *testing.T) { @@ -3005,9 +3107,11 @@ func TestPlanAutoFullDrives_UnscheduledDriveContainer_FreezesGrowth(t *testing.T } // Both placement-deferral causes (unscheduled pod, container being deleted) map to the same -// AutoFullDrivesPlacementDeferred reason, so a pass hitting both must still produce exactly one warning — -// two would let the event throttle (keyed on reason alone) silently drop one of them. -func TestPlanAutoFullDrives_UnscheduledAndDeletingCauses_MergeIntoOneWarning(t *testing.T) { +// AutoFullDrivesPlacementDeferred reason, but describe genuinely different conditions, so a pass hitting +// both must produce two separate warnings, each carrying its own Cause: merging them into one would leave +// the pair sharing a single throttle window downstream, and a later event for either cause landing inside +// it would be silently dropped just as it was before the per-cause split. +func TestPlanAutoFullDrives_UnscheduledAndDeletingCauses_ProduceSeparateWarnings(t *testing.T) { cons := testCons() const bigFree = 1 << 28 @@ -3048,18 +3152,41 @@ func TestPlanAutoFullDrives_UnscheduledAndDeletingCauses_MergeIntoOneWarning(t * transient = append(transient, w) } } - if len(transient) != 1 { - t.Fatalf("WarningKindTransient warnings = %+v, want exactly 1 covering both causes", transient) + if len(transient) != 2 { + t.Fatalf("WarningKindTransient warnings = %+v, want exactly 2 (one per cause)", transient) + } + + byCause := map[WarningCause]Warning{} + for _, w := range transient { + byCause[w.Cause] = w + } + + unscheduledWarning, ok := byCause[CausePlacementUnscheduled] + if !ok { + t.Fatalf("no WarningKindTransient warning with Cause=%q in %+v", CausePlacementUnscheduled, transient) + } + if !strings.Contains(unscheduledWarning.Message, "unscheduled") { + t.Errorf("unscheduled-cause warning = %q, want it to name node %q", unscheduledWarning.Message, "unscheduled") + } + if strings.Contains(unscheduledWarning.Message, "deleting") { + t.Errorf("unscheduled-cause warning = %q, must not name node %q", unscheduledWarning.Message, "deleting") + } + if !strings.Contains(unscheduledWarning.Message, "it retries automatically") { + t.Errorf("unscheduled-cause warning = %q, want the singular retry clause \"it retries automatically\"", unscheduledWarning.Message) } - w := transient[0] - if !strings.Contains(w.Message, "unscheduled") { - t.Errorf("warning message = %q, want it to name the unscheduled node %q", w.Message, "unscheduled") + + deletingWarning, ok := byCause[CausePlacementDriveDeleting] + if !ok { + t.Fatalf("no WarningKindTransient warning with Cause=%q in %+v", CausePlacementDriveDeleting, transient) + } + if !strings.Contains(deletingWarning.Message, "deleting") { + t.Errorf("deleting-cause warning = %q, want it to name node %q", deletingWarning.Message, "deleting") } - if !strings.Contains(w.Message, "deleting") { - t.Errorf("warning message = %q, want it to name the deleting node %q", w.Message, "deleting") + if strings.Contains(deletingWarning.Message, "unscheduled") { + t.Errorf("deleting-cause warning = %q, must not name node %q", deletingWarning.Message, "unscheduled") } - if !strings.Contains(w.Message, "both retry automatically") { - t.Errorf("warning message = %q, want the merged-causes retry clause \"both retry automatically\"", w.Message) + if !strings.Contains(deletingWarning.Message, "it retries automatically") { + t.Errorf("deleting-cause warning = %q, want the singular retry clause \"it retries automatically\"", deletingWarning.Message) } } @@ -3683,3 +3810,702 @@ func TestPlanAutoFullDrives_ComputeGrowth_KeptHugepagesNeverLowered(t *testing.T } } } + +// --- growth blocked by a compute container placed while the drive container was still small --- +// +// Relaxing sizing pins in three steps (numDrives+driveCores -> driveCores -> no pins) can leave a +// hyperconverged fleet permanently infeasible. At the middle step driveCores is still pinned, so the drive +// container has no pending growth and the walk correctly hands a new compute container all of a node's +// remaining headroom. When the pin later drops, that drive container must grow into headroom compute is +// now holding — compute cores/hugepages only ever rise, so the room is not returned. Every reconcile is +// locally correct; the hazard is purely cross-reconcile ordering. hazardCons/hazardInventory/hazardCarry* +// simulate that reconcile-by-reconcile so the test reproduces it instead of asserting it in the abstract. + +// hazardCons isolates hugepages as the only binding dimension: DriveDpdkPerCoreMiB collapses drive +// hugepages to 1464/core + 200/drive, and zeroing the TLC ratio + compute DPDK term collapses compute +// hugepages to a flat 3000/core, so the fixture's one tuned number (d2's hugepages) is easy to reason about. +func hazardCons() *CapacityConstraints { + c := testCons() + c.DriveDpdkPerCoreMiB = 64 + c.ComputeDpdkPerCoreMiB = 0 + c.ComputeHugepagesTlcRatio = 0 + c.FullDrivesComputeToDriveCoreRatio = 1.0 + c.MaxCoresPerContainer = 19 + return c +} + +// hazardInventory rebuilds the two-node fleet's net headroom from raw node capacity and the containers +// this cluster owns going into a pass — exactly what inventory.FullDrivesInventory does in production +// (nodeHeadroom nets every existing container, drive and compute, out of raw allocatable). d1's larger CPU +// headroom is load-bearing: autoPlaceNewCompute sorts placement candidates by core headroom descending, so +// the fleet's first compute container lands on d1, leaving d2 compute-free at creation. +func hazardInventory(existingDrives []ExistingContainer, existingCompute []ExistingComputeContainer, cons *CapacityConstraints, deletingComputeNodes ...string) []NodeCapacity { + deleting := map[string]bool{} + for _, n := range deletingComputeNodes { + deleting[n] = true + } + nodes := []NodeCapacity{ + {NodeName: "d1", FDValue: "d1", AllocatableCPU: 200, AvailableHugepagesMiB: 1 << 20, AvailableMemoryMiB: 1 << 28}, + {NodeName: "d2", FDValue: "d2", AllocatableCPU: 60, AvailableHugepagesMiB: 20000, AvailableMemoryMiB: 1 << 28}, + } + byNode := map[string]ExistingContainer{} + for _, ec := range existingDrives { + byNode[ec.Node] = ec + } + for i := range nodes { + total := uniformDrives(6, 1000) + ec, owns := byNode[nodes[i].NodeName] + if owns { + nodes[i].OwnDriveCapacitiesGiB, nodes[i].DriveCapacitiesGiB = total[:ec.NumDrives], total[ec.NumDrives:] + nodes[i].TlcGiB = sumInts(total) + nodes[i].AllocatableCPU -= physicalCPUCost(&nodes[i], ec.NumCores, cons, true) + nodes[i].AvailableHugepagesMiB -= DriveContainerHugepagesMiB(ec.NumCores, ec.NumDrives, cons) + nodes[i].AvailableMemoryMiB -= ComputeMemoryFootprintMiB(ec.NumCores, cons) + } else { + nodes[i].DriveCapacitiesGiB = total + nodes[i].TlcGiB = sumInts(total) + } + nodes[i].HasDeletingComputeContainer = deleting[nodes[i].NodeName] + } + return netCompute(nodes, existingCompute, cons) +} + +// hazardCarryDrives folds a pass's Create/Grow into the next pass's existingDrives, the way a controller +// feeds back what it applied. +func hazardCarryDrives(prev []ExistingContainer, plan CapacityPlan) []ExistingContainer { + out := append([]ExistingContainer(nil), prev...) + idx := map[string]int{} + for i, e := range out { + idx[e.Name] = i + } + for _, c := range plan.Create { + name := "drive-" + c.Node + out = append(out, ExistingContainer{Name: name, Node: c.Node, FDValue: c.FDValue, TlcGiB: c.TlcGiB, NumCores: c.NumCores, NumDrives: c.NumDrives}) + idx[name] = len(out) - 1 + } + for _, g := range plan.Grow { + if i, ok := idx[g.Name]; ok { + out[i].TlcGiB, out[i].NumCores, out[i].NumDrives = g.NewTlcGiB, g.NewCores, g.NewNumDrives + } + } + return out +} + +// hazardCarryCompute folds a pass's ComputeLayout into the next pass's existingCompute. The max against +// what was already there is redundant with the ratchet the planner itself enforces (autoCommitComputeGrowth +// / autoRederiveKeptHugepages never hand back a smaller figure than their own input) — kept here anyway so +// the test's own bookkeeping states the invariant it depends on rather than trusting it silently. +func hazardCarryCompute(prev []ExistingComputeContainer, layout []ComputeContainerSpec) []ExistingComputeContainer { + prevByNode := map[string]ExistingComputeContainer{} + for _, ec := range prev { + prevByNode[ec.Node] = ec + } + out := make([]ExistingComputeContainer, 0, len(layout)) + for _, l := range layout { + cores, hp := l.NumCores, l.HugepagesMiB + if p, ok := prevByNode[l.Node]; ok { + cores, hp = max(cores, p.NumCores), max(hp, p.HugepagesMiB) + } + out = append(out, ExistingComputeContainer{Name: "compute-" + l.Node, Node: l.Node, NumCores: cores, HugepagesMiB: hp}) + } + return out +} + +// TestPlanAutoFullDrives_ComputeContainerBlocksLaterDriveGrowth reproduces the hazard end to end: create, +// then relax numDrives, then relax driveCores too (which is where the compute container the create pass +// placed on d2 turns out to matter), then drop every pin. The last step must go infeasible on d2 alone, +// bound on hugepages, with the report explaining a compute container may be holding the room. +// hazardStateAfterPassB drives the fixture through create, pass A (numDrives relaxed) and pass B (driveCores +// also relaxed), asserting each intermediate step along the way, and returns the existingDrives/existingCompute +// state pass C is run against. Factored out so both the blocked-growth test and the recovery test start from +// the same, already-verified state instead of each re-deriving it. +func hazardStateAfterPassB(t *testing.T) ([]ExistingContainer, []ExistingComputeContainer, *CapacityConstraints, map[string]bool) { + t.Helper() + cons := hazardCons() + computeNodes := computeNodeSet("d1", "d2") + + // create: both pins set, 2 drives at 2 cores each. + inv := hazardInventory(nil, nil, cons) + create := PlanAutoFullDrives(AutoFullDrivesDesired{NumDrives: 2, DriveCores: 2}, nil, nil, inv, computeNodes, cons) + if create.Infeasible != "" { + t.Fatalf("create pass infeasible: %s", create.Infeasible) + } + if len(create.Create) != 2 { + t.Fatalf("create pass: want 2 Create (one per node), got %d: %+v", len(create.Create), create.Create) + } + for _, c := range create.Create { + if c.NumDrives != 2 || c.NumCores != 2 || c.TlcGiB != 2000 { + t.Fatalf("create pass %s = %+v, want 2 drives/2 cores/2000 GiB", c.Node, c) + } + } + if create.RequiredComputeCores != 4 { + t.Fatalf("create pass RequiredComputeCores = %d, want 4 (2 nodes x 2 drive cores x ratio 1.0)", create.RequiredComputeCores) + } + if len(create.ComputeLayout) != 1 { + t.Fatalf("create pass: want exactly 1 compute container (d1's headroom wins placement), got %+v", create.ComputeLayout) + } + if create.ComputeLayout[0].Node != "d1" { + t.Fatalf("create pass compute landed on %s, want d1 (higher core headroom)", create.ComputeLayout[0].Node) + } + if create.ComputeLayout[0].NumCores != 4 || create.ComputeLayout[0].HugepagesMiB != 12000 { + t.Fatalf("create pass compute = %+v, want 4 cores/12000 MiB", create.ComputeLayout[0]) + } + + existingDrives := hazardCarryDrives(nil, create) + existingCompute := hazardCarryCompute(nil, create.ComputeLayout) + + // pass A: numDrives relaxed to 4, driveCores still pinned at 2 -> no growth, nothing changes. + inv = hazardInventory(existingDrives, existingCompute, cons) + passA := PlanAutoFullDrives(AutoFullDrivesDesired{NumDrives: 4, DriveCores: 2}, existingDrives, existingCompute, inv, computeNodes, cons) + if passA.Infeasible != "" { + t.Fatalf("pass A infeasible: %s", passA.Infeasible) + } + if len(passA.Create) != 0 { + t.Fatalf("pass A: no new node, want no Create, got %+v", passA.Create) + } + // numDrives relaxed from 2 to 4 grows the drive count on both nodes; driveCores stays pinned at 2, so + // compute demand (and therefore d2's compute-container footprint) does not move. + if len(passA.Grow) != 2 { + t.Fatalf("pass A: want both drive containers to grow their drive count, got %+v", passA.Grow) + } + for _, g := range passA.Grow { + if g.NewNumDrives != 4 || g.NewCores != 2 || g.NewTlcGiB != 4000 { + t.Fatalf("pass A Grow[%s] = %+v, want 4 drives/2 cores/4000 GiB", g.Name, g) + } + } + if passA.RequiredComputeCores != 4 { + t.Fatalf("pass A RequiredComputeCores = %d, want 4 (driveCores still pinned at 2 per node)", passA.RequiredComputeCores) + } + if len(passA.ComputeLayout) != 1 || passA.ComputeLayout[0].NumCores != 4 || passA.ComputeLayout[0].HugepagesMiB != 12000 { + t.Fatalf("pass A: compute deficit is 0, want the single kept container unchanged, got %+v", passA.ComputeLayout) + } + existingDrives = hazardCarryDrives(existingDrives, passA) + existingCompute = hazardCarryCompute(existingCompute, passA.ComputeLayout) + + // pass B: driveCores pin dropped too (still numDrives=4) -> drives grow to 4 cores each. d1's compute + // container is pinned in place; d2 is the only placeable node, so the deficit is covered by a NEW + // container there rather than growing d1's. + inv = hazardInventory(existingDrives, existingCompute, cons) + passB := PlanAutoFullDrives(AutoFullDrivesDesired{NumDrives: 4}, existingDrives, existingCompute, inv, computeNodes, cons) + if passB.Infeasible != "" { + t.Fatalf("pass B infeasible: %s", passB.Infeasible) + } + if len(passB.Grow) != 2 { + t.Fatalf("pass B: want both drive containers to grow, got %+v", passB.Grow) + } + for _, g := range passB.Grow { + if g.NewNumDrives != 4 || g.NewCores != 4 { + t.Fatalf("pass B Grow[%s] = %+v, want 4 drives/4 cores", g.Name, g) + } + } + if len(passB.ComputeLayout) != 2 { + t.Fatalf("pass B: want a second compute container placed (d1's kept, d2's new), got %+v", passB.ComputeLayout) + } + var d2Compute *ComputeContainerSpec + for i := range passB.ComputeLayout { + if passB.ComputeLayout[i].Node == "d2" { + d2Compute = &passB.ComputeLayout[i] + } + } + if d2Compute == nil { + t.Fatalf("pass B: want a compute container on d2, got %+v", passB.ComputeLayout) + } + if d2Compute.NumCores != 4 || d2Compute.HugepagesMiB != 12000 { + t.Fatalf("pass B d2 compute = %+v, want 4 cores/12000 MiB", *d2Compute) + } + + existingDrives = hazardCarryDrives(existingDrives, passB) + existingCompute = hazardCarryCompute(existingCompute, passB.ComputeLayout) + return existingDrives, existingCompute, cons, computeNodes +} + +func TestPlanAutoFullDrives_ComputeContainerBlocksLaterDriveGrowth(t *testing.T) { + existingDrives, existingCompute, cons, computeNodes := hazardStateAfterPassB(t) + + // pass C: every pin dropped -> both drive containers must grow to their full 6 drives/6 cores. d1 still + // has room; d2 does not, because its compute container (placed in pass B, on top of d1's from create) + // now holds the hugepages the drive container's growth needs. + inv := hazardInventory(existingDrives, existingCompute, cons) + passC := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, existingCompute, inv, computeNodes, cons) + + if passC.Infeasible == "" { + t.Fatalf("pass C: expected infeasible — d2's drive container cannot grow into headroom its own "+ + "compute container holds, got feasible Grow=%+v", passC.Grow) + } + if passC.Infeasibility == nil { + t.Fatalf("pass C: Infeasibility is nil") + } + if passC.Infeasibility.Binding != "hugepages" { + t.Fatalf("pass C: Infeasibility.Binding = %q, want %q", passC.Infeasibility.Binding, "hugepages") + } + var d2Rejected bool + for _, r := range passC.Infeasibility.RejectedNodes { + if r.Node == "d2" { + d2Rejected = true + } + if r.Node == "d1" { + t.Fatalf("pass C: d1 has room to grow and must not be rejected, got %+v", r) + } + } + if !d2Rejected { + t.Fatalf("pass C: want d2 in RejectedNodes, got %+v", passC.Infeasibility.RejectedNodes) + } + if !strings.Contains(passC.Infeasible, "d2") { + t.Fatalf("pass C: Reason must name d2, got %q", passC.Infeasible) + } + if !strings.Contains(passC.Infeasible, "compute container") { + t.Fatalf("pass C: Reason must explain the growth is blocked by this cluster's own compute container, got %q", passC.Infeasible) + } + if !strings.Contains(passC.Infeasible, "deleting it lets the next reconcile grow the drive container first") { + t.Fatalf("pass C: Reason must name the delete-compute recovery action, got %q", passC.Infeasible) + } + if !hasFix(passC.Infeasibility.Fixes, "delete this cluster's compute container") { + t.Fatalf("pass C: Fixes must include the delete-compute recovery tip, got %+v", passC.Infeasibility.Fixes) + } + if len(passC.Infeasibility.Fixes) == 0 || !strings.Contains(passC.Infeasibility.Fixes[0], "delete this cluster's compute container") { + t.Fatalf("pass C: the delete-compute tip must lead the catalog, got %+v", passC.Infeasibility.Fixes) + } +} + +// TestPlanAutoFullDrives_DeletingComputeContainerDefersInsteadOfFailing covers the other resolution of the +// same pass-C deadlock: the operator's own advice (delete d2's compute container) puts that container into +// deletion, but its pod still holds the hugepages until the deletion actually lands. Failing the whole plan +// here (as a bare fit failure would) is exactly what prevents compute from ever being re-planned — the +// capacity weka needs before it will let the deactivation through. HasDeletingComputeContainer must instead +// defer d2's growth and let d1 and compute proceed. +func TestPlanAutoFullDrives_DeletingComputeContainerDefersInsteadOfFailing(t *testing.T) { + existingDrives, existingCompute, cons, computeNodes := hazardStateAfterPassB(t) + + // The two views of a deleting compute container diverge, and the fixture has to reproduce both: + // inventory.ExistingCompute drops it (it skips utils.IsUnhealthy, which is true for a container marked + // for deletion), so the planner no longer sees it as one of ours to keep — but its pod still holds the + // node's hugepages until the deletion actually lands, so node headroom still nets it out. Passing the + // full set to hazardInventory and the trimmed set to the planner is what that state looks like. + keptCompute := make([]ExistingComputeContainer, 0, len(existingCompute)) + for _, ec := range existingCompute { + if ec.Node != "d2" { + keptCompute = append(keptCompute, ec) + } + } + + inv := hazardInventory(existingDrives, existingCompute, cons, "d2") + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, keptCompute, inv, computeNodes, cons) + + if plan.Infeasible != "" { + t.Fatalf("expected feasible: a deleting compute container defers d2's growth rather than failing the "+ + "plan, got infeasible: %s", plan.Infeasible) + } + if len(plan.Grow) != 1 { + t.Fatalf("want exactly one drive container grown (d1's), got %+v", plan.Grow) + } + if plan.Grow[0].Name != "drive-d1" || plan.Grow[0].NewNumDrives != 6 || plan.Grow[0].NewCores != 6 { + t.Fatalf("Grow[0] = %+v, want d1 grown to its full 6 drives/6 cores", plan.Grow[0]) + } + for _, g := range plan.Grow { + if g.Name == "drive-d2" { + t.Fatalf("d2's drive container must not grow this pass while its blocking compute container is "+ + "still deleting, got %+v", g) + } + } + + var named bool + for _, w := range plan.Warnings { + if strings.Contains(w.Message, "d2") && strings.Contains(w.Message, "placement deferred") { + named = true + } + } + if !named { + t.Fatalf("want a placement-deferred warning naming d2, got %+v", plan.Warnings) + } + + // Every total charges d2 at what it is actually running (4 drives / 4 cores / 4000 GiB), not the 6 it + // would grow to: no Grow entry is written for it this pass. Charging the target would size compute for + // capacity that does not exist — and since tlcGiBTaken drives compute hugepages, it would raise compute + // demand on the very node whose growth a compute container is already blocking. An over-sized compute + // layout can go infeasible, which applies nothing at all, including d1's growth. + if plan.TotalTlcDriveCores != 10 { + t.Fatalf("TotalTlcDriveCores = %d, want 10 (d1 grown to 6 + d2 frozen at 4)", plan.TotalTlcDriveCores) + } + if plan.RequiredComputeCores != 10 { + t.Fatalf("RequiredComputeCores = %d, want 10", plan.RequiredComputeCores) + } + if plan.DriveSizing == nil || plan.DriveSizing.TlcGiBTaken != 10000 || plan.DriveSizing.DrivesTaken != 10 { + t.Fatalf("DriveSizing = %+v, want 10 drives / 10000 GiB (d1 grown to 6, d2 frozen at 4)", plan.DriveSizing) + } + // The denominator still reflects every signed drive on both nodes — the deferral hides nothing. + if plan.DriveSizing.DrivesAvailable != 12 || plan.DriveSizing.TlcGiBAvailable != 12000 { + t.Fatalf("DriveSizing available = %d drives / %d GiB, want the full signed 12 / 12000", + plan.DriveSizing.DrivesAvailable, plan.DriveSizing.TlcGiBAvailable) + } + + // Compute must still be planned — the whole point of deferring rather than failing: this is the extra + // compute capacity weka needs before it will deactivate d2's compute container. + if len(plan.ComputeLayout) != 1 { + t.Fatalf("ComputeLayout = %+v, want exactly d1's container (d2 has no headroom while its compute "+ + "container's pod is still up), got %d entries", plan.ComputeLayout, len(plan.ComputeLayout)) + } + if c := plan.ComputeLayout[0]; c.Node != "d1" || c.NumCores != 10 || c.HugepagesMiB != 30000 { + t.Fatalf("ComputeLayout[0] = %+v, want d1 at 10 cores/30000 MiB", c) + } +} + +// TestPlanAutoFullDrives_DeletingBlockingComputeContainerRecoversGrowth is the manual recovery for the +// hazard above: deleting the cluster's compute container on the blocked node returns its spec footprint to +// node headroom, and the next reconcile grows the drive container first (the drive walk runs before compute +// sizing in the same pass), then re-places compute against what is left. +func TestPlanAutoFullDrives_DeletingBlockingComputeContainerRecoversGrowth(t *testing.T) { + existingDrives, existingCompute, cons, computeNodes := hazardStateAfterPassB(t) + + // Drop d2's compute container from existingCompute — exactly what deleting that WekaContainer produces: + // its 4 cores / 12000 MiB are no longer netted out of d2's headroom by hazardInventory below. + recovered := make([]ExistingComputeContainer, 0, len(existingCompute)) + for _, ec := range existingCompute { + if ec.Node != "d2" { + recovered = append(recovered, ec) + } + } + + inv := hazardInventory(existingDrives, recovered, cons) + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, recovered, inv, computeNodes, cons) + + if plan.Infeasible != "" { + t.Fatalf("expected feasible once d2's compute container is deleted, got infeasible: %s", plan.Infeasible) + } + if len(plan.Grow) != 2 { + t.Fatalf("want both drive containers to grow to their full drive set, got %+v", plan.Grow) + } + for _, g := range plan.Grow { + if g.NewNumDrives != 6 || g.NewCores != 6 || g.NewTlcGiB != 6000 { + t.Fatalf("Grow[%s] = %+v, want the full 6 drives/6 cores/6000 GiB", g.Name, g) + } + } + // 12 required drive cores (6+6 at ratio 1.0) against d1's kept 4 cores leaves an 8-core deficit. The + // growth search grows d1's existing container as far as it can (to 9 cores) before placing the remainder + // as a new container on d2, rather than growing d1 past what its own headroom allows. + if plan.RequiredComputeCores != 12 { + t.Fatalf("RequiredComputeCores = %d, want 12 (6+6 drive cores at ratio 1.0)", plan.RequiredComputeCores) + } + var d1Compute, d2Compute *ComputeContainerSpec + for i := range plan.ComputeLayout { + switch plan.ComputeLayout[i].Node { + case "d1": + d1Compute = &plan.ComputeLayout[i] + case "d2": + d2Compute = &plan.ComputeLayout[i] + } + } + if d1Compute == nil || d2Compute == nil { + t.Fatalf("want a compute container on both d1 and d2, got %+v", plan.ComputeLayout) + } + if d1Compute.NumCores != 9 { + t.Fatalf("d1 compute = %+v, want 9 cores (grown from its kept 4, as far as its own headroom allows)", *d1Compute) + } + if d2Compute.NumCores != 3 { + t.Fatalf("d2 compute = %+v, want a new 3-core container covering the remainder of the deficit", *d2Compute) + } + if d1Compute.NumCores+d2Compute.NumCores != plan.RequiredComputeCores { + t.Fatalf("compute layout totals %d+%d cores, want %d (RequiredComputeCores)", + d1Compute.NumCores, d2Compute.NumCores, plan.RequiredComputeCores) + } +} + +// TestPlanAutoFullDrives_BothPinsDroppedInOnePass_ReachesFullGrowth: the hazard needs the intermediate step +// (pass A/B above) where driveCores is still pinned while numDrives already isn't — that is what lets a +// compute container land on the second node before that node's drive container has any pending growth of +// its own. Dropping both pins in the same pass a create ran in skips that step entirely, so both drive +// containers and the single compute container grow together and the fleet reaches full growth in one step. +func TestPlanAutoFullDrives_BothPinsDroppedInOnePass_ReachesFullGrowth(t *testing.T) { + cons := hazardCons() + computeNodes := computeNodeSet("d1", "d2") + + inv := hazardInventory(nil, nil, cons) + create := PlanAutoFullDrives(AutoFullDrivesDesired{NumDrives: 2, DriveCores: 2}, nil, nil, inv, computeNodes, cons) + if create.Infeasible != "" { + t.Fatalf("create pass infeasible: %s", create.Infeasible) + } + existingDrives := hazardCarryDrives(nil, create) + existingCompute := hazardCarryCompute(nil, create.ComputeLayout) + + inv = hazardInventory(existingDrives, existingCompute, cons) + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, existingCompute, inv, computeNodes, cons) + + if plan.Infeasible != "" { + t.Fatalf("dropping both pins in one pass right after create must reach full growth, got infeasible: %s", plan.Infeasible) + } + if len(plan.Grow) != 2 { + t.Fatalf("want both drive containers to grow to their full drive set, got %+v", plan.Grow) + } + for _, g := range plan.Grow { + if g.NewNumDrives != 6 || g.NewCores != 6 || g.NewTlcGiB != 6000 { + t.Fatalf("Grow[%s] = %+v, want the full 6 drives/6 cores/6000 GiB", g.Name, g) + } + } +} + +// An unscheduled node that also has a compute container being deleted keeps the unscheduled charging +// convention: it never ran a fit, so the zero-valued result must not be read as a compute-blocked failure. +// Only the deleting-compute flag differs from +// TestPlanAutoFullDrives_UnscheduledDriveContainer_ComputeCountsPlannedNotFrozenCapacity, and neither the +// numerator nor the compute sizing may move because of it. +func TestPlanAutoFullDrives_UnscheduledNodeWithDeletingCompute_KeepsPlannedCapacity(t *testing.T) { + for _, deletingCompute := range []bool{false, true} { + cons := testCons() + cons.ComputeHugepagesTlcRatio = 1024 + cons.FullDrivesComputeToDriveCoreRatio = 0 + const bigFree = 1 << 28 + + existingDrives := []ExistingContainer{ + {Name: "drive-unscheduled", Node: "unscheduled", FDValue: "unscheduled", NumCores: 1, NumDrives: 1, Unscheduled: true}, + } + unscheduled := NodeCapacity{ + NodeName: "unscheduled", FDValue: "unscheduled", + OwnDriveCapacitiesGiB: uniformDrives(1, 1000), + DriveCapacitiesGiB: uniformDrives(2, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + HasDeletingComputeContainer: deletingCompute, + } + c1 := NodeCapacity{NodeName: "c1", FDValue: "fdC1", AllocatableCPU: 1000, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree} + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, nil, + []NodeCapacity{unscheduled, c1}, computeNodeSet("c1"), cons) + + if plan.DriveSizing == nil || plan.DriveSizing.TlcGiBTaken != 3000 { + t.Fatalf("deletingCompute=%v: DriveSizing = %+v, want TlcGiBTaken 3000 (the planned 3-drive figure)", + deletingCompute, plan.DriveSizing) + } + if len(plan.ComputeLayout) != 1 || plan.ComputeLayout[0].HugepagesMiB != 4700 { + t.Fatalf("deletingCompute=%v: ComputeLayout = %+v, want one entry at 4700 MiB (planned numerator)", + deletingCompute, plan.ComputeLayout) + } + } +} + +// A compute-blocked node charges the capacity its container actually holds, which cannot be read off +// ExistingContainer.TlcGiB: that field is structurally 0 for auto-full-drives (driveCapacity and +// containerCapacity are both unset, and they are all DriveContainerCapacities reads), so the fixture leaves +// it zero the way inventory.ExistingDrives does. Charging it directly would drop live capacity out of the +// compute-hugepages numerator and under-size compute; charging the ratcheted target would over-size it. +func TestPlanAutoFullDrives_ComputeBlocked_ChargesHeldCapacityNotSpecTlcGiB(t *testing.T) { + cons := testCons() + cons.ComputeHugepagesTlcRatio = 1024 + cons.FullDrivesComputeToDriveCoreRatio = 0 + const bigFree = 1 << 28 + + existingDrives := []ExistingContainer{ + {Name: "drive-grow", Node: "grow", FDValue: "grow", NumCores: 1, NumDrives: 1}, + } + // Holds 1 of its 3 signed drives; too little hugepages headroom to grow, so the fit fails. + grow := NodeCapacity{ + NodeName: "grow", FDValue: "grow", + OwnDriveCapacitiesGiB: uniformDrives(1, 1000), + DriveCapacitiesGiB: uniformDrives(2, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: 1, AvailableMemoryMiB: bigFree, + HasDeletingComputeContainer: true, + } + // No container of ours yet, so nothing is held and nothing is created. + create := NodeCapacity{ + NodeName: "create", FDValue: "create", + DriveCapacitiesGiB: uniformDrives(2, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: 1, AvailableMemoryMiB: bigFree, + HasDeletingComputeContainer: true, + } + c1 := NodeCapacity{NodeName: "c1", FDValue: "fdC1", AllocatableCPU: 1000, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree} + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, nil, + []NodeCapacity{grow, create, c1}, computeNodeSet("c1"), cons) + + if plan.Infeasible != "" { + t.Fatalf("want the deferral, not an infeasibility: %s", plan.Infeasible) + } + if len(plan.Grow) != 0 || len(plan.Create) != 0 { + t.Fatalf("both nodes are compute-blocked, want nothing planned, got Create=%+v Grow=%+v", plan.Create, plan.Grow) + } + // 1000: the grow node's one held drive. The create node contributes nothing — it holds no drives — and + // neither node's unclaimed drives count, since this pass does not claim them. + if plan.DriveSizing == nil || plan.DriveSizing.TlcGiBTaken != 1000 || plan.DriveSizing.DrivesTaken != 1 { + t.Fatalf("DriveSizing = %+v, want 1 drive / 1000 GiB taken (held capacity only)", plan.DriveSizing) + } + // The denominator still names every signed drive on both nodes, so the deferral hides no capacity. + if plan.DriveSizing.DrivesAvailable != 5 || plan.DriveSizing.TlcGiBAvailable != 5000 { + t.Fatalf("DriveSizing available = %d drives / %d GiB, want the full signed 5 / 5000", + plan.DriveSizing.DrivesAvailable, plan.DriveSizing.TlcGiBAvailable) + } +} + +// afdPinnedComputeFleet builds a fleet for the computeCores-pin tests: a non-compute-eligible drive node +// carrying `drives` drives (2*drives required compute cores at the 2.0 ratio), two compute-eligible nodes +// each already hosting a keptCores-core container with ample CPU to grow into, and, when freeNodeCores > 0, +// one free eligible node sized to host exactly that many data cores. +func afdPinnedComputeFleet(drives, keptCores, freeNodeCores int) ( + inv []NodeCapacity, eligible map[string]bool, existing []ExistingComputeContainer, +) { + const big = 1 << 28 + inv = []NodeCapacity{{ + NodeName: "drv", FDValue: "fdDrv", + DriveCapacitiesGiB: afdDrives(drives, 5000), TlcGiB: drives * 5000, + AllocatableCPU: 64, AvailableHugepagesMiB: big, AvailableMemoryMiB: big, + }} + eligible = map[string]bool{} + for _, name := range []string{"e1", "e2"} { + inv = append(inv, NodeCapacity{ + NodeName: name, FDValue: "fd" + name, + AllocatableCPU: 20, AvailableHugepagesMiB: big, AvailableMemoryMiB: big, + }) + eligible[name] = true + existing = append(existing, ExistingComputeContainer{ + Name: "ec-" + name, Node: name, NumCores: keptCores, HugepagesMiB: 1600, + }) + } + if freeNodeCores > 0 { + // A free node reserves one core for management, so hosting freeNodeCores data cores costs one more. + inv = append(inv, NodeCapacity{ + NodeName: "f1", FDValue: "fdF", + AllocatableCPU: freeNodeCores + 1, AvailableHugepagesMiB: big, AvailableMemoryMiB: big, + }) + eligible["f1"] = true + } + return inv, eligible, existing +} + +// A pinned computeCores is the exact size of every compute container, so in-place growth must stop at the +// pin and spread across the kept containers rather than piling the whole deficit onto whichever one has the +// most node headroom. Growth is the only lever here (no free node), and 2x3 is the one layout that both +// covers the requirement and honors the pin. +func TestPlanAutoFullDrives_ComputeGrowth_PinnedCoresBoundsGrowth(t *testing.T) { + const pin = 3 + inv, eligible, existing := afdPinnedComputeFleet(3, 1, 0) + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{ComputeCores: pin}, nil, existing, inv, eligible, testCons()) + + if plan.Infeasible != "" { + t.Fatalf("unexpected infeasible: %s", plan.Infeasible) + } + if plan.RequiredComputeCores != 6 { + t.Fatalf("RequiredComputeCores = %d, want 6 (3 drive cores x the 2.0 ratio)", plan.RequiredComputeCores) + } + if len(plan.ComputeLayout) != 2 { + t.Fatalf("ComputeLayout = %+v, want the 2 kept containers", plan.ComputeLayout) + } + total := 0 + for _, l := range plan.ComputeLayout { + if l.NumCores > pin { + t.Errorf("compute container on %s has %d core(s), want at most the pinned %d (layout %+v)", + l.Node, l.NumCores, pin, plan.ComputeLayout) + } + total += l.NumCores + } + if total != plan.RequiredComputeCores { + t.Errorf("layout supplies %d core(s), want exactly the required %d", total, plan.RequiredComputeCores) + } + if plan.ComputeCores != pin { + t.Errorf("ComputeCores = %d, want the pinned %d", plan.ComputeCores, pin) + } + if got := afdComputeCores(plan.ComputeLayout); got["e1"] != pin || got["e2"] != pin { + t.Errorf("per-node cores = %v, want e1 and e2 each grown to the pinned %d", got, pin) + } +} + +// With every kept container already at the pinned size there is no growth headroom left, so a deficit no +// free node can absorb is infeasible rather than covered by carrying a container past the pin. The report +// names the pinned core count and the compute-node shortfall, and offers the pin as a lever. +func TestPlanAutoFullDrives_ComputeGrowth_PinnedCoresNoPlaceableNode_Infeasible(t *testing.T) { + const pin = 3 + inv, eligible, existing := afdPinnedComputeFleet(4, pin, 0) + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{ComputeCores: pin}, nil, existing, inv, eligible, testCons()) + + if plan.Infeasible == "" { + t.Fatalf("want infeasible (2-core deficit, every kept container at the pinned %d, no free node), "+ + "got layout %+v", pin, plan.ComputeLayout) + } + if !strings.Contains(plan.Infeasible, "of 3 cores") || !strings.Contains(plan.Infeasible, "compute nodes") { + t.Errorf("Infeasible = %q, want it to name the pinned core count and the compute-node shortfall", + plan.Infeasible) + } + if plan.Infeasibility.Pool != "compute" { + t.Errorf("Infeasibility.Pool = %q, want %q", plan.Infeasibility.Pool, "compute") + } + if plan.Infeasibility.Binding != bindingCores { + t.Errorf("Infeasibility.Binding = %q, want %q", plan.Infeasibility.Binding, bindingCores) + } + if !hasFix(plan.Infeasibility.Fixes, "lower dynamicTemplate.computeCores if it is pinned") { + t.Errorf("Fixes = %v, want one offering the computeCores pin as a lever", plan.Infeasibility.Fixes) + } + if plan.ComputeCores != 0 || plan.ComputeContainers != 0 || len(plan.ComputeLayout) != 0 { + t.Errorf("infeasible plan must not emit compute sizing, got %d container(s) x %d core(s), layout=%d", + plan.ComputeContainers, plan.ComputeCores, len(plan.ComputeLayout)) + } +} + +// The steady state a pin implies: a deficit is covered by a new container of exactly the pinned size on a +// free node, leaving every kept container at the pin. New containers are the preferred lever, so bounding +// growth must not divert this case into growth or infeasibility. +func TestPlanAutoFullDrives_ComputeGrowth_PinnedCoresPlacesNewContainer(t *testing.T) { + const pin = 3 + inv, eligible, existing := afdPinnedComputeFleet(4, pin, pin) + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{ComputeCores: pin}, nil, existing, inv, eligible, testCons()) + + if plan.Infeasible != "" { + t.Fatalf("unexpected infeasible: %s", plan.Infeasible) + } + got := afdComputeCores(plan.ComputeLayout) + want := map[string]int{"e1": pin, "e2": pin, "f1": pin} + if !reflect.DeepEqual(got, want) { + t.Errorf("per-node cores = %v, want %v (kept containers at the pin, replacement placed at the pin)", + got, want) + } + if plan.ComputeCores != pin { + t.Errorf("ComputeCores = %d, want the pinned %d", plan.ComputeCores, pin) + } +} + +// A compute shortfall on a node still draining a deleted compute container says so. The planner cannot tell +// whether the returning reservation would close the gap, so the report stays infeasible — but the operator's +// choice between waiting and intervening turns on that fact, and it is carried nowhere else in the report. +func TestPlanAutoFullDrives_ComputeShortfall_NamesDrainingNode(t *testing.T) { + const big = 1 << 28 + cons := testCons() + cons.FullDrivesComputeToDriveCoreRatio = 2.0 + + // One drive node with plenty of drives, and a compute-eligible node too small to host the compute the + // resulting drive cores demand — a genuine shortfall either way. + mk := func(deleting bool) CapacityPlan { + drive := NodeCapacity{ + NodeName: "d1", FDValue: "d1", + DriveCapacitiesGiB: afdDrives(6, 1000), TlcGiB: 6000, + AllocatableCPU: 64, AvailableHugepagesMiB: big, AvailableMemoryMiB: big, + } + compute := NodeCapacity{ + NodeName: "c1", FDValue: "c1", + AllocatableCPU: 1, AvailableHugepagesMiB: 1, AvailableMemoryMiB: 1, + HasDeletingComputeContainer: deleting, + } + return PlanAutoFullDrives(AutoFullDrivesDesired{}, nil, nil, + []NodeCapacity{drive, compute}, computeNodeSet("c1"), cons) + } + + without := mk(false) + if without.Infeasible == "" { + t.Fatalf("fixture must be infeasible to exercise the clause, got feasible") + } + if strings.Contains(without.Infeasible, "still being deleted") { + t.Fatalf("no node is draining, so the clause must be absent, got %q", without.Infeasible) + } + + with := mk(true) + if with.Infeasible == "" { + t.Fatalf("the clause is wording only — the plan must stay infeasible, got feasible") + } + if !strings.Contains(with.Infeasible, "a compute container of this cluster is still being deleted on c1") { + t.Fatalf("shortfall must name the draining node, got %q", with.Infeasible) + } + if !strings.Contains(with.Infeasible, "may clear on its own") { + t.Fatalf("shortfall must say the condition can resolve without intervention, got %q", with.Infeasible) + } + // Wording only: the verdict, pool and binding must be untouched by the presence of a draining node. + if with.Infeasibility.Pool != without.Infeasibility.Pool || + with.Infeasibility.Binding != without.Infeasibility.Binding { + t.Fatalf("clause must not change the classification: %+v vs %+v", with.Infeasibility, without.Infeasibility) + } +} diff --git a/internal/capacityplanner/autofulldrives_warnings.go b/internal/capacityplanner/autofulldrives_warnings.go index 96ef0da79..d3a67a022 100644 --- a/internal/capacityplanner/autofulldrives_warnings.go +++ b/internal/capacityplanner/autofulldrives_warnings.go @@ -5,11 +5,12 @@ import ( "strings" ) -// autofulldrives_warnings.go is where every auto-full-drives planner Warning is worded. Each condition -// gets exactly one Warning per planning pass, naming every affected node, because the controller throttles -// events on reason alone: a second Warning under the same reason would be silently dropped for the whole -// window rather than reported. The walk in autofulldrives.go collects nodes per condition and calls one -// formatter here after it completes. +// autofulldrives_warnings.go is where every auto-full-drives planner Warning is worded. Each condition gets +// exactly one Warning per planning pass, naming every affected node. Distinct conditions that share a +// WarningKind (and so the same event reason) carry distinct Cause values, since the controller throttles +// events on reason+cause: a condition with no dedicated Cause would share its throttle window with every +// other Warning of that Kind, and a second one landing inside that window would be silently dropped. The +// walk in autofulldrives.go collects nodes per condition and calls a formatter here after it completes. func listNodes(parts []string) string { return listNodesCapped(parts, autoFullDrivesMaxNamedNodes) } @@ -49,40 +50,47 @@ func formatStrandedWarning(stranded []strandedNode, pin int) Warning { } // formatIneligibleWarning renders the aggregated NodeIneligible message. Each node's cause travels with it -// in nodes ("h1-2-a (cordoned)"), so unlike stranding and placement-deferral there is no per-cause branching -// to do here. -func formatIneligibleWarning(nodes []string, freeDrives int) Warning { - return fleetWarning(WarningKindNodeIneligible, +// in nodes ("h1-2-a (cordoned)"), already resolved by resources.NodeIneligibleReason to one of exactly three +// values (cordoned, not ready, untolerated taint). reasons is the distinct subset actually present, sorted by +// the caller for a stable Cause — so a node going NotReady gets its own throttle window instead of sharing +// one with a fleet that was merely cordoned. +func formatIneligibleWarning(nodes []string, freeDrives int, reasons []string) Warning { + return fleetWarningWithCause(WarningKindNodeIneligible, WarningCause(strings.Join(reasons, "+")), "auto full drives: %d node(s) holding %d signed free full drive(s) are ineligible for a new drive "+ "container: %s; anything already running on them keeps running and still grows", len(nodes), freeDrives, listNodes(nodes)) } -// formatPlacementDeferredWarning renders the aggregated PlacementDeferred message, one warning covering both -// deferral causes (unscheduled pod, container being deleted) since both map to the single reason -// AutoFullDrivesPlacementDeferred, whose throttle key ignores the message — two warnings would let one -// silently suppress the other. -func formatPlacementDeferredWarning(deferred, deleting []string) Warning { - // Two causes share one message, so halve the budget rather than let each spend the full cap. - limit := autoFullDrivesMaxNamedNodes - if len(deferred) > 0 && len(deleting) > 0 { - limit = autoFullDrivesMaxNamedNodes / 2 +// formatPlacementDeferredWarning renders one Warning per PlacementDeferred cause (unscheduled pod, drive +// container being deleted, compute container being deleted) instead of merging them: each gets its own +// Cause and so its own throttle window, and the full per-warning node-name cap rather than a share of it. +// computeBlockedBinding is the fit dimension every compute-blocked node was short of, or "" when they +// disagree (or it is unknown). It is only wording: the deferral itself fires on any binding, and on the +// create path as well as growth, so the clause must not promise hugepages or growth specifically. +func formatPlacementDeferredWarning(deferred, deleting, computeBlocked []string, computeBlockedBinding string) []Warning { + held := "the resources" + if computeBlockedBinding != "" { + held = "the " + computeBlockedBinding } - - var clauses []string - if len(deferred) > 0 { - clauses = append(clauses, fmt.Sprintf( - "pod not scheduled yet, growth waits for the scheduler: %s", listNodesCapped(deferred, limit))) - } - if len(deleting) > 0 { - clauses = append(clauses, fmt.Sprintf( - "a this-cluster drive container is still being deleted, new placement waits for it: %s", listNodesCapped(deleting, limit))) + causes := []struct { + nodes []string + cause WarningCause + clause string + }{ + {deferred, CausePlacementUnscheduled, "pod not scheduled yet, growth waits for the scheduler"}, + {deleting, CausePlacementDriveDeleting, + "a this-cluster drive container is still being deleted, new placement waits for it"}, + {computeBlocked, CausePlacementComputeDeleting, + "a this-cluster compute container on the node is still being deleted and holds " + held + + " this placement needs"}, } - retry := "it retries automatically" - if len(clauses) > 1 { - retry = "both retry automatically" + var warnings []Warning + for _, c := range causes { + if len(c.nodes) > 0 { + warnings = append(warnings, fleetWarningWithCause(WarningKindTransient, c.cause, + "auto full drives: placement deferred on %d node(s) this pass; %s: %s; it retries automatically", + len(c.nodes), c.clause, listNodes(c.nodes))) + } } - return fleetWarning(WarningKindTransient, - "auto full drives: placement deferred on %d node(s) this pass; %s; %s", - len(deferred)+len(deleting), strings.Join(clauses, "; "), retry) + return warnings } diff --git a/internal/capacityplanner/autofulldrives_warnings_test.go b/internal/capacityplanner/autofulldrives_warnings_test.go index 87bb86480..f23b38c1d 100644 --- a/internal/capacityplanner/autofulldrives_warnings_test.go +++ b/internal/capacityplanner/autofulldrives_warnings_test.go @@ -50,34 +50,107 @@ func TestListNodes_CapsAtMaxNamedNodes(t *testing.T) { } } -// TestFormatPlacementDeferredWarning_SharesBudgetAcrossCauses guards against spending the cap twice: with -// both causes present, deferred and deleting must split one autoFullDrivesMaxNamedNodes budget rather than -// each getting the full cap, or a fleet with plenty of both would name up to 2x the intended maximum. -func TestFormatPlacementDeferredWarning_SharesBudgetAcrossCauses(t *testing.T) { +// TestFormatPlacementDeferredWarning_EachCauseGetsFullBudget covers the per-cause split: deferred and +// deleting are different causes, each its own Warning and event, so each is capped independently at the +// full autoFullDrivesMaxNamedNodes rather than sharing one budget between them — a fleet with plenty of +// both can legitimately name up to 2x the per-warning maximum in total, split across two Warnings. +func TestFormatPlacementDeferredWarning_EachCauseGetsFullBudget(t *testing.T) { var deferred, deleting []string for i := 0; i < 12; i++ { deferred = append(deferred, fmt.Sprintf("d-%d", i)) deleting = append(deleting, fmt.Sprintf("x-%d", i)) } - w := formatPlacementDeferredWarning(deferred, deleting) + warnings := formatPlacementDeferredWarning(deferred, deleting, nil, "") + if len(warnings) != 2 { + t.Fatalf("formatPlacementDeferredWarning() returned %d warning(s), want 2 (one per cause): %+v", len(warnings), warnings) + } + + byCause := map[WarningCause]Warning{} + for _, w := range warnings { + byCause[w.Cause] = w + } + + deferredWarning, ok := byCause[CausePlacementUnscheduled] + if !ok { + t.Fatalf("no warning with Cause=%q in %+v", CausePlacementUnscheduled, warnings) + } named := 0 for _, n := range deferred { - if strings.Contains(w.Message, n) { + if strings.Contains(deferredWarning.Message, n) { named++ } } + if named != autoFullDrivesMaxNamedNodes { + t.Errorf("deferred warning named %d of its own 12 nodes in %q, want the full autoFullDrivesMaxNamedNodes=%d budget", + named, deferredWarning.Message, autoFullDrivesMaxNamedNodes) + } + if !strings.Contains(deferredWarning.Message, "(+") { + t.Errorf("deferred warning = %q, want it to disclose the truncation with a \"(+N more)\" tail", deferredWarning.Message) + } + for _, n := range deleting { + if strings.Contains(deferredWarning.Message, n) { + t.Errorf("deferred warning = %q, must not name deleting node %q", deferredWarning.Message, n) + } + } + + deletingWarning, ok := byCause[CausePlacementDriveDeleting] + if !ok { + t.Fatalf("no warning with Cause=%q in %+v", CausePlacementDriveDeleting, warnings) + } + named = 0 for _, n := range deleting { - if strings.Contains(w.Message, n) { + if strings.Contains(deletingWarning.Message, n) { named++ } } - if named > autoFullDrivesMaxNamedNodes { - t.Errorf("formatPlacementDeferredWarning() named %d nodes across both causes in %q, want at most autoFullDrivesMaxNamedNodes=%d total", - named, w.Message, autoFullDrivesMaxNamedNodes) + if named != autoFullDrivesMaxNamedNodes { + t.Errorf("deleting warning named %d of its own 12 nodes in %q, want the full autoFullDrivesMaxNamedNodes=%d budget", + named, deletingWarning.Message, autoFullDrivesMaxNamedNodes) + } + if !strings.Contains(deletingWarning.Message, "(+") { + t.Errorf("deleting warning = %q, want it to disclose the truncation with a \"(+N more)\" tail", deletingWarning.Message) } - if !strings.Contains(w.Message, "(+") { - t.Errorf("formatPlacementDeferredWarning() = %q, want it to disclose the truncation with a \"(+N more)\" tail", w.Message) + for _, n := range deferred { + if strings.Contains(deletingWarning.Message, n) { + t.Errorf("deleting warning = %q, must not name deferred node %q", deletingWarning.Message, n) + } + } +} + +// TestFormatPlacementDeferredWarning_ComputeBlockedNamesTheBindingDimension covers the wording of the +// compute-blocked cause. The deferral fires on any fit binding (cores, hugepages or memory) and on the +// create path as well as growth, so the clause may only name a dimension when every blocked node agrees on +// one — the same rule autoNodeFitInfeasible applies to Binding. +func TestFormatPlacementDeferredWarning_ComputeBlockedNamesTheBindingDimension(t *testing.T) { + for _, tc := range []struct { + name string + binding string + want string + reject string + }{ + {"agreed on cores", "cores", "holds the cores this placement needs", "hugepages"}, + {"agreed on memory", "memory", "holds the memory this placement needs", "hugepages"}, + {"agreed on hugepages", "hugepages", "holds the hugepages this placement needs", ""}, + {"nodes disagree", "", "holds the resources this placement needs", "hugepages"}, + } { + t.Run(tc.name, func(t *testing.T) { + warnings := formatPlacementDeferredWarning(nil, nil, []string{"n1"}, tc.binding) + if len(warnings) != 1 { + t.Fatalf("want 1 warning, got %+v", warnings) + } + msg := warnings[0].Message + if !strings.Contains(msg, tc.want) { + t.Fatalf("message must contain %q, got %q", tc.want, msg) + } + // The clause must never promise growth: the same deferral covers a create. + if strings.Contains(msg, "this growth needs") { + t.Fatalf("message must not say \"growth\" — the create path defers identically, got %q", msg) + } + if tc.reject != "" && strings.Contains(msg, tc.reject) { + t.Fatalf("message must not name %q when it is not the binding, got %q", tc.reject, msg) + } + }) } } diff --git a/internal/capacityplanner/compute_layout.go b/internal/capacityplanner/compute_layout.go index 149ec7efc..ee7708c72 100644 --- a/internal/capacityplanner/compute_layout.go +++ b/internal/capacityplanner/compute_layout.go @@ -87,7 +87,10 @@ func hugepagesFitCheck(nodeHeadroom, nodeHugepagesMiB []int, count, cores, maxCo // against real per-node headroom (fit is checked only for the nodes actually chosen; see topNMin), returning // a non-empty infeasible reason when it cannot. Invariants when feasible: count in [floor, len(nodeHeadroom)]; // count*cores >= requiredComputeCores whenever either is auto-derived; hugepagesFor (if set) must also pass. -func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxCoresPerContainer int, nodeHeadroom, nodeHugepagesMiB []int, hugepagesFor func(count, cores int) int) (count, cores int, infeasible string, warnings []string) { +// binding classifies infeasible using the InfeasibilityReport.Binding vocabulary (bindingCores/bindingHugepages +// here), so callers never need to re-parse the English reason; "" when infeasible is "" or the cause is a +// structural node/container-count mismatch the vocabulary has no word for. +func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxCoresPerContainer int, nodeHeadroom, nodeHugepagesMiB []int, hugepagesFor func(count, cores int) int) (count, cores int, infeasible, binding string, warnings []string) { d := len(nodeHeadroom) t := requiredComputeCores @@ -97,8 +100,10 @@ func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxC // unset. The cap only needs to hold for the `count` nodes actually used. count = specCount if count > d { + // A pinned container count outrunning the node count is a structural mismatch, not a resource + // dimension — the vocabulary has no "computeContainers" peer to "driveContainers", so unclassified. return 0, 0, fmt.Sprintf( - "computeContainers=%d exceeds the %d compute nodes; compute spreads one-per-node", count, d), nil + "computeContainers=%d exceeds the %d compute nodes; compute spreads one-per-node", count, d), "", nil } cores = specCores if cores == 0 { @@ -109,46 +114,48 @@ func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxC if perContainerCap > 0 && cores > perContainerCap { return 0, 0, fmt.Sprintf( "computeCores=%d exceeds the per-node compute core headroom (%d) after drive placement", - cores, perContainerCap), nil + cores, perContainerCap), bindingCores, nil } if reason := hugepagesFitCheck(nodeHeadroom, nodeHugepagesMiB, count, cores, maxCoresPerContainer, hugepagesFor); reason != "" { - return 0, 0, reason, nil + return 0, 0, reason, bindingHugepages, nil } if count*cores < t { return 0, 0, fmt.Sprintf( "compute:drive core ratio not met: %d compute containers × %d cores = %d compute cores < the %d compute "+ "core(s) required by the compute:drive ratio (at least 1 per drive core); "+ "increase computeContainers or computeCores, or remove them to enable auto-derivation", - count, cores, count*cores, t), nil + count, cores, count*cores, t), bindingCores, nil } - return count, cores, "", warnings + return count, cores, "", "", warnings case specCores != 0: // Cores set, count unset: honor cores exactly, derive count against the floor, then check the cap — // it depends on which `count` nodes end up in play, not a global figure. cores = specCores if cores <= 0 { - return 0, 0, "no compute core headroom on the compute nodes after drive placement", nil + return 0, 0, "no compute core headroom on the compute nodes after drive placement", bindingCores, nil } count = max(floor, util.CeilDiv(t, cores)) if count > d { + // The pinned cores drove the derived count above the node count — cores is the lever that + // would fix it (raise cores, or lower requiredComputeCores), so it is the binding dimension. return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: need %d compute containers of %d cores but only %d compute nodes", - count, cores, d), nil + count, cores, d), bindingCores, nil } perContainerCap := topNMin(nodeHeadroom, count, maxCoresPerContainer) if perContainerCap <= 0 { - return 0, 0, "no compute core headroom on the compute nodes after drive placement", nil + return 0, 0, "no compute core headroom on the compute nodes after drive placement", bindingCores, nil } if cores > perContainerCap { return 0, 0, fmt.Sprintf( "computeCores=%d exceeds the per-node compute core headroom (%d) after drive placement", - cores, perContainerCap), nil + cores, perContainerCap), bindingCores, nil } if reason := hugepagesFitCheck(nodeHeadroom, nodeHugepagesMiB, count, cores, maxCoresPerContainer, hugepagesFor); reason != "" { - return 0, 0, reason, nil + return 0, 0, reason, bindingHugepages, nil } - return count, cores, "", warnings + return count, cores, "", "", warnings default: // Neither set: minimize count subject to one-per-node fit and the required cores. For candidate n, @@ -175,10 +182,10 @@ func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxC break } if found { - return count, cores, "", warnings + return count, cores, "", "", warnings } if d == 0 || topNMinSorted(sortedHeadroom, d, maxCoresPerContainer) <= 0 { - return 0, 0, "no compute core headroom on the compute nodes after drive placement", nil + return 0, 0, "no compute core headroom on the compute nodes after drive placement", bindingCores, nil } // No n reached the requirement — report against n=d (most permissive), diagnosing which dimension // actually failed there rather than always naming both (misleading if only hugepages were short). @@ -189,7 +196,7 @@ func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxC if hugepagesFor == nil { return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: need %d compute containers but only %d compute nodes (max %d compute cores < the %d required compute core(s))", - neededContainers, d, d*capAll, t), nil + neededContainers, d, d*capAll, t), bindingCores, nil } // nil nodeHugepagesMiB disables the check, matching other call sites. hugepagesFit := true @@ -207,28 +214,30 @@ func deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxC } switch { case !coresFit && !hugepagesFit: + // Both dimensions are short and the vocabulary carries only one binding at a time; cores leads + // since it is checked first above and the reason names it first too. return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: neither compute cores nor hugepages suffice for %d compute containers "+ "across %d compute nodes (max %d compute cores < the %d required compute core(s); only %d of %d nodes meet the "+ "%d MiB/container hugepages bar)", - d, d, d*capAll, t, hpFitAlone, d, hpAtD), nil + d, d, d*capAll, t, hpFitAlone, d, hpAtD), bindingCores, nil case !coresFit: return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: need %d compute containers but only %d compute nodes (max %d compute "+ "cores < the %d required compute core(s); hugepages are sufficient)", - neededContainers, d, d*capAll, t), nil + neededContainers, d, d*capAll, t), bindingCores, nil case !hugepagesFit: return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: hugepages insufficient for %d compute containers across %d compute "+ "nodes (only %d of %d nodes meet the %d MiB/container bar at %d cores/container; compute cores are sufficient)", - d, d, hpFitAlone, d, hpAtD, cAtD), nil + d, d, hpFitAlone, d, hpAtD, cAtD), bindingHugepages, nil default: // Unreachable in practice (the scan already tries n=d); kept as a defensive fallback so a - // latent scan bug never surfaces a blank message. + // latent scan bug never surfaces a blank message. Unclassified since it should never fire. return 0, 0, fmt.Sprintf( "cannot satisfy the compute:drive ratio: need %d compute containers but only %d compute nodes (max %d compute "+ "cores < the %d required compute core(s)) [unexpected: cores and hugepages both fit at n=%d]", - neededContainers, d, d*capAll, t, d), nil + neededContainers, d, d*capAll, t, d), "", nil } } } @@ -271,7 +280,7 @@ func ComputeLayoutWouldGrow(specCount, specCores, requiredComputeCores, floor, m } // nil, nil: hugepages-awareness can only push the answer to a larger n (never lowers total cores below // count*cores >= t), so omitting it here can only cause more re-plans, never an incorrectly skipped one. - count, cores, infeasible, _ := deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxCoresPerContainer, headroom, nil, nil) + count, cores, infeasible, _, _ := deriveComputeLayout(specCount, specCores, requiredComputeCores, floor, maxCoresPerContainer, headroom, nil, nil) if infeasible != "" { return true } diff --git a/internal/capacityplanner/compute_layout_test.go b/internal/capacityplanner/compute_layout_test.go index 1d3ab9b42..76c0460e7 100644 --- a/internal/capacityplanner/compute_layout_test.go +++ b/internal/capacityplanner/compute_layout_test.go @@ -31,6 +31,7 @@ func TestDeriveComputeLayout(t *testing.T) { hugepagesFor func(count, cores int) int wantCount, wantCores int wantInfeasibleSub string // substring in infeasible ("" => feasible) + wantBinding string // expected binding when wantInfeasibleSub != ""; "" => not checked wantWarnSub string // substring in a warning ("" => no warning) }{ { @@ -58,7 +59,7 @@ func TestDeriveComputeLayout(t *testing.T) { { name: "both unset: need more containers than compute nodes -> infeasible", totalTlc: 200, floor: 5, maxPerNode: cap, nodeHeadroom: rep(10, 16), - wantInfeasibleSub: "only 10 compute nodes", + wantInfeasibleSub: "only 10 compute nodes", wantBinding: bindingCores, }, { name: "both unset: a node has zero compute headroom -> infeasible", @@ -80,7 +81,7 @@ func TestDeriveComputeLayout(t *testing.T) { { name: "cores set above headroom: fail fast (no clamp)", specCores: 32, totalTlc: 160, floor: 5, maxPerNode: cap, nodeHeadroom: rep(14, 26), - wantInfeasibleSub: "exceeds the per-node compute core headroom (19)", + wantInfeasibleSub: "exceeds the per-node compute core headroom (19)", wantBinding: bindingCores, }, { name: "cores set: real headroom below cap -> fail fast", @@ -125,7 +126,7 @@ func TestDeriveComputeLayout(t *testing.T) { totalTlc: 60, floor: 5, maxPerNode: cap, nodeHeadroom: rep(6, 16), nodeHugepagesMiB: rep(6, 100), hugepagesFor: func(count, cores int) int { return 3000 * cores }, - wantInfeasibleSub: "hugepages insufficient for", + wantInfeasibleSub: "hugepages insufficient for", wantBinding: bindingHugepages, }, { // computeContainers=6 core-fits (cores=10 via ceil(60/6), cap 16) but needs 30000 MiB @@ -135,7 +136,7 @@ func TestDeriveComputeLayout(t *testing.T) { specCount: 6, totalTlc: 60, floor: 5, maxPerNode: cap, nodeHeadroom: rep(6, 16), nodeHugepagesMiB: rep(6, 100), hugepagesFor: func(count, cores int) int { return 3000 * cores }, - wantInfeasibleSub: "needs 30000 MiB hugepages per container", + wantInfeasibleSub: "needs 30000 MiB hugepages per container", wantBinding: bindingHugepages, }, { // Same defect, pinned via specCores=10 instead — covers the other explicit branch @@ -144,13 +145,13 @@ func TestDeriveComputeLayout(t *testing.T) { specCores: 10, totalTlc: 60, floor: 5, maxPerNode: cap, nodeHeadroom: rep(6, 16), nodeHugepagesMiB: rep(6, 100), hugepagesFor: func(count, cores int) int { return 3000 * cores }, - wantInfeasibleSub: "needs 30000 MiB hugepages per container", + wantInfeasibleSub: "needs 30000 MiB hugepages per container", wantBinding: bindingHugepages, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - count, cores, infeasible, warnings := deriveComputeLayout( + count, cores, infeasible, binding, warnings := deriveComputeLayout( c.specCount, c.specCores, c.totalTlc, c.floor, c.maxPerNode, c.nodeHeadroom, c.nodeHugepagesMiB, c.hugepagesFor) @@ -159,6 +160,9 @@ func TestDeriveComputeLayout(t *testing.T) { if !strings.Contains(infeasible, c.wantInfeasibleSub) { t.Fatalf("expected infeasible containing %q, got infeasible=%q (count=%d cores=%d)", c.wantInfeasibleSub, infeasible, count, cores) } + if c.wantBinding != "" && binding != c.wantBinding { + t.Errorf("binding = %q, want %q", binding, c.wantBinding) + } return } if infeasible != "" { @@ -193,14 +197,14 @@ func TestDeriveComputeLayout_AgreesWithAutoFullDrivesHugepagesValidator(t *testi // 5 compute-eligible nodes: ceil(96/5)=20 exceeds the cap of 19 at the only count the sweep can try // (floor==nodeCount==5), so no count fits and the plan is infeasible. - _, _, infeasible, _ := deriveComputeLayout(0, 0, requiredComputeCores, floor, maxCoresPerContainer, rep(5, bigHeadroom), nil, nil) + _, _, infeasible, _, _ := deriveComputeLayout(0, 0, requiredComputeCores, floor, maxCoresPerContainer, rep(5, bigHeadroom), nil, nil) if infeasible == "" { t.Fatalf("expected infeasible with 5 nodes (cap binds at every reachable count), got feasible") } // A sixth node lowers the requirement to ceil(96/6)=16, under the cap, so n=6 fits exactly as the // validator's control case expects. - count, cores, infeasible, _ := deriveComputeLayout(0, 0, requiredComputeCores, floor, maxCoresPerContainer, rep(6, bigHeadroom), nil, nil) + count, cores, infeasible, _, _ := deriveComputeLayout(0, 0, requiredComputeCores, floor, maxCoresPerContainer, rep(6, bigHeadroom), nil, nil) if infeasible != "" { t.Fatalf("expected feasible with 6 nodes, got infeasible: %q", infeasible) } @@ -212,14 +216,14 @@ func TestDeriveComputeLayout_AgreesWithAutoFullDrivesHugepagesValidator(t *testi // max(floor, ceil(required/cores)) — mirrored by validateAutoFullDrivesPinnedComputeCores's own formula. // 18 pinned cores need ceil(96/18)=6 containers; 5 compute-eligible nodes cannot host 6 one-per-node. const pinnedCores = 18 - _, _, infeasible, _ = deriveComputeLayout(0, pinnedCores, requiredComputeCores, floor, maxCoresPerContainer, rep(5, bigHeadroom), nil, nil) + _, _, infeasible, _, _ = deriveComputeLayout(0, pinnedCores, requiredComputeCores, floor, maxCoresPerContainer, rep(5, bigHeadroom), nil, nil) if infeasible == "" { t.Fatalf("expected infeasible with computeCores=%d pinned and only 5 compute nodes (needs 6), got feasible", pinnedCores) } // A sixth node supplies the 6th container the pin needs, so it fits at exactly the pinned cores — // deriveComputeLayout must never re-derive cores away from the pin. - count, cores, infeasible, _ = deriveComputeLayout(0, pinnedCores, requiredComputeCores, floor, maxCoresPerContainer, rep(6, bigHeadroom), nil, nil) + count, cores, infeasible, _, _ = deriveComputeLayout(0, pinnedCores, requiredComputeCores, floor, maxCoresPerContainer, rep(6, bigHeadroom), nil, nil) if infeasible != "" { t.Fatalf("expected feasible with computeCores=%d pinned and 6 compute nodes, got infeasible: %q", pinnedCores, infeasible) } @@ -548,7 +552,7 @@ func Test_ComputeLayoutWouldGrow_HugepagesCannotFlipSkipGate(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - count0, _, infeasible0, _ := deriveComputeLayout( + count0, _, infeasible0, _, _ := deriveComputeLayout( 0, 0, c.totalTlc, c.floor, c.maxPerNode, c.nodeHeadroom, nil, nil) if infeasible0 != "" { t.Fatalf("blind (hugepages-unaware) derivation unexpectedly infeasible: %s", infeasible0) @@ -559,7 +563,7 @@ func Test_ComputeLayoutWouldGrow_HugepagesCannotFlipSkipGate(t *testing.T) { nodeHugepagesMiB := rep(len(c.nodeHeadroom), 1) hugepagesFor := func(count, cores int) int { return 1_000_000 * cores } - count1, _, infeasible1, _ := deriveComputeLayout( + count1, _, infeasible1, _, _ := deriveComputeLayout( 0, 0, c.totalTlc, c.floor, c.maxPerNode, c.nodeHeadroom, nodeHugepagesMiB, hugepagesFor) if infeasible1 == "" && count1 < count0 { diff --git a/internal/capacityplanner/infeasibility.go b/internal/capacityplanner/infeasibility.go index 642d01979..e7af2d1b8 100644 --- a/internal/capacityplanner/infeasibility.go +++ b/internal/capacityplanner/infeasibility.go @@ -50,6 +50,10 @@ type InfeasibilityReport struct { // "(+N more)" tail; RejectedNodes always carries every offender. const autoFullDrivesMaxNamedNodes = 10 +// fixesAutoFullDrivesMaxNamedNodes caps the node names spelled out in the fix catalog's remediation tips — +// shorter than autoFullDrivesMaxNamedNodes because a fix tip is read, not just skimmed. +const fixesAutoFullDrivesMaxNamedNodes = 5 + // autoNodeFitInfeasible turns the auto-full-drives walk's collected fit failures into the plan-wide // infeasibility. There is no partial-fit outcome in that mode — drives are never dropped to make a container // fit — so one node short of resources blocks the whole cluster, and the fixes say how to exclude it if that @@ -58,11 +62,18 @@ func autoNodeFitInfeasible(failures []autoFitFailure) *InfeasibilityReport { names := make([]string, 0, len(failures)) details := make([]string, 0, len(failures)) rejected := make([]NodeRejection, 0, len(failures)) + growthNodes := make([]string, 0, len(failures)) bindings := map[string]int{} for i := range failures { f := &failures[i] names = append(names, f.node) bindings[f.fit.binding]++ + // Growth alone does not make the hazard: the remedy is "delete the compute container on this node", so + // the node must actually host one of ours. Without this the clause fires on any growth failure, naming a + // container that does not exist. + if f.kind == fitKindGrowth && f.ownCompute { + growthNodes = append(growthNodes, f.node) + } if len(details) < autoFullDrivesMaxNamedNodes { details = append(details, fmt.Sprintf( "%s (%s: %d drive(s) at %d core(s) needs %d %s, %d free)", @@ -90,16 +101,31 @@ func autoNodeFitInfeasible(failures []autoFitFailure) *InfeasibilityReport { } } + reason := fmt.Sprintf( + "auto full drives: %d node(s) cannot host a drive container sized for their own signed full drives — "+ + "drives are never dropped to make a container fit, so the whole plan is infeasible and nothing is "+ + "created: %s", + len(failures), list) + if len(growthNodes) > 0 { + // A growth failure (as opposed to create) means the container already exists and must grow into headroom + // that is no longer free. Compute reservations only ever rise, so a compute container this cluster placed + // while the drive container was smaller can be holding exactly that room — "may be", not a claim, since + // the planner cannot see what actually consumes it. Kept to one sentence: this lands in a Kubernetes + // event, and the full remedy catalog travels in Fixes for the CLI to render. + growthList := listNodesCapped(growthNodes, autoFullDrivesMaxNamedNodes) + reason += fmt.Sprintf( + " — this growth may be blocked by this cluster's own compute container on %s, whose reservation only "+ + "ever rises; deleting it lets the next reconcile grow the drive container first. If weka refuses "+ + "that deactivation because active compute would drop too low, add compute capacity elsewhere "+ + "first.", growthList) + } + return &InfeasibilityReport{ - Reason: fmt.Sprintf( - "auto full drives: %d node(s) cannot host a drive container sized for their own signed full drives — "+ - "drives are never dropped to make a container fit, so the whole plan is infeasible and nothing is "+ - "created: %s", - len(failures), list), + Reason: reason, Pool: "drive", Binding: binding, RejectedNodes: rejected, - Fixes: fixesAutoFullDrivesNodeFit(names), + Fixes: fixesAutoFullDrivesNodeFit(names, growthNodes), } } @@ -254,25 +280,36 @@ func fixesDriveCoresAboveDriveCount(numDrives int) []string { // fixesAutoFullDrivesNodeFit: one or more nodes cannot host a drive container sized for their own signed // full drives, which in auto-full-drives mode fails the whole plan (drives are never dropped to fit). -// The first tip leads: lowering driveCores keeps every drive and only reduces the cores they run on, so -// it costs no capacity at all. -func fixesAutoFullDrivesNodeFit(nodes []string) []string { - named := nodes - suffix := "" - if len(named) > 5 { - named, suffix = named[:5], fmt.Sprintf(" (+%d more)", len(nodes)-5) +// growthNodes names the subset (possibly all, possibly none) whose failure is a growth rather than a create: +// when non-empty, a lead tip names the cross-reconcile hazard — a compute container this cluster placed while +// the drive container was still small can be holding the room it now needs — since deleting that container is +// the direct fix and cheaper to try than the general remedies that follow. With no growth nodes, the catalog +// is exactly today's: pinning driveCores lower leads, since it keeps every drive and costs no capacity at all. +func fixesAutoFullDrivesNodeFit(nodes, growthNodes []string) []string { + list := listNodesCapped(nodes, fixesAutoFullDrivesMaxNamedNodes) + + fixes := make([]string, 0, 5) + if len(growthNodes) > 0 { + glist := listNodesCapped(growthNodes, fixesAutoFullDrivesMaxNamedNodes) + fixes = append(fixes, fmt.Sprintf( + "delete this cluster's compute container on %s and let the operator re-place it — compute cores "+ + "and hugepages only ever rise, so a container placed while the drive container was smaller keeps "+ + "the room the growth now needs; the next reconcile grows the drive container first and sizes the "+ + "replacement against what is left. Do one node at a time. If weka refuses the deactivation "+ + "because active compute would drop too low, add capacity elsewhere first — a new container on a "+ + "spare compute-eligible node, or grow an existing one and recreate its pod so the extra cores "+ + "actually become active — then retry.", glist)) } - list := strings.Join(named, ", ") + suffix - return []string{ - "pin dynamicTemplate.driveCores lower — drives are decoupled from cores, so a lower pin keeps every " + + return append(fixes, + "pin dynamicTemplate.driveCores lower — drives are decoupled from cores, so a lower pin keeps every "+ "drive on every node and simply runs them on fewer cores", fmt.Sprintf("or free physical CPU / hugepages / memory on %s (evict other pods, raise the node's "+ "hugepages reservation)", list), fmt.Sprintf("or take those nodes out of the drive role — narrow spec.roleNodeSelector.drive so it no "+ "longer matches %s, or unsign their drives — so the plan is not required to place a container there", list), - "or switch to a drive-sharing mode (containerCapacity or clusterCapacity), which sizes containers from " + + "or switch to a drive-sharing mode (containerCapacity or clusterCapacity), which sizes containers from "+ "a capacity target instead of each node's full drive set", - } + ) } // fixesAutoFullDrivesCompute: compute cannot be sized/placed in auto-full-drives mode. Every drive is diff --git a/internal/capacityplanner/inventory/collect.go b/internal/capacityplanner/inventory/collect.go index 5d814d712..e393f1e1f 100644 --- a/internal/capacityplanner/inventory/collect.go +++ b/internal/capacityplanner/inventory/collect.go @@ -210,6 +210,10 @@ func (c Collector) nodeInventoryFromLists(ctx context.Context, cluster *weka.Wek // TLC/QLC capacity re-enters the fresh-candidate pool — but while its pod lives, cores/hugepages/memory // stay charged via chargeForeignPods. Flagged so the planner deprioritizes fresh placement. deletingDriveNodes := NodesWithDeletingDriveContainer(ownContainers) + // Nodes hosting THIS cluster's compute container being deleted: its pod still holds hugepages/CPU/memory + // (charged via chargeForeignPods), so a drive container on the same node can fail a fit it would pass + // once the deletion lands. Flagged so the auto-full-drives walk defers rather than fails the plan. + deletingComputeNodes := NodesWithDeletingComputeContainer(ownContainers) // Drive candidates: nodes with usable shared-drive capacity, carrying TLC/QLC headroom and an FD key. fdByNode := map[string]string{} @@ -241,17 +245,18 @@ func (c Collector) nodeInventoryFromLists(ctx context.Context, cluster *weka.Wek cpu, hugepagesMiB, memoryMiB := nodeHeadroom(node, consumed) topo := topos[node.Name] driveInv = append(driveInv, capacityplanner.NodeCapacity{ - NodeName: node.Name, - FDValue: fdValue, - TlcGiB: tlcGiB, - QlcGiB: qlcGiB, - AllocatableCPU: cpu, - AvailableHugepagesMiB: hugepagesMiB, - AvailableMemoryMiB: memoryMiB, - IsHt: topo.IsHt, - FullPcpusOnly: topo.FullPcpusOnly, - HasDeletingDriveContainer: deletingDriveNodes[node.Name], - IneligibleReason: resources.NodeIneligibleReason(node, tolerations), + NodeName: node.Name, + FDValue: fdValue, + TlcGiB: tlcGiB, + QlcGiB: qlcGiB, + AllocatableCPU: cpu, + AvailableHugepagesMiB: hugepagesMiB, + AvailableMemoryMiB: memoryMiB, + IsHt: topo.IsHt, + FullPcpusOnly: topo.FullPcpusOnly, + HasDeletingDriveContainer: deletingDriveNodes[node.Name], + HasDeletingComputeContainer: deletingComputeNodes[node.Name], + IneligibleReason: resources.NodeIneligibleReason(node, tolerations), }) } @@ -303,6 +308,7 @@ func (c Collector) FullDrivesInventory(ctx context.Context, cluster *weka.WekaCl tolerations := resources.GetWekaPodTolerationsForCluster(cluster) deletingDriveNodes := NodesWithDeletingDriveContainer(ownContainers) + deletingComputeNodes := NodesWithDeletingComputeContainer(ownContainers) // ownDriveSerials is, per node, the serials allocated to THIS cluster's own drive container that still // holds them, using the same IsDeletingDriveContainer predicate as ExistingDrives (see that function @@ -368,19 +374,20 @@ func (c Collector) FullDrivesInventory(ctx context.Context, cluster *weka.WekaCl cpu, hugepagesMiB, memoryMiB := nodeHeadroom(node, consumed) topo := topos[node.Name] driveInv = append(driveInv, capacityplanner.NodeCapacity{ - NodeName: node.Name, - FDValue: fdValue, - TlcGiB: tlcGiB, - QlcGiB: 0, - DriveCapacitiesGiB: driveCapacitiesGiB, - OwnDriveCapacitiesGiB: fullDriveCapacities(ownDrives), - AllocatableCPU: cpu, - AvailableHugepagesMiB: hugepagesMiB, - AvailableMemoryMiB: memoryMiB, - IsHt: topo.IsHt, - FullPcpusOnly: topo.FullPcpusOnly, - HasDeletingDriveContainer: deletingDriveNodes[node.Name], - IneligibleReason: resources.NodeIneligibleReason(node, tolerations), + NodeName: node.Name, + FDValue: fdValue, + TlcGiB: tlcGiB, + QlcGiB: 0, + DriveCapacitiesGiB: driveCapacitiesGiB, + OwnDriveCapacitiesGiB: fullDriveCapacities(ownDrives), + AllocatableCPU: cpu, + AvailableHugepagesMiB: hugepagesMiB, + AvailableMemoryMiB: memoryMiB, + IsHt: topo.IsHt, + FullPcpusOnly: topo.FullPcpusOnly, + HasDeletingDriveContainer: deletingDriveNodes[node.Name], + HasDeletingComputeContainer: deletingComputeNodes[node.Name], + IneligibleReason: resources.NodeIneligibleReason(node, tolerations), }) } @@ -533,6 +540,28 @@ func NodesWithDeletingDriveContainer(ownContainers []*weka.WekaContainer) map[st return out } +// IsDeletingComputeContainer reports whether c is a compute container on its way out. Such a container's +// pod still physically holds its hugepages/CPU/memory until the pod is actually gone, so a node hosting one +// can fail a drive-growth fit that would pass once the deletion lands. +func IsDeletingComputeContainer(c *weka.WekaContainer) bool { + return c.Spec.Mode == weka.WekaContainerModeCompute && c.IsMarkedForDeletion() +} + +// NodesWithDeletingComputeContainer is the set of nodes hosting one — the source of each node's +// NodeCapacity.HasDeletingComputeContainer. A container with no node resolves to nowhere and is skipped. +func NodesWithDeletingComputeContainer(ownContainers []*weka.WekaContainer) map[string]bool { + out := map[string]bool{} + for _, cont := range ownContainers { + if !IsDeletingComputeContainer(cont) { + continue + } + if n := string(cont.GetNodeAffinity()); n != "" { + out[n] = true + } + } + return out +} + // ExistingDrives builds the planner's view of this cluster's healthy drive containers. func ExistingDrives(ctx context.Context, cluster *weka.WekaCluster, ownContainers []*weka.WekaContainer, fdByNode map[string]string) []capacityplanner.ExistingContainer { fdConfig := cluster.Spec.FailureDomain diff --git a/internal/capacityplanner/nodecapacity.go b/internal/capacityplanner/nodecapacity.go index a2ab1c95b..5b783af95 100644 --- a/internal/capacityplanner/nodecapacity.go +++ b/internal/capacityplanner/nodecapacity.go @@ -23,6 +23,11 @@ type NodeCapacity struct { // HasDeletingDriveContainer: node still runs a this-cluster drive container pending deletion — // excluded from existingDrives but still charged; deprioritizes but doesn't exclude fresh placement. HasDeletingDriveContainer bool + // HasDeletingComputeContainer: node runs a this-cluster compute container pending deletion. Its pod still + // holds hugepages, so a drive growth on this node can fail a fit it would pass once the deletion lands — + // deferred rather than infeasible, since weka may need more active compute elsewhere before it will + // deactivate that container. + HasDeletingComputeContainer bool // DriveCapacitiesGiB: per-drive GiB of each FREE full drive, net of own auto-full-drives allocation // (OwnDriveCapacitiesGiB). Populated only by inventory.FullDrivesInventory; nil for shared-drives nodes. DriveCapacitiesGiB []int diff --git a/internal/capacityplanner/planner.go b/internal/capacityplanner/planner.go index 2b0564dfb..39fea4552 100644 --- a/internal/capacityplanner/planner.go +++ b/internal/capacityplanner/planner.go @@ -174,10 +174,30 @@ const ( WarningKindNodeIneligible WarningKind = "NodeIneligible" ) +// WarningCause further subdivides a WarningKind so the controller's per-reason event throttle can key on +// more than the reason alone: two Warnings of the same Kind but different Cause get independent throttle +// windows, so one cannot silently suppress the other. Empty is legal — a Kind with exactly one cause today +// (DrivesStranded, ComputeLayout) carries "", which reproduces the old reason-only key for it. +type WarningCause string + +const ( + // NodeIneligible has no constants here: it aggregates every ineligible node into one Warning, and its + // Cause is the sorted, "+"-joined set of the distinct resources.NodeIneligibleReason values actually + // present, used verbatim. A reason added there therefore becomes its own cause with no change here, and + // a node going NotReady is never masked by one already cordoned. + // + // CausePlacementUnscheduled / CausePlacementDriveDeleting / CausePlacementComputeDeleting are the three + // PlacementDeferred causes, each rendered as its own Warning instead of merged into one. + CausePlacementUnscheduled WarningCause = "unscheduled-pod" + CausePlacementDriveDeleting WarningCause = "drive-container-deleting" + CausePlacementComputeDeleting WarningCause = "compute-container-deleting" +) + // Warning is one classified planner advisory. Every auto-full-drives warning is fleet-wide: a condition // that can hit several nodes in one pass is reported once, naming every affected node in Message. type Warning struct { Kind WarningKind + Cause WarningCause Message string } @@ -196,7 +216,13 @@ func WarningMessages(warnings []Warning) []string { // fleetWarning builds a classified warning (throttled per reason, not per node). func fleetWarning(kind WarningKind, format string, args ...any) Warning { - return Warning{Kind: kind, Message: fmt.Sprintf(format, args...)} + return fleetWarningWithCause(kind, "", format, args...) +} + +// fleetWarningWithCause is fleetWarning plus a Cause, for a Kind whose Warnings need their own throttle key +// per cause rather than sharing the one key the bare Kind/reason gives them. +func fleetWarningWithCause(kind WarningKind, cause WarningCause, format string, args ...any) Warning { + return Warning{Kind: kind, Cause: cause, Message: fmt.Sprintf(format, args...)} } // CapacityPlan is the planner output. @@ -716,16 +742,17 @@ func planCompute( hugepagesFor := func(count, cores int) int { return ComputeContainerHugepagesMiB(desired.TlcRawGiB, desired.QlcRawGiB, count, cores, cons) } - count, cores, infeasible, warnings := deriveComputeLayout( + count, cores, infeasible, binding, warnings := deriveComputeLayout( desired.ComputeContainers, desired.ComputeCores, plan.RequiredComputeCores, floor, cons.MaxCoresPerContainer, coreHeadroom, nodeHugepagesMiB, hugepagesFor, ) - // Classified at the call site: deriveComputeLayout returns bare strings and is shared with PlanAutoFullDrives. for _, w := range warnings { plan.Warnings = append(plan.Warnings, Warning{Kind: WarningKindComputeLayout, Message: w}) } if infeasible != "" { - setInfeasible(plan, &InfeasibilityReport{Reason: "compute: " + infeasible, Pool: "compute", Fixes: fixesCompute()}) + // ShortfallGiB stays 0: the deficit here is in cores or MiB-hugepages, never GiB, and converting + // either into GiB would invent a number this report never measured. + setInfeasible(plan, &InfeasibilityReport{Reason: "compute: " + infeasible, Pool: "compute", Binding: binding, Fixes: fixesCompute()}) return } diff --git a/internal/controllers/wekaclient/client_reconciler_loop.go b/internal/controllers/wekaclient/client_reconciler_loop.go index 2b86837c2..0c4432734 100644 --- a/internal/controllers/wekaclient/client_reconciler_loop.go +++ b/internal/controllers/wekaclient/client_reconciler_loop.go @@ -82,6 +82,10 @@ type clientReconcilerLoop struct { func ClientReconcileSteps(r *ClientController, wekaClient *weka.WekaClient) lifecycle.StepsEngine { loop := NewClientReconcileLoop(r) loop.wekaClient = wekaClient + // Partitioned once here so the engine's Throttler and loop.RecordEventThrottled's ShouldRun/SetNow calls + // share one partition; NewClientReconcileLoop cannot do this itself since it runs before wekaClient (and + // so its UID) is known. + loop.ThrottlingMap = r.ThrottlingMap.WithPartition("client/" + string(loop.wekaClient.GetUID())) k8sObject := &lifecycle.K8sObject{ Client: loop.Client, @@ -91,7 +95,7 @@ func ClientReconcileSteps(r *ClientController, wekaClient *weka.WekaClient) life return lifecycle.StepsEngine{ StateKeeper: k8sObject, - Throttler: r.ThrottlingMap.WithPartition(string("client/" + loop.wekaClient.GetUID())), + Throttler: loop.ThrottlingMap, Steps: []lifecycle.Step{ &lifecycle.SimpleStep{Run: loop.getCurrentContainers}, &lifecycle.SimpleStep{Run: loop.setApplicableNodes}, diff --git a/internal/controllers/wekacluster/funcs_fd_planning.go b/internal/controllers/wekacluster/funcs_fd_planning.go index f0d820efa..85350be15 100644 --- a/internal/controllers/wekacluster/funcs_fd_planning.go +++ b/internal/controllers/wekacluster/funcs_fd_planning.go @@ -190,7 +190,7 @@ func (r *wekaClusterReconcilerLoop) planAutoFullDrives(ctx context.Context) (*ca // nor own — so a fleet whose every drive is held by one reads exactly like an unsigned fleet here. // Distinguish the two, or the operator is told to sign drives that are already signed. if name, deleting := firstDeletingDriveContainer(r.containers); deleting { - r.emitPlannerEvent(reasonAutoFullDrivesPlacementDeferred, + r.emitPlannerEventWithCause(reasonAutoFullDrivesPlacementDeferred, causeAutoFullDrivesAllDrivesHeldByDeletion, fmt.Sprintf("deferring auto full drives planning: every signed full drive is still held by a drive container being deleted (%s); the drives are signed, just not released yet, and planning resumes on its own once they are", name)) logger.Debug("deferring auto full drives planning while a drive container is being deleted", "container", name) return nil, lifecycle.NewWaitErrorWithDuration( @@ -240,7 +240,7 @@ func (r *wekaClusterReconcilerLoop) planAutoFullDrives(ctx context.Context) (*ca // One reason per cause: each Warning here is already an aggregate naming every node it affects, so one // event per warning is one event per condition, not per node. for _, w := range plan.Warnings { - r.emitPlannerEvent(autoFullDrivesWarningReason(w.Kind), w.Message) + r.emitPlannerEventWithCause(autoFullDrivesWarningReason(w.Kind), string(w.Cause), w.Message) } // Gated on Create only: plan.Grow is applied separately by applyPlannerDriveGrowth, whose caller emits // own cluster-level AutoFullDrivesGrowthDetected and per-container CapacityGrowthApplied events. diff --git a/internal/controllers/wekacluster/funcs_fd_planning_test.go b/internal/controllers/wekacluster/funcs_fd_planning_test.go index d6312139a..beb98f7ce 100644 --- a/internal/controllers/wekacluster/funcs_fd_planning_test.go +++ b/internal/controllers/wekacluster/funcs_fd_planning_test.go @@ -1029,9 +1029,9 @@ func TestPlannerEventSpecsCoverEveryReason(t *testing.T) { {reasonAutoFullDrivesPlanned, corev1.EventTypeNormal, time.Minute}, {reasonAutoFullDrivesGrowthDetected, corev1.EventTypeNormal, time.Minute}, {reasonAutoFullDrivesGrowthDeferred, corev1.EventTypeWarning, plannerConvergedEventInterval}, - // The three fleet-wide aggregates. Each names the affected node set in a message the throttle key - // ignores, so their window bounds how long a node that joins the set after the last event stays - // unreported — it must stay well under the converged-state one. + // The three fleet-wide aggregates. Each names the affected node set in a message the throttle key does + // not see, so a changed set under the same cause still waits out this window before it is + // re-reported — it must stay well under the converged-state one. {reasonAutoFullDrivesDrivesStranded, corev1.EventTypeNormal, plannerAggregateEventInterval}, {reasonAutoFullDrivesPlacementDeferred, corev1.EventTypeNormal, plannerAggregateEventInterval}, {reasonAutoFullDrivesNodeIneligible, corev1.EventTypeNormal, plannerAggregateEventInterval}, @@ -1196,8 +1196,10 @@ func TestAutoFullDrivesWarningReasonAndSeverity(t *testing.T) { // TestPlanAutoFullDrivesAggregatesPlacementDeferredIntoOneEvent is the lab regression: forming a cluster // where several existing drive containers' pods have not bound yet must not fan out into one // AutoFullDrivesPlacementDeferred event per node (lab: 10+ near-identical events on a 14-node cluster, -// differing only in the node name). One pass with N deferred nodes must produce exactly one event naming -// all of them. +// differing only in the node name). Every deferred node here shares the same cause (unscheduled pod), so +// the per-cause split (TestPlanAutoFullDrivesPlacementDeferred_DistinctCausesDoNotShareThrottleWindow) does +// not fragment it: one pass with N deferred nodes under one cause must still produce exactly one event +// naming all of them. func TestPlanAutoFullDrivesAggregatesPlacementDeferredIntoOneEvent(t *testing.T) { withoutFormClusterComputeFloor(t) @@ -1267,6 +1269,118 @@ func TestPlanAutoFullDrivesAggregatesPlacementDeferredIntoOneEvent(t *testing.T) } } +// TestPlanAutoFullDrivesPlacementDeferred_DistinctCausesDoNotShareThrottleWindow is the fix's regression +// test for the lab bug: RecordEventThrottled keyed solely on eventtype+reason, so two +// AutoFullDrivesPlacementDeferred events describing different causes inside one throttle window collided +// and the second was silently dropped (lab: an "unscheduled pod" cause fired, then 18s later a "drive +// container being deleted" cause on a different node was dropped inside the 3-minute window). A repeat of +// the *same* cause inside the window must still be suppressed -- only a genuinely different cause escapes. +func TestPlanAutoFullDrivesPlacementDeferred_DistinctCausesDoNotShareThrottleWindow(t *testing.T) { + withoutFormClusterComputeFloor(t) + + const bigFree = 1 << 28 + + // computeNodes are ample compute-only nodes so each fixture's frozen core demand always fits, keeping + // the plan from going infeasible (which would make the event assertions below vacuous). + computeNodes := func() ([]capacityplanner.NodeCapacity, map[string]string, map[string]bool) { + var inv []capacityplanner.NodeCapacity + fdByNode := map[string]string{} + eligible := map[string]bool{} + for _, name := range []string{"compute-1", "compute-2"} { + inv = append(inv, capacityplanner.NodeCapacity{ + NodeName: name, FDValue: "fd-" + name, + AllocatableCPU: 64, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }) + fdByNode[name] = "fd-" + name + eligible[name] = true + } + return inv, fdByNode, eligible + } + + // unscheduledFixture: an existing drive container whose pod has not bound yet, which ExistingDrives + // marks Unscheduled -- the "unscheduled-pod" cause. + unscheduledFixture := func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + inv, fdByNode, eligible := computeNodes() + inv = append(inv, capacityplanner.NodeCapacity{ + NodeName: "unscheduled-node", FDValue: "fd-unscheduled", + OwnDriveCapacitiesGiB: []int{1000}, + AllocatableCPU: 10, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }) + fdByNode["unscheduled-node"] = "fd-unscheduled" + eligible["unscheduled-node"] = true + return fdByNode, inv, eligible, nil + } + + // deletingFixture: a node with no container of ours yet, whose drive container is mid-deletion -- the + // distinct "drive-container-deleting" cause. + deletingFixture := func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + inv, fdByNode, eligible := computeNodes() + inv = append(inv, capacityplanner.NodeCapacity{ + NodeName: "deleting-node", FDValue: "fd-deleting", + DriveCapacitiesGiB: []int{1000}, TlcGiB: 1000, + AllocatableCPU: 10, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + HasDeletingDriveContainer: true, + }) + fdByNode["deleting-node"] = "fd-deleting" + eligible["deleting-node"] = true + return fdByNode, inv, eligible, nil + } + + unscheduledContainer := &weka.WekaContainer{} + unscheduledContainer.Name = "drive-unscheduled-node" + unscheduledContainer.Spec.Mode = weka.WekaContainerModeDrive + unscheduledContainer.Spec.NodeAffinity = "unscheduled-node" + // Status.NodeAffinity left unset: the pod has not bound, which is what makes ExistingDrives mark it Unscheduled. + unscheduledContainer.Spec.NumDrives = 1 + unscheduledContainer.Spec.DriveCapacity = 1000 + unscheduledContainer.Spec.NumCores = 1 + + phase := 0 + inventoryFn := func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + if phase == 1 { + return deletingFixture() + } + return unscheduledFixture() + } + + r, _ := newAutoFullDrivesLoop([]*weka.WekaContainer{unscheduledContainer}, inventoryFn) + // Generous buffer: FakeRecorder blocks rather than drops once full, and this test drives three passes. + r.Recorder = record.NewFakeRecorder(64) + + // Pass 1: the unscheduled cause fires and is recorded. + if _, err := r.planAutoFullDrives(t.Context()); err != nil { + t.Fatalf("pass 1: planAutoFullDrives() unexpected error: %v", err) + } + // Pass 2, same throttle window: a distinct cause (drive container deleting) under the same reason must + // still be delivered -- this is the bug, the old key could not tell the two apart. deletingFixture's node + // carries no existing container of ours, so r.containers is cleared -- otherwise ExistingDrives would + // still look for unscheduled-node, which this pass's inventory no longer names. + phase = 1 + r.containers = nil + if _, err := r.planAutoFullDrives(t.Context()); err != nil { + t.Fatalf("pass 2: planAutoFullDrives() unexpected error: %v", err) + } + // Pass 3, same window: pass 1's cause repeats and must be suppressed. + phase = 0 + r.containers = []*weka.WekaContainer{unscheduledContainer} + if _, err := r.planAutoFullDrives(t.Context()); err != nil { + t.Fatalf("pass 3: planAutoFullDrives() unexpected error: %v", err) + } + + got := eventsMatching(drainLoopEvents(t, r), "AutoFullDrivesPlacementDeferred") + if len(got) != 2 { + t.Fatalf("got %d AutoFullDrivesPlacementDeferred event(s), want exactly 2 (pass 1's unscheduled cause, "+ + "pass 2's distinct deleting cause; pass 3's repeat of pass 1's cause must be suppressed): %v", + len(got), got) + } + if !strings.Contains(got[0], "unscheduled-node") || !strings.Contains(got[0], "pod not scheduled yet") { + t.Errorf("event[0] = %q, want pass 1's unscheduled-cause event naming unscheduled-node", got[0]) + } + if !strings.Contains(got[1], "deleting-node") || strings.Contains(got[1], "unscheduled-node") { + t.Errorf("event[1] = %q, want pass 2's distinct deleting-cause event naming only deleting-node", got[1]) + } +} + // withoutFormClusterComputeFloor disables the form-cluster compute-container floor // (CapacityConstraints.MinComputeContainers) for a test; these fixtures use small fleets that couldn't // host it otherwise. The floor itself is covered by TestPlanAutoFullDrivesHonorsFormClusterComputeFloor. @@ -1382,3 +1496,120 @@ func TestPlanAutoFullDrivesInfeasibleSuppressesAdvisories(t *testing.T) { } } } + +// TestPlanAutoFullDrivesInfeasibleEventExplainsBlockedGrowth is the controller-level wiring check for the +// event body: a growth-kind node-fit failure (an existing drive container that must grow into headroom it no +// longer has) must produce an AutoFullDrivesInfeasible event naming the node, the shortfall and the one +// remedy that applies — and must NOT carry the rest of the Fixes catalog, which is terminal-side detail that +// tripled the length of a message an operator reads in kubectl describe. +func TestPlanAutoFullDrivesInfeasibleEventExplainsBlockedGrowth(t *testing.T) { + withoutFormClusterComputeFloor(t) + + existingDrive := &weka.WekaContainer{} + existingDrive.Spec.Mode = weka.WekaContainerModeDrive + existingDrive.Spec.NodeAffinity = weka.NodeName("n1") + existingDrive.Spec.NumDrives = 2 + existingDrive.Spec.NumCores = 4 + existingDrive.Spec.DriveCapacity = 1000 + existingDrive.Status.NodeAffinity = "n1" // scheduled: an unscheduled container is skipped before the fit check + + // The clause tells the operator to delete this cluster's compute container on n1, so n1 must actually run + // one — without it the plan is infeasible for an unrelated reason and the advice names nothing real. + existingCompute := &weka.WekaContainer{} + existingCompute.Spec.Mode = weka.WekaContainerModeCompute + existingCompute.Spec.NodeAffinity = weka.NodeName("n1") + existingCompute.Spec.NumCores = 4 + existingCompute.Spec.Hugepages = 12000 + existingCompute.Status.NodeAffinity = "n1" + + loop, _ := newAutoFullDrivesLoop([]*weka.WekaContainer{existingDrive, existingCompute}, + func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + return map[string]string{}, []capacityplanner.NodeCapacity{ + { + NodeName: "n1", FDValue: "n1", + // 2 drives already held by existingDrive (own) plus 2 more signed and free: the node + // walk must grow the container from 2 to 4 drives at the same 4 cores, so only the + // per-drive hugepages term changes — and AvailableHugepagesMiB: 0 makes that fail. + OwnDriveCapacitiesGiB: []int{1000, 1000}, + DriveCapacitiesGiB: []int{1000, 1000}, + AllocatableCPU: 64, AvailableHugepagesMiB: 0, AvailableMemoryMiB: 1 << 28, + }, + }, map[string]bool{"n1": true}, nil + }) + + plan, err := loop.planAutoFullDrives(context.Background()) + if err == nil || plan != nil { + t.Fatalf("want an infeasible plan to return a WaitError and no plan, got plan=%v err=%v", plan, err) + } + + events := drainLoopEvents(t, loop) + got := eventsMatching(events, reasonAutoFullDrivesInfeasible) + if len(got) != 1 { + t.Fatalf("want exactly 1 %s event, got %d: %v", reasonAutoFullDrivesInfeasible, len(got), events) + } + msg := got[0] + if !strings.Contains(msg, "n1") { + t.Fatalf("event must name n1, got %q", msg) + } + if !strings.Contains(msg, "needs 400 MiB hugepages") { + t.Fatalf("event must carry the shortfall figures, got %q", msg) + } + if !strings.Contains(msg, "may be blocked by this cluster's own compute container") { + t.Fatalf("event must explain the blocked growth, got %q", msg) + } + if !strings.Contains(msg, "deleting it lets the next reconcile grow the drive container first") { + t.Fatalf("event must name the remedy, got %q", msg) + } + // The catalog stays in the structured report for the CLI; an event carrying it was unreadably long. + if strings.Contains(msg, "1. ") || strings.Contains(msg, "or switch to a drive-sharing mode") { + t.Fatalf("event must not carry the numbered fix catalog, got %q", msg) + } + if len(msg) > 700 { + t.Fatalf("event message is %d chars; it is read in kubectl describe and must stay terse: %q", len(msg), msg) + } +} + +// TestPlanAutoFullDrivesInfeasibleEventOmitsBlockedGrowthWithoutComputeContainer is the negative half: the +// same growth shortfall on a node running none of this cluster's compute containers must not offer the +// delete-compute remedy. Growth alone does not imply the hazard — the drive container can simply be on a node +// with nothing left — and naming a container that does not exist sends the operator hunting for it. +func TestPlanAutoFullDrivesInfeasibleEventOmitsBlockedGrowthWithoutComputeContainer(t *testing.T) { + withoutFormClusterComputeFloor(t) + + existingDrive := &weka.WekaContainer{} + existingDrive.Spec.Mode = weka.WekaContainerModeDrive + existingDrive.Spec.NodeAffinity = weka.NodeName("n1") + existingDrive.Spec.NumDrives = 2 + existingDrive.Spec.NumCores = 4 + existingDrive.Spec.DriveCapacity = 1000 + existingDrive.Status.NodeAffinity = "n1" + + loop, _ := newAutoFullDrivesLoop([]*weka.WekaContainer{existingDrive}, + func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + return map[string]string{}, []capacityplanner.NodeCapacity{ + { + NodeName: "n1", FDValue: "n1", + OwnDriveCapacitiesGiB: []int{1000, 1000}, + DriveCapacitiesGiB: []int{1000, 1000}, + AllocatableCPU: 64, AvailableHugepagesMiB: 0, AvailableMemoryMiB: 1 << 28, + }, + }, map[string]bool{"n1": true}, nil + }) + + plan, err := loop.planAutoFullDrives(context.Background()) + if err == nil || plan != nil { + t.Fatalf("want an infeasible plan to return a WaitError and no plan, got plan=%v err=%v", plan, err) + } + + got := eventsMatching(drainLoopEvents(t, loop), reasonAutoFullDrivesInfeasible) + if len(got) != 1 { + t.Fatalf("want exactly 1 %s event, got %d", reasonAutoFullDrivesInfeasible, len(got)) + } + msg := got[0] + if !strings.Contains(msg, "needs 400 MiB hugepages") { + t.Fatalf("event must still carry the shortfall figures, got %q", msg) + } + if strings.Contains(msg, "compute container") { + t.Fatalf("no compute container of ours runs on n1, so the event must not blame one, got %q", msg) + } +} diff --git a/internal/controllers/wekacluster/planner_events.go b/internal/controllers/wekacluster/planner_events.go index 5bf2d28e9..2fdcea1f6 100644 --- a/internal/controllers/wekacluster/planner_events.go +++ b/internal/controllers/wekacluster/planner_events.go @@ -36,6 +36,12 @@ const ( // reasonCapacityGrowthApplied lands on the WekaContainer, not the cluster, and is unthrottled — hence no // plannerEventSpecs row. It is named here only so the growth appliers and their tests share one spelling. reasonCapacityGrowthApplied = "CapacityGrowthApplied" + + // causeAutoFullDrivesAllDrivesHeldByDeletion is the pre-plan deferral emitted when every signed full + // drive is still held by a deleting drive container (no plan has run yet, so there is no Warning to + // carry a capacityplanner.WarningCause). It is distinct from capacityplanner.CausePlacementDriveDeleting, + // which covers the same condition once a plan exists, so the two never share a throttle window. + causeAutoFullDrivesAllDrivesHeldByDeletion = "all-signed-drives-held-by-deletion" ) // plannerConvergedEventInterval throttles advisories describing a converged cluster — one that is @@ -44,10 +50,10 @@ const ( const plannerConvergedEventInterval = 15 * time.Minute // plannerAggregateEventInterval throttles the fleet-wide aggregates, whose message names the affected node -// set. RecordEventThrottled keys on eventtype+reason and ignores the message, so a window also withholds an -// aggregate naming a *different* set: a node cordoned two minutes after the first event would otherwise wait -// out the whole converged-state window. Short enough that a changed set is reported promptly, long enough -// that a stable one is not re-posted every reconcile. +// set. emitPlannerEvent keys on eventtype+reason+cause, so a distinct cause (e.g. a node going NotReady vs. +// one merely cordoned) gets its own window instead of waiting out one already open for a different cause. A +// changed node set under the *same* cause is still bounded by this window — short enough that it is reported +// promptly, long enough that a stable set is not re-posted every reconcile. const plannerAggregateEventInterval = 3 * time.Minute type plannerEventSpec struct { @@ -82,15 +88,22 @@ var plannerEventSpecs = map[string]plannerEventSpec{ reasonAutoFullDrivesWarning: {corev1.EventTypeWarning, plannerConvergedEventInterval}, } -// emitPlannerEvent records message on the WekaCluster under reason, with that reason's policy. +// emitPlannerEvent records message on the WekaCluster under reason, with that reason's policy. For +// reasons whose Warnings need their own throttle key per cause, use emitPlannerEventWithCause instead. func (r *wekaClusterReconcilerLoop) emitPlannerEvent(reason, message string) { + r.emitPlannerEventWithCause(reason, "", message) +} + +// emitPlannerEventWithCause is emitPlannerEvent plus a cause that further keys the throttle, so a distinct +// condition under the same reason (e.g. a node going NotReady vs. one merely cordoned) gets its own window. +func (r *wekaClusterReconcilerLoop) emitPlannerEventWithCause(reason, cause, message string) { spec, known := plannerEventSpecs[reason] if !known { // A reason with no row is a programming error that TestPlannerEventSpecsCoverEveryReason catches; still // emit rather than silently drop it. spec = plannerEventSpec{eventType: corev1.EventTypeWarning, interval: time.Minute} } - _ = r.RecordEventThrottled(spec.eventType, reason, message, spec.interval) //nolint:errcheck // best effort + _ = r.RecordEventThrottledKeyed(spec.eventType, reason, cause, message, spec.interval) //nolint:errcheck // best effort } // autoFullDrivesWarningReasons gives each planner warning cause its own reason, so operators can filter with diff --git a/internal/controllers/wekacluster/reconciler_loop.go b/internal/controllers/wekacluster/reconciler_loop.go index 2e1c13854..5fba1ff0b 100644 --- a/internal/controllers/wekacluster/reconciler_loop.go +++ b/internal/controllers/wekacluster/reconciler_loop.go @@ -172,8 +172,17 @@ func (r *wekaClusterReconcilerLoop) RecordEvent(eventtype, reason, message strin return nil } +// RecordEventThrottled throttles on eventtype+reason alone, for callers with no cause to distinguish. func (r *wekaClusterReconcilerLoop) RecordEventThrottled(eventtype, reason, message string, interval time.Duration) error { - if !r.Throttler.ShouldRun(eventtype+reason, &throttling.ThrottlingSettings{ + return r.RecordEventThrottledKeyed(eventtype, reason, "", message, interval) +} + +// RecordEventThrottledKeyed throttles on eventtype+reason+cause, so two events sharing a reason but +// describing different causes get independent windows: one cannot silently suppress the other. cause may be +// empty, which reproduces the plain eventtype+reason key. +func (r *wekaClusterReconcilerLoop) RecordEventThrottledKeyed(eventtype, reason, cause, message string, interval time.Duration) error { + key := eventtype + "|" + reason + "|" + cause + if !r.Throttler.ShouldRun(key, &throttling.ThrottlingSettings{ Interval: interval, DisableRandomPreSetInterval: true, }) { diff --git a/internal/controllers/wekacontainer/controller.go b/internal/controllers/wekacontainer/controller.go index 3290e3f4f..ab9b1d2cf 100644 --- a/internal/controllers/wekacontainer/controller.go +++ b/internal/controllers/wekacontainer/controller.go @@ -50,7 +50,7 @@ type ContainerController struct { ExecService exec.ExecService Manager ctrl.Manager RestClient rest.Interface - ThrottlingMap throttling.Throttler // TODO: Implement GC, so it will be cleaned up(maybe stored in different place as well) when containers are no more. Low priority as we dont expect lots of rotation + ThrottlingMap throttling.Throttler } // +kubebuilder:rbac:groups=weka.weka.io,resources=wekaclusters,verbs=get;list;watch;create;update;patch;delete diff --git a/pkg/go-steps-engine b/pkg/go-steps-engine index 08f7c0d01..dc27ed3f5 160000 --- a/pkg/go-steps-engine +++ b/pkg/go-steps-engine @@ -1 +1 @@ -Subproject commit 08f7c0d01669e7f1cf5a8c4ecc9ce86b767a8860 +Subproject commit dc27ed3f50b6d7beecd19cf0d1449e6da4e264f5