From 16c5e78fa88c9e21842ef4780ca812e91a661f66 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 5 Oct 2026 18:17:52 +0200 Subject: [PATCH] Stop a rollout whose first machine is silent, and choose one that is heard from (hq issue 249, ADR 0218) A first machine that does not report within the bound now stops the module's rollout, naming it. The first machine is the first by name heard from lately; reports are judged by what the store says was last sent. A plan that ends says what it built and never sent, and a failed send's error is kept in the plan's note. --- cmd/mesh-controller/release_plan.go | 90 +++++++++++++++++----- cmd/mesh-controller/rollout_first_test.go | 93 +++++++++++++++++------ 2 files changed, 140 insertions(+), 43 deletions(-) diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 74455b1..8679101 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -391,6 +391,10 @@ func planBuilt(ctx context.Context, open *stores, module, commit, failed string, state.Why = failed p.State = inventory.PlanFailed p.Note = fmt.Sprintf("%s failed to build in tier %d", module, p.Tier) + sayUnsent(p, func(m string) bool { + u, err := inv.UpgradeOf(ctx, m) + return err == nil && u.RollOut + }) } else { state.State = "built" state.BuiltAt = &now @@ -452,8 +456,17 @@ func advanceHeld(ctx context.Context, open *stores) { moved, err := advanceOnce(ctx, open, p, edges, rollsOut) if err != nil { fmt.Printf("%s: %v\n", p.ID, err) + // Kept in the plan, so `plans` says why it has not moved rather than the log alone; + // the state is left as it was and the step is tried again on the next tick. + p.Note = "tier " + fmt.Sprint(p.Tier) + ": " + err.Error() + " — tried again" + if err := inv.SavePlan(ctx, *p); err != nil { + fmt.Printf("%s: cannot keep the plan: %v\n", p.ID, err) + } break } + if p.State == inventory.PlanFailed { + sayUnsent(p, rollsOut) + } if err := inv.SavePlan(ctx, *p); err != nil { fmt.Printf("%s: cannot keep the plan: %v\n", p.ID, err) break @@ -545,13 +558,14 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, return false, err } var reports []inventory.Reported - if state.FirstAt != nil && !policy.Together { + if !policy.Together { + // Read for the choice of the first machine as well as for its report. if reports, err = inv.LastReports(ctx); err != nil { return false, err } } - step := nextRollout(*state, running, policy.Together, reports) now := time.Now().UTC() + step := nextRollout(*state, running, policy.Together, reports, now, planWaitBound) switch { case step.failed != "": state.Why = step.failed @@ -561,11 +575,7 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, fmt.Printf("%s: %s\n", p.ID, p.Note) return true, nil case step.waiting != "": - wait := fmt.Sprintf("%s on %s, sent first at %s", m, step.waiting, state.FirstAt.Format("15:04")) - if now.Sub(*state.FirstAt) > planWaitBound { - wait += " — LATE" - } - pending = append(pending, wait) + pending = append(pending, fmt.Sprintf("%s on %s, sent first at %s", m, step.waiting, state.FirstAt.Format("15:04"))) continue case len(step.send) == 0: // No machine runs it: nothing to send, and nothing to wait for. @@ -665,22 +675,36 @@ type rolloutStep struct { // nextRollout is the next step of one module's rollout in a plan (novox/hq issue 249, ADR 0218). // -// Together, every machine running it at once, as the policy says. Otherwise one machine first — -// the first by name, so the choice is the same on every controller and every resume — and the rest -// once each machine the first send reached has reported, after that send, that it applied the -// declaration it was last sent. A report after the send that failed or refused it is the rollout's -// end: the rest are not sent. A machine that has not reported since is waited for; how long it has -// been is the plan's to say. -func nextRollout(s inventory.PlanModule, running []string, together bool, reports []inventory.Reported) rolloutStep { +// Together, every machine running it at once, as the policy says. Otherwise one machine first — the +// first by name among those that have reported within the bound, so a laptop that is away is not +// the one the rest wait on; the first by name when none has; the same choice on every controller and +// every resume — and the rest once each machine the first send reached reports, about the +// declaration it was last sent, that it applied it. Compared by the store's own record of what was +// sent (Reported.Current), never by this controller's clock against the machine's. +// +// **A first machine that fails, refuses, or does not report within the bound stops the rollout +// there** (ADR 0218 §2), naming the machine; the rest are not sent. A wait with no end is not a +// rollout: it held the plan open for ever, read as work in progress (issue 254). +func nextRollout(s inventory.PlanModule, running []string, together bool, reports []inventory.Reported, + now time.Time, bound time.Duration) rolloutStep { if len(running) == 0 { return rolloutStep{} } if together { return rolloutStep{send: running} } + byNode := map[string]inventory.Reported{} + for _, r := range reports { + byNode[r.Node] = r + } if s.FirstAt == nil { sorted := append([]string{}, running...) sort.Strings(sorted) + for _, n := range sorted { + if r, said := byNode[n]; said && r.At != nil && now.Sub(*r.At) <= bound { + return rolloutStep{send: []string{n}, first: true} + } + } return rolloutStep{send: sorted[:1], first: true} } sentFirst := map[string]bool{} @@ -693,15 +717,11 @@ func nextRollout(s inventory.PlanModule, running []string, together bool, report rest = append(rest, n) } } - byNode := map[string]inventory.Reported{} - for _, r := range reports { - byNode[r.Node] = r - } var waiting, failed []string for _, n := range s.First { r, said := byNode[n] - // Only a report after the send, about what it was last sent, says anything about this build. - if !said || r.At == nil || r.At.Before(*s.FirstAt) || !r.Current { + // Only a report about what it was last sent says anything about this build. + if !said || r.At == nil || !r.Current { waiting = append(waiting, n) continue } @@ -717,11 +737,34 @@ func nextRollout(s inventory.PlanModule, running []string, together bool, report return rolloutStep{failed: strings.Join(failed, "; "), rest: rest} } if len(waiting) > 0 { + if now.Sub(*s.FirstAt) > bound { + return rolloutStep{failed: fmt.Sprintf("%s did not report it applied within %s", + strings.Join(waiting, ", "), bound), rest: rest} + } return rolloutStep{waiting: strings.Join(waiting, ", ")} } return rolloutStep{send: rest} } +// sayUnsent adds to an ended plan's note the modules it built and never sent (novox/hq issue 249). +// An announced move of a module a plan held was left to that plan; a plan that ends without +// sending it — failed elsewhere, or closed by hand — would leave its machines behind with nothing +// saying so. A module whose rollout stopped at its first machine is not among them: that stop was +// the point. Said once. +func sayUnsent(p *inventory.Plan, rollsOut func(string) bool) { + var unsent []string + for name, s := range p.Modules { + if s != nil && s.State == "built" && s.SentAt == nil && s.FirstAt == nil && rollsOut(name) { + unsent = append(unsent, name) + } + } + if len(unsent) == 0 || strings.Contains(p.Note, "built and never sent") { + return + } + sort.Strings(unsent) + p.Note += "; built and never sent: " + strings.Join(unsent, ", ") + " — `push --behind` sends them" +} + // planTicker advances open plans on a timer, for the steps outcomes alone cannot take. func planTicker(ctx context.Context, open *stores) { advancePlans(ctx, open) @@ -899,6 +942,10 @@ func plansCommand(ctx context.Context, args []string) error { } p.State = inventory.PlanFailed p.Note = how + " by hand at tier " + fmt.Sprint(p.Tier) + sayUnsent(&p, func(m string) bool { + u, err := inv.UpgradeOf(ctx, m) + return err == nil && u.RollOut + }) if err := inv.SavePlan(ctx, p); err != nil { return err } @@ -978,9 +1025,10 @@ func planWhatIf(ctx context.Context, inv *inventory.Inventory, repository string how := "built; its policy records, so nothing is sent" if u, err := inv.UpgradeOf(ctx, name); err == nil && u.RollOut { running, _ := inv.Running(ctx, name) + reports, _ := inv.LastReports(ctx) how = "built, then sent to " + orNone(strings.Join(running, ", ")) // One machine first unless the policy says together (novox/hq issue 249). - if first := nextRollout(inventory.PlanModule{}, running, u.Together, nil); first.first && len(running) > 1 { + if first := nextRollout(inventory.PlanModule{}, running, u.Together, reports, time.Now(), planWaitBound); first.first && len(running) > 1 { how = fmt.Sprintf("built, then sent to %s first and to the rest once it has applied it", first.send[0]) } diff --git a/cmd/mesh-controller/rollout_first_test.go b/cmd/mesh-controller/rollout_first_test.go index cb8ce79..644f4be 100644 --- a/cmd/mesh-controller/rollout_first_test.go +++ b/cmd/mesh-controller/rollout_first_test.go @@ -14,47 +14,51 @@ import ( // once, as before. func TestAPlanSendsOneMachineFirstAndTheRestAfterItsReport(t *testing.T) { running := []string{"novox", "ace", "g14"} + sentAt := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) + now := sentAt.Add(5 * time.Minute) + bound := 30 * time.Minute + after := sentAt.Add(time.Minute) + next := func(s inventory.PlanModule, running []string, together bool, reports []inventory.Reported) rolloutStep { + return nextRollout(s, running, together, reports, now, bound) + } // Together: every machine at once. - if step := nextRollout(inventory.PlanModule{}, running, true, nil); !reflect.DeepEqual(step.send, running) || step.first { + if step := next(inventory.PlanModule{}, running, true, nil); !reflect.DeepEqual(step.send, running) || step.first { t.Fatalf("a together policy did not send every machine at once: %+v", step) } - // Otherwise the first by name, alone. - step := nextRollout(inventory.PlanModule{}, running, false, nil) + // Otherwise the first by name, alone, when none has reported lately. + step := next(inventory.PlanModule{}, running, false, nil) if !step.first || !reflect.DeepEqual(step.send, []string{"ace"}) { t.Fatalf("the first send was %+v, wanted ace alone", step) } - sentAt := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) - before, after := sentAt.Add(-time.Minute), sentAt.Add(time.Minute) state := inventory.PlanModule{First: []string{"ace"}, FirstAt: &sentAt} - report := func(at time.Time, outcome string, current bool) []inventory.Reported { - return []inventory.Reported{{Node: "ace", At: &at, Outcome: outcome, Current: current}, + report := func(outcome string, current bool) []inventory.Reported { + return []inventory.Reported{{Node: "ace", At: &after, Outcome: outcome, Current: current}, {Node: "g14", At: &after, Outcome: inventory.OutcomeApplied, Current: true}} } - // No report yet, a report from before the send, or one about an older declaration: wait. + // No report yet, or one about an older declaration than it was last sent: wait. for what, reports := range map[string][]inventory.Reported{ - "no report": nil, - "a report before the send": report(before, inventory.OutcomeApplied, true), - "a report about older": report(after, inventory.OutcomeApplied, false), + "no report": nil, + "a report about older": report(inventory.OutcomeApplied, false), } { - step := nextRollout(state, running, false, reports) + step := next(state, running, false, reports) if len(step.send) != 0 || step.waiting != "ace" || step.failed != "" { t.Errorf("%s: %+v, wanted to wait for ace", what, step) } } - // Applied after the send: the rest, and only the rest. - step = nextRollout(state, running, false, report(after, inventory.OutcomeApplied, true)) + // Applied what it was last sent: the rest, and only the rest. + step = next(state, running, false, report(inventory.OutcomeApplied, true)) if step.first || !reflect.DeepEqual(step.send, []string{"novox", "g14"}) { t.Fatalf("after ace applied it the plan sent %+v, wanted novox and g14", step) } - // Failed or refused after the send: stop, the rest untouched. + // Failed or refused: stop, the rest untouched. for _, outcome := range []string{inventory.OutcomeFailed, inventory.OutcomeRefused} { - step := nextRollout(state, running, false, report(after, outcome, true)) + step := next(state, running, false, report(outcome, true)) if len(step.send) != 0 || !strings.Contains(step.failed, "ace "+outcome) || !reflect.DeepEqual(step.rest, []string{"novox", "g14"}) { t.Errorf("a first machine that %s it: %+v", outcome, step) @@ -64,25 +68,50 @@ func TestAPlanSendsOneMachineFirstAndTheRestAfterItsReport(t *testing.T) { // The machine holding the bus went with the first send: the rest wait for it too, and it is // not sent again. both := inventory.PlanModule{First: []string{"novox", "ace"}, FirstAt: &sentAt} - half := report(after, inventory.OutcomeApplied, true) - if step := nextRollout(both, running, false, half); step.waiting != "novox" { + half := report(inventory.OutcomeApplied, true) + if step := next(both, running, false, half); step.waiting != "novox" { t.Fatalf("the plan did not wait for the bus's machine sent first: %+v", step) } all := append(half, inventory.Reported{Node: "novox", At: &after, Outcome: inventory.OutcomeApplied, Current: true}) - if step := nextRollout(both, running, false, all); !reflect.DeepEqual(step.send, []string{"g14"}) { + if step := next(both, running, false, all); !reflect.DeepEqual(step.send, []string{"g14"}) { t.Fatalf("after both applied it the plan sent %+v, wanted g14 alone", step) } // One machine, or none: nothing is waited for that cannot come. - if step := nextRollout(inventory.PlanModule{}, nil, false, nil); len(step.send) != 0 || step.first { + if step := next(inventory.PlanModule{}, nil, false, nil); len(step.send) != 0 || step.first { t.Fatalf("a module nothing runs was sent: %+v", step) } - only := inventory.PlanModule{First: []string{"ace"}, FirstAt: &sentAt} - if step := nextRollout(only, []string{"ace"}, false, report(after, inventory.OutcomeApplied, true)); len(step.send) != 0 || step.waiting != "" { + if step := next(state, []string{"ace"}, false, report(inventory.OutcomeApplied, true)); len(step.send) != 0 || step.waiting != "" { t.Fatalf("a module on one machine waited for more: %+v", step) } } +// ADR 0218 §2: a first machine that does not report within the bound stops the rollout there, +// naming the machine and the bound; the rest are left alone. +func TestAFirstMachineThatDoesNotReportStopsTheRollout(t *testing.T) { + sentAt := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) + state := inventory.PlanModule{First: []string{"ace"}, FirstAt: &sentAt} + step := nextRollout(state, []string{"ace", "g14"}, false, nil, sentAt.Add(31*time.Minute), 30*time.Minute) + if !strings.Contains(step.failed, "ace did not report it applied within 30m") || + !reflect.DeepEqual(step.rest, []string{"g14"}) || len(step.send) != 0 { + t.Fatalf("a silent first machine: %+v", step) + } +} + +// The first machine is the first by name among those heard from lately: a laptop that is away is +// not the one the rest wait on. When none has been heard from, the first by name. +func TestTheFirstMachineIsOneThatHasReportedLately(t *testing.T) { + now := time.Date(2026, 10, 5, 12, 0, 0, 0, time.UTC) + lately, long := now.Add(-time.Minute), now.Add(-3*time.Hour) + reports := []inventory.Reported{ + {Node: "ace", At: &long}, {Node: "g14", At: &lately}, {Node: "novox", At: &lately}, + } + step := nextRollout(inventory.PlanModule{}, []string{"novox", "ace", "g14"}, false, reports, now, 30*time.Minute) + if !step.first || !reflect.DeepEqual(step.send, []string{"g14"}) { + t.Fatalf("the first send was %+v, wanted g14, the first heard from lately", step) + } +} + // An announced move of a module an open plan is still rolling out is left to the plan: sending it // here as well put the bundle on every machine at once (novox/hq issue 249). func TestAnAnnouncedMoveIsLeftToThePlanRollingItOut(t *testing.T) { @@ -101,3 +130,23 @@ func TestAnAnnouncedMoveIsLeftToThePlanRollingItOut(t *testing.T) { } } } + +// A plan that ends without sending what it built says so, with the remedy; a module whose rollout +// stopped at its first machine is not among them. +func TestAnEndedPlanSaysWhatItBuiltAndNeverSent(t *testing.T) { + at := time.Now() + p := inventory.Plan{State: inventory.PlanFailed, Note: "closed by hand at tier 1", + Modules: map[string]*inventory.PlanModule{ + "agent": {State: "built"}, + "stopped": {State: "built", First: []string{"ace"}, FirstAt: &at}, + "sent": {State: "built", SentAt: &at}, + "notes": {State: "built"}, + "later": {}, + }} + rollsOut := func(m string) bool { return m != "notes" } + sayUnsent(&p, rollsOut) + sayUnsent(&p, rollsOut) + if p.Note != "closed by hand at tier 1; built and never sent: agent — `push --behind` sends them" { + t.Fatalf("the note reads %q", p.Note) + } +}