diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index 6fe315fb..554214d9 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -871,34 +871,49 @@ func TestTheControllersOwnGrantToAskIsReadFromTheComposition(t *testing.T) { } } -// The bus module's answer, judged with the check before it as the floor (the review of PR 188): an answer -// without in_force is an older module, judged by whether the list sent to its machine is the one composed now; -// each way it is not held names its own remedy. +// The bus module's answer, with the record's word on the list sent as the check only where the answer does +// not settle it (reviews 22 and 31 of PR 188); each way it is not held names its own remedy. func TestTheBusModulesAnswerNamesTheRemedyForEachCase(t *testing.T) { + const ( + reloaded = `{"allowed":true,"in_force":true,"file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:56:05Z"}` + notLoaded = `{"allowed":true,"in_force":false,"why":"the user list on the machine allows it, and the server has not reloaded since it was written","file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:50:00Z"}` + noVarz = `{"allowed":true,"in_force":false,"why":"the server's last configuration load could not be read: refused"}` + noStat = `{"allowed":true,"in_force":false,"why":"when the bus's user list was written could not be read inside mesh-broker-nats: exit 1"}` + notAllowed = `{"allowed":false,"in_force":false,"why":"the user list on the machine does not allow it"}` + ) + current := busListState{Machine: "anchor"} + behind := busListState{Machine: "anchor", Behind: true} + stepped := busListState{Machine: "anchor", Behind: true, BusStep: "sending anchor would replace the bus; `bus upgrade --why …`"} + uncomposed := busListState{Machine: "anchor", Behind: true, Uncomposed: errors.New("module foo requires a provision nobody provides")} for _, c := range []struct { name, answer string - behind bool - behindErr error + askErr error + list busListState + listErr error held bool says, not string }{ - {"in force", `{"allowed":true,"in_force":true}`, false, nil, true, "", "push"}, - {"in force, list behind by the record", `{"allowed":true,"in_force":true}`, true, nil, true, "", "push"}, - {"written, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, - false, nil, false, "within about 5 s", "push"}, - {"not allowed", `{"allowed":false,"in_force":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, - {"in force and not allowed", `{"allowed":false,"in_force":true}`, false, nil, false, "`push anchor`", "5 s"}, - {"older module, list current", `{"allowed":true}`, false, nil, true, "not confirmed", "push"}, - {"older module, list behind", `{"allowed":true}`, true, nil, false, "`push anchor` carries it", "5 s"}, - {"older module, list not allowed", `{"allowed":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, - {"older module, list unknown", `{"allowed":true}`, false, errors.New("the store is away"), false, "`bus upgrade`", "push"}, - {"not json", `not json`, false, nil, false, "could not be read", ""}, + {"in force", reloaded, nil, behind, nil, true, "", "push"}, + {"written, not reloaded", notLoaded, nil, current, nil, false, "within about 5 s", "push"}, + {"in force not known: varz", noVarz, nil, current, nil, false, "is not known: the server's last configuration load", "wait"}, + {"in force not known: stat", noStat, nil, current, nil, false, "is not known: when the bus's user list was written", "wait"}, + {"not allowed, list behind", notAllowed, nil, behind, nil, false, "`push anchor` carries it", "5 s"}, + {"not allowed, bus step pending", notAllowed, nil, stepped, nil, false, "`bus upgrade", "`push anchor`"}, + {"not allowed, list uncomposed", notAllowed, nil, uncomposed, nil, false, "`plan anchor`", "`push anchor`"}, + {"older module, allowed, list current", `{"allowed":true}`, nil, current, nil, true, "not confirmed", "push"}, + {"older module, allowed, list behind", `{"allowed":true}`, nil, behind, nil, true, "not confirmed", "push"}, + {"older module, allowed, list unknown", `{"allowed":true}`, nil, current, errors.New("the store is away"), true, "`bus upgrade`", "push"}, + {"older module, not allowed, list behind", `{"allowed":false}`, nil, behind, nil, false, "`push anchor` carries it", "5 s"}, + {"module cannot answer, list current", "", errors.New("no responders"), current, nil, true, "could not answer", "push"}, + {"module cannot answer, list behind", "", errors.New("no responders"), behind, nil, false, "`push anchor` carries it", ""}, + {"module cannot answer, list unknown", "", errors.New("no responders"), current, errors.New("the store is away"), false, "could not be worked out", ""}, + {"answer unreadable, list behind", `not json`, nil, behind, nil, false, "could not be read", ""}, } { judge := busGrantBy( func(context.Context) (string, json.RawMessage, error) { - return "anchor", json.RawMessage(c.answer), nil + return "anchor", json.RawMessage(c.answer), c.askErr }, - func(context.Context) (string, bool, error) { return "anchor", c.behind, c.behindErr }) + func(context.Context) (busListState, error) { return c.list, c.listErr }) held, why, _ := judgeGrantOnce(judge) if held != c.held || !strings.Contains(why, c.says) || (c.not != "" && strings.Contains(why, c.not)) { t.Errorf("%s: held %v (%q)", c.name, held, why) @@ -916,23 +931,30 @@ func judgeGrantOnce(bus grantJudge) (bool, string, error) { return grantHeldBy(yes, bus, time.Now)(context.Background()) } -// The order the review of PR 188 found: this controller live before the bus module that answers in_force. An -// older module with the list sent current asks; with the list behind it asks nothing and the condition names the -// push; a new module that has not reloaded names the wait, not a push. +// The orders reviews 22 and 31 of PR 188 found: this controller live before the bus module that answers +// in_force, with the bus's machine behind on its list — and, after mesh-catalog #166 merged, unpushable until a +// person's bus upgrade. An older module that allows the ask asks (once, with the caveat said); a new module that +// has not reloaded names the wait, not a push; one that cannot say whether it reloaded names what is not known. func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing.T) { for _, c := range []struct { name, answer string - behind bool + list busListState asks bool says, not string }{ - {"older module, list current", `{"allowed":true}`, false, true, "", ""}, - {"older module, list behind", `{"allowed":true}`, true, false, "`push anchor` carries it", "5 s"}, - {"new module, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, - false, false, "within about 5 s", "push"}, + {"older module, list current", `{"allowed":true}`, busListState{Machine: "anchor"}, true, "not confirmed", ""}, + {"older module, list behind, bus step pending", `{"allowed":true}`, + busListState{Machine: "anchor", Behind: true, BusStep: "a planned step"}, true, "not confirmed", ""}, + {"new module, not reloaded", + `{"allowed":true,"in_force":false,"why":"not reloaded","file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:50:00Z"}`, + busListState{Machine: "anchor"}, false, "within about 5 s", "push"}, + {"new module, reload not known", `{"allowed":true,"in_force":false,"why":"the server's last configuration load could not be read: refused"}`, + busListState{Machine: "anchor"}, false, "is not known", "wait"}, } { t.Run(c.name, func(t *testing.T) { r := newAskerRig(t) + var said []string + r.a.logf = func(f string, a ...any) { said = append(said, fmt.Sprintf(f, a...)) } var raised [][]conditions.Observation r.a.raise = func(_ context.Context, obs []conditions.Observation) error { raised = append(raised, obs) @@ -943,15 +965,25 @@ func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing func(context.Context) (string, json.RawMessage, error) { return "anchor", json.RawMessage(c.answer), nil }, - func(context.Context) (string, bool, error) { return "anchor", c.behind, nil }), + func(context.Context) (busListState, error) { return c.list, nil }), func() time.Time { return r.now }) r.open = []conditions.Condition{heldCondition()} _ = r.a.reconcile(context.Background()) + _ = r.a.reconcile(context.Background()) sent := r.asksSent(t) if c.asks { if len(sent) != 1 || sent[0].About != heldCondition().Key { t.Fatalf("not asked: %+v", sent) } + n := 0 + for _, s := range said { + if strings.Contains(s, c.says) { + n++ + } + } + if n != 1 { + t.Errorf("the caveat said %d times: %q", n, said) + } return } if len(sent) != 0 { diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index 4b711d54..aadff952 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -336,9 +336,9 @@ func composedGrantIn(open *stores) grantJudge { } } -// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus, with the -// judgement main made before it (novox/hq issue 353, PR 186: is the user list sent to the bus's machine the one -// composed now) as the floor for a bus module too old to say whether a grant is in force. +// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus, with the check +// main made before it (novox/hq issue 353, PR 186: is the user list sent to the bus's machine the one composed +// now) kept for the cases the module's word does not settle. func busGrantIn(open *stores, conn *nats.Conn) grantJudge { ask := func(ctx context.Context) (string, json.RawMessage, error) { if open == nil || open.inventory == nil || conn == nil { @@ -362,90 +362,144 @@ func busGrantIn(open *stores, conn *nats.Conn) grantJudge { } return h.Node, a.Result, nil } - behind := func(ctx context.Context) (string, bool, error) { + list := func(ctx context.Context) (busListState, error) { if open == nil || open.inventory == nil { - return "", false, errors.New("the mesh's records are not open") + return busListState{}, errors.New("the mesh's records are not open") } - return brokerBehind(ctx, open, nil) + s, err := brokerListState(ctx, open, nil) + if err != nil || s.Machine == "" || !s.Behind { + return s, err + } + step, err := busHeld(ctx, open.inventory, []string{s.Machine}) + if err != nil { + return busListState{}, err + } + s.BusStep = step[s.Machine] + return s, nil } - return busGrantBy(ask, behind) + return busGrantBy(ask, list) +} + +// busListState is the user list on the bus's machine as the controller's record knows it: whether the list +// sent there is behind the one composed now, why it could not be composed, and the bus step a send there waits +// on (that machine cannot be pushed until a person's `bus upgrade`). +type busListState struct { + Machine string + Behind bool + Uncomposed error + BusStep string +} + +// remedy is the one thing that sends the bus's machine the list composed now: the plan's own reason when it +// cannot be composed, the bus upgrade while sends there wait on it, and otherwise a push of that machine. +func (s busListState) remedy(node string) string { + if s.Machine != "" { + node = s.Machine + } + switch { + case s.Uncomposed != nil: + return fmt.Sprintf("the user list for %s cannot be composed (%v); `plan %s` says what to fix first", + node, s.Uncomposed, node) + case s.BusStep != "": + return fmt.Sprintf("sends to %s wait on the bus's planned step, so a push will not carry it: %s", node, s.BusStep) + } + return fmt.Sprintf("`push %s` carries it", node) } // busGrantBy judges the bus's word, and names for each way it is not held the one thing that makes it held: // -// - **the list there does not grant it**: `push ` sends the list composed now; -// - **written, not reloaded**: the bus reloads it in place within about 5 s, so the remedy is to wait; -// - **the module is older than `in_force`**: the bus cannot confirm a reload, so the floor is main's check -// before it — held while the list sent there is the one composed now, `push ` while it is behind, and -// the bus upgrade (which carries a module that answers `in_force`) when even that cannot be worked out. A -// bus module too old is not a reason to ask nothing: an order of merges that leaves it so would silence -// every ask until a person's bus upgrade (the review of PR 188). +// - **in force**: held; +// - **allowed, from a module older than `in_force`**: held, with the caveat that the reload is not confirmed. +// The file allows the publish, and the bus reloads within about 5 s of a change to it, so the worst case is +// one ask refused in those seconds, which is asked again (issue 369). The list check is not consulted: +// while a bus step is pending the bus's machine cannot be pushed, so any change to the composed list would +// read as behind for good and silence every ask until a person's bus upgrade (the review of PR 188); +// - **allowed, written, not reloaded** (the module gives both times): wait about 5 s; +// - **allowed, in force not known** (the module could not read a time): what is not known, said; +// - **not allowed, or the module cannot answer**: the list check decides — the remedy that sends the list +// when it is behind (busListState.remedy), and, for a module that cannot answer, held while the list sent +// is current, as main judged it. // -// ask answers the machine holding the bus and its module's answer ("" when nothing holds the bus); behind is -// whether the list sent to that machine is behind what the mesh composes. +// ask answers the machine holding the bus and its module's answer ("" when nothing holds the bus); list is the +// record's word on the list sent to that machine. func busGrantBy(ask func(ctx context.Context) (string, json.RawMessage, error), - behind func(ctx context.Context) (string, bool, error)) grantJudge { + list func(ctx context.Context) (busListState, error)) grantJudge { return func(ctx context.Context) (bool, string, error) { - node, raw, err := ask(ctx) - if err != nil { - return false, "", err - } - if node == "" { + node, raw, askErr := ask(ctx) + if askErr == nil && node == "" { return false, "no module holds the bus, so nothing says what it enforces", nil } - answer, err := readBusAnswer(raw) - if err != nil { - return false, "", fmt.Errorf("the bus module's answer on %s could not be read: %w", node, err) + var answer busAnswer + if askErr == nil { + if answer, askErr = readBusAnswer(raw); askErr != nil { + askErr = fmt.Errorf("its answer could not be read: %w", askErr) + } } - pushIt := func(what string) string { - return fmt.Sprintf("%s, so the bus has not been given the controller's grant to ask; `push %s` carries it", - what, node) + if askErr != nil { + if node == "" { + node = "the bus's machine" + } + s, err := list(ctx) + if err != nil { + return false, "", fmt.Errorf("the bus module on %s could not answer (%v), and whether the list sent "+ + "there is the one composed now could not be worked out: %w", node, askErr, err) + } + if s.Behind { + return false, fmt.Sprintf("the bus module on %s could not answer (%v), and the user list sent there is "+ + "behind what the mesh composes; %s", node, askErr, s.remedy(node)), nil + } + return true, fmt.Sprintf("the bus module on %s could not answer (%v); judged by the user list sent there, "+ + "which is the one composed now", node, askErr), nil + } + notAllowed := func(older string) (bool, string, error) { + s, err := list(ctx) + if err != nil { + return false, "", fmt.Errorf("the user list on %s does not allow the controller to ask%s, and whether "+ + "the list sent there is the one composed now could not be worked out: %w", node, older, err) + } + if s.Behind { + return false, fmt.Sprintf("the user list on %s does not allow the controller to ask%s: it is behind "+ + "what the mesh composes; %s", node, older, s.remedy(node)), nil + } + return false, fmt.Sprintf("the user list on %s does not allow the controller to ask%s, though the list "+ + "sent there is the one composed now; %s again", node, older, s.remedy(node)), nil } switch { + case answer.InForce == nil && answer.Allowed: + return true, fmt.Sprintf("the user list on %s allows the controller to ask, but its reload is not "+ + "confirmed: the bus module there is older than the answer whether a grant is in force, and the bus "+ + "upgrade (`bus upgrade`) carries one that confirms it", node), nil case answer.InForce == nil: - const older = "the bus module there is older than the answer whether a grant is in force, so its " + - "reload is not confirmed" - if !answer.Allowed { - return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)) + - " (" + older + ")", nil - } - machine, late, err := behind(ctx) - if err != nil { - return false, "", fmt.Errorf("%s, and whether the list sent to %s is the one composed now could "+ - "not be worked out (%v); the bus upgrade (`bus upgrade`) carries a module that says", older, - node, err) - } - if late { - if machine == "" { - machine = node - } - return false, fmt.Sprintf("the bus's user list on %s is behind what the mesh composes, so the bus has "+ - "not been given the controller's grant to ask; `push %s` carries it (%s)", machine, machine, older), nil - } - return true, fmt.Sprintf("the user list on %s is the one composed now and allows the controller to ask; "+ - "%s, and the bus upgrade (`bus upgrade`) carries a module that confirms it", node, older), nil - case !*answer.InForce && answer.Allowed: - why := "the server has not reloaded since it was written" - if answer.Why != "" { - why = answer.Why - } - return false, fmt.Sprintf("the bus on %s has the controller's grant to ask written but not in force yet "+ - "(%s); the bus reloads it in place within about 5 s, so nothing is to be done but wait", node, why), nil + return notAllowed(" (the bus module there is older than the answer whether a grant is in force)") case !answer.Allowed: - return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)), nil + return notAllowed("") + case *answer.InForce: + return true, "", nil + case answer.FileWritten != "" && answer.ServerLoaded != "": + return false, fmt.Sprintf("the bus on %s has the controller's grant to ask written (%s) but has not "+ + "reloaded since (last load %s); it reloads in place within about 5 s, so nothing is to be done but wait", + node, answer.FileWritten, answer.ServerLoaded), nil } - return true, "", nil + why := answer.Why + if why == "" { + why = "it gives no reason" + } + return false, fmt.Sprintf("the user list on %s allows the controller to ask, but whether the bus enforces it "+ + "is not known: %s", node, why), nil } } // busUserCanTool is the bus module's answer to whether a user may do something, and whether it is in force. const busUserCanTool = "nats_user_can" -// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field. +// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field, +// and FileWritten and ServerLoaded are given only when it could read both times (mesh-catalog's nats InForce). type busAnswer struct { - Allowed bool `json:"allowed"` - InForce *bool `json:"in_force"` - Why string `json:"why"` + Allowed bool `json:"allowed"` + InForce *bool `json:"in_force"` + Why string `json:"why"` + FileWritten string `json:"file_written"` + ServerLoaded string `json:"server_loaded"` } func readBusAnswer(raw json.RawMessage) (busAnswer, error) { diff --git a/cmd/mesh-controller/push.go b/cmd/mesh-controller/push.go index 236b9020..8aef75c2 100644 --- a/cmd/mesh-controller/push.go +++ b/cmd/mesh-controller/push.go @@ -1084,26 +1084,33 @@ func brokerFirst(names []string, holder string, behind bool) []string { // send anywhere, and a module running there always put it in its first wave. A digest of the list // last sent is kept for this (ADR 0043: the list is composed on each push, never kept itself). func brokerBehind(ctx context.Context, open *stores, names []string) (string, bool, error) { + s, err := brokerListState(ctx, open, names) + return s.Machine, s.Behind, err +} + +// brokerListState is brokerBehind with why the list could not be composed, when it could not: the plan's own +// reason, which a remedy names rather than a push that would fail on it (the review of hq issue 353). +func brokerListState(ctx context.Context, open *stores, names []string) (busListState, error) { inv := open.inventory holders, err := seatHolders(ctx, inv) if err != nil { - return "", false, err + return busListState{}, err } h, held := holders[theBrokerSeat] if !held || h.Node == "" { - return "", false, nil + return busListState{}, nil } shelf, err := inv.Catalogue(ctx) if err != nil { - return "", false, err + return busListState{}, err } if m, known := shelf[h.Module]; !known || m.BusUsers == "" { // A holder that is sent no user list carries no grant: nothing to send first. - return "", false, nil + return busListState{}, nil } for _, n := range names { if n == h.Node { - return h.Node, false, nil + return busListState{Machine: h.Node}, nil } } // **What cannot be worked out is behind** (the review of hq issue 353). The list it would be sent cannot be @@ -1120,9 +1127,9 @@ func brokerBehind(ctx context.Context, open *stores, names []string) (string, bo } sent, err := inv.SentBusUsers(ctx, h.Node) if err != nil { - return "", false, err + return busListState{}, err } - return h.Node, listBehind(list, composeErr, sent), nil + return busListState{Machine: h.Node, Behind: listBehind(list, composeErr, sent), Uncomposed: composeErr}, nil } // listBehind is whether the bus's machine is behind: the list composed now is not the one last sent, or it