fix: aggregate auto-full-drives planner warnings into one event per condition instead of per node - #2768
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @kristina-solovyova's task in 4m 7s —— View job Review: aggregate auto-full-drives planner warnings
The core change is right and well executed: the walk collects Four findings, posted inline: 1. 2. 3. 4. No security concerns; no performance concerns (the aggregation replaces per-node allocations with three small slices). Test coverage for the new behaviour is solid — the One caveat: |
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/20/26)2 reviewers were added to this PR based on Anton Bykov's automation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if row.DrivesUsed < row.DrivesAvail { | ||
| row.Note = warningForNode(plan.Warnings, n.NodeName) | ||
| // A stranded node has no per-node Subject to match above (see hasFleetWarning) — point at the | ||
| // fleet warning instead, but only for a row the plan actually placed something on: | ||
| // nodeStateNotPlanned means the walk never sized this node at all, so "held back by the pin" | ||
| // would misattribute an infeasibility skip to the pin. | ||
| if row.Note == "" && row.State != nodeStateNotPlanned && | ||
| hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded) { | ||
| switch { | ||
| case row.State == nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindNodeIneligible): | ||
| row.Note = "cordoned/not ready/untolerated taint — see WARNINGS" | ||
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | ||
| row.Note = "drives held back by the numDrives pin — see WARNINGS" | ||
| } | ||
| } |
There was a problem hiding this comment.
Misattributed NOTE for not-planned rows. With Subject gone, this branch fires for every nodeStateNotPlanned row as soon as any node in the fleet is ineligible — the row itself may have nothing to do with cordon/taint. Two concrete cases:
- a node skipped for
HasDeletingDriveContainer(autofulldrives.go:146) isnotPlannedwithDrivesUsed=0 < DrivesAvail, and gets labelledcordoned/not ready/untolerated taintif some other node happens to be cordoned. Before this PR,warningForNodegave it its own "container being deleted" text. - on an infeasible plan, every fit-failure node is
notPlannedtoo, and gets the same wrong label.
The row already has the authoritative per-node answer in hand — NodeCapacity.IneligibleReason / HasDeletingDriveContainer (internal/capacityplanner/nodecapacity.go:36) — so the aggregated warning doesn't need to be consulted for this at all:
| if row.DrivesUsed < row.DrivesAvail { | |
| row.Note = warningForNode(plan.Warnings, n.NodeName) | |
| // A stranded node has no per-node Subject to match above (see hasFleetWarning) — point at the | |
| // fleet warning instead, but only for a row the plan actually placed something on: | |
| // nodeStateNotPlanned means the walk never sized this node at all, so "held back by the pin" | |
| // would misattribute an infeasibility skip to the pin. | |
| if row.Note == "" && row.State != nodeStateNotPlanned && | |
| hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded) { | |
| switch { | |
| case row.State == nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindNodeIneligible): | |
| row.Note = "cordoned/not ready/untolerated taint — see WARNINGS" | |
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | |
| row.Note = "drives held back by the numDrives pin — see WARNINGS" | |
| } | |
| } | |
| if row.DrivesUsed < row.DrivesAvail { | |
| switch { | |
| case row.State == nodeStateNotPlanned && n.IneligibleReason != "": | |
| row.Note = n.IneligibleReason + " — see WARNINGS" | |
| case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer: | |
| row.Note = "drive container being deleted — see WARNINGS" | |
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | |
| row.Note = "drives held back by the numDrives pin — see WARNINGS" | |
| } | |
| } |
That also makes the NOTE strictly more informative (the actual reason verbatim, matching what the warning says about that node) and leaves hasFleetWarning with a single caller.
| for _, w := range warnings { | ||
| plan.Warnings = append(plan.Warnings, Warning{Kind: WarningKindComputeLayout, Message: w}) | ||
| if len(warnings) > 0 { | ||
| plan.Warnings = append(plan.Warnings, fleetWarning(WarningKindComputeLayout, "%s", strings.Join(warnings, "; "))) |
There was a problem hiding this comment.
Joining these is a real improvement, but it doesn't finish the job for ComputeLayout: autoPlaceNewCompute appends a second WarningKindComputeLayout warning of its own (autofulldrives_compute.go:379, the form-cluster-floor surplus advisory), and it runs on the very next line — so a pass that hits both produces two warnings under the same reason.
That is exactly the hazard formatPlacementDeferredWarning documents ("two warnings would let one silently suppress the other"): emitPlannerEvent now throttles on eventtype+reason only, so the second one is silently dropped for the whole 15-minute AutoFullDrivesComputeLayout window. The doc change in this PR also asserts the property that doesn't hold yet — "Every compute-sizing advisory from the shared compute layout step, joined into one message per pass" (act-as-daemonset.md:864).
Cheapest fix that keeps the invariant "one Warning per reason per pass": accumulate compute advisories on in (or return them from autoPlaceNewCompute) and emit the single joined fleetWarning once at the end of planComputeAutoFullDrives, the same shape the drive walk uses.
| reasonAutoFullDrivesDrivesStranded: {corev1.EventTypeNormal, plannerConvergedEventInterval}, | ||
| // 1 minute, not plannerConvergedEventInterval: the message is a fleet-wide snapshot naming every deferred | ||
| // node, and a short window lets it refresh as pods bind during formation. | ||
| reasonAutoFullDrivesPlacementDeferred: {corev1.EventTypeNormal, time.Minute}, | ||
| // 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. 1 minute so the named node list stays current as nodes drain out of the set. | ||
| reasonAutoFullDrivesNodeIneligible: {corev1.EventTypeNormal, time.Minute}, |
There was a problem hiding this comment.
Dropping these two from 15 min to 1 min looks like the wrong half of the trade. Aggregation removed the reason for the per-node key, but not the reason for the long window — the doc paragraph right above the table still says it: "The advisories that describe a converged state use a long window, because a permanently compute-limited cluster is healthy and re-posting … every minute forever trips alerting", and the sentence this PR deletes from the NodeIneligible row made the same point for a node left cordoned for maintenance.
Concretely, a node cordoned for a day-long maintenance now posts an AutoFullDrivesNodeIneligible event every minute for that whole day instead of 4/hour. They're Normal, so alerting is mostly spared, but identical repeats are not free: client-side event spam filtering is per (source, involvedObject), so a minute-cadence event burns the WekaCluster object's budget and can suppress a genuinely new event on the same cluster.
The freshness argument is also weaker than it reads: the throttle key ignores the message, so a changed node set is not what re-posts — the window expiring is. 15 min still refreshes the list, just more slowly, which for cordon/taint (an administrative state that persists for minutes-to-hours) seems like the right cadence.
If refresh-on-change is genuinely what you want here, the honest way to get it is to include the message in the throttle key for these two reasons — quiet while the set is stable, immediate re-post when a node enters or leaves it — rather than a blanket 15× cadence increase. DrivesStranded is left at 15 min while being the same shape of fleet-wide snapshot, so as written the three rows don't follow a consistent rule either.
| // listNodes joins per-node parts, capping the list at autoFullDrivesMaxNamedNodes with a "(+N more)" tail | ||
| // so a large fleet cannot turn one aggregated event into a multi-KB message. | ||
| func listNodes(parts []string) string { | ||
| if len(parts) <= autoFullDrivesMaxNamedNodes { | ||
| return strings.Join(parts, ", ") | ||
| } | ||
| return fmt.Sprintf("%s (+%d more)", strings.Join(parts[:autoFullDrivesMaxNamedNodes], ", "), | ||
| len(parts)-autoFullDrivesMaxNamedNodes) |
There was a problem hiding this comment.
Nit: listNodes, formatIneligibleWarning and formatPlacementDeferredWarning don't belong in this file by its own header — "the pure sizing layer … every semantics rule about how big a container is, with no resource model at all". Eligibility and placement-deferral messages are neither. formatStrandedWarning fits (stranding is a sizing outcome of the numDrives pin); the two new ones read like they want to live next to their producer in autofulldrives.go, or in a small autofulldrives_warnings.go alongside listNodes.
Also worth a thought while the cap is fresh: listNodes bounds each list at 10, but formatPlacementDeferredWarning can emit two of them (up to 20 names) and formatIneligibleWarning's entries each carry a reason suffix, so the aggregated message is a good deal longer than the 10-name cap suggests for an event message.
8303204 to
ba8c753
Compare
|
Claude finished @kristina-solovyova's task in 5m 3s —— View job Re-review: aggregate auto-full-drives planner warnings
All four findings from the previous round are addressed, and addressed properly rather than papered over:
Four new comments posted inline, one of which I'd want resolved before merge: 1. 2. 3. 4. No security concerns. No performance concerns — the aggregation replaces per-node Test coverage for the new behaviour is genuinely good — the Caveat, same as last round: |
| 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)) | ||
| } |
There was a problem hiding this comment.
Moving the emission after the walk drops these three warnings on the autoSizeNode early return. autoSizeNode failing (:157-160) does setInfeasible(&plan, report); return plan, ... — above this block — so everything ineligible/deleting/deferred collected from the nodes already walked is discarded. Before the PR those were appended inline as the walk hit each node, so they survived that return.
Two things make it worth fixing rather than accepting:
- it contradicts the other infeasibility path six lines down:
autoNodeFitInfeasible(failures)runs after this block, so a fit-failure infeasibility keeps its ineligible/deferred advisories while a pin-conflict infeasibility (driveCoresabove a node's drive count,numDrivesabove its signed count) silently loses them; - the visible consumer is the
weka-capacitydry-run, which renders WARNINGS for an infeasible plan. A run that hits the pin conflict on node 3 of 14 now shows nothing about the 2 cordoned nodes it already walked past. (No production event change —planAutoFullDrivesreturns early onplan.Infeasible != ""before emitting warnings.)
Cheapest fix is to make the emission unconditional at every exit — hoist lines 239-247 into a small emitWalkWarnings(&plan) closure defined above the loop and call it on the autoSizeNode return too, or defer it.
| 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) | ||
| } |
There was a problem hiding this comment.
Not introduced here, but this PR's whole thesis makes it stand out and it's on lines you're already touching: these three loops are the same bug you just fixed for auto-full-drives, still standing for clusterCapacity.
Each emits N messages under one reason, and RecordEventThrottled keys on eventtype+reason only (reconciler_loop.go:176). So on a plan with 3 shrink events, the first posts and events 2-3 are dropped; the next reconcile re-emits the same list in the same order, so element 0 wins again and elements 1..N-1 are never posted at all — not delayed, invisible. Same for plan.Warnings under ClusterCapacityHeterogeneousGrowth and plan.OverProvisions.
Auto-full-drives now guarantees one Warning per reason per pass at the producer; clusterCapacity has no such guarantee and its producer (planner.go:1200) appends one heterogeneous-growth advisory per pool. Either join at the call site here (strings.Join(capacityplanner.WarningMessages(plan.Warnings), "; "), and likewise for the two []string slices), or aggregate in the planner the way formatStrandedWarning does. Fine as a follow-up if you'd rather keep this PR scoped to the auto path — but worth a line in the commit or a ticket, since the doc paragraph you added ("A per-reason window is enough because each planner advisory is already aggregated") now reads as a repo-wide claim that only holds for auto-full-drives.
| // 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 one that fires in practice; deriveComputeLayout feeds the same join. | ||
| 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 |
There was a problem hiding this comment.
The join is the right shape and closes the double-emit hazard. Two small things:
"deriveComputeLayout feeds the same join" doesn't describe live behaviour. deriveComputeLayout declares warnings []string as a named return and never assigns to it — all three success paths are return count, cores, "", warnings with warnings still nil (compute_layout.go:90,124,151,178). So advisories starts empty every time and autoPlaceNewCompute's surplus advisory is not just "the one that fires in practice", it's the only one that can fire at all today. The comment reads as if two producers are live; worth saying it's a seam kept open for when deriveComputeLayout grows advisories, or dropping that clause.
plan.Warnings still gets the advisory when autoPlaceNewCompute sets the plan infeasible (:392 surplus append happens before the count > len(candidates) setInfeasible at :399). Same as before the PR and harmless — the controller returns on Infeasible != "" before reading warnings — but the CLI shows both. Only worth a thought if you're already tidying the infeasible-plan-carries-no-advisories rule that planAutoFullDrivesDrives states in its own comment.
| 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) |
There was a problem hiding this comment.
Nit on wording: "placement deferred on %d node(s)" overstates the deferred group — those nodes already host a container, and what's deferred is its growth, not placement. The deleting group is the one where placement waits. The clause texts get this right individually ("growth waits for the scheduler" / "new placement waits for it"), so only the lead-in is off; "auto full drives: %d node(s) skipped this pass" would cover both without picking a side. The reason name AutoFullDrivesPlacementDeferred is stuck with the same slight stretch, so this is cosmetic.
ba8c753 to
2fd3b85
Compare
|
Claude finished @kristina-solovyova's task in 4m 16s —— View job Re-review (round 3): aggregate auto-full-drives planner warnings
Round-2 findings 1, 3 and 4 (the doc half of it) are addressed:
Also new since last round and worth calling out as good: the CLI gained a fourth NOTE case for an unscheduled pod ( Two findings, posted inline: 1. 2. Still open from round 2, both fine to leave: the No security concerns. No performance concerns. Determinism holds — Caveat, unchanged from previous rounds: |
| case row.State == nodeStateNotPlanned && n.IneligibleReason != "": | ||
| row.Note = n.IneligibleReason + " — see WARNINGS" | ||
| case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer: | ||
| row.Note = "drive container being deleted — see WARNINGS" | ||
| case existingByNode[n.NodeName].Unscheduled: | ||
| row.Note = "pod has not been scheduled yet — see WARNINGS" | ||
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | ||
| row.Note = "drives held back by the numDrives pin — see WARNINGS" |
There was a problem hiding this comment.
flushWarnings fixes the prefix of the walk, but the NOTE column still promises WARNINGS entries for its suffix. These three per-node cases read the node's own inventory, which is populated for every node regardless of how far the walk got — while the aggregated warnings only cover nodes visited before an autoSizeNode infeasibility returned (autofulldrives.go:171-176). Nodes are walked and rendered in name order, so a node sorted after the failing one gets "cordoned — see WARNINGS" with no NodeIneligible entry in WARNINGS to see.
Concretely, flip the fixture in TestPlanAutoFullDrives_InfeasibleMidWalk_KeepsWarningsAlreadyCollected (a-pinned / z-cordoned instead of a-cordoned / z-pinned): the plan is infeasible, z-cordoned's row says "cordoned — see WARNINGS", and WARNINGS has nothing about it. Same for HasDeletingDriveContainer and Unscheduled.
The suffix is what creates the promise, and for these three cases it earns nothing — the reason is already spelled out verbatim on the row, and the aggregated warning adds only the fleet total. Dropping it makes the note true on every plan:
| case row.State == nodeStateNotPlanned && n.IneligibleReason != "": | |
| row.Note = n.IneligibleReason + " — see WARNINGS" | |
| case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer: | |
| row.Note = "drive container being deleted — see WARNINGS" | |
| case existingByNode[n.NodeName].Unscheduled: | |
| row.Note = "pod has not been scheduled yet — see WARNINGS" | |
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | |
| row.Note = "drives held back by the numDrives pin — see WARNINGS" | |
| switch { | |
| case row.State == nodeStateNotPlanned && n.IneligibleReason != "": | |
| row.Note = n.IneligibleReason | |
| case row.State == nodeStateNotPlanned && n.HasDeletingDriveContainer: | |
| row.Note = "drive container being deleted" | |
| case existingByNode[n.NodeName].Unscheduled: | |
| row.Note = "pod has not been scheduled yet" | |
| case row.State != nodeStateNotPlanned && hasFleetWarning(plan.Warnings, capacityplanner.WarningKindDrivesStranded): | |
| row.Note = "drives held back by the numDrives pin — see WARNINGS" | |
| } |
The DrivesStranded case keeps its pointer legitimately: it is gated on the warning actually being present.
| // 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 |
There was a problem hiding this comment.
The middle ground is a fair call and the rationale is now written down — but it moves DrivesStranded from 15 min to 3 min as collateral, and it's the one reason of the three that describes a permanent, operator-requested state. A cluster with numDrives pinned re-posts it 20×/hour forever, where the whole point of plannerConvergedEventInterval was that a converged-but-constrained cluster should be quiet.
The cost isn't just noise. RecordEvent goes to a plain record.EventRecorder, so every event passes client-go's EventSourceObjectSpamFilter — a token bucket of burst 25 refilling at 1 per 300 s, keyed by (source, involvedObject) in the default correlator, i.e. shared by every event this controller posts on that WekaCluster. Three aggregate reasons at 3 min ≈ 60 events/hour against a 12/hour refill: a fleet that is persistently stranded and has a cordoned node drains the bucket in roughly half an hour, after which the next AutoFullDrivesInfeasible or ClusterCapacityInfeasible on that cluster is dropped rather than delayed. At 15 min the three together sat at 12/hour, right at the refill rate. (I couldn't read the vendored client-go v0.36.3 source in this sandbox to re-confirm the spam-key function, so treat the exact key as "the default one".)
Two ways out that don't reintroduce the stale-set problem:
- Keep
plannerAggregateEventIntervalfor the two that genuinely change set membership (NodeIneligible,PlacementDeferred) and leaveDrivesStrandedonplannerConvergedEventInterval— its set only changes when someone edits the pin or signs drives, both of which already produce other events. - Or make the throttle key include the message for the aggregates, which is what the comment actually wants ("the throttle key cannot tell one node list from another"): silent while the set is stable, immediate re-post the moment a node joins or leaves it.
RecordEventThrottled's key iseventtype+reasonatreconciler_loop.go:176, so this is a caller-side change —emitPlannerEventcould passreason+"/"+hash(message)for the reasons flagged as aggregates.
Non-blocking if you've weighed it; the first option is a one-line change and gets most of the benefit.
2fd3b85 to
796b25d
Compare
|
Claude finished @kristina-solovyova's task in 3m 46s —— View job Re-review (round 4): aggregate auto-full-drives planner warnings
Both round-3 findings are answered:
Also new and good since round 3: Three findings, posted inline; only the first is worth resolving: 1. 2. 3. Still open from earlier rounds and fine to carry: the No security concerns. No performance concerns. Determinism holds — Caveat, unchanged from every prior round: |
| // 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 | ||
| } |
There was a problem hiding this comment.
Gating the pointer on the aggregate being present is the right shape, and it closes the "no warning at all" half of the previous finding — but it doesn't close the half where the aggregate exists yet doesn't cover this node. flushWarnings snapshots the nodes walked before the abort; the row conditions come from NodeCapacity, which is populated for all of them. Since both are name-ordered, a node sorted after the failing one can share a kind with an earlier node and still be missing from the message:
a-cordoned → ineligible = ["a-cordoned (cordoned)"]
m-pinned → autoSizeNode reports; flushWarnings(); return
z-cordoned → never walked
WARNINGS then reads 1 node(s) ... are ineligible ...: a-cordoned (cordoned) while z-cordoned's row says cordoned — see WARNINGS. Same for WarningKindTransient, which is worse because it carries two causes: one deferred node before the abort makes hasFleetWarning(Transient) true, so a HasDeletingDriveContainer node after the abort points at a message whose only clause is "pod not scheduled yet".
That also makes the hasFleetWarning doc's second sentence too strong — "gating a pointer at the WARNINGS section is sound for any kind" holds only when kind-presence implies this row is named in it, which is exactly what the abort breaks.
Matching on the node name inside the message is not the fix (it reintroduces the n1/n10 collision warningForNode was written to avoid). Either drop the suffix on the three per-node cases — the reason is already verbatim on the row and the aggregate adds only a fleet total, so nothing is lost — or have the planner mark whether the walk ran to completion and gate on that, since warnings are complete for the autoNodeFitInfeasible path and incomplete only for the mid-walk one.
The existing regression test only covers plan.Warnings == nil; the case above needs a fixture where the aggregate is present but partial. Fix this →
| // 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) { |
There was a problem hiding this comment.
The budget-sharing test is a good addition, but it replaced TestFormatPlacementDeferredWarning_OmitsEmptyGroups rather than joining it, so the single-cause path lost its only coverage. Nothing now exercises:
clauseswith exactly one entry — i.e. that the empty group is omitted and no stray;separator or empty node list is emitted;- the
"it retries automatically"singular tail (autofulldrives_warnings.go:81), which is currently unreachable from any test in the repo — a grep for that string hits only the source line; limit == autoFullDrivesMaxNamedNodeswhen only one cause is present, which is the branch the new test'sif len(deferred) > 0 && len(deleting) > 0guard skips.
Both are cheap to keep — a two-case subtest (deferred only / deleting only) alongside the shared-budget one restores it.
| // 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 | ||
| } |
There was a problem hiding this comment.
Nit: a flat halving spends the budget by group count rather than by need, so a lopsided pass names far fewer nodes than the cap allows. With 1 deferred and 30 deleting, both get limit = 5: the message names 6 nodes and truncates the deleting list at 5, while 9 of the 10-name budget was available. Giving the smaller group what it actually needs and the rest to the other covers both cases with one extra line:
limit := autoFullDrivesMaxNamedNodes
if len(deferred) > 0 && len(deleting) > 0 {
limit = autoFullDrivesMaxNamedNodes / 2
}
deferredLimit, deletingLimit := limit, limit
if spare := limit - len(deferred); spare > 0 {
deletingLimit += spare
}
if spare := limit - len(deleting); spare > 0 {
deferredLimit += spare
}Not worth much on its own — the cap is a message-size guard, not a display budget — so fine to leave if you'd rather keep the two lines.
b6b214a to
8f3d30b
Compare
796b25d to
256aa42
Compare
8f3d30b to
c168aa3
Compare
256aa42 to
2db8a12
Compare
Merge activity
|
…ondition instead of per node
c168aa3 to
e4a3d56
Compare
2db8a12 to
678d205
Compare
2 participants
No description provided.