fix(foreman): clear Dispatched condition on terminal Workloads - #1741
Conversation
A terminal Workload kept Dispatched=True with a stale message claiming work was still in flight, because the terminal branches of computeTerminalState wrote the Completed condition and never touched Dispatched. The condition was write-only on the in-flight path and never reconciled elsewhere. Flip Dispatched to ConditionFalse (with a Reason naming the terminal outcome) on every terminal branch: AllChildrenSucceeded, AllAlreadyResolved, the mixed Completed case, and ChildrenFailed / ChildrenIncomplete. The condition is kept rather than deleted so `kubectl wait --for=condition=Dispatched=false` stays usable and LastTransitionTime records when dispatch actually ended. The in-flight default branch is unchanged (still True / ChildrenInFlight). Adds a table-driven regression test covering all four terminal branches plus the in-flight branch. Removing any terminal Dispatched=False write makes the test fail, so the fix is mutation-surviving. Refs defilantech#1739 Signed-off-by: Foreman Bot <chris@mahercode.io>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Defilan
left a comment
Defilan
left a comment
There was a problem hiding this comment.
Reviewed the diff. Correct, and it covers the case a partial fix would miss.
All four terminal branches are covered, not just the obvious one. I checked
each call site: setDispatchedTerminal is invoked from the all-succeeded
branch, the pure already-resolved branch, the mixed
succeeded-plus-already-resolved branch, and the failed branch. Missing any one
of them would leave exactly the same stale condition on that path, which is the
most likely way this fix gets half-done. It is not half-done here.
Flipping to False rather than deleting is the right call and the godoc says
why. kubectl wait --for=condition=Dispatched=false stays usable, and
LastTransitionTime records when dispatch actually ended. A deleted condition
would have neither property.
The second test is what gives the first one meaning. "A Workload still in
flight keeps Dispatched True" is what stops an implementation that simply
hard-codes the condition to False from passing. Worth keeping if this area is
refactored later.
Small scope addition, and I think a good one. The change also replaces the
"Dispatched" string literal with a conditionTypeDispatched constant, which
was not strictly required. It matches how conditionTypeCompleted and the other
condition types in this file are already declared, and it is what made it easy
to verify that the literal appeared in exactly one place before this change.
Merge ordering. This PR and #1743 both edit the default: branch of the
same switch. git merge-tree reports no conflict markers, but a real git merge
fails on workload_controller.go. Merge one, rebase the other; the test file
auto-merges cleanly.
Scope is clean otherwise: classifyChildren untouched, and the Completed
condition's reason and message unchanged.
Context on where the bug came from, since it is not in the diff: this was
observed live on two separate completed Workloads (wl-1729-depwait-default
and wl-1684-scope-inert), both reporting Dispatched True, "1 in-flight"
alongside Completed True, "3/3 child tasks on-target Succeeded".
1 participant
What
Flip the
Dispatchedcondition toFalsewhen a Workload reaches a terminalphase, so a Completed Workload stops advertising work in flight.
Why
Refs #1739
"Dispatched"was written in exactly one place, thedefault:branch of theterminal-state switch, and never touched again. The terminal branches set the
Completedcondition and leftDispatchedholding whatever the last in-flightpass wrote. The condition was write-only on one path and never reconciled on the
others.
Observed on
wl-1729-depwait-default, and identically onwl-1684-scope-inert:All three children had reached Succeeded. The Workload is Completed. The
condition still claimed one child was in flight.
How
A
setDispatchedTerminalhelper sets the condition toConditionFalsewith aReasonnaming the terminal outcome, called from every branch that sets aterminal phase: all-succeeded, pure already-resolved, the mixed
succeeded-plus-already-resolved case, and the failed case. Missing any one of
them would leave the same stale condition on that path.
The condition is flipped rather than deleted. A condition that disappears is
harder to consume than one that reads False:
kubectl wait --for=condition=Dispatched=falsestays usable, andLastTransitionTimethenrecords when dispatch actually ended. Leaving it True with corrected counts
would still misreport the state, which is the bug.
The string literal is also replaced with a
conditionTypeDispatchedconstant,matching how
conditionTypeCompletedand the other condition types in this fileare already declared.
Scope
Deliberately separate from #1738, which reconciles the child count against the
planned count. Both touch this switch, but they are different branches,
different fixes and different tests.
classifyChildrenis untouched and theCompletedcondition's reason and message are unchanged.Tests
Dispatchedis present andConditionFalseDispatchedremainsConditionTrueThe second case is what stops a naive implementation from simply hard-coding the
condition to False.
Checklist
make testpasses locally — ran in full in the clean-room gate Job forthis branch (GATE-PASS), including the envtest packages
make lintpasses locally — the same gate Job runs fmt, vet, lint andlint-deadcode
git commit -s) per DCO(DeepSeek V4 Flash, self-hosted) under band 3 of CONTRIBUTING.md, reviewed
by the automated reviewer and by the maintainer, who owns this review
conversation.
Note for merging: this PR and the one for #1738 both edit the
default:branchof the same switch and do conflict. Merge one, then rebase the other.