🤖 Generated by the Agentic Engineer
A refused node cordon claim can report no reason at all, in the one case where the reason matters most: another actor cordoned the node, took ownership, or replaced it while the roll was claiming it.
Evidence
Found while reviewing #2977 (its bounded CAS retry is what introduces the path). In claim_node_cordon_ownership:
build_and_apply_cordon_claim ... "${result_file}" ... && return 0 # conflict written to result_file
...
kubectl get node "${node_name}" -o json >"${state_file}" 2>"${result_file}" # <-- truncates it
...
if ! node_claim_preconditions_still_hold ...; then break; fi
...
emit_safe_operation_output "cordon-claim" "${result_file}"The retry re-reads the Node to decide whether the rejected claim is still safe to retry, and points that read's stderr at the same result_file holding the conflict output. A successful read writes nothing, truncating the file — and emit_safe_operation_output returns early on an empty file:
[[ -s "${result_file}" ]] || return 0So the operator gets a bare ::error::Could not atomically claim and cordon Talos node X; refusing to drain it. with no cause attached.
Affected path only. The exhausted-retry-budget break happens before the re-read, so it still reports the conflict; a failed re-read leaves its own stderr, which is meaningful. Only the "preconditions no longer hold" break — the concurrent-actor case — loses its cause.
Impact
Safety is unaffected: the claim still refuses and no drain proceeds. What is lost is the explanation, on a production node-drain refusal, at the moment a second actor is competing for the node. That is the least actionable moment to hand someone a reasonless error.
Why it was not caught
TestConcurrentCordonBeforeAtomicClaimStopsTheRoll asserts the refusal line but never asserts a cause is emitted, so it passes with the output empty.
Fix (implemented and proven)
Give the re-read its own stderr sink, and promote it to the reported cause only when the read itself fails. Plus the missing assertion.
RED/GREEN, one substitution (the mechanism moved, not the test deleted):
| Arm |
Result |
| with fix |
rc=0 |
stderr sink restored to the shared result_file |
rc=1 — rollout_core_test.go:380: expected document to contain "cordon-claim: " |
| restored |
rc=0 |
Full package green (ok … 117.291s). shfmt -i 2 -ci clean; shellcheck output byte-identical to base (3× pre-existing SC1091 on source lines).
Merge order: the fix is built on #2977's branch, so it lands after #2977 merges.
A refused node cordon claim can report no reason at all, in the one case where the reason matters most: another actor cordoned the node, took ownership, or replaced it while the roll was claiming it.
Evidence
Found while reviewing #2977 (its bounded CAS retry is what introduces the path). In
claim_node_cordon_ownership:The retry re-reads the Node to decide whether the rejected claim is still safe to retry, and points that read's stderr at the same
result_fileholding the conflict output. A successful read writes nothing, truncating the file — andemit_safe_operation_outputreturns early on an empty file:So the operator gets a bare
::error::Could not atomically claim and cordon Talos node X; refusing to drain it.with no cause attached.Affected path only. The exhausted-retry-budget break happens before the re-read, so it still reports the conflict; a failed re-read leaves its own stderr, which is meaningful. Only the "preconditions no longer hold" break — the concurrent-actor case — loses its cause.
Impact
Safety is unaffected: the claim still refuses and no drain proceeds. What is lost is the explanation, on a production node-drain refusal, at the moment a second actor is competing for the node. That is the least actionable moment to hand someone a reasonless error.
Why it was not caught
TestConcurrentCordonBeforeAtomicClaimStopsTheRollasserts the refusal line but never asserts a cause is emitted, so it passes with the output empty.Fix (implemented and proven)
Give the re-read its own stderr sink, and promote it to the reported cause only when the read itself fails. Plus the missing assertion.
RED/GREEN, one substitution (the mechanism moved, not the test deleted):
rc=0result_filerc=1—rollout_core_test.go:380: expected document to contain "cordon-claim: "rc=0Full package green (
ok … 117.291s).shfmt -i 2 -ciclean;shellcheckoutput byte-identical to base (3× pre-existing SC1091 onsourcelines).Merge order: the fix is built on #2977's branch, so it lands after #2977 merges.