diff --git a/cmd/mesh-controller/asker.go b/cmd/mesh-controller/asker.go index 8a7233dd..4bf3b117 100644 --- a/cmd/mesh-controller/asker.go +++ b/cmd/mesh-controller/asker.go @@ -213,6 +213,8 @@ type asker struct { saidNoRouter bool saidNoGrant bool + // saidGrantCaveat is whether a grant held with a caveat was said, so it is said once. + saidGrantCaveat bool mu sync.Mutex nudged chan struct{} @@ -317,6 +319,13 @@ func (a *asker) reconcile(ctx context.Context) error { } return a.sayUnasked(ctx, unasked, "the bus does not hold the controller's grant to ask yet: "+why) } + if why != "" && !a.saidGrantCaveat { + // Held, with a caveat worth one line (an older bus module cannot confirm the reload). + a.logf("the controller asks: %s", why) + a.saidGrantCaveat = true + } else if why == "" { + a.saidGrantCaveat = false + } a.saidNoGrant = false } channels := "" diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index fc56f9af..6fe315fb 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -871,24 +871,102 @@ func TestTheControllersOwnGrantToAskIsReadFromTheComposition(t *testing.T) { } } -// The bus module's answer: in force only when it says so in so many words. -func TestTheBusModulesAnswerIsInForceOnlyWhenItSaysSo(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. +func TestTheBusModulesAnswerNamesTheRemedyForEachCase(t *testing.T) { for _, c := range []struct { - answer string - want bool - says string + name, answer string + behind bool + behindErr error + held bool + says, not string }{ - {`{"allowed":true,"in_force":true}`, true, ""}, - {`{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, false, "not reloaded"}, - {`{"allowed":false,"in_force":false}`, false, "not in force"}, - {`{"allowed":true}`, false, "older than that answer"}, - {`{"allowed":false,"in_force":true}`, false, "not allowed"}, - {`not json`, false, "could not be read"}, + {"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", ""}, } { - if got, why := grantInForce(json.RawMessage(c.answer)); got != c.want || !strings.Contains(why, c.says) { - t.Errorf("%s: %v %q", c.answer, got, why) + judge := busGrantBy( + func(context.Context) (string, json.RawMessage, error) { + return "anchor", json.RawMessage(c.answer), nil + }, + func(context.Context) (string, bool, error) { return "anchor", c.behind, c.behindErr }) + 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) } } + none := busGrantBy(func(context.Context) (string, json.RawMessage, error) { return "", nil, nil }, nil) + if held, why, _ := judgeGrantOnce(none); held || !strings.Contains(why, "no module holds the bus") { + t.Errorf("no bus: held %v (%q)", held, why) + } +} + +// judgeGrantOnce is the whole judgement with the composed grant held, as the asker makes it. +func judgeGrantOnce(bus grantJudge) (bool, string, error) { + yes := func(context.Context) (bool, string, error) { return true, "", nil } + 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. +func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing.T) { + for _, c := range []struct { + name, answer string + behind bool + 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"}, + } { + t.Run(c.name, func(t *testing.T) { + r := newAskerRig(t) + var raised [][]conditions.Observation + r.a.raise = func(_ context.Context, obs []conditions.Observation) error { + raised = append(raised, obs) + return nil + } + yes := func(context.Context) (bool, string, error) { return true, "", nil } + r.a.grantHeld = grantHeldBy(yes, busGrantBy( + 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() time.Time { return r.now }) + r.open = []conditions.Condition{heldCondition()} + _ = 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) + } + return + } + if len(sent) != 0 { + t.Fatalf("asked while the bus does not hold the grant: %+v", sent) + } + if len(raised) == 0 || len(raised[len(raised)-1]) != 1 || + raised[len(raised)-1][0].Kind != "asks-undelivered" { + t.Fatalf("not said as undelivered: %+v", raised) + } + summary := raised[len(raised)-1][0].Summary + if !strings.Contains(summary, c.says) || (c.not != "" && strings.Contains(summary, c.not)) { + t.Errorf("the condition names the wrong remedy: %q", summary) + } + }) + } } // The serving asker is wired to the real judgement: with records and a bus it cannot read, the grant is not diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index 6c34aa9d..4b711d54 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -272,8 +272,10 @@ func grantHeldBy(composed, inForce grantJudge, now func() time.Time) func(ctx co } } -// judgeGrant is held only when both facts hold; an error is a fact not known, never a yes. +// judgeGrant is held only when both facts hold; an error is a fact not known, never a yes. A fact that holds +// with a caveat (a reload not confirmed) gives that caveat as the why of a yes. func judgeGrant(ctx context.Context, composed, inForce grantJudge) (bool, string) { + var caveat []string for _, fact := range []struct { judge grantJudge what string @@ -288,8 +290,11 @@ func judgeGrant(ctx context.Context, composed, inForce grantJudge) (bool, string if !ok { return false, why } + if why != "" { + caveat = append(caveat, why) + } } - return true, "" + return true, strings.Join(caveat, "; ") } // controllerMayAsk says whether the controller's own grant, as composed from these records, lets it publish its @@ -331,62 +336,122 @@ func composedGrantIn(open *stores) grantJudge { } } -// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus. +// 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. func busGrantIn(open *stores, conn *nats.Conn) grantJudge { - return func(ctx context.Context) (bool, string, error) { + ask := func(ctx context.Context) (string, json.RawMessage, error) { if open == nil || open.inventory == nil || conn == nil { - return false, "", errors.New("the mesh's records or the bus are not open") + return "", nil, errors.New("the mesh's records or the bus are not open") } holders, err := seatHolders(ctx, open.inventory) if err != nil { - return false, "", err + return "", nil, err } h, held := holders[theBrokerSeat] if !held || h.Node == "" || h.Module == "" { - return false, "no module holds the bus, so nothing says what it enforces", nil + return "", nil, nil } a, err := link.AskModuleToolOn(ctx, conn, h.Module, busUserCanTool, h.Node, map[string]any{ "user": broker.ControllerName, "action": "publish", "subject": asks.AskSubject(askerName)}, 15*time.Second) if err != nil { - return false, "", err + return h.Node, nil, err } if a.Error != "" { - return false, "", errors.New(a.Error) + return h.Node, nil, errors.New(a.Error) } - inForce, why := grantInForce(a.Result) - if !inForce { - why = fmt.Sprintf("the bus on %s does not enforce the controller's grant to ask yet: %s; `push %s` "+ - "carries it, and the bus reloads it in place", h.Node, why, h.Node) + return h.Node, a.Result, nil + } + behind := func(ctx context.Context) (string, bool, error) { + if open == nil || open.inventory == nil { + return "", false, errors.New("the mesh's records are not open") } - return inForce, why, nil + return brokerBehind(ctx, open, nil) + } + return busGrantBy(ask, behind) +} + +// 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). +// +// 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. +func busGrantBy(ask func(ctx context.Context) (string, json.RawMessage, error), + behind func(ctx context.Context) (string, bool, error)) grantJudge { + return func(ctx context.Context) (bool, string, error) { + node, raw, err := ask(ctx) + if err != nil { + return false, "", err + } + if 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) + } + 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) + } + switch { + 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 + case !answer.Allowed: + return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)), nil + } + return true, "", 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" -// grantInForce reads the bus module's answer: in force only when it says so, in so many words. An answer -// without `in_force` is a bus module older than the question, and is not a yes. -func grantInForce(raw json.RawMessage) (bool, string) { - var answer struct { - Allowed bool `json:"allowed"` - InForce *bool `json:"in_force"` - Why string `json:"why"` - } - if err := json.Unmarshal(raw, &answer); err != nil { - return false, "the bus module's answer could not be read" - } - switch { - case answer.InForce == nil: - return false, "the bus module does not say whether a grant is in force (it is older than that answer)" - case !*answer.InForce && answer.Why != "": - return false, answer.Why - case !*answer.InForce: - return false, "the bus module says it is not in force" - case !answer.Allowed: - return false, "the bus module says it is in force and not allowed" - } - return true, "" +// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field. +type busAnswer struct { + Allowed bool `json:"allowed"` + InForce *bool `json:"in_force"` + Why string `json:"why"` +} + +func readBusAnswer(raw json.RawMessage) (busAnswer, error) { + var answer busAnswer + err := json.Unmarshal(raw, &answer) + return answer, err } // grantLookEvery is how often the bus's user list is judged against the one its machine was last sent.