diff --git a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go index 6da9587..fed2320 100644 --- a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go +++ b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go @@ -30,7 +30,8 @@ type passer struct { asks *composeAsks // now is the clock; nil is time.Now. now func() time.Time - // neither is, by node and module, each module on neither list whose condition this process raised (issue 1945). + // neither is, by node and module, each module on neither list whose condition this process raised (issue 1945). Nil until the + // first pass after a start, which clears what a previous process may have raised (startClears). neither map[string]neitherOne } @@ -97,7 +98,13 @@ func (p *passer) wants(ctx context.Context) { names[name] = true } neither := map[string]neitherOne{} - defer p.sayNeither(neither) + // The first pass after a start remembers nothing a previous process said: a neither or held condition it raised would stay + // raised for ever once the module has its want or is no longer held. That pass clears, once, the neither condition of every + // assigned module with a want and the held condition of every module not held; a clear of a condition that is not open is + // quiet at the router. + first := p.neither == nil + stale := map[string]neitherOne{} + defer func() { p.sayNeither(neither, stale) }() for name := range names { fp := trunk[name].FP2 name := name @@ -132,6 +139,12 @@ func (p *passer) wants(ctx context.Context) { } out := assign.Wants(m, nodes, wanted[name], last) gatherNeither(neither, name, out) + if first { + startClears(stale, m, wanted[name], out) + if fp != "" && !m.Held && out.Held == nil && !out.Lifted { + p.clearHeld(name, fp) + } + } if out.Held != nil { key, err := delivery.HeldKey(name, fp) if err == nil { @@ -177,11 +190,32 @@ func gatherNeither(into map[string]neitherOne, module string, out assign.Outcome } } +// startClears adds, for the first pass after a start, every node of m that holds a want for it after the pass and is not on +// neither list: a neither condition a previous process raised for it is cleared. +func startClears(into map[string]neitherOne, m assign.Module, before map[string]delivery.Want, out assign.Outcome) { + for _, node := range m.Nodes { + w, ok := out.Wants[node] + if !ok { + w, ok = before[node] + } + if _, on := out.Neither[node]; ok && !w.Removed && !on { + into[node+"."+m.Name] = neitherOne{node: node, module: m.Name} + } + } +} + // sayNeither raises, once, the condition of every module on neither list in this pass and writes it to the journal, and clears, // once, the condition of each one said before that has a want again (issue 1945). A say that fails is made again at the next pass. -func (p *passer) sayNeither(now map[string]neitherOne) { +// stale is, on the first pass after a start, the modules whose condition a previous process may have raised and that have a want: +// each is cleared once. +func (p *passer) sayNeither(now, stale map[string]neitherOne) { if p.neither == nil { p.neither = map[string]neitherOne{} + for key, one := range stale { + if _, on := now[key]; !on { + p.neither[key] = one + } + } } for key, one := range now { if _, said := p.neither[key]; said { @@ -363,7 +397,8 @@ func (p *passer) heldCondition(ctx context.Context, module, fp string, fresh *de return } p.said[c.Key+".raised"] = true - delete(p.said, c.Key+".lifted") // a hold raised again and lifted again is said lifted again + delete(p.said, c.Key+".lifted") // a hold raised again and lifted again is said lifted again + delete(p.said, c.Key+".cleared") // and released again is cleared again } // restoreAsk writes, once, the ask about a module holding data whose held fingerprint failed its health check on node. diff --git a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go index 8ba0322..d62c765 100644 --- a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go +++ b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass_test.go @@ -12,6 +12,7 @@ import ( "git.novox.be/novox/mesh-sdk/go/asks" "git.novox.be/novox/mesh-sdk/go/delivery" + "git.novox.be/novox/mesh-sdk/go/provider" "mesh-delivery-assigner/internal/assign" ) @@ -121,17 +122,17 @@ func TestAModuleOnNeitherListIsRaisedOnceAndClearedOnce(t *testing.T) { p.logf = func(f string, a ...any) { logged = append(logged, fmt.Sprintf(f, a...)) } one := map[string]neitherOne{"g14.screen-lock": {node: "g14", module: "screen-lock", holding: assign.Holding{Rule: delivery.RuleProviders, Words: "no provider of x11-display runs healthy on g14 or at its canary"}}} - p.sayNeither(one) // the say fails - p.sayNeither(one) - p.sayNeither(one) + p.sayNeither(one, nil) // the say fails + p.sayNeither(one, nil) + p.sayNeither(one, nil) if len(said) != 1 || said[0].State != asks.StateRaised || said[0].Subject.Machine != "g14" || said[0].Subject.ID != "screen-lock" { t.Fatalf("said = %+v, want one raised condition of screen-lock on g14", said) } if !strings.Contains(strings.Join(logged, "\n"), "screen-lock") { t.Errorf("the journal does not name the module on neither list: %q", logged) } - p.sayNeither(map[string]neitherOne{}) - p.sayNeither(map[string]neitherOne{}) + p.sayNeither(map[string]neitherOne{}, nil) + p.sayNeither(map[string]neitherOne{}, nil) if len(said) != 2 || said[1].State != asks.StateCleared || said[1].Key != said[0].Key { t.Errorf("said = %+v, want the condition cleared once under its key", said) } @@ -146,3 +147,41 @@ func TestThePassGathersTheModulesOnNeitherList(t *testing.T) { t.Errorf("gathered = %+v", got) } } + +// A neither or held condition a previous process raised is cleared by the first pass after a start: a fresh pass remembers +// nothing it said, so it clears, once, the neither condition of every assigned module that has a want and the held condition of +// every module that is not held. Clearing a condition that is not open is quiet at the router. +func TestTheFirstPassAfterAStartClearsWhatAPreviousProcessRaised(t *testing.T) { + wantsKV := &memKV{values: map[string][]byte{}} + p := newPasser(buckets{delivery.AssignerWantsBucket: wantsKV, delivery.AssignerAsksBucket: &memKV{values: map[string][]byte{}}}, time.Now()) + var said []asks.Condition + p.say = func(c asks.Condition) error { said = append(said, c); return nil } + mod, _ := delivery.EncodeModule(delivery.Module{FP2: fpTest, Repository: "novox/web", Commit: strings.Repeat("1", 40), Path: delivery.RootPath}) + p.w.put(delivery.ModulesBucket, "web", mod) + p.w.put(provider.AssignmentsBucket, "g14.web", []byte(`{"node":"g14","module":"web","build":"b-1"}`)) + on, _ := delivery.EncodePresence(delivery.Presence{Online: true}) + p.w.put(delivery.PresenceBucket, "g14", on) + have, _ := delivery.EncodeHave(delivery.Have{FP: fpTest, Phase: delivery.PhaseRunning, Health: delivery.HealthOK}) + p.w.put(delivery.HaveBucket, "g14.web", have) + key, _ := delivery.WantKey("g14", "web") + want, _ := delivery.EncodeWant(delivery.Want{FP: fpTest, Composed: "sha256:" + strings.Repeat("c", 64)}) + p.w.put(delivery.AssignerWantsBucket, key, want) + p.wants(context.Background()) + cleared := map[string]bool{} + for _, c := range said { + if c.State != asks.StateCleared { + t.Errorf("the first pass raised %+v", c) + } + cleared[c.Key] = true + } + neither := assign.NeitherCondition("g14", "web", assign.Holding{}, false).Key + held := assign.HeldCondition("web", fpTest, "", "", false, false).Key + if !cleared[neither] || !cleared[held] { + t.Errorf("cleared = %v, want %s and %s", cleared, neither, held) + } + n := len(said) + p.wants(context.Background()) + if len(said) != n { + t.Errorf("a second pass said again: %+v", said[n:]) + } +} diff --git a/modules/mesh-delivery-assigner/internal/assign/wants.go b/modules/mesh-delivery-assigner/internal/assign/wants.go index 1e2d5ed..273faf9 100644 --- a/modules/mesh-delivery-assigner/internal/assign/wants.go +++ b/modules/mesh-delivery-assigner/internal/assign/wants.go @@ -262,7 +262,12 @@ func Wants(m Module, n Nodes, wants map[string]delivery.Want, lastHealthy map[st next.Backup, next.Words = before.Backup, before.Words // the answer to a failed backup stays while its target stays (ADR 0539 ยง16) } running, ran := n.Haves[node][m.Name] - if need, wait := at.mustAsk(node, before, had, ran, running.FP, running.Composed == "", d); wait { + // A removed want is no want: a first want after it goes through the record ask as a first want does. + asked, hadLive := before, had + if before.Removed { + asked, hadLive = delivery.Want{}, false + } + if need, wait := at.mustAsk(node, asked, hadLive, ran, running.FP, running.Composed == "", d); wait { if need != nil { out.Record = append(out.Record, *need) } @@ -315,7 +320,8 @@ func Wants(m Module, n Nodes, wants map[string]delivery.Want, lastHealthy map[st return out } -// failed says whether a have's health check failed once the node is done moving: phase running or failed. A failed health while it +// failed says whether a have's health check failed once the node is done moving: phase running or failed (never removed, held, or +// moving). A failed health while it // backs up, applies or checks is a step of the move, not its outcome (on novox at 15:27:36 on 2026-10-25, "health failed, phase // applying" held the controller's canary two seconds before the new build was healthy). Every healthy read (healthy, providedHealthy, // the world's lastHealthy) already asks for phase running. @@ -328,15 +334,16 @@ func runs(h delivery.Have) bool { return h.Phase == delivery.PhaseRunning } -// allRunHealthy says whether every node of m that runs the module (has a have of it) runs m.FP healthy, and at least one does: -// a hold whose fingerprint runs healthy wherever the module runs holds nothing back any more (issue 1906: seven modules stayed -// held on every machine after one transient failed check). A node set back to another fingerprint does not run m.FP, so its -// hold stays. +// allRunHealthy says whether every node of m that runs the module (its have's phase is running, or failed) runs m.FP healthy, and +// at least one does: a hold whose fingerprint runs healthy wherever the module runs holds nothing back any more (issue 1906: seven +// modules stayed held on every machine after one transient failed check). A node set back to another fingerprint does not run +// m.FP, and a failed one does not run it healthy, so the hold stays; a have that is removed, held for its backup or still moving +// (backing up, applying, checking) is not counted, or it would keep the hold up for ever. func allRunHealthy(m Module, n Nodes) bool { ran := 0 for _, node := range m.Nodes { h, ok := n.Haves[node][m.Name] - if !ok { + if !ok || (h.Phase != delivery.PhaseRunning && h.Phase != delivery.PhaseFailed) { continue } if !healthy(h, m.FP) { diff --git a/modules/mesh-delivery-assigner/internal/assign/wants_test.go b/modules/mesh-delivery-assigner/internal/assign/wants_test.go index be6c0ee..031121e 100644 --- a/modules/mesh-delivery-assigner/internal/assign/wants_test.go +++ b/modules/mesh-delivery-assigner/internal/assign/wants_test.go @@ -782,3 +782,58 @@ func TestAFailedHealthWhileTheNodeMovesHoldsNothing(t *testing.T) { } } } + +// A hold is lifted by itself once every node that runs the module runs F healthy: a have of a node that is not running the module +// (removed, held for its backup, or still moving) is not counted, so it does not keep the hold up for ever; a node running another +// fingerprint, or failed, still does. +func TestAHoldIsLiftedOverHavesThatAreNotRunning(t *testing.T) { + setup := func(phase delivery.Phase, fp string, health delivery.Health) (Module, Nodes) { + m := Module{Name: "web", FP: fNew, Built: true, Granted: true, Held: true, Composed: composedEverywhere, Nodes: []string{"ace", "g14"}} + n := Nodes{Online: map[string]bool{"ace": true, "g14": true}, Haves: map[string]map[string]delivery.Have{ + "ace": {"web": {FP: fNew, Phase: delivery.PhaseRunning, Health: delivery.HealthOK}}, + "g14": {"web": {FP: fp, Phase: phase, Health: health}}}} + return m, n + } + for _, phase := range []delivery.Phase{delivery.PhaseRemoved, delivery.PhaseHeld, delivery.PhaseApplying, delivery.PhaseBackingUp, delivery.PhaseChecking} { + m, n := setup(phase, fOld, delivery.HealthUnknown) + if out := Wants(m, n, nil, nil); !out.Lifted { + t.Errorf("g14's have in phase %s kept the hold up", phase) + } + } + for what, h := range map[string]delivery.Have{ + "running another fingerprint": {FP: fOld, Phase: delivery.PhaseRunning, Health: delivery.HealthOK}, + "failed": {FP: fNew, Phase: delivery.PhaseFailed, Health: delivery.HealthFailed}, + } { + m, n := setup(h.Phase, h.FP, h.Health) + if out := Wants(m, n, nil, nil); out.Lifted { + t.Errorf("g14 %s: the hold was lifted", what) + } + } + // a failed health on a removed have holds nothing + m, n := setup(delivery.PhaseRemoved, fNew, delivery.HealthFailed) + m.Held = false + if out := Wants(m, n, nil, nil); out.Held != nil { + t.Errorf("a removed have with a failed health held F: %+v", out.Held) + } +} + +// A module of policy record whose want was removed but which the node still runs gets its first want through the record ask: a +// removed want is no want, so a move the controller judges to recreate or move data waits for the operator's yes. +func TestAFirstWantAfterARemovedWantGoesThroughTheRecordAsk(t *testing.T) { + m, n := x11() + recording(&m) + m.Composed = func(_, fp string) (delivery.ComposedDigest, bool) { + return delivery.ComposedDigest{Digest: dig(fp), Judgement: delivery.JudgedRecreates, Policy: delivery.PolicyRecord}, true + } + out := Wants(m, n, map[string]delivery.Want{"shanks": {Removed: true}}, nil) + if w, ok := out.Wants["shanks"]; ok { + t.Errorf("shanks's first want after a removed want was written with no record ask: %+v", w) + } + asked := false + for _, r := range out.Record { + asked = asked || (r.Node == "shanks" && r.Digest == dig(fOld) && r.Judgement == delivery.JudgedRecreates) + } + if !asked { + t.Errorf("the record asks = %+v, want one for shanks at the composition it runs", out.Record) + } +}