docs(placement): the drain half of that fix was never the broken half
drain-midmission passed 3/3 twice, but the "re-placing this phase" line the last commit added never appeared in the log. It cannot: `online_for_backend` filters on `status = 'online'`, so a draining node is not a candidate, never reaches `unfit`, and the pin simply falls through to ranking — on the old code as well as the new. So the scenario passes either way and proves the affinity decision, not the `TargetUnfit` bug. The path that genuinely used to fail a phase is "the previous phase's node has since FILLED UP": that puts it in `unfit`, which returned a non-transient error, which never reached the queue. That is what the unit test now says, in place of a claim about draining the harness does not support. The accidental mission-to-node affinity was real and unconditional either way. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
13a35138e9
commit
42108c840d
@@ -446,9 +446,13 @@ pub async fn choose(
|
|||||||
// which is simply where the PREVIOUS phase ran — not an operator's choice.
|
// which is simply where the PREVIOUS phase ran — not an operator's choice.
|
||||||
// Treating it as a requirement had two consequences, both wrong:
|
// Treating it as a requirement had two consequences, both wrong:
|
||||||
//
|
//
|
||||||
// - a drained or full previous node failed the phase outright, because
|
// - a previous node that had since filled up (or gone unreadable) failed
|
||||||
// the resulting error was not transient and so never reached the queue.
|
// the phase outright: `TargetUnfit` is not transient, so it never
|
||||||
// `drain-midmission` was written to exercise exactly that path.
|
// reached the queue. Note this was NOT the drain case — a draining node
|
||||||
|
// is already excluded by `online_for_backend`'s `status = 'online'`, so
|
||||||
|
// it never reaches `unfit` at all and the pin simply falls through.
|
||||||
|
// `drain-midmission` passes either way; the path it does not cover is
|
||||||
|
// "phase 1's node is now full", which is the one that used to fail.
|
||||||
// - and while the node stayed fit, every later phase went back to it
|
// - and while the node stayed fit, every later phase went back to it
|
||||||
// regardless of ranking — accidental mission-to-node affinity, which
|
// regardless of ranking — accidental mission-to-node affinity, which
|
||||||
// this module's own header says must not exist.
|
// this module's own header says must not exist.
|
||||||
@@ -508,13 +512,19 @@ mod tests {
|
|||||||
/// holding roughly 1 GiB of its 8 GiB, observed usage is ~6314 MiB;
|
/// holding roughly 1 GiB of its 8 GiB, observed usage is ~6314 MiB;
|
||||||
/// inferring the baseline as 6314 - 16384 goes negative, clamps to the
|
/// inferring the baseline as 6314 - 16384 goes negative, clamps to the
|
||||||
/// 2048 floor, and invents 2266 MiB — exactly one more VM than exists.
|
/// 2048 floor, and invents 2266 MiB — exactly one more VM than exists.
|
||||||
/// A drained previous node re-places the next phase; it does not fail it.
|
/// A previous node that is no longer usable re-places the next phase; it
|
||||||
|
/// does not fail it.
|
||||||
///
|
///
|
||||||
/// `drain-midmission` found this: `choose` treated `missions.target_node_id`
|
/// `choose` treated `missions.target_node_id` — which is only ever "where
|
||||||
/// — which is only ever "where the last phase ran" — as a hard requirement,
|
/// the last phase ran" — as a hard requirement, so a pinned node that had
|
||||||
/// so a node an operator cordoned mid-mission produced a non-transient
|
/// since FILLED UP produced `TargetUnfit`, which is not transient, and the
|
||||||
/// error that failed the phase instead of moving it. The same path also
|
/// phase failed instead of queueing or moving. It also gave every later
|
||||||
/// gave every later phase silent affinity back to the first node.
|
/// phase silent affinity back to the first node.
|
||||||
|
///
|
||||||
|
/// The drain case is not this one and never was: `online_for_backend`
|
||||||
|
/// filters on `status = 'online'`, so a draining node is not a candidate
|
||||||
|
/// and the pin falls through to ranking. `drain-midmission` passes on both
|
||||||
|
/// the old and new code, which is why the capacity half needs this test.
|
||||||
#[test]
|
#[test]
|
||||||
fn an_unfit_previous_node_is_re_placed_not_refused() {
|
fn an_unfit_previous_node_is_re_placed_not_refused() {
|
||||||
let drained = uuid::Uuid::from_u128(1);
|
let drained = uuid::Uuid::from_u128(1);
|
||||||
|
|||||||
Reference in New Issue
Block a user