Skip to content

fix: aggregate auto-full-drives planner warnings into one event per condition instead of per node #2768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kristina-solovyova wants to merge 1 commit into from
Closed

fix: aggregate auto-full-drives planner warnings into one event per condition instead of per node #2768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 125 additions & 67 deletions cmd/weka-capacity/autofulldrives_test.go
View file
Open in desktop

Large diffs are not rendered by default.

62 changes: 32 additions & 30 deletions cmd/weka-capacity/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ type planData struct {
}

// autoFullDrivesNodeRow is the per-node dry-run view: DrivesAvail vs DrivesUsed, with State one of
// create/grow/existing/not-planned. Note surfaces the matching plan.Warnings message when
// DrivesUsed < DrivesAvail.
// create/grow/existing/not-planned. Note explains a row holding fewer drives than it offers — derived from
// that node's own condition, not from the warning text — and points at WARNINGS for the fleet-wide detail.
type autoFullDrivesNodeRow struct {
Node string `json:"node"`
FD string `json:"fd"`
Expand Down Expand Up @@ -599,25 +599,14 @@ func autoFullDrivesDriveGrowDiff(existing []capacityplanner.ExistingContainer, g
return rows
}

// warningForNode returns the plan.Warning whose Subject matches node (set by the producer in
// autofulldrives.go), avoiding a fragile substring match on the warning's prose message. Returns "" if
// none match.
func warningForNode(warnings []capacityplanner.Warning, node string) string {
for _, w := range warnings {
if w.Subject == node {
return w.Message
}
}
return ""
}

// hasFleetWarning reports whether plan carries a fleet-wide warning of the given kind — one with no
// Subject, because its cause (e.g. a numDrives pin) applies across the whole fleet rather than to one
// node. DrivesStranded is the live example: formatStrandedWarning aggregates every stranded node into a
// single warning instead of fanning one out per node, so warningForNode can never find it by Subject.
// hasFleetWarning reports whether plan carries a warning of the given kind. Every planner warning is
// fleet-wide (see capacityplanner.Warning), naming every affected node in its Message rather than in a
// per-warning field, so a row can only point at the warning by Kind — it can never pull its own text out of
// one. Deriving a row's text from a kind is only sound for a single-cause kind (DrivesStranded); gating a
// pointer at the WARNINGS section is sound for any kind.
func hasFleetWarning(warnings []capacityplanner.Warning, kind capacityplanner.WarningKind) bool {
for _, w := range warnings {
if w.Kind == kind && w.Subject == "" {
if w.Kind == kind {
return true
}
}
Expand All @@ -628,10 +617,12 @@ func hasFleetWarning(warnings []capacityplanner.Warning, kind capacityplanner.Wa
// skipped). State is grow/existing/create by cross-referencing plan.Grow/Create, else nodeStateNotPlanned
// — reachable on a FEASIBLE plan for a node withheld from a new container (cordoned/not ready/untolerated
// taint), not only on an infeasible one. DrivesAvail sums own-claimed + free (not free-only, so
// self-claimed nodes still show); Note surfaces warningForNode when DrivesUsed < DrivesAvail — which
// covers both the per-node WarningKindNodeIneligible case and the stranded case below — falling back to a
// short pointer at the fleet-wide DrivesStranded warning (a numDrives pin, never attributed to one node's
// Subject — see hasFleetWarning) on a row the plan actually placed something on. Sorted by node name.
// self-claimed nodes still show); when DrivesUsed < DrivesAvail, Note explains the gap. For a row the walk
// never sized at all (nodeStateNotPlanned) the node's own inventory holds the authoritative answer, so the
// reason is read from IneligibleReason/HasDeletingDriveContainer rather than from the aggregated warning —
// which names every affected node in one message and so cannot say which condition applies to THIS row. A
// row the walk did size but couldn't claim every signed drive on gets a pointer at DrivesStranded (a
// numDrives pin), whose one cause needs no per-node disambiguation. Sorted by node name.
func autoFullDrivesNodeRows(nodeInv []capacityplanner.NodeCapacity, existing []capacityplanner.ExistingContainer, plan *capacityplanner.CapacityPlan) []autoFullDrivesNodeRow {
existingByNode := make(map[string]capacityplanner.ExistingContainer, len(existing))
for _, e := range existing {
Expand All @@ -646,6 +637,16 @@ func autoFullDrivesNodeRows(nodeInv []capacityplanner.NodeCapacity, existing []c
createByNode[cr.Node] = cr
}

// The reason is the node's own; the pointer is only earned when the matching aggregate reached
// plan.Warnings — a mid-walk infeasible abort stops collecting, leaving later nodes a condition
// with no warning to point at.
note := func(kind capacityplanner.WarningKind, reason string) string {
if hasFleetWarning(plan.Warnings, kind) {
return reason + " — see WARNINGS"
}
return reason
}
Comment on lines +640 to +648

@claude claude Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating the pointer on the aggregate being present is the right shape, and it closes the "no warning at all" half of the previous finding — but it doesn't close the half where the aggregate exists yet doesn't cover this node. flushWarnings snapshots the nodes walked before the abort; the row conditions come from NodeCapacity, which is populated for all of them. Since both are name-ordered, a node sorted after the failing one can share a kind with an earlier node and still be missing from the message:

a-cordoned   →  ineligible = ["a-cordoned (cordoned)"]
m-pinned     →  autoSizeNode reports; flushWarnings(); return
z-cordoned   →  never walked

WARNINGS then reads 1 node(s) ... are ineligible ...: a-cordoned (cordoned) while z-cordoned's row says cordoned — see WARNINGS. Same for WarningKindTransient, which is worse because it carries two causes: one deferred node before the abort makes hasFleetWarning(Transient) true, so a HasDeletingDriveContainer node after the abort points at a message whose only clause is "pod not scheduled yet".

That also makes the hasFleetWarning doc's second sentence too strong — "gating a pointer at the WARNINGS section is sound for any kind" holds only when kind-presence implies this row is named in it, which is exactly what the abort breaks.

Matching on the node name inside the message is not the fix (it reintroduces the n1/n10 collision warningForNode was written to avoid). Either drop the suffix on the three per-node cases — the reason is already verbatim on the row and the aggregate adds only a fleet total, so nothing is lost — or have the planner mark whether the walk ran to completion and gate on that, since warnings are complete for the autoNodeFitInfeasible path and incomplete only for the mid-walk one.

The existing regression test only covers plan.Warnings == nil; the case above needs a fixture where the aggregate is present but partial. Fix this →


rows := make([]autoFullDrivesNodeRow, 0, len(nodeInv))
for i := range nodeInv {
n := &nodeInv[i]
Expand Down Expand Up @@ -687,13 +688,14 @@ func autoFullDrivesNodeRows(nodeInv []capacityplanner.NodeCapacity, existing []c
row.DrivesUsed = 0
}
if row.DrivesUsed < row.DrivesAvail {
row.Note = warningForNode(plan.Warnings, n.NodeName)
// A stranded node has no per-node Subject to match above (see hasFleetWarning) — point at the
// fleet warning instead, but only for a row the plan actually placed something on:
// nodeStateNotPlanned means the walk never sized this node at all, so "held back by the pin"
// would misattribute an infeasibility skip to the pin.
if row.Note == "" && row.State != nodeStateNotPlanned &&
hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded) {
switch {
case row.State == nodeStateNotPlanned && n.IneligibleReason != "":
row.Note = note(capacityplanner.WarningKindNodeIneligible, n.IneligibleReason)
case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer:
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")
case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded):
row.Note = "drives held back by the numDrives pin — see WARNINGS"
Comment on lines +692 to 699

@claude claude Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flushWarnings fixes the prefix of the walk, but the NOTE column still promises WARNINGS entries for its suffix. These three per-node cases read the node's own inventory, which is populated for every node regardless of how far the walk got — while the aggregated warnings only cover nodes visited before an autoSizeNode infeasibility returned (autofulldrives.go:171-176). Nodes are walked and rendered in name order, so a node sorted after the failing one gets "cordoned — see WARNINGS" with no NodeIneligible entry in WARNINGS to see.

Concretely, flip the fixture in TestPlanAutoFullDrives_InfeasibleMidWalk_KeepsWarningsAlreadyCollected (a-pinned / z-cordoned instead of a-cordoned / z-pinned): the plan is infeasible, z-cordoned's row says "cordoned — see WARNINGS", and WARNINGS has nothing about it. Same for HasDeletingDriveContainer and Unscheduled.

The suffix is what creates the promise, and for these three cases it earns nothing — the reason is already spelled out verbatim on the row, and the aggregated warning adds only the fleet total. Dropping it makes the note true on every plan:

Suggested change
case row.State == nodeStateNotPlanned && n.IneligibleReason != "":
row.Note = n.IneligibleReason + " — see WARNINGS"
case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer:
row.Note = "drive container being deleted — see WARNINGS"
case existingByNode[n.NodeName].Unscheduled:
row.Note = "pod has not been scheduled yet — see WARNINGS"
case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded):
row.Note = "drives held back by the numDrives pin — see WARNINGS"
switch {
case row.State == nodeStateNotPlanned && n.IneligibleReason != "":
row.Note = n.IneligibleReason
case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer:
row.Note = "drive container being deleted"
case existingByNode[n.NodeName].Unscheduled:
row.Note = "pod has not been scheduled yet"
case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded):
row.Note = "drives held back by the numDrives pin — see WARNINGS"
}

The DrivesStranded case keeps its pointer legitimately: it is gated on the warning actually being present.

}
}
Comment on lines 690 to 701

@claude claude Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misattributed NOTE for not-planned rows. With Subject gone, this branch fires for every nodeStateNotPlanned row as soon as any node in the fleet is ineligible — the row itself may have nothing to do with cordon/taint. Two concrete cases:

  • a node skipped for HasDeletingDriveContainer (autofulldrives.go:146) is notPlanned with DrivesUsed=0 < DrivesAvail, and gets labelled cordoned/not ready/untolerated taint if some other node happens to be cordoned. Before this PR, warningForNode gave it its own "container being deleted" text.
  • on an infeasible plan, every fit-failure node is notPlanned too, and gets the same wrong label.

The row already has the authoritative per-node answer in hand — NodeCapacity.IneligibleReason / HasDeletingDriveContainer (internal/capacityplanner/nodecapacity.go:36) — so the aggregated warning doesn't need to be consulted for this at all:

Suggested change
if row.DrivesUsed < row.DrivesAvail {
row.Note = warningForNode(plan.Warnings, n.NodeName)
// A stranded node has no per-node Subject to match above (see hasFleetWarning) — point at the
// fleet warning instead, but only for a row the plan actually placed something on:
// nodeStateNotPlanned means the walk never sized this node at all, so "held back by the pin"
// would misattribute an infeasibility skip to the pin.
if row.Note == "" && row.State != nodeStateNotPlanned &&
hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded) {
switch {
case row.State == nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindNodeIneligible):
row.Note = "cordoned/not ready/untolerated taint — see WARNINGS"
case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded):
row.Note = "drives held back by the numDrives pin — see WARNINGS"
}
}
if row.DrivesUsed < row.DrivesAvail {
switch {
case row.State == nodeStateNotPlanned && n.IneligibleReason != "":
row.Note = n.IneligibleReason + " — see WARNINGS"
case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer:
row.Note = "drive container being deleted — see WARNINGS"
case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded):
row.Note = "drives held back by the numDrives pin — see WARNINGS"
}
}

That also makes the NOTE strictly more informative (the actual reason verbatim, matching what the warning says about that node) and leaves hasFleetWarning with a single caller.

Expand Down
3 changes: 2 additions & 1 deletion cmd/weka-capacity/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,8 @@ func renderAutoFullDrivesPlanText(d *autoFullDrivesPlanData) string {
}

// NODES: one row per node with a signed full drive — drives used/avail, TLC, cores, and STATE
// (create/grow/existing/not-planned). NOTE surfaces the matching Warnings entry directly on the row.
// (create/grow/existing/not-planned). NOTE explains a row holding fewer drives than it offers and points
// at the WARNINGS list below for the fleet-wide detail.
if len(d.Nodes) > 0 {
fmt.Fprintln(&buf, "\nNODES") //nolint:errcheck // writes to an in-memory buffer/tabwriter; cannot fail in practice
tw := tabwriter.NewWriter(&buf, 0, 2, 2, ' ', 0)
Expand Down
36 changes: 20 additions & 16 deletions doc/operator/deployment/act-as-daemonset.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,24 +845,27 @@ Every reason below lands on the **`WekaCluster`** except `UnschedulableDriveCont
**`WekaContainer`** — so `kubectl describe wekacluster <name>` alone will not show them. Check
`kubectl describe wekacontainer <name>` when you need per-container detail.

Events are throttled per reason: a repeat within the window is dropped rather than re-posted. The
advisories that describe a **converged** state use a long window, because a permanently
compute-limited cluster is healthy and re-posting a Warning every minute forever trips alerting.
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
`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.
Per-node reasons throttle per **node** rather than per message.

| Reason | Type | Object | Throttle | When it fires |
|--------|------|--------|----------|---------------|
| `AutoFullDrivesPlanned` | Normal | Cluster | 1 min | A feasible plan that creates ≥1 drive container. Steady-state reconciles stay silent. The message summarizes the create leg. |
| `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 | 15 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 | 15 min, **per node** | A node still hosts a this-cluster drive container that is being deleted, so placement waits; or an existing container's growth is deferred because its pod is not yet scheduled. Clears itself on a later reconcile. |
| `AutoFullDrivesNodeIneligible` | Normal | Cluster | 15 min, **per node** | 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. Rate-limited per node, since a node left cordoned for maintenance would otherwise post forever. Anything already running there keeps running and still grows. See [Troubleshooting](#troubleshooting). |
| `AutoFullDrivesComputeLayout` | Warning | Cluster | 15 min | A compute-sizing advisory from the shared compute layout step. |
| `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 | One aggregated message covering every node where placement is waiting this pass, for either cause: 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. Clears itself as pods bind. |
| `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). |
| `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. |
| `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. |
Expand Down Expand Up @@ -971,20 +974,21 @@ it has but never gets. Look for a Warning `UnschedulableComputeContainer` event

**A node is cordoned, `NotReady`, or carries a taint the Weka pods don't tolerate.**
No new container is placed on that node while the condition lasts, and a Normal
`AutoFullDrivesNodeIneligible` event on the `WekaCluster` names the node and the reason. Nothing is
taken away either: a container already there keeps running, its drives and resources still count as
used, and it can still grow in place — cordoning does not evict, so a node briefly down for maintenance
is not treated as lost capacity. Its unclaimed drives still count toward the fleet total the plan
reports, so a summary reading *"40 of 48 drive(s) would be claimed"* is telling you eight drives are out
of reach, not that they vanished.
`AutoFullDrivesNodeIneligible` event on the `WekaCluster` names every ineligible node together with
its own reason. Nothing is taken away either: a container already there keeps running, its drives
and resources still count as used, and it can still grow in place — cordoning does not evict, so a
node briefly down for maintenance is not treated as lost capacity. Its unclaimed drives still count
toward the fleet total the plan reports, so a summary reading *"40 of 48 drive(s) would be
claimed"* is telling you eight drives are out of reach, not that they vanished.

This is a **skip, not an infeasibility** — the plan proceeds on the remaining nodes. It only becomes
fatal indirectly, when so many nodes are ineligible that what is left cannot satisfy the form-cluster
minimum or the compute ratio. When that happens the binding message describes the shortfall on the
nodes that *remain* (it will say the compute ratio cannot be met across *N* nodes, not that a node was
cordoned), so read the per-node rejection breakdown — which lists an excluded node as
`ineligible (cordoned)` — and the `AutoFullDrivesNodeIneligible` events alongside it. A plan that went
infeasible right after a maintenance cordon is usually short exactly that node.
`ineligible (cordoned)`; the `AutoFullDrivesNodeIneligible` advisory itself is suppressed on an
infeasible plan. A plan that went infeasible right after a maintenance cordon is usually short exactly
that node.

Run `weka-capacity explore-nodes` and read the `INELIGIBLE` column for the reason — `cordoned`,
`not ready`, or `untolerated taint`. Clear the condition (uncordon the node, remove the taint, get it
Expand Down
Loading
Loading