diff --git a/cmd/weka-capacity/autofulldrives_test.go b/cmd/weka-capacity/autofulldrives_test.go index 1191ad994..13834372d 100644 --- a/cmd/weka-capacity/autofulldrives_test.go +++ b/cmd/weka-capacity/autofulldrives_test.go @@ -205,21 +205,26 @@ func TestDescribeSizingMode(t *testing.T) { } // TestAutoFullDrivesNodeRows covers the four per-node states (create/grow/existing/not-planned), -// drives-used-vs-avail accounting, and surfacing a node's Warnings entry in its row Note. +// drives-used-vs-avail accounting, and reading a not-planned row's Note from the node's own +// IneligibleReason rather than from the aggregated fleet warning. func TestAutoFullDrivesNodeRows(t *testing.T) { nodeInv := []capacityplanner.NodeCapacity{ {NodeName: "n-create", FDValue: "n-create", DriveCapacitiesGiB: []int{3840, 3840}}, {NodeName: "n-grow", FDValue: "n-grow", DriveCapacitiesGiB: []int{3840, 3840, 3840}}, {NodeName: "n-existing", FDValue: "n-existing", DriveCapacitiesGiB: []int{3840}}, - {NodeName: "n-deferred", FDValue: "n-deferred", DriveCapacitiesGiB: []int{3840, 3840}}, + // IneligibleReason, not the plan's aggregated warning, is what the row's Note must read. + {NodeName: "n-ineligible", FDValue: "n-ineligible", DriveCapacitiesGiB: []int{3840, 3840}, IneligibleReason: "not ready"}, {NodeName: "n-empty", FDValue: "n-empty", DriveCapacitiesGiB: nil}, // 0 signed drives — must be skipped entirely // n-own-only: both drives already own-claimed (0 free); avail must be len(Own)+len(Drive) (plan.go §7). {NodeName: "n-own-only", FDValue: "n-own-only", OwnDriveCapacitiesGiB: []int{3840, 3840}, DriveCapacitiesGiB: nil}, + // n-unsched: growth deferred because its pod has not bound, so used < avail with no numDrives pin. + {NodeName: "n-unsched", FDValue: "n-unsched", OwnDriveCapacitiesGiB: []int{3840}, DriveCapacitiesGiB: []int{3840, 3840}}, } existing := []capacityplanner.ExistingContainer{ {Name: "c-grow", Node: "n-grow", TlcGiB: 7680, NumCores: 2, NumDrives: 2}, {Name: "c-existing", Node: "n-existing", TlcGiB: 3840, NumCores: 1, NumDrives: 1}, {Name: "c-own-only", Node: "n-own-only", TlcGiB: 7680, NumCores: 2, NumDrives: 2}, + {Name: "c-unsched", Node: "n-unsched", TlcGiB: 3840, NumCores: 1, NumDrives: 1, Unscheduled: true}, } plan := &capacityplanner.CapacityPlan{ Create: []capacityplanner.NewContainer{ @@ -228,19 +233,15 @@ func TestAutoFullDrivesNodeRows(t *testing.T) { Grow: []capacityplanner.ContainerGrowth{ {Name: "c-grow", NewTlcGiB: 11520, NewCores: 3, NewNumDrives: 3}, }, - // Subject is what warningForNode matches on, so it must be set exactly as the planner sets it. - // Transient (placement deferred) is now the ONLY node-subject warning the planner emits: the two - // per-node fit warnings became whole-plan infeasibilities, and stranding is fleet-wide. Warnings: []capacityplanner.Warning{{ - Kind: capacityplanner.WarningKindTransient, - Subject: "n-deferred", - Message: "node n-deferred still hosts a drive container being deleted — placement deferred", + Kind: capacityplanner.WarningKindNodeIneligible, + Message: "1 node(s) holding 2 signed free full drive(s) are ineligible for a new drive container: n-ineligible (not ready)", }}, } rows := autoFullDrivesNodeRows(nodeInv, existing, plan) - if len(rows) != 5 { - t.Fatalf("autoFullDrivesNodeRows() returned %d rows, want 5 (n-empty must be skipped, n-own-only must NOT be); rows=%+v", len(rows), rows) + if len(rows) != 6 { + t.Fatalf("autoFullDrivesNodeRows() returned %d rows, want 6 (n-empty must be skipped, n-own-only must NOT be); rows=%+v", len(rows), rows) } byNode := map[string]autoFullDrivesNodeRow{} for _, r := range rows { @@ -265,12 +266,12 @@ func TestAutoFullDrivesNodeRows(t *testing.T) { t.Errorf("existing row = %+v, want state=existing used=1 avail=1 tlc=3840 cores=1", existingRow) } - deferred := byNode["n-deferred"] - if deferred.State != nodeStateNotPlanned || deferred.DrivesUsed != 0 || deferred.DrivesAvail != 2 { - t.Errorf("unplanned row = %+v, want state=%s used=0 avail=2", deferred, nodeStateNotPlanned) + ineligible := byNode["n-ineligible"] + if ineligible.State != nodeStateNotPlanned || ineligible.DrivesUsed != 0 || ineligible.DrivesAvail != 2 { + t.Errorf("unplanned row = %+v, want state=%s used=0 avail=2", ineligible, nodeStateNotPlanned) } - if !strings.Contains(deferred.Note, "placement deferred") { - t.Errorf("unplanned row Note = %q, want it to carry the matching Warnings entry", deferred.Note) + if !strings.Contains(ineligible.Note, "not ready") || !strings.Contains(ineligible.Note, "WARNINGS") { + t.Errorf("unplanned row Note = %q, want the node's own reason verbatim plus a pointer to WARNINGS", ineligible.Note) } ownOnly, ok := byNode["n-own-only"] @@ -283,6 +284,14 @@ func TestAutoFullDrivesNodeRows(t *testing.T) { if ownOnly.Note != "" { t.Errorf("n-own-only row Note = %q, want empty (used == avail, nothing held back)", ownOnly.Note) } + + unsched := byNode["n-unsched"] + if unsched.State != "existing" || unsched.DrivesUsed != 1 || unsched.DrivesAvail != 3 { + t.Errorf("n-unsched row = %+v, want state=existing used=1 avail=3 (no Grow entry is written while the pod is unbound)", unsched) + } + if !strings.Contains(unsched.Note, "not been scheduled") { + t.Errorf("n-unsched row Note = %q, want the deferred-growth reason", unsched.Note) + } } // TestAutoFullDrivesNodeRows_FeasiblePlanPlacesEveryNode is the semantic guard behind the not-planned @@ -312,70 +321,70 @@ func TestAutoFullDrivesNodeRows_FeasiblePlanPlacesEveryNode(t *testing.T) { } } -// TestWarningForNode covers correlating a Warnings entry to its node row via Subject, not message prose; -// the n1/n10 case proves the old prefix-collision risk is structurally impossible. -// -// Three WarningKinds survive — DrivesStranded, Transient, ComputeLayout — and only Transient carries a -// node Subject (autofulldrives.go's placement-deferred warning is its sole producer). So the NODES -// table's NOTE column can now say exactly one thing. The two per-node fit warnings this function used -// to correlate became whole-plan infeasibilities and are rendered by renderRejectedNodes instead. -func TestWarningForNode(t *testing.T) { - warnings := []capacityplanner.Warning{ - { - Kind: capacityplanner.WarningKindTransient, Subject: "n1", - Message: "node n1 still hosts a drive container being deleted — placement deferred", - }, - { - Kind: capacityplanner.WarningKindTransient, Subject: "n10", - Message: "node n10 still hosts a drive container being deleted — placement deferred", - }, - } - if got := warningForNode(warnings, "n1"); !strings.Contains(got, "node n1 ") { - t.Errorf("warningForNode(n1) = %q, want the n1 message", got) - } - if got := warningForNode(warnings, "n10"); !strings.Contains(got, "node n10 ") { - t.Errorf("warningForNode(n10) = %q, want the n10 message", got) - } - if got := warningForNode(warnings, "n2"); got != "" { - t.Errorf("warningForNode(n2) = %q, want empty (no warning names n2)", got) - } - // A fleet-wide warning has no Subject and must never be attributed to a node's row. DrivesStranded is - // the live example: one aggregated message for the whole fleet, not a per-node warning. - fleet := []capacityplanner.Warning{{Kind: capacityplanner.WarningKindDrivesStranded, Message: "numDrives=2 pinned; 3 node(s) leave drives unused"}} - if got := warningForNode(fleet, "n1"); got != "" { - t.Errorf("warningForNode(n1) = %q, want empty — a subject-less fleet warning belongs to no node row", got) - } -} - -// TestHasFleetWarning covers the helper autoFullDrivesNodeRows uses to point a stranded row's NOTE at the -// fleet-wide DrivesStranded warning it can never match via warningForNode's Subject comparison (see -// TestWarningForNode's fleet case). +// TestHasFleetWarning covers the helper autoFullDrivesNodeRows uses to point a row's NOTE at the +// fleet-wide DrivesStranded/NodeIneligible warning that explains it. func TestHasFleetWarning(t *testing.T) { stranded := capacityplanner.Warning{Kind: capacityplanner.WarningKindDrivesStranded, Message: "numDrives=2 pinned"} - nodeScoped := capacityplanner.Warning{Kind: capacityplanner.WarningKindTransient, Subject: "n1", Message: "deferred"} + other := capacityplanner.Warning{Kind: capacityplanner.WarningKindTransient, Message: "deferred"} if !hasFleetWarning([]capacityplanner.Warning{stranded}, capacityplanner.WarningKindDrivesStranded) { - t.Error("hasFleetWarning() = false, want true for a subject-less DrivesStranded warning") + t.Error("hasFleetWarning() = false, want true for a matching DrivesStranded warning") } - if hasFleetWarning([]capacityplanner.Warning{nodeScoped}, capacityplanner.WarningKindDrivesStranded) { + if hasFleetWarning([]capacityplanner.Warning{other}, capacityplanner.WarningKindDrivesStranded) { t.Error("hasFleetWarning() = true, want false — no DrivesStranded warning present") } if hasFleetWarning(nil, capacityplanner.WarningKindDrivesStranded) { t.Error("hasFleetWarning(nil) = true, want false") } - // formatStrandedWarning always builds a DrivesStranded warning via fleetWarning (no Subject), but the - // helper must key on Subject=="" rather than Kind alone in case that ever changes. - subjected := capacityplanner.Warning{Kind: capacityplanner.WarningKindDrivesStranded, Subject: "n1", Message: "x"} - if hasFleetWarning([]capacityplanner.Warning{subjected}, capacityplanner.WarningKindDrivesStranded) { - t.Error("hasFleetWarning() = true, want false for a Subject-bearing warning even of the fleet kind") +} + +// A not-planned row must take its NOTE from its own node state, never from the presence of a fleet-wide +// NodeIneligible warning: that warning names every ineligible node in the fleet, so keying off its mere +// existence labels EVERY not-planned row "cordoned/not ready/untolerated taint" — including a node skipped +// only because a drive container on it is still being deleted, and every fit-failure node on an infeasible +// plan. Each of the three nodes below is not-planned with used < avail for a different reason, and each must +// get its own answer. +func TestAutoFullDrivesNodeRows_NotPlannedNoteFromNodeStateNotFleetWarning(t *testing.T) { + nodeInv := []capacityplanner.NodeCapacity{ + {NodeName: "n-cordoned", FDValue: "n-cordoned", DriveCapacitiesGiB: []int{3840}, IneligibleReason: "cordoned"}, + {NodeName: "n-deleting", FDValue: "n-deleting", DriveCapacitiesGiB: []int{3840}, HasDeletingDriveContainer: true}, + // Neither ineligible nor mid-deletion — a plain fit failure, which renderRejectedNodes explains + // instead. Its NOTE must stay empty rather than borrow n-cordoned's reason. + {NodeName: "n-unfit", FDValue: "n-unfit", DriveCapacitiesGiB: []int{3840}}, + } + plan := &capacityplanner.CapacityPlan{ + Warnings: []capacityplanner.Warning{{ + Kind: capacityplanner.WarningKindNodeIneligible, + Message: "auto full drives: 1 node(s) holding 1 signed free full drive(s) are ineligible for a new drive container: n-cordoned (cordoned)", + }}, + } + + byNode := map[string]autoFullDrivesNodeRow{} + for _, r := range autoFullDrivesNodeRows(nodeInv, nil, plan) { + if r.State != nodeStateNotPlanned { + t.Fatalf("row %+v is not %s — the fixture no longer exercises the not-planned branch", r, nodeStateNotPlanned) + } + byNode[r.Node] = r + } + + if got := byNode["n-cordoned"].Note; !strings.Contains(got, "cordoned") { + t.Errorf("n-cordoned Note = %q, want its own IneligibleReason verbatim", got) + } + if got := byNode["n-deleting"].Note; !strings.Contains(got, "being deleted") { + t.Errorf("n-deleting Note = %q, want the deletion reason — not n-cordoned's condition", got) + } + if got := byNode["n-deleting"].Note; strings.Contains(got, "cordoned") { + t.Errorf("n-deleting Note = %q, must not inherit another node's cordon reason", got) + } + if got := byNode["n-unfit"].Note; got != "" { + t.Errorf("n-unfit Note = %q, want empty — no per-node condition applies, so nothing to attribute", got) } } // TestAutoFullDrivesNodeRows_StrandedNodeNoteFromFleetWarning covers §8b: a node stranded by a numDrives -// pin (used < avail) has no per-node Subject to match via warningForNode — formatStrandedWarning -// aggregates every stranded node into one fleet-wide warning — so the NOTE column used to stay empty even -// though the row visibly shows used < avail. autoFullDrivesNodeRows must fall back to a pointer at that -// fleet warning instead. +// pin (used < avail) — formatStrandedWarning aggregates every stranded node into one fleet-wide warning +// rather than fanning one out per node — so the NOTE column must fall back to a pointer at that fleet +// warning instead of staying empty. func TestAutoFullDrivesNodeRows_StrandedNodeNoteFromFleetWarning(t *testing.T) { nodeInv := []capacityplanner.NodeCapacity{ {NodeName: "n1", FDValue: "n1", DriveCapacitiesGiB: []int{3840, 3840, 3840}}, @@ -427,6 +436,55 @@ func TestAutoFullDrivesNodeRows_NotPlannedRowGetsNoStrandedNote(t *testing.T) { } } +// TestAutoFullDrivesNodeRows_ConditionWithoutWarning_NoWarningsPointer is the regression for the +// mid-walk-abort shape: the walk stops collecting fleet warnings the moment it hits an infeasible node +// (autofulldrives.go), so a node sorting after it can carry a condition in the inventory with +// plan.Warnings left empty. Each of the three gated arms must still surface the node's own reason, but +// none may point at a WARNINGS section that was never written. +func TestAutoFullDrivesNodeRows_ConditionWithoutWarning_NoWarningsPointer(t *testing.T) { + nodeInv := []capacityplanner.NodeCapacity{ + {NodeName: "n-ineligible", FDValue: "n-ineligible", DriveCapacitiesGiB: []int{3840, 3840}, IneligibleReason: "cordoned"}, + {NodeName: "n-deleting", FDValue: "n-deleting", DriveCapacitiesGiB: []int{3840, 3840}, HasDeletingDriveContainer: true}, + {NodeName: "n-unsched", FDValue: "n-unsched", DriveCapacitiesGiB: []int{3840, 3840}}, + } + existing := []capacityplanner.ExistingContainer{ + {Name: "c-unsched", Node: "n-unsched", TlcGiB: 3840, NumCores: 1, NumDrives: 1, Unscheduled: true}, + } + plan := &capacityplanner.CapacityPlan{ + Infeasible: "some other node cannot fit", + Warnings: nil, + } + + byNode := map[string]autoFullDrivesNodeRow{} + for _, r := range autoFullDrivesNodeRows(nodeInv, existing, plan) { + byNode[r.Node] = r + } + + ineligible := byNode["n-ineligible"] + if !strings.Contains(ineligible.Note, "cordoned") { + t.Errorf("n-ineligible Note = %q, want its own IneligibleReason verbatim", ineligible.Note) + } + if strings.Contains(ineligible.Note, "WARNINGS") { + t.Errorf("n-ineligible Note = %q, must not point at WARNINGS — plan.Warnings is empty", ineligible.Note) + } + + deleting := byNode["n-deleting"] + if !strings.Contains(deleting.Note, "being deleted") { + t.Errorf("n-deleting Note = %q, want the deletion reason", deleting.Note) + } + if strings.Contains(deleting.Note, "WARNINGS") { + t.Errorf("n-deleting Note = %q, must not point at WARNINGS — plan.Warnings is empty", deleting.Note) + } + + unsched := byNode["n-unsched"] + if !strings.Contains(unsched.Note, "not been scheduled") { + t.Errorf("n-unsched Note = %q, want the deferred-growth reason", unsched.Note) + } + if strings.Contains(unsched.Note, "WARNINGS") { + t.Errorf("n-unsched Note = %q, must not point at WARNINGS — plan.Warnings is empty", unsched.Note) + } +} + // TestAutoFullDrivesDriveGrowDiff covers the drive-count transition (FromNumDrives/ToNumDrives) // alongside TLC/cores, joined by container name — including the newly-possible growths where drives and // cores move independently. @@ -494,7 +552,7 @@ func TestAutoFullDrivesPlanSummary(t *testing.T) { t.Errorf("autoFullDrivesPlanSummary() = %q, want no warning mention when Warnings is empty", steady) } - steadyWithWarnings := autoFullDrivesPlanSummary(&capacityplanner.CapacityPlan{Warnings: []capacityplanner.Warning{{Kind: capacityplanner.WarningKindTransient, Subject: "n1", Message: "placement deferred"}}}, signedSteady) + steadyWithWarnings := autoFullDrivesPlanSummary(&capacityplanner.CapacityPlan{Warnings: []capacityplanner.Warning{{Kind: capacityplanner.WarningKindTransient, Message: "placement deferred"}}}, signedSteady) if !strings.Contains(steadyWithWarnings, "steady state") || !strings.Contains(steadyWithWarnings, "1 warning") { t.Errorf("autoFullDrivesPlanSummary() = %q, want steady state AND a warning count", steadyWithWarnings) } @@ -556,7 +614,7 @@ func TestRenderAutoFullDrivesPlanText(t *testing.T) { d := autoFullDrivesPlanData{ Cluster: "test-cluster", Plan: &capacityplanner.CapacityPlan{ - Warnings: []capacityplanner.Warning{{Kind: capacityplanner.WarningKindTransient, Subject: "n-deferred", Message: "node n-deferred still hosts a drive container being deleted — placement deferred"}}, + Warnings: []capacityplanner.Warning{{Kind: capacityplanner.WarningKindTransient, Message: "node n-deferred still hosts a drive container being deleted — placement deferred"}}, }, Nodes: []autoFullDrivesNodeRow{ {Node: "n-create", FD: "n-create", DrivesUsed: 2, DrivesAvail: 2, TlcGiB: 7680, Cores: 2, State: "create"}, diff --git a/cmd/weka-capacity/plan.go b/cmd/weka-capacity/plan.go index 2134325ef..1e4eee645 100644 --- a/cmd/weka-capacity/plan.go +++ b/cmd/weka-capacity/plan.go @@ -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"` @@ -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 } } @@ -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 { @@ -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 + } + rows := make([]autoFullDrivesNodeRow, 0, len(nodeInv)) for i := range nodeInv { n := &nodeInv[i] @@ -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" } } diff --git a/cmd/weka-capacity/render.go b/cmd/weka-capacity/render.go index 5f61a68f3..4189ea944 100644 --- a/cmd/weka-capacity/render.go +++ b/cmd/weka-capacity/render.go @@ -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) diff --git a/doc/operator/deployment/act-as-daemonset.md b/doc/operator/deployment/act-as-daemonset.md index f8fc67291..74c8ed354 100644 --- a/doc/operator/deployment/act-as-daemonset.md +++ b/doc/operator/deployment/act-as-daemonset.md @@ -845,24 +845,27 @@ 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: 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. | @@ -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 diff --git a/doc/summary.xml b/doc/summary.xml index 1af8bb952..2a7870980 100644 --- a/doc/summary.xml +++ b/doc/summary.xml @@ -28,7 +28,7 @@ act as daemonset, daemonset mode, auto full drives, AutoFullDrives, implicit sizing mode, mode detection, no flag, empty dynamicTemplate, both-or-neither, computeContainers driveContainers set together, CEL rejection, full drives, exclusive drives, per-node sizing, heterogeneous nodes, node-pinned drive container, drives decoupled from cores, numDrives override, numDrives pin, driveCores pin, drive core limit 19, maxCoresPerContainer, signed full drives, weka.io/weka-full-drives, hugepages budget, compute hugepages ceiling, hugepagesTlcRatio, computeMaxHugepagesMiB, capacity-based hugepages, compute sizing, computeToDriveCoreRatio, compute 1:1 floor, compute shortfall coverage, hard infeasibility, whole plan infeasible, one bad node blocks the cluster, RejectedNodes, mode flip rejection, one-way mode switch, changing sizing mode on a live cluster, adopting the daemonset mode, supported mode switches, cluster_auto_full_drives_pin_exceeds_node_drives, cluster_auto_full_drives_compute_hugepages, cluster_sizing_mode_flip, cluster_auto_full_drives_min_nodes, form-cluster minimum nodes, MinContainersNotReady, node selector sets container count, admission policy summary, AutoFullDrivesInfeasible, AutoFullDrivesDrivesStranded, AutoFullDrivesGrowthDetected, CapacityGrowthApplied, UnschedulableDriveContainer, UnschedulableComputeContainer, unschedulable planner container GC, AutoFullDrivesNodeIneligible, node eligibility, cordoned nodes, NotReady nodes, untolerated taints, driveHugepages override, computeHugepages override, FORM_CLUSTER_MIN_COMPUTE_CONTAINERS, drive cores never traded for compute, pod restart, expand-only reconciliation, drive-only growth is free, QLC drives excluded, QLCDrivesSkipped, upgrade note, nodeSelector scope - Deployment guide for the implicit "act as daemonset" sizing mode (internally auto full drives / AutoFullDrives): an exclusive full-drives mode selected by NOT setting container counts — active iff computeContainers, driveContainers, clusterCapacity, containerCapacity and driveCapacity are all unset — that creates one node-pinned drive container per eligible node and auto-derives numDrives, drive cores, hugepages and memory from that node's own signed full drives. There is no flag; an empty or absent dynamicTemplate is the mode. Covers the nine-row mode-detection decision table and the both-or-neither CEL rule rejecting exactly one of computeContainers/driveContainers (with the verbatim admission message); the comparison with explicit container counts and the drive-sharing modes; prerequisites (signing drives before cluster creation); the drives/cores decoupling — a container always takes ALL its node's signed drives (or the numDrives pin) while driveCores = pin else min(drives, 19), so numDrives == driveCores is NO LONGER an invariant, a driveCores pin below the drive count is lossless, and a pin above it is infeasible; numDrives as a per-node largest-drives override (above a node's signed count is infeasible, below strands the rest as an expected Normal event); the per-container 19-core limit capping cores only; the hugepages budget (1664 MiB/drive core, 3064 MiB/compute core floor); compute sizing from a configurable ratio (default 2:1 full-drives) with a hard 1:1 floor and how shortfalls are covered; the compute-hugepages ceiling as the practical limit — because all drives are claimed the capacity-based term is fixed and cannot be shrunk by capping cores — worked end to end on an 8-node lab fleet (8 x 6 x 14307 GiB = 686736 GiB claimed, 87902 MiB capacity share per container at 8 compute containers vs 50016 MiB free, needing 18 compute-eligible nodes) with four remedies (more compute nodes, raise hugepagesTlcRatio, lower computeMaxHugepagesMiB, pin numDrives lower) and admission enforcement; hard infeasibility when ANY node cannot fit a container sized for all its drives (nothing created anywhere, every offending node named with the binding dimension, and one bad node blocking the whole cluster called out); the one-way mode-switch rule on a live cluster enforced by cluster_sizing_mode_flip (Error in both modes, UPDATE-only, covering every sizing-mode pair since nothing else guards the capacity transitions) — explicit container counts to daemonset is ALLOWED, the running drive containers being adopted via the pod's node and grown in place to that node's full drive set with a pod recreation owed where cores rose, while daemonset to counts and every capacity transition except drive-sharing to clusterCapacity are rejected with reverting as the remedy; the note that all the other policies run on updates too, so the switch still faces the min-nodes, compute-hugepages and pin-exceeds gates; continuous expand-only reconciliation where drive-only growth is always free and needs no pod restart; the pod-restart caveat for core changes; the upgrade note that existing containers gain drives while cores stay put; QLC exclusion from full-drives signing; the full AutoFullDrives* event table with types and throttles, plus the container-level UnschedulableDriveContainer/UnschedulableComputeContainer reaps, gated on a confirmed PodScheduled=False/Unschedulable condition rather than age alone so a pod merely Pending during a slow drivers build is left alone; and troubleshooting for unsigned drives, stranded drives, fewer cores than drives, the two admission rejections, infeasibility, never-scheduled drive and compute containers (both reaped on the same terms, since an unscheduled compute container's cores are counted but never served — the reap requires a node-pinned container whose pod NEVER bound and a scheduler verdict standing longer than the timeout, timed from the verdict rather than from container creation), and nodes barred from NEW placement as cordoned, NotReady or carrying an untolerated taint (a Normal per-node AutoFullDrivesNodeIneligible event; existing containers there keep running, stay charged and still grow, and their unclaimed drives still count toward the plan's reported fleet total). + Deployment guide for the implicit "act as daemonset" sizing mode (internally auto full drives / AutoFullDrives): an exclusive full-drives mode selected by NOT setting container counts — active iff computeContainers, driveContainers, clusterCapacity, containerCapacity and driveCapacity are all unset — that creates one node-pinned drive container per eligible node and auto-derives numDrives, drive cores, hugepages and memory from that node's own signed full drives. There is no flag; an empty or absent dynamicTemplate is the mode. Covers the nine-row mode-detection decision table and the both-or-neither CEL rule rejecting exactly one of computeContainers/driveContainers (with the verbatim admission message); the comparison with explicit container counts and the drive-sharing modes; prerequisites (signing drives before cluster creation); the drives/cores decoupling — a container always takes ALL its node's signed drives (or the numDrives pin) while driveCores = pin else min(drives, 19), so numDrives == driveCores is NO LONGER an invariant, a driveCores pin below the drive count is lossless, and a pin above it is infeasible; numDrives as a per-node largest-drives override (above a node's signed count is infeasible, below strands the rest as an expected Normal event); the per-container 19-core limit capping cores only; the hugepages budget (1664 MiB/drive core, 3064 MiB/compute core floor); compute sizing from a configurable ratio (default 2:1 full-drives) with a hard 1:1 floor and how shortfalls are covered; the compute-hugepages ceiling as the practical limit — because all drives are claimed the capacity-based term is fixed and cannot be shrunk by capping cores — worked end to end on an 8-node lab fleet (8 x 6 x 14307 GiB = 686736 GiB claimed, 87902 MiB capacity share per container at 8 compute containers vs 50016 MiB free, needing 18 compute-eligible nodes) with four remedies (more compute nodes, raise hugepagesTlcRatio, lower computeMaxHugepagesMiB, pin numDrives lower) and admission enforcement; hard infeasibility when ANY node cannot fit a container sized for all its drives (nothing created anywhere, every offending node named with the binding dimension, and one bad node blocking the whole cluster called out); the one-way mode-switch rule on a live cluster enforced by cluster_sizing_mode_flip (Error in both modes, UPDATE-only, covering every sizing-mode pair since nothing else guards the capacity transitions) — explicit container counts to daemonset is ALLOWED, the running drive containers being adopted via the pod's node and grown in place to that node's full drive set with a pod recreation owed where cores rose, while daemonset to counts and every capacity transition except drive-sharing to clusterCapacity are rejected with reverting as the remedy; the note that all the other policies run on updates too, so the switch still faces the min-nodes, compute-hugepages and pin-exceeds gates; continuous expand-only reconciliation where drive-only growth is always free and needs no pod restart; the pod-restart caveat for core changes; the upgrade note that existing containers gain drives while cores stay put; QLC exclusion from full-drives signing; the full AutoFullDrives* event table with types and throttles, plus the container-level UnschedulableDriveContainer/UnschedulableComputeContainer reaps, gated on a confirmed PodScheduled=False/Unschedulable condition rather than age alone so a pod merely Pending during a slow drivers build is left alone; and troubleshooting for unsigned drives, stranded drives, fewer cores than drives, the two admission rejections, infeasibility, never-scheduled drive and compute containers (both reaped on the same terms, since an unscheduled compute container's cores are counted but never served — the reap requires a node-pinned container whose pod NEVER bound and a scheduler verdict standing longer than the timeout, timed from the verdict rather than from container creation), and nodes barred from NEW placement as cordoned, NotReady or carrying an untolerated taint (all such nodes arrive in one Normal AutoFullDrivesNodeIneligible event naming each with its reason; existing containers there keep running, stay charged and still grow, and their unclaimed drives still count toward the plan's reported fleet total). diff --git a/internal/capacityplanner/autofulldrives.go b/internal/capacityplanner/autofulldrives.go index be9e0160a..acbfa5045 100644 --- a/internal/capacityplanner/autofulldrives.go +++ b/internal/capacityplanner/autofulldrives.go @@ -104,10 +104,30 @@ func planAutoFullDrivesDrives( remaining[nodes[i].nc.NodeName] = nodes[i].nc } - // Both collected across the whole walk and reported after it: stranding because its cause is one - // fleet-wide pin, failures so the infeasibility names every offending node rather than the first. + // All collected across the whole walk and reported by flushWarnings, one aggregated Warning per condition + // instead of one per node: stranding because its cause is one fleet-wide pin, the rest because the + // 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 + + // 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 + // at these warnings, so dropping them would leave a row citing a WARNINGS entry that was never written. + flushWarnings := func() { + if len(stranded) > 0 { + plan.Warnings = append(plan.Warnings, formatStrandedWarning(stranded, desired.NumDrives)) + } + if len(ineligible) > 0 { + plan.Warnings = append(plan.Warnings, formatIneligibleWarning(ineligible, ineligibleDrives)) + } + if len(deferred) > 0 || len(deleting) > 0 { + plan.Warnings = append(plan.Warnings, formatPlacementDeferredWarning(deferred, deleting)) + } + } for i := range nodes { n := &nodes[i] @@ -133,19 +153,14 @@ func planAutoFullDrivesDrives( if n.nc.IneligibleReason != "" { totals.drivesAvailable += len(drives) totals.tlcGiBAvailable += sumInts(drives) - plan.Warnings = append(plan.Warnings, nodeWarning(WarningKindNodeIneligible, name, - "auto full drives: node %s has %d signed free full drive(s) but is ineligible for a new "+ - "drive container (%s)", - name, len(drives), n.nc.IneligibleReason)) + ineligible = append(ineligible, fmt.Sprintf("%s (%s)", name, n.nc.IneligibleReason)) + ineligibleDrives += len(drives) continue } // The one per-node skip that is a skip rather than an infeasibility: it clears itself, so failing // the plan would stall every reconcile behind one deletion. if n.nc.HasDeletingDriveContainer { - plan.Warnings = append(plan.Warnings, nodeWarning(WarningKindTransient, name, - "auto full drives: node %s still hosts a this-cluster drive container that is being deleted — "+ - "skipping new container placement this pass (retried automatically once deletion completes)", - name)) + deleting = append(deleting, name) continue } if len(drives) == 0 { @@ -156,6 +171,7 @@ func planAutoFullDrivesDrives( np, report := autoSizeNode(name, drives, desired, cons) if report != nil { setInfeasible(&plan, report) + flushWarnings() return plan, totals, remaining } @@ -195,10 +211,7 @@ func planAutoFullDrivesDrives( // 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 { - plan.Warnings = append(plan.Warnings, nodeWarning(WarningKindTransient, name, - "auto full drives: node %s hosts a drive container whose pod has not been scheduled yet — "+ - "skipping growth this pass (retried automatically once the pod is scheduled)", - name)) + deferred = append(deferred, name) continue } @@ -239,9 +252,7 @@ func planAutoFullDrivesDrives( remaining[name] = nc } - if len(stranded) > 0 { - plan.Warnings = append(plan.Warnings, formatStrandedWarning(stranded, desired.NumDrives)) - } + flushWarnings() // The infeasibility gate: after the full walk so every offender is named, and before compute is sized, so // an infeasible plan carries no ComputeLayout. Partial Create/Grow entries stay on the plan for diff --git a/internal/capacityplanner/autofulldrives_compute.go b/internal/capacityplanner/autofulldrives_compute.go index e90f8eba1..e3d0eadfb 100644 --- a/internal/capacityplanner/autofulldrives_compute.go +++ b/internal/capacityplanner/autofulldrives_compute.go @@ -3,6 +3,7 @@ package capacityplanner import ( "fmt" "sort" + "strings" ) // autofulldrives_compute.go sizes the compute side of an auto-full-drives plan. Compute is derived from the @@ -160,10 +161,16 @@ func planComputeAutoFullDrives(in *autoComputeInput, plan *CapacityPlan) { if !autoRederiveKeptHugepages(kept, len(kept)+count, in, plan) { return } - for _, w := range warnings { - plan.Warnings = append(plan.Warnings, Warning{Kind: WarningKindComputeLayout, Message: w}) + // Every compute advisory accumulates here and leaves as one Warning: they share the single reason + // AutoFullDrivesComputeLayout, whose throttle key ignores the message, so a second Warning under that + // reason is silently dropped for the whole window instead of reported. autoPlaceNewCompute's surplus + // advisory is the only contributor today: deriveComputeLayout never populates its warnings return. + advisories := append([]string(nil), warnings...) + autoPlaceNewCompute(in, plan, kept, placeable, coreHeadroom, count, cores, newTarget, &advisories) + if len(advisories) > 0 { + plan.Warnings = append(plan.Warnings, + fleetWarning(WarningKindComputeLayout, "auto full drives: %s", strings.Join(advisories, "; "))) } - autoPlaceNewCompute(in, plan, kept, placeable, coreHeadroom, count, cores, newTarget) return } @@ -336,7 +343,7 @@ func autoRederiveKeptHugepages(kept []autoComputeEntry, totalCount int, in *auto // them to cover `target` cores, and finishes the plan. func autoPlaceNewCompute( in *autoComputeInput, plan *CapacityPlan, kept []autoComputeEntry, - placeable []string, coreHeadroom []int, count, cores, target int, + placeable []string, coreHeadroom []int, count, cores, target int, advisories *[]string, ) { layout := autoComputeSpecs(kept) if count <= 0 { @@ -375,9 +382,9 @@ func autoPlaceNewCompute( if in.desired.ComputeCores > 0 { shortfall = count * cores } else if target < count { - plan.Warnings = append(plan.Warnings, fleetWarning(WarningKindComputeLayout, - "auto full drives: a cluster cannot form below %d compute container(s) but the compute:drive ratio "+ - "needs only %d more core(s); the surplus container(s) are created with the 1-core minimum", + *advisories = append(*advisories, fmt.Sprintf( + "a cluster cannot form below %d compute container(s) but the compute:drive ratio needs only %d more "+ + "core(s); the surplus container(s) are created with the 1-core minimum", count, target)) } diff --git a/internal/capacityplanner/autofulldrives_sizing.go b/internal/capacityplanner/autofulldrives_sizing.go index cf6d22613..2c246d795 100644 --- a/internal/capacityplanner/autofulldrives_sizing.go +++ b/internal/capacityplanner/autofulldrives_sizing.go @@ -1,9 +1,6 @@ package capacityplanner -import ( - "fmt" - "strings" -) +import "fmt" // autofulldrives_sizing.go is the pure sizing layer of the auto-full-drives mode: every semantics rule about // how big a container is, with no resource model at all. The create and growth paths share it verbatim. @@ -91,29 +88,3 @@ func autoSizeNode( return autoNodePlan{node: name, drives: drives[:taken], cores: cores}, nil } - -// strandedNode is one node where a pinned dynamicTemplate.numDrives left signed full drives unused, -// collected during the walk so the whole fleet is reported in a single DrivesStranded warning. -type strandedNode struct { - node string - signed int // drives signed on the node - used int // drives the container takes -} - -// formatStrandedWarning renders the aggregated DrivesStranded message. The only cause is a pinned numDrives, -// an operator choice, hence Normal rather than Warning downstream. Aggregated because the cause is one -// fleet-wide setting: per-node fan-out would turn one condition into one near-identical Warning per node, -// each repeating the same ~300-character remedy. -func formatStrandedWarning(stranded []strandedNode, pin int) Warning { - parts := make([]string, 0, len(stranded)) - unused := 0 - for _, s := range stranded { - parts = append(parts, fmt.Sprintf("%s (%d of %d)", s.node, s.used, s.signed)) - unused += s.signed - s.used - } - return fleetWarning(WarningKindDrivesStranded, - "auto full drives: dynamicTemplate.numDrives=%d is pinned, so each container takes only its node's %d "+ - "largest drive(s) — %d node(s) have more, leaving %d drive(s) unused in total; per node (used of "+ - "signed): %s; unset numDrives to claim every signed full drive on every node", - pin, pin, len(stranded), unused, strings.Join(parts, ", ")) -} diff --git a/internal/capacityplanner/autofulldrives_test.go b/internal/capacityplanner/autofulldrives_test.go index 51b5e0029..3d284f2c5 100644 --- a/internal/capacityplanner/autofulldrives_test.go +++ b/internal/capacityplanner/autofulldrives_test.go @@ -1327,8 +1327,8 @@ func TestPlanAutoFullDrives_SkipsNodeWithDeletingDriveContainer(t *testing.T) { t.Fatalf("expected no Grow entry (no existing container in existingByNode to grow), got %+v", plan.Grow) } joined := strings.Join(WarningMessages(plan.Warnings), " | ") - if !strings.Contains(joined, "still hosts a this-cluster drive container that is being deleted") { - t.Errorf("expected a warning explaining the skip, got %q", joined) + if !strings.Contains(joined, "n1") || !strings.Contains(joined, "still being deleted") { + t.Errorf("expected a warning naming n1 and explaining the skip, got %q", joined) } } @@ -1434,15 +1434,15 @@ func TestPlanAutoFullDrives_IneligibleNode_ExistingCapacityCountedNoNewContainer } found := false for _, w := range plan.Warnings { - if w.Kind == WarningKindNodeIneligible && w.Subject == "n2" { + if w.Kind == WarningKindNodeIneligible { found = true if !strings.Contains(w.Message, "n2") || !strings.Contains(w.Message, "not ready") { - t.Errorf("n2's NodeIneligible warning message = %q, want it to name n2 and its reason verbatim (not ready)", w.Message) + t.Errorf("NodeIneligible warning message = %q, want it to name n2 and its reason verbatim (not ready)", w.Message) } } } if !found { - t.Fatalf("Warnings = %+v, want a WarningKindNodeIneligible warning for n2", plan.Warnings) + t.Fatalf("Warnings = %+v, want a WarningKindNodeIneligible warning naming n2", plan.Warnings) } // DriveSizing.TlcGiBTaken is the exact number PlanAutoFullDrives hands to compute as its capacity // numerator, but pin it there too: 29600 only comes out of n1's 4000 GiB alone. If n2's free drives (which @@ -2152,17 +2152,24 @@ func TestPlanAutoFullDrives_FormClusterFloorAboveDeficit_NoZeroCoreContainers(t "containers, not empty ones", spec.Node, spec.NumCores) } } - // The surplus is surfaced, not silent. - var warned bool + // The surplus is surfaced, not silent — and in exactly one ComputeLayout warning. Every advisory from the + // compute step is joined into one, because they all land on the single reason AutoFullDrivesComputeLayout + // whose throttle key ignores the message: a second one would be dropped for the whole window, not shown. + var layout []Warning for _, w := range plan.Warnings { - if strings.Contains(w.Message, "cannot form below 5 compute container(s)") && - strings.Contains(w.Message, "1-core minimum") { - warned = true - break + if w.Kind == WarningKindComputeLayout { + layout = append(layout, w) } } - if !warned { - t.Errorf("want a warning naming the 5-container floor and the 1-core minimum, got: %v", plan.Warnings) + if len(layout) != 1 { + t.Fatalf("WarningKindComputeLayout warnings = %+v, want exactly 1 joining every compute advisory", layout) + } + if !strings.Contains(layout[0].Message, "cannot form below 5 compute container(s)") || + !strings.Contains(layout[0].Message, "1-core minimum") { + t.Errorf("warning = %q, want it to name the 5-container floor and the 1-core minimum", layout[0].Message) + } + if !strings.HasPrefix(layout[0].Message, "auto full drives: ") { + t.Errorf("warning = %q, want the shared \"auto full drives: \" prefix the joined message carries once", layout[0].Message) } } @@ -2985,8 +2992,11 @@ func TestPlanAutoFullDrives_UnscheduledDriveContainer_FreezesGrowth(t *testing.T found := false for _, w := range plan.Warnings { - if w.Kind == WarningKindTransient && w.Subject == "unscheduled" { + if w.Kind == WarningKindTransient { found = true + if !strings.Contains(w.Message, "unscheduled") { + t.Errorf("WarningKindTransient warning message = %q, want it to name node %q", w.Message, "unscheduled") + } } } if !found { @@ -2994,6 +3004,103 @@ 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) { + cons := testCons() + const bigFree = 1 << 28 + + existingDrives := []ExistingContainer{ + {Name: "drive-scheduled", Node: "scheduled", FDValue: "scheduled", TlcGiB: 3000, NumCores: 3, NumDrives: 3}, + {Name: "drive-unscheduled", Node: "unscheduled", FDValue: "unscheduled", TlcGiB: 3000, NumCores: 3, NumDrives: 3, Unscheduled: true}, + // No entry for "deleting" — mirrors ExistingDrives already filtering out the mid-deletion container. + } + inv := []NodeCapacity{ + { + NodeName: "scheduled", FDValue: "scheduled", + OwnDriveCapacitiesGiB: uniformDrives(3, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }, + { + NodeName: "unscheduled", FDValue: "unscheduled", + OwnDriveCapacitiesGiB: uniformDrives(3, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }, + { + NodeName: "deleting", FDValue: "deleting", + DriveCapacitiesGiB: uniformDrives(3, 1000), TlcGiB: 3000, + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + HasDeletingDriveContainer: true, + }, + } + computeNodes := computeNodeSet("scheduled", "unscheduled", "deleting") + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{}, existingDrives, nil, inv, computeNodes, cons) + + if plan.Infeasible != "" { + t.Fatalf("unexpected infeasible: %s", plan.Infeasible) + } + + var transient []Warning + for _, w := range plan.Warnings { + if w.Kind == WarningKindTransient { + transient = append(transient, w) + } + } + if len(transient) != 1 { + t.Fatalf("WarningKindTransient warnings = %+v, want exactly 1 covering both causes", transient) + } + 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") + } + 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(w.Message, "both retry automatically") { + t.Errorf("warning message = %q, want the merged-causes retry clause \"both retry automatically\"", w.Message) + } +} + +// The walk returns from inside the loop when a node's pins cannot be satisfied. Warnings collected before +// that node describe the plan it returns and must survive it: the CLI renders an ineligible node's row as +// "cordoned — see WARNINGS", so losing the warning leaves a row citing an entry nothing wrote. +func TestPlanAutoFullDrives_InfeasibleMidWalk_KeepsWarningsAlreadyCollected(t *testing.T) { + cons := testCons() + const bigFree = 1 << 28 + + // Nodes are walked in name order, so a-cordoned records its warning before z-pinned aborts the walk. + inv := []NodeCapacity{ + { + NodeName: "a-cordoned", FDValue: "a-cordoned", + DriveCapacitiesGiB: uniformDrives(3, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + IneligibleReason: "cordoned", + }, + // One signed drive under numDrives=2: autoSizeNode reports and planAutoFullDrivesDrives returns. + { + NodeName: "z-pinned", FDValue: "z-pinned", + DriveCapacitiesGiB: uniformDrives(1, 1000), + AllocatableCPU: 100, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }, + } + + plan := PlanAutoFullDrives(AutoFullDrivesDesired{NumDrives: 2}, nil, nil, inv, + computeNodeSet("a-cordoned", "z-pinned"), cons) + + if plan.Infeasible == "" { + t.Fatalf("plan is feasible, so it no longer exercises the mid-walk return; plan=%+v", plan) + } + for _, w := range plan.Warnings { + if w.Kind == WarningKindNodeIneligible && strings.Contains(w.Message, "a-cordoned") { + return + } + } + t.Errorf("Warnings = %+v, want the NodeIneligible warning naming a-cordoned to survive the mid-walk return", + plan.Warnings) +} + // The unscheduled node gets 2 free drives beyond the 1 it owns, so planned (3 drives, 3000 GiB) and // frozen (1 drive, 1000 GiB) diverge — checks that compute sizing uses the planned figure, the same // "count it anyway, after the totals" convention as the drive-side freeze (file header :204-205). diff --git a/internal/capacityplanner/autofulldrives_warnings.go b/internal/capacityplanner/autofulldrives_warnings.go new file mode 100644 index 000000000..96ef0da79 --- /dev/null +++ b/internal/capacityplanner/autofulldrives_warnings.go @@ -0,0 +1,88 @@ +package capacityplanner + +import ( + "fmt" + "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. + +func listNodes(parts []string) string { return listNodesCapped(parts, autoFullDrivesMaxNamedNodes) } + +// listNodesCapped joins per-node parts, capping the list at limit with a "(+N more)" tail so a large fleet +// cannot turn one aggregated event into a multi-KB message. +func listNodesCapped(parts []string, limit int) string { + if len(parts) <= limit { + return strings.Join(parts, ", ") + } + return fmt.Sprintf("%s (+%d more)", strings.Join(parts[:limit], ", "), len(parts)-limit) +} + +// strandedNode is one node where a pinned dynamicTemplate.numDrives left signed full drives unused, +// collected during the walk so the whole fleet is reported in a single DrivesStranded warning. +type strandedNode struct { + node string + signed int // drives signed on the node + used int // drives the container takes +} + +// formatStrandedWarning renders the aggregated DrivesStranded message. The only cause is a pinned numDrives, +// an operator choice, hence Normal rather than Warning downstream. Aggregated because the cause is one +// fleet-wide setting: per-node fan-out would turn one condition into one near-identical Warning per node, +// each repeating the same ~300-character remedy. +func formatStrandedWarning(stranded []strandedNode, pin int) Warning { + parts := make([]string, 0, len(stranded)) + unused := 0 + for _, s := range stranded { + parts = append(parts, fmt.Sprintf("%s (%d of %d)", s.node, s.used, s.signed)) + unused += s.signed - s.used + } + return fleetWarning(WarningKindDrivesStranded, + "auto full drives: dynamicTemplate.numDrives=%d is pinned, so each container takes only its node's %d "+ + "largest drive(s) — %d node(s) have more, leaving %d drive(s) unused in total; per node (used of "+ + "signed): %s; unset numDrives to claim every signed full drive on every node", + pin, pin, len(stranded), unused, listNodes(parts)) +} + +// 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, + "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 + } + + 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))) + } + retry := "it retries automatically" + if len(clauses) > 1 { + retry = "both retry automatically" + } + return fleetWarning(WarningKindTransient, + "auto full drives: placement deferred on %d node(s) this pass; %s; %s", + len(deferred)+len(deleting), strings.Join(clauses, "; "), retry) +} diff --git a/internal/capacityplanner/autofulldrives_warnings_test.go b/internal/capacityplanner/autofulldrives_warnings_test.go new file mode 100644 index 000000000..87bb86480 --- /dev/null +++ b/internal/capacityplanner/autofulldrives_warnings_test.go @@ -0,0 +1,83 @@ +package capacityplanner + +import ( + "fmt" + "strings" + "testing" +) + +// TestListNodes_CapsAtMaxNamedNodes covers the shared primitive every aggregated auto-full-drives warning +// routes through: below the cap every name is spelled out, above it the list is truncated with a +// "(+N more)" tail rather than growing one event into a multi-KB message. +func TestListNodes_CapsAtMaxNamedNodes(t *testing.T) { + for _, tc := range []struct { + name string + count int + }{ + {"under cap", autoFullDrivesMaxNamedNodes - 1}, + {"at cap", autoFullDrivesMaxNamedNodes}, + {"over cap", autoFullDrivesMaxNamedNodes + 5}, + } { + t.Run(tc.name, func(t *testing.T) { + var parts []string + for i := 0; i < tc.count; i++ { + parts = append(parts, fmt.Sprintf("n%d", i)) + } + + got := listNodes(parts) + + if tc.count <= autoFullDrivesMaxNamedNodes { + want := strings.Join(parts, ", ") + if got != want { + t.Errorf("listNodes() = %q, want %q (no truncation at or under the cap)", got, want) + } + return + } + for i := 0; i < autoFullDrivesMaxNamedNodes; i++ { + if !strings.Contains(got, parts[i]) { + t.Errorf("listNodes() = %q, missing named node %q", got, parts[i]) + } + } + if strings.Contains(got, parts[autoFullDrivesMaxNamedNodes]) { + t.Errorf("listNodes() = %q, must not spell out node %q past the cap", got, parts[autoFullDrivesMaxNamedNodes]) + } + overflow := tc.count - autoFullDrivesMaxNamedNodes + wantTail := fmt.Sprintf("(+%d more)", overflow) + if !strings.HasSuffix(got, wantTail) { + t.Errorf("listNodes() = %q, want it to end with %q", got, wantTail) + } + }) + } +} + +// 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) { + 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) + + named := 0 + for _, n := range deferred { + if strings.Contains(w.Message, n) { + named++ + } + } + for _, n := range deleting { + if strings.Contains(w.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 !strings.Contains(w.Message, "(+") { + t.Errorf("formatPlacementDeferredWarning() = %q, want it to disclose the truncation with a \"(+N more)\" tail", w.Message) + } +} diff --git a/internal/capacityplanner/planner.go b/internal/capacityplanner/planner.go index 78fd0837d..2b0564dfb 100644 --- a/internal/capacityplanner/planner.go +++ b/internal/capacityplanner/planner.go @@ -174,13 +174,10 @@ const ( WarningKindNodeIneligible WarningKind = "NodeIneligible" ) -// Warning is one classified planner advisory. Subject is the node/container it's about and the event -// throttling key (one warning per subject per window); empty for fleet-wide warnings, throttled per -// reason instead — throttling on Message alone breaks once a count in the text drifts (e.g. "6 node(s)" -// -> "5 node(s)"). +// 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 - Subject string Message string } @@ -197,12 +194,7 @@ func WarningMessages(warnings []Warning) []string { return out } -// nodeWarning builds a per-node classified warning, formatting the message from args. -func nodeWarning(kind WarningKind, node, format string, args ...any) Warning { - return Warning{Kind: kind, Subject: node, Message: fmt.Sprintf(format, args...)} -} - -// fleetWarning builds a classified warning with no single subject (throttled per reason). +// 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...)} } diff --git a/internal/controllers/wekacluster/funcs_fd_planning.go b/internal/controllers/wekacluster/funcs_fd_planning.go index 0ffacb435..a5b7c31f0 100644 --- a/internal/controllers/wekacluster/funcs_fd_planning.go +++ b/internal/controllers/wekacluster/funcs_fd_planning.go @@ -65,7 +65,7 @@ func (r *wekaClusterReconcilerLoop) planClusterCapacity(ctx context.Context) (*c // failure-domain set is temporarily reduced, and planning against that snapshot would wrongly // concentrate capacity onto the survivors. Defer here; the reconcile retries once pods settle. if name, transient := firstUnscheduledDriveContainer(r.containers); transient { - r.emitPlannerEvent(reasonClusterCapacityDeferred, "", + r.emitPlannerEvent(reasonClusterCapacityDeferred, fmt.Sprintf("deferring clusterCapacity planning: drive container %s is unscheduled (pod (re)scheduling); will retry once it settles", name)) logger.Debug("deferring clusterCapacity planning while a drive container is transiently unscheduled", "container", name) return r.noopCapacityPlan(ctx, cons), nil @@ -111,24 +111,24 @@ func (r *wekaClusterReconcilerLoop) planClusterCapacity(ctx context.Context) (*c // the shrink/heterogeneous-growth/over-provision advisories (they would just be noise on a plan // that creates/grows nothing). if plan.Infeasible != "" { - r.emitPlannerEvent(reasonClusterCapacityInfeasible, "", plan.Infeasible) + r.emitPlannerEvent(reasonClusterCapacityInfeasible, plan.Infeasible) return nil, lifecycle.NewWaitErrorWithDuration(fmt.Errorf("clusterCapacity infeasible: %s", plan.Infeasible), time.Minute) } for _, msg := range plan.ShrinkEvents { - r.emitPlannerEvent(reasonClusterCapacityShrink, "", msg) + r.emitPlannerEvent(reasonClusterCapacityShrink, msg) } // clusterCapacity uses a single reason for all its warnings (layout advisories); only the message is // read from the classified Warning. Auto full drives instead splits by cause (autoFullDrivesWarningReason). for _, w := range plan.Warnings { - r.emitPlannerEvent(reasonClusterCapacityHeterogeneousGrowth, "", w.Message) + r.emitPlannerEvent(reasonClusterCapacityHeterogeneousGrowth, w.Message) } for _, msg := range plan.OverProvisions { - r.emitPlannerEvent(reasonClusterCapacityOverProvisioned, "", msg) + r.emitPlannerEvent(reasonClusterCapacityOverProvisioned, msg) } // Feasible plan that places capacity: emit a Normal summary event, gated on Create/Grow so steady-state // reconciles stay silent. if len(plan.Create) > 0 || len(plan.Grow) > 0 { - r.emitPlannerEvent(reasonClusterCapacityPlanned, "", + r.emitPlannerEvent(reasonClusterCapacityPlanned, formatCapacityPlanSummary(&plan, desired, s, existingDrives)) } return &plan, nil @@ -186,7 +186,7 @@ func (r *wekaClusterReconcilerLoop) planAutoFullDrives(ctx context.Context) (*ca } } if !hasSignedDrives { - r.emitPlannerEvent(reasonAutoFullDrivesNoSignedDrives, "", + r.emitPlannerEvent(reasonAutoFullDrivesNoSignedDrives, "deferring auto full drives planning: no node matching the drive-role selector has any signed, non-blocked full drive yet; sign drives (weka.io/weka-full-drives) and the operator will pick them up on its own") logger.Debug("deferring auto full drives planning: no node has signed full drives yet", "candidateNodes", len(nodeInv)) return nil, lifecycle.NewWaitErrorWithDuration(fmt.Errorf("auto full drives: no node has signed full drives yet"), time.Minute) @@ -218,7 +218,7 @@ func (r *wekaClusterReconcilerLoop) planAutoFullDrives(ctx context.Context) (*ca // An infeasible plan is the sole signal: emit only AutoFullDrivesInfeasible and return, skipping the // warnings advisory (it would just be noise on a plan that creates nothing). if plan.Infeasible != "" { - r.emitPlannerEvent(reasonAutoFullDrivesInfeasible, "", plan.Infeasible) + r.emitPlannerEvent(reasonAutoFullDrivesInfeasible, plan.Infeasible) return nil, lifecycle.NewWaitErrorWithDuration(fmt.Errorf("auto full drives infeasible: %s", plan.Infeasible), time.Minute) } // Drive cores are never traded away to make compute fit — a fleet that cannot host the required @@ -227,15 +227,15 @@ func (r *wekaClusterReconcilerLoop) planAutoFullDrives(ctx context.Context) (*ca // plan.Grow is not announced here: applyPlannerDriveGrowth can decline an entry or fail its Update, // and emits AutoFullDrivesGrowthDetected for what it actually wrote. - // One reason per cause, throttled per subject (not message) so N constrained nodes each get their own - // event instead of the first starving the rest. + // 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.Subject, w.Message) + r.emitPlannerEvent(autoFullDrivesWarningReason(w.Kind), 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. if len(plan.Create) > 0 { - r.emitPlannerEvent(reasonAutoFullDrivesPlanned, "", formatAutoFullDrivesPlanSummary(&plan)) + r.emitPlannerEvent(reasonAutoFullDrivesPlanned, formatAutoFullDrivesPlanSummary(&plan)) } return &plan, nil } diff --git a/internal/controllers/wekacluster/funcs_fd_planning_test.go b/internal/controllers/wekacluster/funcs_fd_planning_test.go index 212063ddf..4d9cbb831 100644 --- a/internal/controllers/wekacluster/funcs_fd_planning_test.go +++ b/internal/controllers/wekacluster/funcs_fd_planning_test.go @@ -937,40 +937,31 @@ func TestPlannerEventSpecsCoverEveryReason(t *testing.T) { // The rows the docs are explicit about, and where a drift would be silent in production. for _, tc := range []struct { - reason string - wantType string - wantLong bool // 15-minute converged-state window rather than the 1-minute default - wantPerNode bool + reason string + wantType string + wantInterval time.Duration }{ - {reasonAutoFullDrivesInfeasible, corev1.EventTypeWarning, false, false}, - {reasonAutoFullDrivesPlanned, corev1.EventTypeNormal, false, false}, - {reasonAutoFullDrivesGrowthDetected, corev1.EventTypeNormal, false, false}, - {reasonAutoFullDrivesGrowthDeferred, corev1.EventTypeWarning, true, false}, - // Expected under an explicit numDrives pin, so Normal and rate-limited as a converged state. - {reasonAutoFullDrivesDrivesStranded, corev1.EventTypeNormal, true, false}, - // Per node: one constrained node must not starve the others' events. - {reasonAutoFullDrivesPlacementDeferred, corev1.EventTypeNormal, true, true}, - // An administrative state (cordon/taint/NotReady), not a planner failure: Normal, per node, and - // rate-limited — a node left cordoned for maintenance must not post a Warning every minute. - {reasonAutoFullDrivesNodeIneligible, corev1.EventTypeNormal, true, true}, - {reasonAutoFullDrivesComputeLayout, corev1.EventTypeWarning, true, false}, - {reasonClusterCapacityHeterogeneousGrowth, corev1.EventTypeWarning, false, false}, - {reasonClusterCapacityPlanned, corev1.EventTypeNormal, false, false}, + {reasonAutoFullDrivesInfeasible, corev1.EventTypeWarning, time.Minute}, + {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. + {reasonAutoFullDrivesDrivesStranded, corev1.EventTypeNormal, plannerAggregateEventInterval}, + {reasonAutoFullDrivesPlacementDeferred, corev1.EventTypeNormal, plannerAggregateEventInterval}, + {reasonAutoFullDrivesNodeIneligible, corev1.EventTypeNormal, plannerAggregateEventInterval}, + {reasonAutoFullDrivesComputeLayout, corev1.EventTypeWarning, plannerConvergedEventInterval}, + {reasonClusterCapacityHeterogeneousGrowth, corev1.EventTypeWarning, time.Minute}, + {reasonClusterCapacityPlanned, corev1.EventTypeNormal, time.Minute}, } { t.Run(tc.reason, func(t *testing.T) { spec := plannerEventSpecs[tc.reason] if spec.eventType != tc.wantType { t.Errorf("eventType = %q, want %q", spec.eventType, tc.wantType) } - wantInterval := time.Minute - if tc.wantLong { - wantInterval = plannerConvergedEventInterval - } - if spec.interval != wantInterval { - t.Errorf("interval = %v, want %v", spec.interval, wantInterval) - } - if got := spec.key == keyPerNode; got != tc.wantPerNode { - t.Errorf("keyPerNode = %v, want %v", got, tc.wantPerNode) + if spec.interval != tc.wantInterval { + t.Errorf("interval = %v, want %v", spec.interval, tc.wantInterval) } }) } @@ -1118,35 +1109,77 @@ func TestAutoFullDrivesWarningReasonAndSeverity(t *testing.T) { } } -// TestAutoFullDrivesWarningsThrottlePerSubject: N constrained nodes must each get their own event, and a message -// whose numbers drift between reconciles must not spawn a second event for a subject already reported (lab: -// two event objects for one condition, "held 6 node(s)" then "held 5 node(s)"). -func TestAutoFullDrivesWarningsThrottlePerSubject(t *testing.T) { - loop := newAutoFullDrivesGrowthLoop(t, nil) +// 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. +func TestPlanAutoFullDrivesAggregatesPlacementDeferredIntoOneEvent(t *testing.T) { + withoutFormClusterComputeFloor(t) - emit := func(subject, message string) { - if err := loop.RecordEventThrottledPerSubject(corev1.EventTypeWarning, "AutoFullDrivesComputeLayout", - subject, message, time.Minute); err != nil { - t.Fatalf("RecordEventThrottledPerSubject: %v", err) - } + const bigFree = 1 << 28 + nodeNames := []string{"h1-2-a", "h1-3-d", "h4-5-d"} + var containers []*weka.WekaContainer + var nodeInv []capacityplanner.NodeCapacity + fdByNode := map[string]string{} + eligible := map[string]bool{} + for _, name := range nodeNames { + c := &weka.WekaContainer{} + c.Name = "drive-" + name + c.Spec.Mode = weka.WekaContainerModeDrive + c.Spec.NodeAffinity = weka.NodeName(name) + // Status.NodeAffinity left unset: the pod has not bound yet, which is what makes the container + // Unscheduled and its node deferred. + c.Spec.NumDrives = 1 + c.Spec.DriveCapacity = 1000 + c.Spec.NumCores = 1 + containers = append(containers, c) + + nodeInv = append(nodeInv, capacityplanner.NodeCapacity{ + NodeName: name, + FDValue: "fd-" + name, + OwnDriveCapacitiesGiB: []int{1000}, + AllocatableCPU: 10, + AvailableHugepagesMiB: bigFree, + AvailableMemoryMiB: bigFree, + }) + fdByNode[name] = "fd-" + name + eligible[name] = true + } + // Ample compute-only nodes so the deferred containers' frozen core demand always fits, keeping the + // assertion below from going vacuous on an infeasible plan. + for _, name := range []string{"compute-1", "compute-2"} { + nodeInv = append(nodeInv, capacityplanner.NodeCapacity{ + NodeName: name, FDValue: "fd-" + name, + AllocatableCPU: 64, AvailableHugepagesMiB: bigFree, AvailableMemoryMiB: bigFree, + }) + fdByNode[name] = "fd-" + name + eligible[name] = true } - emit("node-a", "node node-a cannot host its drives (node has 822 MiB free)") - emit("node-b", "node node-b cannot host its drives (node has 640 MiB free)") - // Same subject, drifting figure in the text: must be throttled, since node-a is already reported. - emit("node-a", "node node-a cannot host its drives (node has 118 MiB free)") - got := eventsMatching(drainLoopEvents(t, loop), "AutoFullDrivesComputeLayout") - if len(got) != 2 { - t.Fatalf("got %d event(s), want exactly 2 — one per subject, and the re-report of node-a with a "+ - "changed number must be throttled: %v", len(got), got) + r, _ := newAutoFullDrivesLoop(containers, func() (map[string]string, []capacityplanner.NodeCapacity, map[string]bool, error) { + return fdByNode, nodeInv, eligible, nil + }) + rec := record.NewFakeRecorder(16) + r.Recorder = rec + + plan, err := r.planAutoFullDrives(t.Context()) + if err != nil { + t.Fatalf("planAutoFullDrives() unexpected error: %v", err) } - var sawA, sawB bool - for _, ev := range got { - sawA = sawA || strings.Contains(ev, "node-a") - sawB = sawB || strings.Contains(ev, "node-b") + if plan.Infeasible != "" { + t.Fatalf("fixture went infeasible (%q) -- the event assertion below would be vacuous", plan.Infeasible) } - if !sawA || !sawB { - t.Errorf("want one event per constrained node (node-a and node-b), got: %v", got) + + got := eventsMatching(drainEvents(rec), "AutoFullDrivesPlacementDeferred") + if len(got) != 1 { + t.Fatalf("got %d AutoFullDrivesPlacementDeferred event(s), want exactly 1 covering all %d nodes: %v", + len(got), len(nodeNames), got) + } + for _, name := range nodeNames { + if !strings.Contains(got[0], name) { + t.Errorf("event does not name deferred node %q: %s", name, got[0]) + } } } diff --git a/internal/controllers/wekacluster/funcs_upgrade_test.go b/internal/controllers/wekacluster/funcs_upgrade_test.go index aeb8246d8..6eb44c481 100644 --- a/internal/controllers/wekacluster/funcs_upgrade_test.go +++ b/internal/controllers/wekacluster/funcs_upgrade_test.go @@ -50,7 +50,7 @@ func newFakeClient(t *testing.T, objs ...client.Object) client.Client { // newUpgradeLoop builds a wekaClusterReconcilerLoop wired with a real fake client seeded with cluster and // containers, ready to exercise HandleSpecUpdates end-to-end. Throttler is a real (not nil) SyncMapThrottler // since FetchCluster — which normally wires it — is never called by tests that build the loop directly, and -// RecordEventThrottled/RecordEventThrottledPerSubject panic on a nil Throttler. +// RecordEventThrottled panics on a nil Throttler. func newUpgradeLoop(t *testing.T, cluster *weka.WekaCluster, containers []*weka.WekaContainer) *wekaClusterReconcilerLoop { t.Helper() objs := make([]client.Object, 0, len(containers)+1) diff --git a/internal/controllers/wekacluster/planner_events.go b/internal/controllers/wekacluster/planner_events.go index db697cec1..5bf2d28e9 100644 --- a/internal/controllers/wekacluster/planner_events.go +++ b/internal/controllers/wekacluster/planner_events.go @@ -43,61 +43,53 @@ const ( // alerting. Reasons describing a transition or a hard stop keep the shorter window. const plannerConvergedEventInterval = 15 * time.Minute -type throttleKey int - -const ( - // keyPerReason: one event per reason per window. - keyPerReason throttleKey = iota - // keyPerNode: one event per node per window, so N constrained nodes each get one instead of the first - // starving the rest. Keying on the message instead would post a fresh event every time an embedded - // number drifted. - keyPerNode -) +// 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. +const plannerAggregateEventInterval = 3 * time.Minute type plannerEventSpec struct { eventType string interval time.Duration - key throttleKey } var plannerEventSpecs = map[string]plannerEventSpec{ - reasonClusterCapacityPlanned: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonClusterCapacityInfeasible: {corev1.EventTypeWarning, time.Minute, keyPerReason}, - reasonClusterCapacityDeferred: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonClusterCapacityShrink: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonClusterCapacityOverProvisioned: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonClusterCapacityHeterogeneousGrowth: {corev1.EventTypeWarning, time.Minute, keyPerReason}, + reasonClusterCapacityPlanned: {corev1.EventTypeNormal, time.Minute}, + reasonClusterCapacityInfeasible: {corev1.EventTypeWarning, time.Minute}, + reasonClusterCapacityDeferred: {corev1.EventTypeNormal, time.Minute}, + reasonClusterCapacityShrink: {corev1.EventTypeNormal, time.Minute}, + reasonClusterCapacityOverProvisioned: {corev1.EventTypeNormal, time.Minute}, + reasonClusterCapacityHeterogeneousGrowth: {corev1.EventTypeWarning, time.Minute}, - reasonAutoFullDrivesPlanned: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonAutoFullDrivesInfeasible: {corev1.EventTypeWarning, time.Minute, keyPerReason}, - reasonAutoFullDrivesNoSignedDrives: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonAutoFullDrivesGrowthDetected: {corev1.EventTypeNormal, time.Minute, keyPerReason}, - reasonAutoFullDrivesGrowthDeferred: {corev1.EventTypeWarning, plannerConvergedEventInterval, keyPerReason}, + reasonAutoFullDrivesPlanned: {corev1.EventTypeNormal, time.Minute}, + reasonAutoFullDrivesInfeasible: {corev1.EventTypeWarning, time.Minute}, + reasonAutoFullDrivesNoSignedDrives: {corev1.EventTypeNormal, time.Minute}, + reasonAutoFullDrivesGrowthDetected: {corev1.EventTypeNormal, time.Minute}, + reasonAutoFullDrivesGrowthDeferred: {corev1.EventTypeWarning, plannerConvergedEventInterval}, // Stranding is expected under a numDrives pin and a transient deferral clears itself, so neither is a // Warning — emitting them as such made a healthy converged cluster accumulate Warnings. - reasonAutoFullDrivesDrivesStranded: {corev1.EventTypeNormal, plannerConvergedEventInterval, keyPerReason}, - reasonAutoFullDrivesPlacementDeferred: {corev1.EventTypeNormal, plannerConvergedEventInterval, keyPerNode}, + reasonAutoFullDrivesDrivesStranded: {corev1.EventTypeNormal, plannerAggregateEventInterval}, + reasonAutoFullDrivesPlacementDeferred: {corev1.EventTypeNormal, plannerAggregateEventInterval}, // Normal, not Warning: withholding a node costs nothing on its own — the plan proceeds on the rest, and // when the loss does matter the plan turns infeasible and AutoFullDrivesInfeasible carries that as a - // Warning. Keyed per node so one node's condition cannot starve another's out of the throttle window. - reasonAutoFullDrivesNodeIneligible: {corev1.EventTypeNormal, plannerConvergedEventInterval, keyPerNode}, - reasonAutoFullDrivesComputeLayout: {corev1.EventTypeWarning, plannerConvergedEventInterval, keyPerReason}, - reasonAutoFullDrivesWarning: {corev1.EventTypeWarning, plannerConvergedEventInterval, keyPerReason}, + // Warning. Aggregate window, not the converged one: cordon/taint is an administrative state that persists + // for minutes-to-hours, but the set of cordoned nodes changes within that, and the throttle key cannot + // tell one node list from another. + reasonAutoFullDrivesNodeIneligible: {corev1.EventTypeNormal, plannerAggregateEventInterval}, + reasonAutoFullDrivesComputeLayout: {corev1.EventTypeWarning, plannerConvergedEventInterval}, + reasonAutoFullDrivesWarning: {corev1.EventTypeWarning, plannerConvergedEventInterval}, } -// emitPlannerEvent records message on the WekaCluster under reason, with that reason's policy. subject is the -// node name, used only by keyPerNode rows. -func (r *wekaClusterReconcilerLoop) emitPlannerEvent(reason, subject, message string) { +// emitPlannerEvent records message on the WekaCluster under reason, with that reason's policy. +func (r *wekaClusterReconcilerLoop) emitPlannerEvent(reason, 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} } - if spec.key == keyPerNode { - _ = r.RecordEventThrottledPerSubject(spec.eventType, reason, subject, message, spec.interval) //nolint:errcheck // best effort - return - } _ = r.RecordEventThrottled(spec.eventType, reason, message, spec.interval) //nolint:errcheck // best effort } diff --git a/internal/controllers/wekacluster/reconciler_loop.go b/internal/controllers/wekacluster/reconciler_loop.go index 4cfe63add..2e1c13854 100644 --- a/internal/controllers/wekacluster/reconciler_loop.go +++ b/internal/controllers/wekacluster/reconciler_loop.go @@ -183,21 +183,6 @@ func (r *wekaClusterReconcilerLoop) RecordEventThrottled(eventtype, reason, mess return r.RecordEvent(eventtype, reason, message) } -// RecordEventThrottledPerSubject throttles on eventtype+reason+subject — the node/container the event is -// about — instead of the message text. Use for a reason whose messages embed changing numbers (e.g. free -// hugepages, node counts): per-message throttling would key on that changing prose and never dedupe. -// Empty subject collapses to RecordEventThrottled's per-reason behavior. -func (r *wekaClusterReconcilerLoop) RecordEventThrottledPerSubject(eventtype, reason, subject, message string, interval time.Duration) error { - if !r.Throttler.ShouldRun(eventtype+reason+subject, &throttling.ThrottlingSettings{ - Interval: interval, - DisableRandomPreSetInterval: true, - }) { - return nil - } - - return r.RecordEvent(eventtype, reason, message) -} - func (r *wekaClusterReconcilerLoop) ClusterDeletionCancelled() bool { return r.cluster.Spec.GetOverrides().CancelDeletion } diff --git a/internal/controllers/wekacluster/steps_planner_apply.go b/internal/controllers/wekacluster/steps_planner_apply.go index 506a051d7..200cd8d66 100644 --- a/internal/controllers/wekacluster/steps_planner_apply.go +++ b/internal/controllers/wekacluster/steps_planner_apply.go @@ -326,11 +326,11 @@ func (r *wekaClusterReconcilerLoop) announceDriveGrowth(plan *capacityplanner.Ca msg += fmt.Sprintf("; %d of %d planned container(s) could not be grown and will be retried (see the operator log)", failed, len(plan.Grow)) } - r.emitPlannerEvent(reasonAutoFullDrivesGrowthDetected, "", msg) + r.emitPlannerEvent(reasonAutoFullDrivesGrowthDetected, msg) return } if err != nil { - r.emitPlannerEvent(reasonAutoFullDrivesGrowthDeferred, "", + r.emitPlannerEvent(reasonAutoFullDrivesGrowthDeferred, fmt.Sprintf("auto full drives growth was planned for %d drive container(s) but none could be applied: %v; the operator retries on the next reconcile, but a later plan may no longer offer the same growth", len(plan.Grow), err)) }