diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 8c3723e7..c240d478 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -977,8 +977,12 @@ func failFirstSend(ctx context.Context, open *stores, p *inventory.Plan, module }() g := state.Gate if g != nil && g.Verdict == inventory.GatePassed { - // **A build that passed its gate is never put back for what came after** (novox/hq issue 335): its - // verdict stands; the plan stops, and says why, but the build is not marked failed. + // **A guard: a build that passed its gate is never put back for what came after** (novox/hq issue 335). + // Not reached while nextRollout answers a passed gate with the rest to send, and advanceOnce reads it + // after a carried module takes over its lead's pass; it is here so that a path added later cannot + // overturn a verdict. If it is reached, the plan stops and says why, and the build is not marked failed. + // The module is left a stopped rollout (its Why said, sent first and not to the rest), which + // `plans retry` takes: it sends the first machine again, and the passed gate then sends the rest. state.Why = "passed its gate; its walk then stopped: " + why p.State = inventory.PlanFailed p.Note = fmt.Sprintf("%s passed its gate on %s in tier %d (%s) and is kept; its walk stopped after: %s", diff --git a/cmd/mesh-controller/replay335_test.go b/cmd/mesh-controller/replay335_test.go index 925a31ea..1b0c4d33 100644 --- a/cmd/mesh-controller/replay335_test.go +++ b/cmd/mesh-controller/replay335_test.go @@ -69,3 +69,64 @@ func testAWalkStoppedAfterItsGatePassedKeepsThePass(t *testing.T) { t.Fatalf("the note reads %q", p.Note) } } + +// novox/hq issue 335 review: **a module carried by its lead's send takes over the lead's pass before its own +// step is read.** The state is the one a step leaves when the lead's gate passed and the step ended before the +// carried module's turn (an error read before it, kept with the plan): the lead passed, the carried module has +// no gate of its own yet. Since then another send reached the first machine and it has not reported on it, and +// the first send is older than the wait for a first machine's report. Read before the pass is taken over, the +// carried module's step said the first machine never applied it, and the passed build was put back. +func TestACarriedModuleTakesItsLeadsPassBeforeItsStepIsRead(t *testing.T) { + tm := aTierMesh(t, "m01", "m02") + ctx := t.Context() + inv := tm.open.inventory + + advancePlans(ctx, tm.open) + if !reflect.DeepEqual(tm.sent, [][]string{{"anchor"}}) { + t.Fatalf("sent %v: the first machine once, for the tier", tm.sent) + } + p := tm.plan(t) + lead, carried := p.Modules["m01"], p.Modules["m02"] + if lead.Gate == nil || carried.GatedBy != "m01" { + t.Fatalf("m02 is not carried by m01's send: lead %+v, carried %+v", lead.Gate, carried) + } + // The lead passed; the carried module's turn did not come. The first send is past the wait's bound. + sent := time.Now().UTC().Add(-planWaitBound - time.Minute) + judged := sent.Add(2 * time.Minute) + lead.FirstAt, carried.FirstAt, lead.Gate.Since = &sent, &sent, &sent + lead.Gate.Verdict, lead.Gate.Why, lead.Gate.JudgedAt, lead.Gate.Kept = inventory.GatePassed, + "healthy 3 times over 2m5s", &judged, true + carried.Gate = nil + if err := inv.SavePlan(ctx, &p); err != nil { + t.Fatal(err) + } + // Another walk's send reached anchor, which has not reported on it. + if err := inv.RecordSent(ctx, nodeID(t, tm.open, "anchor"), "d-anchor-elsewhere", + map[string]string{"m01": "c2", "m02": "c2"}); err != nil { + t.Fatal(err) + } + + advancePlans(ctx, tm.open) + p = tm.plan(t) + if p.State == inventory.PlanFailed { + t.Fatalf("the plan failed after its gate passed: %s", p.Note) + } + if !reflect.DeepEqual(tm.sent, [][]string{{"anchor"}, {"laptop"}}) { + t.Fatalf("sent %v: the rest once, as the pass said", tm.sent) + } + current, err := inv.CurrentBuilds(ctx) + if err != nil { + t.Fatal(err) + } + for _, m := range []string{"m01", "m02"} { + if current[m].Commit != "c2" { + t.Errorf("%s was put back to %s after its gate passed", m, current[m].Commit) + } + if failed, _ := inv.GateFailed(ctx, "build-"+m+"-2"); failed { + t.Errorf("%s's build was marked failed at its gate after the gate passed", m) + } + } + if g := p.Modules["m02"].Gate; g == nil || g.Verdict != inventory.GatePassed { + t.Errorf("m02 did not take over m01's pass: %+v", g) + } +}