From 91d041b62a1aca0051abffee63bc0bf8fa3aa023 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 25 Oct 2026 17:07:50 +0100 Subject: [PATCH] mesh-delivery-assigner: a held module is never on neither list (issue 1945) systemd-resolved, held on shanks by the operator under rule 2, was raised as an urgent "on no list" condition and reminded every hour, although its hold is already said and answered on the held list. A node now goes on neither list only when it gets no want and the module is not held, by a hold read at its fingerprint or one this pass raises. The first pass after a start clears the neither condition of every assigned node not on neither list, not only those with a want, so a neither raised before the restart for a held module (or a move waiting for its record ask) is cleared too. --- .../cmd/mesh-delivery-assigner/pass.go | 18 +++---- .../cmd/mesh-delivery-assigner/pass_test.go | 53 ++++++++++++++++++- .../internal/assign/wants.go | 10 ++-- .../internal/assign/wants_test.go | 27 ++++++++++ 4 files changed, 93 insertions(+), 15 deletions(-) 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 fed2320..5a9402d 100644 --- a/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go +++ b/modules/mesh-delivery-assigner/cmd/mesh-delivery-assigner/pass.go @@ -100,7 +100,7 @@ func (p *passer) wants(ctx context.Context) { neither := map[string]neitherOne{} // 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 + // assigned module not on neither list 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{} @@ -140,7 +140,7 @@ 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) + startClears(stale, m, out) if fp != "" && !m.Held && out.Held == nil && !out.Lifted { p.clearHeld(name, fp) } @@ -190,15 +190,13 @@ 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) { +// startClears adds, for the first pass after a start, every node of m that is not on neither list after the pass: a neither +// condition a previous process raised for it is cleared. That covers a node with a want and one with none that is not on neither +// list, such as a held module (issue 1945: the hold is said on its own, so a neither raised for it before the restart would stay up +// for ever) or a move waiting for its record ask. Clearing a condition that is not open is quiet at the router. +func startClears(into map[string]neitherOne, m assign.Module, 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 { + if _, on := out.Neither[node]; !on { into[node+"."+m.Name] = neitherOne{node: node, module: m.Name} } } 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 d62c765..1af98fa 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 @@ -149,7 +149,7 @@ func TestThePassGathersTheModulesOnNeitherList(t *testing.T) { } // 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 +// nothing it said, so it clears, once, the neither condition of every assigned module not on neither list 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{}} @@ -185,3 +185,54 @@ func TestTheFirstPassAfterAStartClearsWhatAPreviousProcessRaised(t *testing.T) { t.Errorf("a second pass said again: %+v", said[n:]) } } + +// 1945/AC24: given a module held at F under rule 2 on a node that runs it with no want, when the assigner passes, then it raises no +// neither condition for it, a neither condition this process raised for it before is cleared, and so is one a previous process +// raised (the first pass after a start), each once. +func TestAHeldModuleRaisesNoNeitherAndClearsAStandingOne(t *testing.T) { + setup := func() (*passer, *[]asks.Condition) { + p := newPasser(buckets{delivery.AssignerWantsBucket: &memKV{values: map[string][]byte{}}, delivery.AssignerAsksBucket: &memKV{values: map[string][]byte{}}}, time.Now()) + 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/resolved", Commit: strings.Repeat("1", 40), Path: delivery.RootPath}) + p.w.put(delivery.ModulesBucket, "systemd-resolved", mod) + p.w.put(provider.AssignmentsBucket, "shanks.systemd-resolved", []byte(`{"node":"shanks","module":"systemd-resolved","build":"b-1"}`)) + on, _ := delivery.EncodePresence(delivery.Presence{Online: true}) + p.w.put(delivery.PresenceBucket, "shanks", on) + have, _ := delivery.EncodeHave(delivery.Have{FP: fpTest, Phase: delivery.PhaseRunning, Health: delivery.HealthFailed}) + p.w.put(delivery.HaveBucket, "shanks.systemd-resolved", have) + hkey, _ := delivery.HeldKey("systemd-resolved", fpTest) + held, _ := delivery.EncodeHeld(delivery.Held{Node: "shanks", Words: "health failed"}) + p.w.put(delivery.AssignerWantsBucket, hkey, held) + return p, said + } + key := assign.NeitherCondition("shanks", "systemd-resolved", assign.Holding{}, false).Key + neithers := func(said []asks.Condition) (raised, cleared int) { + for _, c := range said { + if c.Key != key { + continue + } + if c.State == asks.StateRaised { + raised++ + } else { + cleared++ + } + } + return + } + // a standing neither this process raised + p, said := setup() + p.neither = map[string]neitherOne{"shanks.systemd-resolved": {node: "shanks", module: "systemd-resolved", holding: assign.Holding{Rule: delivery.RuleNotHeld}}} + p.wants(context.Background()) + p.wants(context.Background()) + if r, c := neithers(*said); r != 0 || c != 1 { + t.Errorf("standing in this process: %d raised, %d cleared, want 0 and 1: %+v", r, c, *said) + } + // one a previous process raised: the first pass after a start clears it + p, said = setup() + p.wants(context.Background()) + p.wants(context.Background()) + if r, c := neithers(*said); r != 0 || c != 1 { + t.Errorf("after a start: %d raised, %d cleared, want 0 and 1: %+v", r, c, *said) + } +} diff --git a/modules/mesh-delivery-assigner/internal/assign/wants.go b/modules/mesh-delivery-assigner/internal/assign/wants.go index 273faf9..f49a6ee 100644 --- a/modules/mesh-delivery-assigner/internal/assign/wants.go +++ b/modules/mesh-delivery-assigner/internal/assign/wants.go @@ -166,8 +166,8 @@ type Outcome struct { Restore []string // Rules is, by node, the rule holding the node back from m.FP and why. Rules map[string]Holding - // Neither is, by node, the nodes the module is assigned to that run it (or whose have names it) and hold no want for it after this - // pass, with the rule holding the first want back: the node's own composition does not hold a module with a fingerprint of the new form, so such a module is on + // Neither is, by node, the nodes the module is assigned to that run it (or whose have names it), hold no want for it after this + // pass and are not held (a held module is on the held list), with the rule holding the first want back: the node's own composition does not hold a module with a fingerprint of the new form, so such a module is on // neither list and nothing applies it on that node (issue 1945). A module with no fingerprint of the new form goes with the node's // own composition and is never here. Neither map[string]Holding @@ -297,8 +297,10 @@ func Wants(m Module, n Nodes, wants map[string]delivery.Want, lastHealthy map[st out.Rules[node] = Holding{Rule: rule, Words: words} if !had || before.Removed { // The node runs the module, or its have names it, and gets no want for it: the module is on neither list (issue 1945), and - // the pass says so. A new assignment, with no have, waits for the rules and is on no list yet by design. - if h, there := n.Haves[node][m.Name]; there && h.Phase != delivery.PhaseRemoved { + // the pass says so. A new assignment, with no have, waits for the rules and is on no list yet by design. A held module (rule 2, + // held before or by this pass) is on the held list instead, whose hold is said and answered on its own: it is never on neither + // list (issue 1945: systemd-resolved, held on shanks by the operator, was said as urgent every hour). + if h, there := n.Haves[node][m.Name]; there && h.Phase != delivery.PhaseRemoved && !held { out.Neither[node] = Holding{Rule: rule, Words: words} } continue diff --git a/modules/mesh-delivery-assigner/internal/assign/wants_test.go b/modules/mesh-delivery-assigner/internal/assign/wants_test.go index 031121e..9372367 100644 --- a/modules/mesh-delivery-assigner/internal/assign/wants_test.go +++ b/modules/mesh-delivery-assigner/internal/assign/wants_test.go @@ -837,3 +837,30 @@ func TestAFirstWantAfterARemovedWantGoesThroughTheRecordAsk(t *testing.T) { t.Errorf("the record asks = %+v, want one for shanks at the composition it runs", out.Record) } } + +// 1945/AC23: given a module held at F under rule 2 (the operator holds systemd-resolved on shanks, 2026-10-25), or held by the +// assigner in this pass after a failed health check of F, when a node that runs it holds no want for it, then the node is not on +// neither list: a held module is on the held list, whose hold is said and answered on its own. +func TestAHeldModuleWithNoWantIsNotOnNeitherList(t *testing.T) { + for what, change := range map[string]func(*Module, *Nodes){ + "held at F": func(m *Module, _ *Nodes) { m.Held, m.Kept = true, true }, + "held at F, running an old-form build": func(m *Module, n *Nodes) { + m.Held, m.Kept = true, true + n.Haves["shanks"]["screen-lock"] = delivery.Have{FP: "build-2", Phase: delivery.PhaseRunning, Health: delivery.HealthUnknown} + }, + "held by this pass": func(_ *Module, n *Nodes) { + n.Haves["g14"]["screen-lock"] = delivery.Have{FP: fNew, Phase: delivery.PhaseRunning, Health: delivery.HealthFailed} + n.Haves["shanks"]["screen-lock"] = delivery.Have{FP: "build-2", Phase: delivery.PhaseRunning, Health: delivery.HealthUnknown} + }, + } { + m, n := x11() + change(&m, &n) + out := Wants(m, n, nil, nil) + if len(out.Neither) != 0 { + t.Errorf("%s: a held module is on neither list: %+v", what, out.Neither) + } + if out.Rules["shanks"].Rule != delivery.RuleNotHeld { + t.Errorf("%s: shanks is held back by %+v, want rule 2", what, out.Rules["shanks"]) + } + } +}