From 363898ec8a4414c62e09f6c288a9093e66c70dbd Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 14:51:53 +0200 Subject: [PATCH 1/3] Give every module of a failed send a verdict, and excuse only the wait a build's own send brought (hq issue 318 review) --- cmd/mesh-controller/gate.go | 111 ++++++--- cmd/mesh-controller/gate_followup_test.go | 279 ++++++++++++++++++++++ cmd/mesh-controller/module_health.go | 151 +++++++++--- cmd/mesh-controller/person_wait_test.go | 78 +++++- cmd/mesh-controller/plain_words.go | 7 +- cmd/mesh-controller/release.go | 16 +- cmd/mesh-controller/release_plan.go | 18 +- cmd/mesh-controller/tier_send_test.go | 9 +- internal/conditions/store.go | 10 + internal/inventory/plans.go | 7 + 10 files changed, 599 insertions(+), 87 deletions(-) create mode 100644 cmd/mesh-controller/gate_followup_test.go diff --git a/cmd/mesh-controller/gate.go b/cmd/mesh-controller/gate.go index e821b716..714dce2d 100644 --- a/cmd/mesh-controller/gate.go +++ b/cmd/mesh-controller/gate.go @@ -442,11 +442,12 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs words[node] = aboutTheMachine(node, moved, *g.Since, facts) } // **Each module is judged on its own** (novox/hq ADR 0254, issue 318): its reading is the worst of its - // machines', its passes are counted apart, and the send's verdict is still one — but a module that was - // healthy on its own for the passes the gate asks keeps a pass when another beside it fails. + // machines', its passes are counted apart, and the send's verdict is still one. **Every module of a send + // leaves it with a verdict** (issue 318 review): one healthy for the passes the gate asks, after the + // settle time, keeps a pass when another beside it fails; one found broken holds the send's verdict until + // the modules beside it have theirs, or the bound; and at the verdict, every module that did not pass is + // put back with what failed, so none is left on the machine unjudged for other walks to wait on. worst, why := healthGood, "" - var failing []string - broken := map[string]bool{} reading := map[string]health{} waits := map[string]string{} var modules []string @@ -465,18 +466,21 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs if _, seen := reading[j.module]; !seen { modules = append(modules, j.module) } + if h == healthBroken && !slices.Contains(g.Broken, j.module) { + g.Broken = append(g.Broken, j.module) + if g.BrokenWhy == "" { + g.BrokenWhy = said + } + } + if slices.Contains(g.Broken, j.module) { + h = healthBroken // found broken once, broken for the rest of the judging + } if h > reading[j.module] { reading[j.module] = h } if h == healthPerson { waits[j.module] = joinSaid(waits[j.module], said) } - if h > healthPerson && !slices.Contains(failing, j.module) { - failing = append(failing, j.module) - } - if h == healthBroken { - broken[j.module] = true - } if h > worst { worst, why = h, said } else if h == worst && h > healthPerson && why == "" { @@ -486,54 +490,77 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs if g.Healthy == nil { g.Healthy = map[string]int{} } + if g.HealthyAt == nil { + g.HealthyAt = map[string]time.Time{} + } g.Waits = nil + var failing []string for _, m := range modules { - switch { - case reading[m] > healthPerson: + if reading[m] > healthPerson { + failing = append(failing, m) g.Healthy[m] = 0 - default: + delete(g.HealthyAt, m) + continue + } + // **A pass is counted only gateEvery after the one before** (issue 318 review): a send judged more + // often while another module beside it is not yet healthy counts no faster. + if at, counted := g.HealthyAt[m]; !counted || now.Sub(at) >= gateEvery { g.Healthy[m]++ - if w := waits[m]; w != "" { - if g.Waits == nil { - g.Waits = map[string]string{} - } - g.Waits[m] = w + g.HealthyAt[m] = now + } + if w := waits[m]; w != "" { + if g.Waits == nil { + g.Waits = map[string]string{} } + g.Waits[m] = w } } - // passedAlone is every module that passed on its own while the send as a whole did not. - passedAlone := func() []string { - var out []string - if now.Sub(*g.Since) < gateSettle { - return nil - } + settled := now.Sub(*g.Since) >= gateSettle + passedAlone := func(m string) bool { + return settled && reading[m] <= healthPerson && g.Healthy[m] >= gatePasses + } + // fail decides the send failed: what passed on its own keeps its pass, everything else is put back. + fail := func(why string) { + g.Passing, g.Failing = nil, nil for _, m := range modules { - if reading[m] <= healthPerson && g.Healthy[m] >= gatePasses { - out = append(out, m) + if passedAlone(m) { + g.Passing = append(g.Passing, m) + } else { + g.Failing = append(g.Failing, m) } } - return out + decide(g, inventory.GateFailed, why, now) } + pastBound := now.Sub(*g.Since) > gateBound switch { case worst == healthBroken: - // What broke is put back; what was only not yet healthy beside it is too — they moved together. - g.Failing, g.Passing = failing, passedAlone() - decide(g, inventory.GateFailed, why, now) + var judging []string + for _, m := range modules { + if reading[m] != healthBroken && !passedAlone(m) { + judging = append(judging, m) + } + } + if len(judging) == 0 || pastBound { + fail(g.BrokenWhy) + break + } + // What broke fails the send; what is beside it is judged to its own verdict first, within the bound. + g.Passes, g.LastPass, g.Failing = 0, nil, failing + g.Last = fmt.Sprintf("%s; %s judged to its own verdict before the send's", g.BrokenWhy, strings.Join(judging, ", ")) case worst == healthWaiting: // Waiting on a provider that is unhealthy: not a pass, and not a failure at the bound either — // the provider's own condition says what is wrong (ADR 0240 rule 5). g.Passes, g.LastPass, g.Last, g.Failing = 0, nil, why, failing case worst == healthNotYet: g.Passes, g.LastPass, g.Last, g.Failing = 0, nil, why, failing - if now.Sub(*g.Since) > gateBound { - g.Passing = passedAlone() - decide(g, inventory.GateFailed, fmt.Sprintf("not healthy within %s of its apply: %s", gateBound, why), now) + if pastBound { + fail(fmt.Sprintf("not healthy within %s of its apply: %s", gateBound, why)) } default: // Healthy, or waiting for a person (ADR 0254): a pass, the wait carried along in the verdict. g.Passes++ g.LastPass, g.Last, g.Failing = &now, "", nil - if g.Passes >= gatePasses && now.Sub(*g.Since) >= gateSettle { + if g.Passes >= gatePasses && settled { decide(g, inventory.GatePassed, fmt.Sprintf("healthy %d times over %s", g.Passes, now.Sub(*g.Since).Round(time.Second))+waitsSaid(g.Waits), now) } @@ -541,6 +568,18 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs return g.Verdict, nil } +// whyFor is a passing gate's why as one module's verdict says it: the send's, and that module's own wait +// for a person, never another's (issue 318 review). +func whyFor(g *inventory.PlanGate, module string) string { + why, _, _ := strings.Cut(g.Why, waitsPrefix) + if w := g.Waits[module]; w != "" { + why += waitsPrefix + w + } + return why +} + +const waitsPrefix = "; and it waits for a person: " + // waitsSaid is the waits for a person a passing gate carries, as its verdict says them. func waitsSaid(waits map[string]string) string { if len(waits) == 0 { @@ -555,7 +594,7 @@ func waitsSaid(waits map[string]string) string { for _, m := range modules { said = append(said, waits[m]) } - return "; and it waits for a person: " + strings.Join(said, "; ") + return waitsPrefix + strings.Join(said, "; ") } func joinSaid(a, b string) string { @@ -579,7 +618,7 @@ func gatePassed(ctx context.Context, open *stores, p *inventory.Plan, module str g := state.Gate err := open.inventory.RecordGate(ctx, inventory.GateVerdict{Build: state.Build, Module: module, Commit: state.Commit, Previous: state.Previous, Plan: p.ID, Machines: g.Machines, - Verdict: inventory.GatePassed, Why: g.Why, Component: g.Component, JudgingFrom: g.Since}) + Verdict: inventory.GatePassed, Why: whyFor(g, module), Component: g.Component, JudgingFrom: g.Since}) if err != nil && state.Build != "" { fmt.Printf("%s: %s passed its gate, and the verdict could not be kept: %v\n", p.ID, module, err) } diff --git a/cmd/mesh-controller/gate_followup_test.go b/cmd/mesh-controller/gate_followup_test.go new file mode 100644 index 00000000..3229da1f --- /dev/null +++ b/cmd/mesh-controller/gate_followup_test.go @@ -0,0 +1,279 @@ +package main + +import ( + "context" + "encoding/json" + "reflect" + "strings" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/conditions" + "github.com/novox/mesh-controller/internal/inventory" + "github.com/novox/mesh-controller/internal/lease" + "github.com/novox/mesh-controller/internal/link" +) + +// What the review of issue 318's fix found (novox/hq ADR 0254): every module of a send leaves it with a +// verdict, a pass is counted no faster than the gate's spacing, a carried build's verdict carries only its own +// wait, and a provider waiting for a login says who waits on it. + +// withGateBounds sets the gate's bounds for one test. +func withGateBounds(t *testing.T, settle, every, bound time.Duration) { + t.Helper() + wasSettle, wasEvery, wasBound := gateSettle, gateEvery, gateBound + gateSettle, gateEvery, gateBound = settle, every, bound + t.Cleanup(func() { gateSettle, gateEvery, gateBound = wasSettle, wasEvery, wasBound }) +} + +// factsOn is a judging's facts for one machine that applied its send: the node tools answer, and what it says +// of its modules' resources. +func factsOn(t *testing.T, machine string, since time.Time, rs ...inventory.ResourceHealth) func() gateFacts { + return func() gateFacts { + now := time.Now() + at := now + return gateFacts{now: now, + reports: map[string]inventory.Reported{machine: {Node: machine, Outcome: inventory.OutcomeApplied, At: &at, Current: true}}, + engines: map[string]string{}, + rolledBack: map[string][]lease.Rollback{}, + served: map[string]served{machine: {runtime: true, tools: map[string]bool{}}}, + health: map[string]inventory.NodeHealth{machine: {Node: machine, HeardAt: now, Resources: rs}}, + } + } +} + +func healthyContainer(module string) inventory.ResourceHealth { + return inventory.ResourceHealth{Module: module, Resource: module + ".web", Kind: "container", Target: module, + State: link.StateHealthy} +} + +// **A fault decided before the settle time does not strand the module beside it** (review (a)): the node +// tools' witness put its build back at once, and the send waits for the healthy module's own verdict — a pass, +// counted over the gate's spacing — before it says its own. The broken one alone is put back. +func TestAFaultBeforeTheSettleTimeWaitsForTheModuleBesideIt(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + withGateBounds(t, 150*time.Millisecond, 20*time.Millisecond, 10*time.Second) + since := time.Now().Add(-time.Second) + base := factsOn(t, "laptop", since, healthyContainer("app")) + wasGather := gatherGateFacts + t.Cleanup(func() { gatherGateFacts = wasGather }) + gatherGateFacts = func(context.Context, *stores, string) (gateFacts, error) { + f := base() + f.rolledBack["laptop"] = []lease.Rollback{{Component: lease.ComponentNodeTools, Outcome: lease.OutcomeRolledBack, + At: since.Add(100 * time.Millisecond), Why: "the node tools did not answer"}} + return f, nil + } + g := &inventory.PlanGate{Machines: []string{"laptop"}, Since: &since} + pairs := []judged{{module: broker_runtime, node: "laptop"}, {module: "app", node: "laptop"}} + judgings := 0 + for deadline := time.Now().Add(5 * time.Second); g.Verdict == "" && time.Now().Before(deadline); { + if _, err := judgeMoves(ctx, open, g, pairs, time.Now()); err != nil { + t.Fatal(err) + } + judgings++ + time.Sleep(30 * time.Millisecond) + } + if g.Verdict != inventory.GateFailed || judgings < gatePasses { + t.Fatalf("verdict %q after %d judging(s): %+v; want failed, after the module beside it was judged", g.Verdict, + judgings, g) + } + if !reflect.DeepEqual(g.Passing, []string{"app"}) || !reflect.DeepEqual(g.Failing, []string{broker_runtime}) { + t.Fatalf("passing %v, failing %v; want app kept and the node tools put back", g.Passing, g.Failing) + } + if !strings.Contains(g.Why, "witness") { + t.Errorf("the verdict does not say what broke: %q", g.Why) + } +} + +// broker_runtime is the node tools' module name, a core component a witness judges. +const broker_runtime = "node-tools" + +// **A pass is counted only the gate's spacing after the one before** (review (c)): a send judged every tick +// while a module beside it is not yet healthy counts the healthy one once. +func TestAPassIsCountedNoFasterThanTheGatesSpacing(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + withGateBounds(t, 0, time.Hour, time.Hour) + since := time.Now().Add(-time.Minute) + base := factsOn(t, "laptop", since, healthyContainer("app"), inventory.ResourceHealth{Module: "late", + Resource: "late.web", Kind: "container", Target: "late", State: link.StateUnhealthy, Reason: "down"}) + wasGather := gatherGateFacts + t.Cleanup(func() { gatherGateFacts = wasGather }) + gatherGateFacts = func(context.Context, *stores, string) (gateFacts, error) { return base(), nil } + g := &inventory.PlanGate{Machines: []string{"laptop"}, Since: &since} + pairs := []judged{{module: "app", node: "laptop"}, {module: "late", node: "laptop"}} + now := time.Now() + for i := 0; i < 3; i++ { + if _, err := judgeMoves(ctx, open, g, pairs, now.Add(time.Duration(i)*time.Second)); err != nil { + t.Fatal(err) + } + } + if g.Healthy["app"] != 1 || g.Healthy["late"] != 0 { + t.Fatalf("counted %v in three judgings within a second; want app once and late never", g.Healthy) + } +} + +// **A carried build's pass carries its own wait, never another's** (review (e)). +func TestACarriedPassCarriesOnlyItsOwnWait(t *testing.T) { + b := aBacklog(t) + ctx := t.Context() + inv := b.open.inventory + since := time.Now().Add(-time.Minute) + g := &inventory.PlanGate{Machines: []string{"laptop"}, Since: &since, Verdict: inventory.GatePassed, + Why: "healthy 3 times over 2m0s" + waitsSaid(map[string]string{"late": "relogin needed on laptop: late waits"}), + Waits: map[string]string{"late": "relogin needed on laptop: late waits"}, + Carried: []inventory.CarriedMove{{Module: "app", Node: "laptop", From: "c1", To: "c2", Build: "build-app-c2"}, + {Module: "late", Node: "laptop", From: "c1", To: "c2", Build: "build-late-c2"}}} + passCarried(ctx, b.open, &inventory.Plan{ID: "release-test"}, g, "") + app, _, _ := inv.GateOf(ctx, "build-app-c2") + late, _, _ := inv.GateOf(ctx, "build-late-c2") + if strings.Contains(app.Why, "waits for a person") || app.Verdict != inventory.GatePassed { + t.Errorf("app's pass: %+v; want it without late's wait", app) + } + if !strings.Contains(late.Why, "relogin needed on laptop") { + t.Errorf("late's pass: %+v; want its own wait", late) + } +} + +// **The tier path keeps the pass of the module the gate is kept on** (review (b)): a plan's first send +// carried its own module, healthy, and a build waiting on that machine that never became healthy. At the +// bound the waiting one is put back and marked; the plan's own module keeps its pass, so no walk waits on it. +func TestThePlansOwnModuleKeepsItsPassWhenItsSendFails(t *testing.T) { + g := aGateMesh(t) + ctx := t.Context() + inv := g.open.inventory + // `late` waits on anchor for a gate: c1 sent, c2 registered and built. + for _, b := range []inventory.Build{ + {ID: "build-late-1", Module: "late", Commit: "l1", Repository: "novox/mesh-catalog", Path: "modules/late", + Asked: time.Now().Add(-2 * time.Hour), At: time.Now().Add(-2 * time.Hour)}, + {ID: "build-late-2", Module: "late", Commit: "l2", Repository: "novox/mesh-catalog", Path: "modules/late", + Asked: time.Now().Add(-time.Minute), At: time.Now().Add(-time.Minute)}, + } { + manifest, _ := json.Marshal(catalogue.Manifest{Module: "late", Version: b.Commit}) + b.Manifest = manifest + b.Made = []inventory.Artifact{{Name: "x", Kind: "bundle", Reference: "sha256:" + b.Commit}} + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "late", Version: b.Commit}, inventory.Source{ + Repository: "novox/mesh-catalog", Seat: "git", Path: "modules/late", BuiltFrom: b.Commit, Head: b.Commit, + Asked: b.Asked}); err != nil { + t.Fatal(err) + } + if b.Commit == "l1" { + if _, err := inv.Assign(ctx, "anchor", "late"); err != nil { + t.Fatal(err) + } + if err := inv.RecordSent(ctx, nodeID(t, g.open, "anchor"), "d-anchor-late", map[string]string{"app": "c1", + "late": "l1"}); err != nil { + t.Fatal(err) + } + } + } + wasMoves := machineMoves + t.Cleanup(func() { machineMoves = wasMoves }) + machineMoves = func(ctx context.Context, open *stores, f moveFacts, node string, all bool) ([]inventory.CarriedMove, error) { + modules, err := open.inventory.Assigned(ctx, node) + if err != nil { + return nil, err + } + sent, known, err := open.inventory.SentBuilds(ctx, node) + if err != nil { + return nil, err + } + return f.moves(node, modules, sent, known, all), nil + } + gather := gatherGateFacts + gatherGateFacts = func(ctx context.Context, open *stores, component string) (gateFacts, error) { + f, err := gather(ctx, open, component) + f.health = map[string]inventory.NodeHealth{"anchor": {Node: "anchor", HeardAt: time.Now(), Resources: []inventory.ResourceHealth{ + healthyContainer("app"), {Module: "late", Resource: "late.web", Kind: "container", Target: "late", + State: link.StateUnhealthy, Reason: "down"}}}} + return f, err + } + gateEvery, gateBound = 0, 400*time.Millisecond + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); { + advancePlans(ctx, g.open) + if p := g.plan(t); p.State == inventory.PlanFailed || p.State == inventory.PlanDone { + break + } + time.Sleep(30 * time.Millisecond) + } + p := g.plan(t) + if p.State != inventory.PlanFailed { + t.Fatalf("the plan is %s: %s; want it failed on late", p.State, p.Note) + } + if v, found, err := inv.GateOf(ctx, "build-2"); err != nil || !found || v.Verdict != inventory.GatePassed || + !strings.Contains(v.Why, "on its own") { + t.Fatalf("app's verdict: %+v (found %v, %v); want its own pass kept", v, found, err) + } + if failed, _ := inv.GateFailed(ctx, "build-late-2"); !failed { + t.Fatal("late's build is not marked failed at its gate") + } + if current, _ := inv.CurrentBuilds(ctx); current["app"].Commit != "c2" || current["late"].Commit != "l1" { + t.Fatalf("registered app %s, late %s; want app kept at c2 and late put back to l1", current["app"].Commit, + current["late"].Commit) + } +} + +// **A provider waiting for a login says who waits on it** (review (g)): its consumer, failing a check that +// needs it, is held under it, and the provider's relogin-needed condition lists it and is urgent. +func TestAProviderWaitingForALoginSaysWhoWaitsOnIt(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + inv := open.inventory + k := conditionsFrom + register(t, open, catalogue.Manifest{Module: "db", Version: "1", + Provides: []catalogue.Offer{{Name: "postgres-database", Scope: catalogue.ScopeMesh}}}) + register(t, open, catalogue.Manifest{Module: "shop", Version: "1", Requires: []string{"postgres-database"}}) + for _, a := range [][2]string{{"anchor", "db"}, {"laptop", "shop"}} { + if _, err := inv.Assign(ctx, a[0], a[1]); err != nil { + t.Fatal(err) + } + } + if err := inv.RecordBindings(ctx, "laptop", []inventory.Binding{{Machine: "laptop", Consumer: "shop", + Provision: "postgres-database", Provider: catalogue.Chosen{Node: "anchor", Module: "db"}}}); err != nil { + t.Fatal(err) + } + at := h0 + say := func(machine string, rs ...link.ResourceHealth) { + t.Helper() + at = at.Add(time.Second) + for i := range rs { + rs[i].Since = at + } + if err := stateHealth(ctx, inv, k, machine, link.Health{Contract: link.ReadinessContract, At: at, Resources: rs}, at); err != nil { + t.Fatal(err) + } + } + account := link.ResourceHealth{Module: "db", Resource: "db.operator", Kind: link.KindAccount, Target: "operator", + Account: "operator", State: link.StateUnhealthy, Reason: link.ReasonRelogin + ": operator is in the group db"} + unit := link.ResourceHealth{Module: "db", Resource: "db.server", Kind: link.KindUnit, Target: "db.service", + Account: "operator", State: link.StateUnhealthy, Reason: "failed in the account's own service manager (exit-code)"} + shop := link.ResourceHealth{Module: "shop", Resource: "shop.web", Kind: "container", Target: "shop", + State: link.StateUnhealthy, Reason: "http /health on web: answered 500", Check: "http", Needs: "postgres-database"} + for look := 0; look < 2; look++ { + say("anchor", account, unit) + say("laptop", shop) + } + list, err := k.Open(ctx) + if err != nil { + t.Fatal(err) + } + var keys []string + var c conditions.Condition + for _, x := range list { + keys = append(keys, x.Key) + if x.Key == "module.db.anchor.relogin-needed" { + c = x + } + } + if !reflect.DeepEqual(keys, []string{"module.db.anchor.relogin-needed"}) { + t.Fatalf("open %v; want the provider's wait alone, its consumer held under it", keys) + } + if c.Severity != conditions.Urgent || !strings.Contains(c.Evidence[0].Said, "shop on laptop") { + t.Fatalf("the provider's wait: %s, %q; want it urgent and naming its consumer", c.Severity, c.Evidence[0].Said) + } +} diff --git a/cmd/mesh-controller/module_health.go b/cmd/mesh-controller/module_health.go index 7596f4a6..b40c6818 100644 --- a/cmd/mesh-controller/module_health.go +++ b/cmd/mesh-controller/module_health.go @@ -152,14 +152,23 @@ func judgeModuleHealth(ctx context.Context, inv *inventory.Inventory, k *conditi } sort.Strings(modules) seen := map[string]bool{} + // became is, per module, the kind of condition this statement says of it: what a standing one of the + // other kind turned into. + became := map[string]string{} heldOn := map[string]string{} providers := map[catalogue.Chosen]bool{} for _, m := range modules { // **A wait for a person's new login is said as that** (novox/hq ADR 0254): one plain sentence to the // operator, never urgent, cleared on the first statement that no longer says it. if said, waits := personWait(m, node, unhealthy[m]); waits { - o := reloginObservation(m, node, said, unhealthy[m]) + o := reloginObservation(m, node, said, operatorOn(ctx, inv, node), unhealthy[m]) + // **A provider waiting for a login says who waits on it** (novox/hq issue 318 review): its + // consumers are held under it, and its own condition is where they are said. + if hold != nil { + sayWaitingOn(&o, hold.waitersOn(catalogue.Chosen{Node: node, Module: m})) + } seen[o.Key()] = true + became[m] = kindReloginNeeded if _, isOpen := standing[o.Key()]; streaks[m] < moduleUnhealthyAfter && !isOpen { continue } @@ -176,14 +185,10 @@ func judgeModuleHealth(ctx context.Context, inv *inventory.Inventory, k *conditi providers[p] = true continue } - if waiters := hold.waitersOn(catalogue.Chosen{Node: node, Module: m}); len(waiters) > 0 { - o.Severity = conditions.Urgent - o.Said += "; " + waitingWords(waiters) - o.Summary += fmt.Sprintf("; %d consumer(s) wait on it", len(waiters)) - o.Explanation += fmt.Sprintf(" %d module(s) that depend on it wait for it.", len(waiters)) - } + sayWaitingOn(&o, hold.waitersOn(catalogue.Chosen{Node: node, Module: m})) } seen[o.Key()] = true + became[m] = kindModuleUnhealthy c, isOpen := standing[o.Key()] if streaks[m] < moduleUnhealthyAfter && !isOpen { continue // unconfirmed: one statement can be wrong; `node show` lists it @@ -207,7 +212,18 @@ func judgeModuleHealth(ctx context.Context, inv *inventory.Inventory, k *conditi if on, held := heldOn[key]; held { why = fmt.Sprintf("what %s finds on %s waits on %s, which is unhealthy: held under its condition", module, node, on) } - if _, err := k.Clear(ctx, key, why); err != nil { + // **A condition that became the other kind** is not "working again" (issue 318 review): its clearing + // line says what it became. + resolved := "" + switch { + case c.Kind == kindModuleUnhealthy && became[module] == kindReloginNeeded: + why = fmt.Sprintf("%s on %s now waits only for a new login", module, node) + resolved = fmt.Sprintf("%s on %s now waits only for a new login", module, node) + case c.Kind == kindReloginNeeded && became[module] == kindModuleUnhealthy: + why = fmt.Sprintf("%s on %s no longer waits for a new login, and is not healthy", module, node) + resolved = fmt.Sprintf("The new login on %s is done, and %s still does not work", node, module) + } + if _, err := k.ClearSaying(ctx, key, why, resolved); err != nil { problems = append(problems, err.Error()) } } @@ -229,7 +245,7 @@ func judgeModuleHealth(ctx context.Context, inv *inventory.Inventory, k *conditi func sayWaiters(ctx context.Context, k *conditions.Keeper, hold *holding, p catalogue.Chosen, now time.Time) error { var raisedAt *conditions.Condition for i, c := range hold.open { - if c.Key == moduleUnhealthyKey(p.Module, p.Node) { + if c.Key == moduleUnhealthyKey(p.Module, p.Node) || c.Key == reloginKey(p.Module, p.Node) { raisedAt = &hold.open[i] } } @@ -246,35 +262,97 @@ func sayWaiters(ctx context.Context, k *conditions.Keeper, hold *holding, p cata return nil } o := moduleUnhealthyObservation(p.Module, p.Node, rs) - if waiters := hold.waitersOn(p); len(waiters) > 0 { - o.Severity = conditions.Urgent - o.Said += "; " + waitingWords(waiters) - o.Summary += fmt.Sprintf("; %d consumer(s) wait on it", len(waiters)) + if said, waits := personWait(p.Module, p.Node, rs); waits { + o = reloginObservation(p.Module, p.Node, said, operatorOn(ctx, hold.inv, p.Node), rs) } + if o.Key() != raisedAt.Key { + return nil // its own statement says it next + } + sayWaitingOn(&o, hold.waitersOn(p)) _, err := k.Observe(ctx, o) return err } +// reloginKey is a module's relogin-needed condition on a machine. +func reloginKey(module, node string) string { + return conditions.Key(conditions.ScopeModule, module+"."+node, kindReloginNeeded) +} + +// operatorOn is the operator's account on a machine, or "" when it is not known (to-be 29). +func operatorOn(ctx context.Context, inv *inventory.Inventory, node string) string { + if inv == nil { + return "" + } + n, err := inv.NodeByName(ctx, node) + if err != nil { + return "" + } + return n.Account +} + // reloginObservation is a module waiting for a person's new login on a machine (novox/hq ADR 0254): the // operator's, a warning, its summary the one sentence that says what to do; the resources are evidence. Its // plain words (ADR 0253) are the kind's: it needs the operator, and offers no answer — no verb can log a -// person in again, and a restart of the module's service would start it in the same session. -func reloginObservation(module, node, said string, rs []inventory.ResourceHealth) conditions.Observation { +// person in again, and a restart of the module's service would start it in the same session. operator is +// the machine's operator account, when known: the words say "your account" only for it. +func reloginObservation(module, node, said, operator string, rs []inventory.ResourceHealth) conditions.Observation { o := moduleUnhealthyObservation(module, node, rs) o.Token, o.Kind, o.Resolver, o.Summary = kindReloginNeeded, kindReloginNeeded, conditions.ResolverOperator, said - w := reloginWords(module, node) + accounts := waitingAccounts(module, rs) + yours := operator != "" && len(accounts) > 0 + for _, a := range accounts { + yours = yours && a == operator + } + w := reloginWords(module, node, yours) o.Headline, o.Explanation, o.Resolved, o.Needs, o.Actions = w.Headline, w.Explanation, w.Resolved, w.Needs, nil return o } -// reloginWords is what the operator reads of a module waiting for their new login on a machine (ADR 0253, -// ADR 0254): never quiet, since only the operator can do it, and no button, since nothing else can. -func reloginWords(module, node string) words { +// reloginWords is what the operator reads of a module waiting for a new login on a machine (ADR 0253, +// ADR 0254): never quiet, since only a person can do it, and no button, since nothing else can. yours says +// the account waiting is the operator's own on that machine; otherwise the words do not claim it is. +func reloginWords(module, node string, yours bool) words { + if yours { + return words{Headline: fmt.Sprintf("%s waits for a new login on %s", module, node), + Needs: fmt.Sprintf("log out of %s completely and log in again, or restart it.", node), + Explanation: fmt.Sprintf("%s put your account in a group it needs. You logged in before that, so %s "+ + "cannot run until you log in again. Its update is in place and nothing was undone.", + conditions.Capital(module), module), + Resolved: fmt.Sprintf("%s runs on %s after your new login", module, node)} + } return words{Headline: fmt.Sprintf("%s waits for a new login on %s", module, node), - Needs: fmt.Sprintf("log out of %s completely and log in again, or restart it.", node), - Explanation: fmt.Sprintf("%s put your account in a group it needs. You logged in before that, so %s "+ - "cannot run until you log in again. Its update is in place and nothing was undone.", conditions.Capital(module), module), - Resolved: fmt.Sprintf("%s runs on %s after your new login", module, node)} + Needs: fmt.Sprintf("have the account it names log out of %s completely and log in again, or restart %s.", node, node), + Explanation: fmt.Sprintf("%s put an account on %s in a group it needs. That account logged in before that, "+ + "so %s cannot run until it logs in again. Its update is in place and nothing was undone.", + conditions.Capital(module), node, module), + Resolved: fmt.Sprintf("%s runs on %s after the new login", module, node)} +} + +// waitingAccounts is every account of a module whose resource says it waits for a new login, sorted. +func waitingAccounts(module string, rs []inventory.ResourceHealth) []string { + seen := map[string]bool{} + var out []string + for _, r := range rs { + if r.Module == module && r.Kind == link.KindAccount && r.State == link.StateUnhealthy && + strings.HasPrefix(r.Reason, link.ReasonRelogin) && accountOf(r) != "" && !seen[accountOf(r)] { + seen[accountOf(r)] = true + out = append(out, accountOf(r)) + } + } + sort.Strings(out) + return out +} + +// sayWaitingOn adds to a module's condition the consumers held under it (to-be 48 §6): urgent while anyone +// waits on it, whether it is not working or waits for a new login. +func sayWaitingOn(o *conditions.Observation, waiters []string) { + if len(waiters) == 0 { + return + } + o.Severity = conditions.Urgent + o.Said += "; " + waitingWords(waiters) + o.Summary += fmt.Sprintf("; %d consumer(s) wait on it", len(waiters)) + o.Explanation += fmt.Sprintf(" %d module(s) that depend on it wait for it.", len(waiters)) } // moduleUnhealthyObservation is a module unhealthy on a machine, in words: the summary names the module, @@ -352,18 +430,14 @@ const kindReloginNeeded = "relogin-needed" // not in its group or that could not be read — is not a wait, and the module is judged as before. A // resource still starting is not unhealthy and does not make a wait either. Pure. func personWait(module, machine string, rs []inventory.ResourceHealth) (string, bool) { - waiting := map[string]bool{} - var accounts []string - for _, r := range rs { - if r.Module == module && r.Kind == link.KindAccount && r.State == link.StateUnhealthy && - strings.HasPrefix(r.Reason, link.ReasonRelogin) && accountOf(r) != "" && !waiting[accountOf(r)] { - waiting[accountOf(r)] = true - accounts = append(accounts, accountOf(r)) - } - } + accounts := waitingAccounts(module, rs) if len(accounts) == 0 { return "", false } + waiting := map[string]bool{} + for _, a := range accounts { + waiting[a] = true + } var units []string for _, r := range rs { if r.Module != module || r.State != link.StateUnhealthy { @@ -377,7 +451,6 @@ func personWait(module, machine string, rs []inventory.ResourceHealth) (string, return "", false } } - sort.Strings(accounts) said := fmt.Sprintf("relogin needed on %s: %s waits for a new login of %s, which is in its group and whose "+ "running session began before it was; log out of every session and in again, or reboot", machine, module, strings.Join(accounts, ", ")) @@ -415,6 +488,18 @@ func moduleHealthWord(module, machine string, since time.Time, f gateFacts) (hea return healthNotYet, fmt.Sprintf("%s has not said how what %s runs is since it was sent", machine, module) } wait, waits := personWait(module, machine, h.Resources) + // **Only the build whose own send put the account into its wait is excused** (issue 318 review): an + // account already waiting before this send was not brought to it by this build, and a later build of + // the module sent while the login is still owed is judged as before. Two seconds of skew between the + // machine's clock and the controller's, as the witness allows. + if waits { + for _, r := range h.Resources { + if r.Module == module && r.Kind == link.KindAccount && r.State == link.StateUnhealthy && + strings.HasPrefix(r.Reason, link.ReasonRelogin) && r.Since.Before(since.Add(-2*time.Second)) { + waits = false + } + } + } for _, r := range h.Resources { if r.Module != module { continue diff --git a/cmd/mesh-controller/person_wait_test.go b/cmd/mesh-controller/person_wait_test.go index 4670fa0c..2393cc71 100644 --- a/cmd/mesh-controller/person_wait_test.go +++ b/cmd/mesh-controller/person_wait_test.go @@ -75,11 +75,24 @@ func TestOnlyWhatDependsOnTheNewLoginIsAWait(t *testing.T) { func TestAWaitForAPersonIsAPassCarriedAlong(t *testing.T) { now := time.Now() since := now.Add(-time.Minute) + account := relogin("lights", "operator") + account.Since = since.Add(10 * time.Second) // the send put it in its wait f := gateFacts{now: now, health: map[string]inventory.NodeHealth{"laptop": {Node: "laptop", HeardAt: now, - Resources: []inventory.ResourceHealth{relogin("lights", "operator"), userUnit("lights", "operator")}}}} + Resources: []inventory.ResourceHealth{account, userUnit("lights", "operator")}}}} if h, why := moduleHealthWord("lights", "laptop", since, f); h != healthPerson || !strings.Contains(why, "relogin needed on laptop") { t.Fatalf("a wait for a new login reads %v %q; want a wait for a person", h, why) } + // **Only the send that put the account in its wait is excused** (issue 318 review): an account waiting + // since before the send is judged as before, so a later build of the module is not passed on an old wait. + older := f.health["laptop"] + older.Resources = []inventory.ResourceHealth{relogin("lights", "operator"), userUnit("lights", "operator")} + older.Resources[0].Since = since.Add(-time.Hour) + f.health["laptop"] = older + if h, why := moduleHealthWord("lights", "laptop", since, f); h != healthNotYet { + t.Fatalf("a wait from before the send reads %v %q; want not yet", h, why) + } + f.health["laptop"] = inventory.NodeHealth{Node: "laptop", HeardAt: now, + Resources: []inventory.ResourceHealth{account, userUnit("lights", "operator")}} h := f.health["laptop"] h.Resources = append(h.Resources, inventory.ResourceHealth{Module: "lights", Resource: "lights.web", Kind: "container", Target: "lights", State: link.StateUnhealthy, Reason: "down"}) @@ -197,7 +210,7 @@ func TestAModuleHealthyOnItsOwnKeepsItsPassWhenItsSendFails(t *testing.T) { // What the operator reads of the wait (ADR 0253, ADR 0254): it needs them, so it is never quiet, and it offers // no button, since nothing but their own new login can do it. func TestTheReloginConditionNeedsTheOperatorAndOffersNoButton(t *testing.T) { - o := reloginObservation("openrazer", "g14", "relogin needed on g14: …", []inventory.ResourceHealth{ + o := reloginObservation("openrazer", "g14", "relogin needed on g14: …", "operator", []inventory.ResourceHealth{ relogin("openrazer", "operator"), userUnit("openrazer", "operator")}) plainExample(t, o, "openrazer waits for a new login on g14", "Needs you: log out of g14 completely and log in again, or restart it. Openrazer put your account in a "+ @@ -206,9 +219,64 @@ func TestTheReloginConditionNeedsTheOperatorAndOffersNoButton(t *testing.T) { if len(o.Actions) != 0 || o.Needs == "" { t.Errorf("relogin: needs %q, actions %+v; want needs and no button", o.Needs, o.Actions) } - // The kind's own wording, for a condition raised without words, says the same. - if w := plainWordings[kindReloginNeeded](conditions.Observation{Scope: conditions.ScopeModule, ID: "openrazer.g14", Machine: "g14"}); w.Needs != o.Needs || - w.Headline != o.Headline || len(w.Actions) != 0 { + // **Not "your account" when it is not the operator's** (issue 318 review). + other := reloginObservation("openrazer", "g14", "relogin needed on g14: …", "operator", []inventory.ResourceHealth{ + relogin("openrazer", "guest"), userUnit("openrazer", "guest")}) + plainExample(t, other, "openrazer waits for a new login on g14", + "Needs you: have the account it names log out of g14 completely and log in again, or restart g14. Openrazer "+ + "put an account on g14 in a group it needs. That account logged in before that, so openrazer cannot run "+ + "until it logs in again. Its update is in place and nothing was undone.") + if unknown := reloginObservation("openrazer", "g14", "…", "", []inventory.ResourceHealth{relogin("openrazer", "operator")}); strings.Contains(unknown.Explanation, "your account") { + t.Errorf("an operator not known is still told it is their account: %q", unknown.Explanation) + } + // The kind's own wording, for a condition raised without words, names the module whole, dots and all, + // and never claims the account is the operator's. + w := plainWordings[kindReloginNeeded](conditions.Observation{Scope: conditions.ScopeModule, ID: "razer.lights.g14", Machine: "g14"}) + if w.Headline != "razer.lights waits for a new login on g14" || w.Needs == "" || len(w.Actions) != 0 || + strings.Contains(w.Explanation, "your account") { t.Errorf("the kind's wording: %+v", w) } } + +// When a module not working turns into one waiting for a new login, and back, the clearing line says what it +// became, never that it works again (issue 318 review). +func TestAConditionThatBecameTheOtherKindSaysSoWhenItClears(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + store := conditions.NewInMemory() + told := &conditions.Told{} + k := conditions.NewKeeper(ctx, conditions.Options{Store: store, History: store, Teller: told}) + t.Cleanup(func() { k.Close(context.Background()) }) + at := h0 + say := func(rs ...link.ResourceHealth) { + t.Helper() + at = at.Add(time.Second) + for i := range rs { + rs[i].Since = at + } + if err := stateHealth(ctx, open.inventory, k, "laptop", link.Health{Contract: link.ReadinessContract, At: at, + Resources: rs}, at); err != nil { + t.Fatal(err) + } + } + unit := link.ResourceHealth{Module: "lights", Resource: "lights.daemon", Kind: link.KindUnit, Target: "lights.service", + Account: "operator", State: link.StateUnhealthy, Reason: "failed in the account's own service manager (exit-code)"} + account := link.ResourceHealth{Module: "lights", Resource: "lights.operator", Kind: link.KindAccount, Target: "operator", + Account: "operator", State: link.StateUnhealthy, Reason: link.ReasonRelogin + ": operator is in the group lights"} + say(unit) + say(unit) // lights not working + say(account, unit) + say(account, unit) // now it waits for a login + var resolved []string + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline) && len(resolved) == 0; { + time.Sleep(20 * time.Millisecond) + for _, e := range told.Said() { + if e.Change == conditions.ChangeCleared { + resolved = append(resolved, e.Condition.Key+": "+e.Condition.Resolved) + } + } + } + if len(resolved) != 1 || !strings.Contains(resolved[0], "module.lights.laptop.unhealthy: lights on laptop now waits only for a new login") { + t.Fatalf("cleared %v; want the not-working condition cleared as now waiting for a login", resolved) + } +} diff --git a/cmd/mesh-controller/plain_words.go b/cmd/mesh-controller/plain_words.go index 846667ba..40fb6898 100644 --- a/cmd/mesh-controller/plain_words.go +++ b/cmd/mesh-controller/plain_words.go @@ -184,11 +184,12 @@ var plainWordings = map[string]func(conditions.Observation) words{ Resolved: conditions.Capital(thing) + " works again"} }), kindReloginNeeded: worded(func(o conditions.Observation) words { + // A module's name may hold dots: it is the id without its machine. module := "" - if o.Scope == conditions.ScopeModule { - module = idPart(o, 0) + if o.Scope == conditions.ScopeModule && o.Machine != "" { + module = strings.TrimSuffix(o.ID, "."+o.Machine) } - return reloginWords(orModule(module), machineOr(o, "a machine")) + return reloginWords(orModule(module), machineOr(o, "a machine"), false) }), kindProviderFailing: worded(func(o conditions.Observation) words { thing, consumer := conditions.ThingWords(o), idPart(o, 2) diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index ef480603..29e76cea 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -319,7 +319,7 @@ func passCarried(ctx context.Context, open *stores, p *inventory.Plan, g *invent } seen[c.Build] = true if err := open.inventory.RecordGate(ctx, inventory.GateVerdict{Build: c.Build, Module: c.Module, Commit: c.To, - Previous: c.From, Plan: p.ID, Machines: []string{c.Node}, Verdict: inventory.GatePassed, Why: g.Why, + Previous: c.From, Plan: p.ID, Machines: []string{c.Node}, Verdict: inventory.GatePassed, Why: whyFor(g, c.Module), Component: coreComponent(c.Module), JudgingFrom: g.Since}); err != nil { fmt.Printf("%s: %s passed its gate on %s, and the verdict could not be kept: %v\n", p.ID, c.Module, c.Node, err) } @@ -375,10 +375,7 @@ func keepPassing(ctx context.Context, open *stores, p *inventory.Plan, g *invent continue } seen[c.Build] = true - why := fmt.Sprintf("healthy %d times on its own while the send failed: %s", g.Healthy[c.Module], g.Why) - if w := g.Waits[c.Module]; w != "" { - why += "; and it waits for a person: " + w - } + why := passedAloneWhy(g, c.Module) if err := open.inventory.RecordGate(ctx, inventory.GateVerdict{Build: c.Build, Module: c.Module, Commit: c.To, Previous: c.From, Plan: p.ID, Machines: []string{c.Node}, Verdict: inventory.GatePassed, Why: why, Component: coreComponent(c.Module), JudgingFrom: g.Since}); err != nil { @@ -393,6 +390,15 @@ func keepPassing(ctx context.Context, open *stores, p *inventory.Plan, g *invent return kept } +// passedAloneWhy is the verdict of a module that passed on its own while its send failed, with its own wait. +func passedAloneWhy(g *inventory.PlanGate, module string) string { + why := fmt.Sprintf("healthy %d times on its own while the send failed: %s", g.Healthy[module], g.Why) + if w := g.Waits[module]; w != "" { + why += waitsPrefix + w + } + return why +} + // sentTheBuild is every machine running a module that was last sent this build of it. func sentTheBuild(ctx context.Context, open *stores, module, commit string) ([]string, error) { running, err := open.inventory.Running(ctx, module) diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 58c1fa0c..7d179181 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -703,7 +703,7 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, if lead.Gate.Verdict != inventory.GatePassed { continue } - passedWith(state, state.GatedBy, lead.Gate) + passedWith(m, state, state.GatedBy, lead.Gate) } switch { case step.failed != "": @@ -937,14 +937,14 @@ func firstSend(ctx context.Context, open *stores, p *inventory.Plan, node string // passedWith keeps, on a module sent with others, the verdict of the gate that judged the send: the gate // on the first of them passed, and its pass was kept for every build it carried (passCarried). -func passedWith(s *inventory.PlanModule, lead string, g *inventory.PlanGate) { +func passedWith(module string, s *inventory.PlanModule, lead string, g *inventory.PlanGate) { if s.Gate == nil { s.Gate = &inventory.PlanGate{Machines: g.Machines, From: s.Previous, To: s.Commit, Since: g.Since} } if s.Gate.Verdict != "" { return } - s.Gate.Verdict, s.Gate.Why, s.Gate.JudgedAt, s.Gate.Took = g.Verdict, "judged with "+lead+": "+g.Why, g.JudgedAt, g.Took + s.Gate.Verdict, s.Gate.Why, s.Gate.JudgedAt, s.Gate.Took = g.Verdict, "judged with "+lead+": "+whyFor(g, module), g.JudgedAt, g.Took s.Gate.Passes, s.Gate.Kept = g.Passes, true } @@ -963,6 +963,18 @@ func failFirstSend(ctx context.Context, open *stores, p *inventory.Plan, module g := state.Gate if g == nil || g.Verdict == "" || len(g.Failing) == 0 || slices.Contains(g.Failing, module) { gateFailed(batched, open, p, module, state, machines, why) + } else if slices.Contains(g.Passing, module) && state.Build != "" { + // **The module the gate is kept on keeps its own pass too** (issue 318 review): healthy for the passes + // asked while another of its send failed, its build has a verdict, and other walks do not wait on it. + if err := open.inventory.RecordGate(ctx, inventory.GateVerdict{Build: state.Build, Module: module, + Commit: state.Commit, Previous: state.Previous, Plan: p.ID, Machines: g.Machines, Verdict: inventory.GatePassed, + Why: passedAloneWhy(g, module), Component: g.Component, JudgingFrom: g.Since}); err != nil { + fmt.Printf("%s: %s passed its gate on its own, and the verdict could not be kept: %v\n", p.ID, module, err) + } + state.Why = "passed on its own; stopped with the send that carried it: " + g.Why + p.State = inventory.PlanFailed + p.Note = fmt.Sprintf("the send to %s in tier %d failed its gate: %s; %s passed on its own and is kept", + strings.Join(g.Machines, ", "), p.Tier, g.Why, module) } else { state.Why = "not found wanting; stopped with the send that carried it: " + g.Why p.State = inventory.PlanFailed diff --git a/cmd/mesh-controller/tier_send_test.go b/cmd/mesh-controller/tier_send_test.go index dc93fbbc..a6e6b5fe 100644 --- a/cmd/mesh-controller/tier_send_test.go +++ b/cmd/mesh-controller/tier_send_test.go @@ -255,8 +255,13 @@ func TestACoreBehindWhileTheNodeToolsSettleFailsNoOtherModule(t *testing.T) { t.Fatalf("%s's build was marked failed for the node tools' condition", m) } } - if !strings.Contains(p.Note, "app1 was not found wanting") { - t.Fatalf("the plan blames the module its gate was kept on: %s", p.Note) + // Healthy for the passes asked, the module the gate is kept on keeps its own pass (novox/hq issue 318 + // review): it is not blamed, and not left without a verdict. + if !strings.Contains(p.Note, "app1 passed on its own and is kept") { + t.Fatalf("the plan blames the module its gate was kept on, or leaves it unjudged: %s", p.Note) + } + if v, found, _ := inv.GateOf(ctx, "build-app1-2"); !found || v.Verdict != inventory.GatePassed { + t.Fatalf("app1's own pass was not kept: %+v", v) } } diff --git a/internal/conditions/store.go b/internal/conditions/store.go index bdee8e69..cf016f4a 100644 --- a/internal/conditions/store.go +++ b/internal/conditions/store.go @@ -280,6 +280,13 @@ func (k *Keeper) Observe(ctx context.Context, o Observation) (Condition, error) // Clear removes a condition an observation says is resolved, and says so. False when none was open. func (k *Keeper) Clear(ctx context.Context, key, why string) (bool, error) { + return k.ClearSaying(ctx, key, why, "") +} + +// ClearSaying is Clear with the line the operator reads when it clears, in place of the condition's own +// resolved line: for a condition that ends because it became another (novox/hq ADR 0254: a module not +// working that now waits for a new login is not "working again"). Empty keeps the condition's own. +func (k *Keeper) ClearSaying(ctx context.Context, key, why, resolved string) (bool, error) { for i := 0; i < tries; i++ { entry, found, err := k.store.Get(ctx, key) if err != nil { @@ -296,6 +303,9 @@ func (k *Keeper) Clear(ctx context.Context, key, why string) (bool, error) { if err := k.stamp(&c); err != nil { return false, err } + if resolved != "" { + c.Resolved = resolved + } if err := k.store.Delete(ctx, key, entry.Revision); errors.Is(err, ErrMoved) { continue } else if err != nil { diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index 70b9d253..b3176a65 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -152,6 +152,13 @@ type PlanGate struct { // Passing names the modules that passed on their own when the send failed (ADR 0254): each keeps its // pass, and is not put back. Passing []string `json:"passing,omitempty"` + // HealthyAt is, per module, when its newest healthy judging was counted: a pass is counted only gateEvery + // after the one before (issue 318 review). + HealthyAt map[string]time.Time `json:"healthy_at,omitempty"` + // Broken names the modules a judging found broken, kept for the rest of the judging, and BrokenWhy the + // first reason: the send fails, and the modules beside them are judged to their own verdict first. + Broken []string `json:"broken,omitempty"` + BrokenWhy string `json:"broken_why,omitempty"` } // CarriedMove is one module's build moving on a machine with a gated send. -- 2.54.0 From 7ad9dbcb5d2f44b5b4557c1172a6ec950e15d0e7 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 15:05:53 +0200 Subject: [PATCH 2/3] Say a pending new login in the same words everywhere, without 'session' (issue 318 review) --- cmd/mesh-controller/module_health.go | 8 ++++++-- cmd/mesh-controller/plain_words.go | 2 +- cmd/mesh-controller/plain_words_test.go | 5 ++++- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/cmd/mesh-controller/module_health.go b/cmd/mesh-controller/module_health.go index b40c6818..17f83ec4 100644 --- a/cmd/mesh-controller/module_health.go +++ b/cmd/mesh-controller/module_health.go @@ -388,6 +388,10 @@ func moduleUnhealthyObservation(module, node string, rs []inventory.ResourceHeal // reasonWords is why a resource is unhealthy, as a person reads it. func reasonWords(r inventory.ResourceHealth) string { + // An account waiting for a new login (ADR 0252) is said in the mesh's words, not the engine's. + if r.Kind == link.KindAccount && strings.HasPrefix(r.Reason, link.ReasonRelogin) { + return fmt.Sprintf("waits for a new login of %s, which is in the group and logged in before it was", accountOf(r)) + } switch r.Reason { case "restarting": return fmt.Sprintf("keeps restarting (%d restart(s) counted)", r.Restarts) @@ -452,11 +456,11 @@ func personWait(module, machine string, rs []inventory.ResourceHealth) (string, } } said := fmt.Sprintf("relogin needed on %s: %s waits for a new login of %s, which is in its group and whose "+ - "running session began before it was; log out of every session and in again, or reboot", machine, module, + "login began before it was; log out of %[1]s completely and log in again, or restart it", machine, module, strings.Join(accounts, ", ")) if len(units) > 0 { sort.Strings(units) - said += fmt.Sprintf(" (until then %s cannot run in that session)", strings.Join(units, ", ")) + said += fmt.Sprintf(" (until then %s cannot run)", strings.Join(units, ", ")) } return said, true } diff --git a/cmd/mesh-controller/plain_words.go b/cmd/mesh-controller/plain_words.go index 40fb6898..54befa7f 100644 --- a/cmd/mesh-controller/plain_words.go +++ b/cmd/mesh-controller/plain_words.go @@ -672,7 +672,7 @@ const FromMeshMCPServer = "from the mesh MCP server; this notification cannot do // reloginNeeds is what an account waiting for its groups needs (ADR 0252). func reloginNeeds(node string) string { - return fmt.Sprintf("log out of every session on %s and log in again.", node) + return fmt.Sprintf("log out of %s completely and log in again, or restart it.", node) } // stalledWords are the plain words of a delivery held past its bound, as mesh-delivery says it. diff --git a/cmd/mesh-controller/plain_words_test.go b/cmd/mesh-controller/plain_words_test.go index abb8355e..a24290df 100644 --- a/cmd/mesh-controller/plain_words_test.go +++ b/cmd/mesh-controller/plain_words_test.go @@ -94,7 +94,7 @@ func TestAModuleUnhealthyAsksForARestartInWords(t *testing.T) { // An account waiting for a new login (ADR 0252) asks for the login, held to the plain rule. o = moduleUnhealthyObservation("openrazer", "g14", []inventory.ResourceHealth{{Kind: "account", Resource: "operator-in-group", Target: "jochen", Reason: "relogin needed: the account is in the group"}}) - if o.Needs != "log out of every session on g14 and log in again." || len(o.Actions) != 0 { + if o.Needs != "log out of g14 completely and log in again, or restart it." || len(o.Actions) != 0 { t.Errorf("relogin: %q %+v", o.Needs, o.Actions) } w := conditions.Words{Headline: o.Headline, Explanation: o.Explanation, Resolved: o.Resolved, Needs: o.Needs} @@ -107,6 +107,9 @@ func TestAModuleUnhealthyAsksForARestartInWords(t *testing.T) { if why, ok := conditions.PlainWords(rw, "g14"); !ok || rw.Needs == "" || len(rw.Actions) != 0 { t.Errorf("relogin-needed: %s %+v", why, rw) } + if strings.Contains(o.Summary, "session") || !strings.Contains(o.Summary, "waits for a new login of jochen") { + t.Errorf("relogin summary: %q", o.Summary) + } } // **Failed units on a machine**: "shanks's service manager is degraded: 3 failed unit(s) no module places — -- 2.54.0 From a63939160ef376b5dfa05f2dc46de5474cfb7e23 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 15:25:58 +0200 Subject: [PATCH 3/3] Put a broken module back at once, and excuse a wait only for a move that added an account group (hq issue 318 review) --- cmd/mesh-controller/gate.go | 38 +++++- cmd/mesh-controller/gate_followup_test.go | 144 ++++++++++++++++++++++ cmd/mesh-controller/module_health.go | 52 ++++++-- cmd/mesh-controller/person_wait_test.go | 52 ++++++-- cmd/mesh-controller/release.go | 65 ++++++++++ cmd/mesh-controller/release_plan.go | 11 +- cmd/mesh-controller/replay318_test.go | 10 +- internal/inventory/plans.go | 2 + 8 files changed, 348 insertions(+), 26 deletions(-) diff --git a/cmd/mesh-controller/gate.go b/cmd/mesh-controller/gate.go index 714dce2d..54b26eb0 100644 --- a/cmd/mesh-controller/gate.go +++ b/cmd/mesh-controller/gate.go @@ -119,6 +119,9 @@ type gateFacts struct { healthErr error // heldOn is, per "@", the provider its findings are held under (ADR 0240 rule 5). heldOn map[string]string + // groupsAdded is, per module, whether the move judged puts an account in a group its previous build did + // not (issue 318 review): the only move whose wait for a new login is excused. + groupsAdded map[string]bool } // gatherGateFacts reads what a judging needs, from the store, the bus and this controller's memory. A @@ -430,6 +433,7 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs if err != nil { return "", err } + facts.groupsAdded = movesAddingGroups(ctx, open.inventory, g, pairs, shelf) // **What is wrong with a machine itself is the machine's** (novox/hq issue 281): read once for each // machine judged, apart from what is wrong with a module there, and never pinned on the module the // gate happens to be kept on. @@ -544,7 +548,8 @@ func judgeMoves(ctx context.Context, open *stores, g *inventory.PlanGate, pairs fail(g.BrokenWhy) break } - // What broke fails the send; what is beside it is judged to its own verdict first, within the bound. + // What broke fails the send, and is put back at once by the caller (putBackBroken); what is beside + // it is judged to its own verdict first, within the bound. g.Passes, g.LastPass, g.Failing = 0, nil, failing g.Last = fmt.Sprintf("%s; %s judged to its own verdict before the send's", g.BrokenWhy, strings.Join(judging, ", ")) case worst == healthWaiting: @@ -607,6 +612,37 @@ func joinSaid(a, b string) string { return a + "; " + b } +// movesAddingGroups is, per module a gate judges, whether its move puts an account in a group the build it +// moved from did not: read from the builds' manifests. A module whose move is not known adds none. +func movesAddingGroups(ctx context.Context, inv *inventory.Inventory, g *inventory.PlanGate, pairs []judged, + shelf map[string]catalogue.Manifest) map[string]bool { + out := map[string]bool{} + for _, j := range pairs { + if _, done := out[j.module]; done { + continue + } + from, to := g.From, g.To + for _, c := range g.Carried { + if c.Module == j.module { + from, to = c.From, c.To + break + } + } + target, found, err := inv.ManifestAt(ctx, j.module, to) + if err != nil || !found { + target = shelf[j.module] + } + before, had, err := inv.ManifestAt(ctx, j.module, from) + if err != nil || (from != "" && !had) { + // What it moved from is not known: no wait is excused, rather than one the move did not bring. + out[j.module] = false + continue + } + out[j.module] = addsAccountGroups(before, had, target) + } + return out +} + // decide sets a gate's verdict. func decide(g *inventory.PlanGate, verdict, why string, now time.Time) { g.Verdict, g.Why, g.JudgedAt = verdict, why, &now diff --git a/cmd/mesh-controller/gate_followup_test.go b/cmd/mesh-controller/gate_followup_test.go index 3229da1f..bd17ac8d 100644 --- a/cmd/mesh-controller/gate_followup_test.go +++ b/cmd/mesh-controller/gate_followup_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "reflect" + "slices" "strings" "testing" "time" @@ -277,3 +278,146 @@ func TestAProviderWaitingForALoginSaysWhoWaitsOnIt(t *testing.T) { t.Fatalf("the provider's wait: %s, %q; want it urgent and naming its consumer", c.Severity, c.Evidence[0].Said) } } + +// **A broken module is put back at once** (issue 318 review): the laptop's witness put the node tools back +// within a minute of a send that also moved `app`, and `app` reads not yet healthy because of it. The node +// tools are marked and put back in the very judging that found them broken — their registered build is the +// one before, and the laptop is sent it — while `app` goes on being judged, recovers, and keeps its own pass. +func TestABrokenModuleIsPutBackAtOnceAndTheOneBesideItGetsItsOwnVerdict(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + inv := open.inventory + withConditionsInMemory(t) + was := doctorFrom + doctorFrom = nil + t.Cleanup(func() { doctorFrom = was }) + old, recent := time.Now().Add(-3*time.Hour), time.Now().Add(-time.Minute) + build := func(module, commit string, asked time.Time) { + manifest, _ := json.Marshal(catalogue.Manifest{Module: module, Version: "1"}) + if err := inv.RecordBuild(ctx, inventory.Build{ID: "build-" + module + "-" + commit, Module: module, Commit: commit, + Repository: "novox/mesh-catalog", Path: "modules/" + module, Manifest: manifest, Asked: asked, At: asked, + Made: []inventory.Artifact{{Name: "x", Kind: "bundle", Reference: "sha256:" + module + commit}}}); err != nil { + t.Fatal(err) + } + if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: module, Version: "1"}, inventory.Source{ + Repository: "novox/mesh-catalog", Seat: "git", Path: "modules/" + module, BuiltFrom: commit, Head: commit, + Asked: asked}); err != nil { + t.Fatal(err) + } + } + for _, m := range []string{"app", broker_runtime} { + build(m, "c1", old) + if _, err := inv.Assign(ctx, "laptop", m); err != nil { + t.Fatal(err) + } + } + if err := inv.RecordSent(ctx, nodeID(t, open, "laptop"), "d-laptop", map[string]string{"app": "c1", broker_runtime: "c1"}); err != nil { + t.Fatal(err) + } + build("app", "c2", recent) + build(broker_runtime, "c2", recent) + + wasMoves, wasHeard, wasSend, wasGather := machineMoves, releaseHeard, sendRollout, gatherGateFacts + t.Cleanup(func() { + machineMoves, releaseHeard, sendRollout, gatherGateFacts = wasMoves, wasHeard, wasSend, wasGather + }) + machineMoves = func(ctx context.Context, open *stores, f moveFacts, node string, all bool) ([]inventory.CarriedMove, error) { + modules, _ := open.inventory.Assigned(ctx, node) + sent, known, err := open.inventory.SentBuilds(ctx, node) + if err != nil { + return nil, err + } + return f.moves(node, modules, sent, known, all), nil + } + releaseHeard = func(context.Context, *stores) (map[string]bool, error) { return map[string]bool{"laptop": true}, nil } + var sends []string + n := 0 + sendRollout = func(ctx context.Context, open *stores, names []string) ([]string, error) { + current, err := open.inventory.CurrentBuilds(ctx) + if err != nil { + return nil, err + } + for _, node := range names { + n++ + carried := map[string]string{"app": current["app"].Commit, broker_runtime: current[broker_runtime].Commit} + sends = append(sends, node+":"+carried[broker_runtime]) + digest := "d-" + node + "-" + time.Now().Format("150405.000000") + string(rune('a'+n)) + if err := open.inventory.RecordSent(ctx, nodeID(t, open, node), digest, carried); err != nil { + return nil, err + } + if _, err := open.inventory.RecordDoing(ctx, nodeID(t, open, node), inventory.Doing{Node: node, + Outcome: inventory.OutcomeApplied, Declared: digest, Applied: 1, At: time.Now()}); err != nil { + return nil, err + } + } + return names, nil + } + var seen []string // the node tools' registered build at each judging + gatherGateFacts = func(ctx context.Context, open *stores, component string) (gateFacts, error) { + now := time.Now() + f := gateFacts{now: now, reports: map[string]inventory.Reported{}, engines: map[string]string{}, + rolledBack: map[string][]lease.Rollback{}, served: map[string]served{}} + reports, _ := open.inventory.LastReports(ctx) + for _, r := range reports { + f.reports[r.Node] = r + } + current, _ := open.inventory.CurrentBuilds(ctx) + seen = append(seen, current[broker_runtime].Commit) + app := healthyContainer("app") + if current[broker_runtime].Commit == "c2" { + // The witness reverted the node tools; app cannot reach them yet. + f.rolledBack["laptop"] = []lease.Rollback{{Component: lease.ComponentNodeTools, Outcome: lease.OutcomeRolledBack, + From: "c2", To: "c1", At: now, Why: "the node tools did not answer"}} + app.State, app.Reason = link.StateUnhealthy, "down" + } + f.served["laptop"] = served{runtime: current[broker_runtime].Commit == "c1", tools: map[string]bool{}} + f.health = map[string]inventory.NodeHealth{"laptop": {Node: "laptop", HeardAt: now, Resources: []inventory.ResourceHealth{app}}} + return f, nil + } + withGateBounds(t, 100*time.Millisecond, 0, 10*time.Second) + + release := func() inventory.Plan { + t.Helper() + plans, _ := inv.RecentPlans(ctx, 10) + for _, p := range plans { + if p.Release != nil { + return p + } + } + t.Fatal("no walk") + return inventory.Plan{} + } + // Judged until the first judging has found the node tools broken, and one more. + for i := 0; i < 20 && len(seen) < 2; i++ { + advancePlans(ctx, open) + } + if len(seen) < 2 || seen[0] != "c2" || seen[1] != "c1" { + t.Fatalf("the node tools' registered build at each judging: %v; want c2, then c1 at the very next judging", seen) + } + if failed, _ := inv.GateFailed(ctx, "build-"+broker_runtime+"-c2"); !failed { + t.Fatal("the node tools' build is not marked failed at once") + } + if p := release(); p.State != inventory.PlanRolling || p.Release.Gate == nil || p.Release.Gate.Verdict != "" { + t.Fatalf("the walk is %s with gate %+v; want it still judging app", p.State, p.Release.Gate) + } + if !slices.Contains(sends, "laptop:c1") { + t.Fatalf("sends %v; want the laptop sent the node tools' earlier build at once", sends) + } + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); { + advancePlans(ctx, open) + if release().State != inventory.PlanRolling { + break + } + time.Sleep(20 * time.Millisecond) + } + p := release() + if p.State != inventory.PlanFailed { + t.Fatalf("the walk is %s: %s; want failed, for the node tools", p.State, p.Note) + } + if v, found, _ := inv.GateOf(ctx, "build-app-c2"); !found || v.Verdict != inventory.GatePassed || !strings.Contains(v.Why, "on its own") { + t.Fatalf("app's verdict: %+v; want its own pass", v) + } + if current, _ := inv.CurrentBuilds(ctx); current["app"].Commit != "c2" { + t.Fatalf("app is registered at %s; want it kept at c2", current["app"].Commit) + } +} diff --git a/cmd/mesh-controller/module_health.go b/cmd/mesh-controller/module_health.go index 17f83ec4..f34f999d 100644 --- a/cmd/mesh-controller/module_health.go +++ b/cmd/mesh-controller/module_health.go @@ -492,17 +492,12 @@ func moduleHealthWord(module, machine string, since time.Time, f gateFacts) (hea return healthNotYet, fmt.Sprintf("%s has not said how what %s runs is since it was sent", machine, module) } wait, waits := personWait(module, machine, h.Resources) - // **Only the build whose own send put the account into its wait is excused** (issue 318 review): an - // account already waiting before this send was not brought to it by this build, and a later build of - // the module sent while the login is still owed is judged as before. Two seconds of skew between the - // machine's clock and the controller's, as the witness allows. - if waits { - for _, r := range h.Resources { - if r.Module == module && r.Kind == link.KindAccount && r.State == link.StateUnhealthy && - strings.HasPrefix(r.Reason, link.ReasonRelogin) && r.Since.Before(since.Add(-2*time.Second)) { - waits = false - } - } + // **Only a build whose own send put the account in a new group is excused** (issue 318 review): read from + // what the controller sent, never from when the machine says the wait began — that time is the engine's + // memory, reset by its restart and moved by a change of words. A build that adds no account group cannot + // have brought a wait, so a later build sent while the login is still owed is judged as before. + if waits && !f.groupsAdded[module] { + waits = false } for _, r := range h.Resources { if r.Module != module { @@ -565,3 +560,38 @@ func healthLines(h inventory.NodeHealth, had bool, now time.Time) []string { } return out } + +// accountGroups is every account group a manifest declares, as "/": a user resource's +// groups (ADR 0252). +func accountGroups(m catalogue.Manifest) map[string]bool { + out := map[string]bool{} + for _, r := range m.Resources { + if t, _ := r["type"].(string); t != "user" { + continue + } + name, _ := r["name"].(string) + groups, _ := r["groups"].([]any) + for _, g := range groups { + if group, ok := g.(string); ok && group != "" { + out[name+"/"+group] = true + } + } + } + return out +} + +// addsAccountGroups is whether a move from one manifest of a module to another puts an account in a group the +// earlier one did not (issue 318 review): the only send that can bring a wait for a new login. A module new to +// the machine (no earlier manifest) adds every group it declares. +func addsAccountGroups(from catalogue.Manifest, hadFrom bool, to catalogue.Manifest) bool { + before := map[string]bool{} + if hadFrom { + before = accountGroups(from) + } + for g := range accountGroups(to) { + if !before[g] { + return true + } + } + return false +} diff --git a/cmd/mesh-controller/person_wait_test.go b/cmd/mesh-controller/person_wait_test.go index 2393cc71..e52812b4 100644 --- a/cmd/mesh-controller/person_wait_test.go +++ b/cmd/mesh-controller/person_wait_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/novox/mesh-controller/internal/catalogue" "github.com/novox/mesh-controller/internal/conditions" "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" @@ -76,23 +77,19 @@ func TestAWaitForAPersonIsAPassCarriedAlong(t *testing.T) { now := time.Now() since := now.Add(-time.Minute) account := relogin("lights", "operator") - account.Since = since.Add(10 * time.Second) // the send put it in its wait f := gateFacts{now: now, health: map[string]inventory.NodeHealth{"laptop": {Node: "laptop", HeardAt: now, - Resources: []inventory.ResourceHealth{account, userUnit("lights", "operator")}}}} + Resources: []inventory.ResourceHealth{account, userUnit("lights", "operator")}}}, + groupsAdded: map[string]bool{"lights": true}} if h, why := moduleHealthWord("lights", "laptop", since, f); h != healthPerson || !strings.Contains(why, "relogin needed on laptop") { t.Fatalf("a wait for a new login reads %v %q; want a wait for a person", h, why) } - // **Only the send that put the account in its wait is excused** (issue 318 review): an account waiting - // since before the send is judged as before, so a later build of the module is not passed on an old wait. - older := f.health["laptop"] - older.Resources = []inventory.ResourceHealth{relogin("lights", "operator"), userUnit("lights", "operator")} - older.Resources[0].Since = since.Add(-time.Hour) - f.health["laptop"] = older + // **Only a move that put the account in a new group is excused** (issue 318 review): a later build that + // adds no group did not bring the wait, and is judged as before. + f.groupsAdded["lights"] = false if h, why := moduleHealthWord("lights", "laptop", since, f); h != healthNotYet { - t.Fatalf("a wait from before the send reads %v %q; want not yet", h, why) + t.Fatalf("a wait the move did not bring reads %v %q; want not yet", h, why) } - f.health["laptop"] = inventory.NodeHealth{Node: "laptop", HeardAt: now, - Resources: []inventory.ResourceHealth{account, userUnit("lights", "operator")}} + f.groupsAdded["lights"] = true h := f.health["laptop"] h.Resources = append(h.Resources, inventory.ResourceHealth{Module: "lights", Resource: "lights.web", Kind: "container", Target: "lights", State: link.StateUnhealthy, Reason: "down"}) @@ -273,6 +270,12 @@ func TestAConditionThatBecameTheOtherKindSaysSoWhenItClears(t *testing.T) { for _, e := range told.Said() { if e.Change == conditions.ChangeCleared { resolved = append(resolved, e.Condition.Key+": "+e.Condition.Resolved) + // The clearing line is held to the plain rule too (ADR 0253). + w := conditions.Words{Headline: e.Condition.Headline, Explanation: e.Condition.Explanation, + Resolved: e.Condition.Resolved, Needs: e.Condition.Needs} + if why, ok := conditions.PlainWords(w, "laptop"); !ok { + t.Errorf("%s: its clearing words are not plain: %s", e.Condition.Key, why) + } } } } @@ -280,3 +283,30 @@ func TestAConditionThatBecameTheOtherKindSaysSoWhenItClears(t *testing.T) { t.Fatalf("cleared %v; want the not-working condition cleared as now waiting for a login", resolved) } } + +// Which moves add an account group, read from the builds' manifests (issue 318 review). +func TestOnlyAMoveThatAddsAnAccountGroupCanBringAWait(t *testing.T) { + user := func(groups ...any) catalogue.Manifest { + return catalogue.Manifest{Module: "lights", Resources: []map[string]any{{"id": "operator", "type": "user", + "name": "operator", "groups": groups}}} + } + none := catalogue.Manifest{Module: "lights"} + for _, c := range []struct { + name string + from catalogue.Manifest + had bool + to catalogue.Manifest + addsSome bool + }{ + {"a group added", none, true, user("lights"), true}, + {"the same group kept", user("lights"), true, user("lights"), false}, + {"a second group added", user("lights"), true, user("lights", "video"), true}, + {"a group taken away", user("lights", "video"), true, user("lights"), false}, + {"new to the machine, with a group", none, false, user("lights"), true}, + {"new to the machine, no group", none, false, none, false}, + } { + if got := addsAccountGroups(c.from, c.had, c.to); got != c.addsSome { + t.Errorf("%s: adds %v; want %v", c.name, got, c.addsSome) + } + } +} diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index 29e76cea..ac9967c8 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -334,6 +334,9 @@ func failCarried(ctx context.Context, open *stores, p *inventory.Plan, g *invent notes = append(notes, p.Note) } done := map[string]bool{except: true} + for _, m := range g.Returned { + done[m] = true // put back at once when it broke + } // **What passed on its own keeps its pass** (novox/hq ADR 0254, issue 318): healthy for the passes the // gate asks while another module of the send failed, it is neither put back nor left unjudged — a build // left without a verdict is one more move every other walk would wait for. @@ -390,6 +393,65 @@ func keepPassing(ctx context.Context, open *stores, p *inventory.Plan, g *invent return kept } +// putBackBroken puts back, at once, every module a gate found broken and has not put back yet (issue 318 +// review): a witness that reverted a core component, or a machine that refused or failed its send, is not +// left registered at the build that broke for as long as the modules beside it take to be judged — a resend +// meanwhile would declare it again. The send goes on judging the others to their own verdicts; its own +// verdict is still failed. lead and leadState are the module a plan's gate is kept on, when it is a plan's. +// Answers whether anything was put back. +func putBackBroken(ctx context.Context, open *stores, p *inventory.Plan, g *inventory.PlanGate, lead string, + leadState *inventory.PlanModule) bool { + var todo []string + for _, m := range g.Broken { + if !slices.Contains(g.Returned, m) { + todo = append(todo, m) + } + } + if len(todo) == 0 || g.Verdict != "" { + return false + } + stateWas, noteWas := p.State, p.Note + batched, back := batchingRollbacks(ctx) + var said []string + for _, m := range todo { + g.Returned = append(g.Returned, m) + machines := g.Machines + var state *inventory.PlanModule + if m == lead && leadState != nil { + judged := *leadState + judged.Gate = nil // its own record of the failure; the send's gate goes on judging + state = &judged + } else { + i := slices.IndexFunc(g.Carried, func(c inventory.CarriedMove) bool { return c.Module == m }) + if i < 0 { + continue + } + c := g.Carried[i] + state = &inventory.PlanModule{Build: c.Build, Previous: c.From, Commit: c.To} + if sent, err := sentTheBuild(ctx, open, m, c.To); err == nil && len(sent) > 0 { + machines = sent + } else { + machines = []string{c.Node} + } + } + p.Note = "" + gateFailed(batched, open, p, m, state, machines, g.BrokenWhy) + if m == lead && leadState != nil { + leadState.Why = "put back at once, its send still judged: " + g.BrokenWhy + } + said = append(said, m) + } + sendRollbacks(ctx, open, p, back) + p.State = stateWas + p.Note = noteWas + if len(said) > 0 { + p.Note = fmt.Sprintf("put back at once: %s (%s); the rest of the send judged to their own verdicts", + strings.Join(said, ", "), g.BrokenWhy) + fmt.Printf("%s: %s\n", p.ID, p.Note) + } + return true +} + // passedAloneWhy is the verdict of a module that passed on its own while its send failed, with its own wait. func passedAloneWhy(g *inventory.PlanGate, module string) string { why := fmt.Sprintf("healthy %d times on its own while the send failed: %s", g.Healthy[module], g.Why) @@ -617,6 +679,9 @@ func advanceRelease(ctx context.Context, open *stores, p *inventory.Plan) (bool, } switch verdict { case "": + if putBackBroken(ctx, open, p, g, "", nil) { + return true, nil + } note := fmt.Sprintf("judging %s: %s", strings.Join(g.Machines, ", "), gateLine(g)) changed := p.Note != note p.Note = note diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 7d179181..d67db4d8 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -729,6 +729,9 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, } switch verdict { case "": + if putBackBroken(ctx, open, p, state.Gate, m, state) { + return true, nil + } pending = append(pending, fmt.Sprintf("%s judged on %s: %s", m, strings.Join(state.Gate.Machines, ", "), gateLine(state.Gate))) continue @@ -961,7 +964,13 @@ func failFirstSend(ctx context.Context, open *stores, p *inventory.Plan, module fmt.Printf("%s: %s\n", p.ID, p.Note) }() g := state.Gate - if g == nil || g.Verdict == "" || len(g.Failing) == 0 || slices.Contains(g.Failing, module) { + if g != nil && slices.Contains(g.Returned, module) { + // Put back at once when it broke: its rollback was made then, and is not made again. + state.Why = "put back when it broke; its send failed: " + g.Why + p.State = inventory.PlanFailed + p.Note = fmt.Sprintf("the send to %s in tier %d failed its gate: %s; %s was put back when it broke", + strings.Join(g.Machines, ", "), p.Tier, g.Why, module) + } else if g == nil || g.Verdict == "" || len(g.Failing) == 0 || slices.Contains(g.Failing, module) { gateFailed(batched, open, p, module, state, machines, why) } else if slices.Contains(g.Passing, module) && state.Build != "" { // **The module the gate is kept on keeps its own pass too** (issue 318 review): healthy for the passes diff --git a/cmd/mesh-controller/replay318_test.go b/cmd/mesh-controller/replay318_test.go index d829a5e7..09e212d0 100644 --- a/cmd/mesh-controller/replay318_test.go +++ b/cmd/mesh-controller/replay318_test.go @@ -40,13 +40,19 @@ func TestReplay318(t *testing.T) { t.Cleanup(func() { doctorFrom = was }) build := func(module, repository, commit string, asked time.Time) { - manifest, _ := json.Marshal(catalogue.Manifest{Module: module, Version: "1"}) + m := catalogue.Manifest{Module: module, Version: "1"} + if module == "openrazer" && commit == "c78b5fc9" { + // The build that put the operator's account in the group `openrazer` (ADR 0252). + m.Resources = []map[string]any{{"id": "operator", "type": "user", "name": "operator", + "groups": []any{"openrazer"}}} + } + manifest, _ := json.Marshal(m) if err := inv.RecordBuild(ctx, inventory.Build{ID: "build-" + module + "-" + commit, Module: module, Commit: commit, Repository: repository, Path: "modules/" + module, Manifest: manifest, Asked: asked, At: asked, Made: []inventory.Artifact{{Name: "x", Kind: "bundle", Reference: "sha256:" + module + "-" + commit}}}); err != nil { t.Fatal(err) } - if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: module, Version: "1"}, inventory.Source{ + if err := inv.RegisterModule(ctx, m, inventory.Source{ Repository: repository, Seat: "git", Path: "modules/" + module, BuiltFrom: commit, Head: commit, Asked: asked}); err != nil { t.Fatal(err) diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index b3176a65..00842dd4 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -159,6 +159,8 @@ type PlanGate struct { // first reason: the send fails, and the modules beside them are judged to their own verdict first. Broken []string `json:"broken,omitempty"` BrokenWhy string `json:"broken_why,omitempty"` + // Returned names the broken modules already put back, at once, while the rest of the send is judged. + Returned []string `json:"returned,omitempty"` } // CarriedMove is one module's build moving on a machine with a gated send. -- 2.54.0