diff --git a/go/buildstep/askrun.go b/go/buildstep/askrun.go index 496019b..06156e1 100644 --- a/go/buildstep/askrun.go +++ b/go/buildstep/askrun.go @@ -25,7 +25,7 @@ type StepFunc func(ctx context.Context, args map[string]any) (any, error) // AskSteps are the step tools a build module runs an ask with. Copy is the image module's alone; Digest the Go module's. type AskSteps struct { Fetch, Build, Copy, Digest, Test StepFunc - // Toolchain is the toolchain the module builds with now; a build ask naming another is refused. Nil checks nothing + // Toolchain is the toolchain the module builds with now; a build ask naming another is declined (Declining). Nil checks nothing // (an image or an archive names none). Toolchain func(ctx context.Context) (delivery.ToolchainUsed, error) // Kept looks up a kept verdict by level and digest (the checker's kept bucket, delivery.KeptKey): whether one is @@ -107,19 +107,32 @@ func (s AskSteps) fetch(ctx context.Context, args map[string]any) (json.RawMessa return r, f, nil } +// onToolchain is the toolchain this node builds with when the ask names one and this module reports its own, nil when +// either is absent. A node on another toolchain than the ask names answers a Declining: it is not on the agreed one and +// sits out, for a node that is to take the ask (novox/hq ADR 0584 clause 2). +func (s AskSteps) onToolchain(ctx context.Context, want *delivery.ToolchainUsed) (*delivery.ToolchainUsed, error) { + if want == nil || s.Toolchain == nil { + return nil, nil + } + tc, err := s.Toolchain(ctx) + if err != nil { + return nil, err + } + if tc.Version != want.Version || tc.Recipe != want.Recipe { + return nil, &Declining{Lacks: []string{fmt.Sprintf("the agreed toolchain %s (recipe %s)", want.Version, want.Recipe)}, + Text: fmt.Sprintf("the ask is for %s (recipe %s) and this node builds with %s (recipe %s): it sits out, and the ask is left to a node on the agreed toolchain", + want.Version, want.Recipe, tc.Version, tc.Recipe)} + } + return &tc, nil +} + func (s AskSteps) build(ctx context.Context, job delivery.BuildJob) (any, error) { answer := delivery.BuildAnswer{} - if job.Toolchain != nil && s.Toolchain != nil { - tc, err := s.Toolchain(ctx) - if err != nil { - return nil, err - } - if tc.Version != job.Toolchain.Version || tc.Recipe != job.Toolchain.Recipe { - return nil, Refuse("the ask is for %s (recipe %s) and this node builds with %s (recipe %s)", - job.Toolchain.Version, job.Toolchain.Recipe, tc.Version, tc.Recipe) - } - answer.Toolchain = &tc + tc, err := s.onToolchain(ctx, job.Toolchain) + if err != nil { + return nil, err } + answer.Toolchain = tc var f fetched defer func() { s.release(f.Workspace) }() for _, a := range job.Artifacts { @@ -181,6 +194,9 @@ func (s AskSteps) build(ctx context.Context, job delivery.BuildJob) (any, error) } func (s AskSteps) check(ctx context.Context, job delivery.CheckJob) (any, error) { + if _, err := s.onToolchain(ctx, job.Toolchain); err != nil { + return nil, err + } r, f, err := s.fetch(ctx, job.Fetch) if err != nil { return nil, err diff --git a/go/buildstep/askrun_test.go b/go/buildstep/askrun_test.go index a21dd88..30d4072 100644 --- a/go/buildstep/askrun_test.go +++ b/go/buildstep/askrun_test.go @@ -103,7 +103,7 @@ build {"artifact":"lib","module":"modules/m","publish":true,"workspace":"/w/1"}` } } -// A build ask for another toolchain is refused, so its step ends refused and the asker reads why. +// A build ask for another toolchain is declined (ADR 0584 clause 2), so the node sits out and the asker puts it again. func TestRunAskRefusesAnotherToolchain(t *testing.T) { r := &recorder{fetch: map[string]any{"workspace": "/w"}} s := r.steps() @@ -112,10 +112,16 @@ func TestRunAskRefusesAnotherToolchain(t *testing.T) { } _, err := RunAsk(context.Background(), s, ask(delivery.BuildJob{Module: "m", Path: ".", Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}, Artifacts: []delivery.AskArtifact{{Artifact: "bundle"}}, Toolchain: &delivery.ToolchainUsed{Version: "go1.26", Recipe: "3"}})) - var ref *Refusal - if !errors.As(err, &ref) || len(r.calls) != 0 { + var dec *Declining + if !errors.As(err, &dec) || len(dec.Lacks) != 1 || len(r.calls) != 0 { t.Fatalf("err %v, calls %v", err, r.calls) } + // A check ask names the toolchain too (ADR 0584), and a node on another declines it before it fetches. + _, err = RunAsk(context.Background(), s, ask(delivery.CheckJob{Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}, Levels: []string{delivery.LevelUnit}, + Toolchain: &delivery.ToolchainUsed{Version: "go1.26", Recipe: "3"}})) + if !errors.As(err, &dec) || len(r.calls) != 0 { + t.Fatalf("a check ask: err %v, calls %v", err, r.calls) + } } // 1887/A3, A5a: a check ask runs the levels of one check after one fetch: it digests the unit level, takes the kept diff --git a/go/buildstep/pull.go b/go/buildstep/pull.go index da1e540..b1cf7f3 100644 --- a/go/buildstep/pull.go +++ b/go/buildstep/pull.go @@ -538,6 +538,28 @@ func (p *Puller) decline(msg *Pulled, ask delivery.StepAsk, lacks []string) { p.mu.Unlock() } +// declineRun ends the step of an ask that began and then found this node should not run it (Declining), as decline does +// before it begins: refused as declined, the message dropped for its asker to put again, and the worker it came from left +// alone until the next pull's wait. +func (p *Puller) declineRun(msg *Pulled, ask delivery.StepAsk, key string, step delivery.Step, d *Declining) { + result, _ := json.Marshal(delivery.StepDeclined{Declined: true, Needs: ask.Needs, Lacks: d.Lacks}) + step.Ended = now().UTC() + step.State, step.Reason, step.Result = delivery.StepRefused, d.Text, result + if err := writeEnd(p.Put, key, step); err != nil { + p.logf("the ask %s cannot be declined on %s, so it is given back: %v", ask.ID, p.Node, err) + _ = msg.Nak() + return + } + p.logf("the ask %s is declined on %s: %s", ask.ID, p.Node, d.Text) + _ = msg.Term() + p.mu.Lock() + if p.declined == nil { + p.declined = map[string]bool{} + } + p.declined[msg.Kind] = true + p.mu.Unlock() +} + func withKind(msg *Pulled, kind string) *Pulled { if msg != nil { msg.Kind = kind @@ -625,6 +647,11 @@ func (p *Puller) run(ctx context.Context, msg *Pulled, ask delivery.StepAsk, key }) step = ph.end() // its end keeps the phase it ended in close(done) + var dec *Declining + if errors.As(err, &dec) && ctx.Err() == nil { + p.declineRun(msg, ask, key, step, dec) + return + } if ctx.Err() != nil { // The process stops: the ask is neither ended nor acknowledged here, so the stream gives it to a node that runs. return diff --git a/go/buildstep/pull_test.go b/go/buildstep/pull_test.go index 24656f6..d50f28c 100644 --- a/go/buildstep/pull_test.go +++ b/go/buildstep/pull_test.go @@ -230,6 +230,21 @@ func TestPullDropsWhatIsNoAsk(t *testing.T) { Run: func(ctx context.Context, a delivery.StepAsk) (any, error) { return nil, nil }} go q.Pull(ctx) waitFor(t, "the placed ask acked where its need is met", func() bool { a, _, _ := has.Fates(); return len(a) == 1 }) + // A run that finds its node is not on the agreed toolchain declines the ask after it began (ADR 0584 clause 2); a node on it builds. + sit, sitSteps := NewFakeAsks(), newSteps() + toolchain := testAsk(delivery.AskDelivery) + _ = sit.PublishAsk(toolchain) + s2 := &Puller{Source: sit, Language: delivery.AskGo, Node: "ace", Put: sitSteps.put, Capacity: func() int { return 1 }, PullWait: 10 * time.Millisecond, + Run: func(ctx context.Context, a delivery.StepAsk) (any, error) { + return nil, &Declining{Text: "the ask is for 26.13.0 and this node builds with 26.12.1", Lacks: []string{"the agreed toolchain 26.13.0 (recipe 3)"}} + }} + go s2.Pull(ctx) + waitFor(t, "the ask termed on the node that sits out", func() bool { _, tm, _ := sit.Fates(); return len(tm) == 1 }) + sk, _ := delivery.AskStepKey("ace", toolchain.ID) + ss, _ := sitSteps.get(sk) + if d, ok := delivery.DeclinedOf(ss); !ok || len(d.Lacks) != 1 || !strings.Contains(ss.Reason, "26.12.1") { + t.Fatalf("the sitting-out step = %+v", ss) + } } // load reads the recorded steps state's raw values: the pause key is not a step, so it is kept apart. diff --git a/go/buildstep/refusal.go b/go/buildstep/refusal.go index dcf0c48..9e4fa73 100644 --- a/go/buildstep/refusal.go +++ b/go/buildstep/refusal.go @@ -16,3 +16,14 @@ func (r *Refusal) Error() string { return r.Text } // Refuse is a Refusal with its text formatted. func Refuse(format string, args ...any) *Refusal { return &Refusal{Text: fmt.Sprintf(format, args...)} } + +// Declining is a step a node does not run because another node should: it ends refused as declined (delivery.StepDeclined), +// the ask's message is dropped, and the asker puts the ask again under a new id (issue 890). A build ask naming a +// toolchain other than this node's is one (novox/hq ADR 0584 clause 2): a node that is not in the toolchain majority +// sits out and never fails a build. +type Declining struct { + Text string + Lacks []string // what this node lacks, in words the operator reads +} + +func (d *Declining) Error() string { return d.Text } diff --git a/go/delivery/buildrun/standing_test.go b/go/delivery/buildrun/standing_test.go index e60b5a6..6a55ef4 100644 --- a/go/delivery/buildrun/standing_test.go +++ b/go/delivery/buildrun/standing_test.go @@ -91,8 +91,8 @@ func TestToolchainGate(t *testing.T) { gate, set, said := start(t, true) key, value := gateReport(t, "ace", "v26.12.0", old) gate.Toolchain(NodeBuildModule, key, value, false) - if _, none := agreedOf(set); !strings.HasPrefix(none, "builders disagree") { - t.Fatalf("before the placement is settled every report counts; got %q", none) + if a := delivery.ToolchainAgreement(set.Entries("typescript")); len(a.SittingOut) != 1 || a.SittingOut[0].Node != "ace" { // ADR 0584: the majority is agreed, ace's report counts as the one that sits out + t.Fatalf("before the placement is settled every report counts; got %+v", a) } k, v := placing(t, "ace", GoBuildModule) // another build module on ace: no matter gate.Placement(k, v, false, old) @@ -111,8 +111,8 @@ func TestToolchainGate(t *testing.T) { // Placed on ace with a report written since: it counts, and the builders really disagree. k, v = placing(t, "ace", GoBuildModule, NodeBuildModule) gate.Placement(k, v, false, old.Add(-time.Minute)) - if _, none := agreedOf(set); !strings.HasPrefix(none, "builders disagree: v26.12.0 recipe f59571cf4be75adf on ace") { - t.Fatalf("builders that run different versions still disagree; got %q", none) + if a := delivery.ToolchainAgreement(set.Entries("typescript")); a.Toolchain.Version != "v26.12.1" || len(a.SittingOut) != 1 || a.SittingOut[0].Version != "v26.12.0" { + t.Fatalf("builders that run different versions: the majority is agreed and ace sits out; got %+v", a) } // Taken off ace again (its key deleted): its report goes again. if !gate.Placement(k, nil, true, placed) { diff --git a/go/delivery/stepasks.go b/go/delivery/stepasks.go index 17ec656..155a685 100644 --- a/go/delivery/stepasks.go +++ b/go/delivery/stepasks.go @@ -369,6 +369,9 @@ type CheckJob struct { // has to list them. The build step hands them to its test tool as the argument NodeNamesArg, and the build module // gives them to the tests in NodeNamesEnv. NodeNames []string `json:"node-names,omitempty"` + // Toolchain is the toolchain the checker agreed for the check's language (novox/hq ADR 0584): a node building with + // another declines the ask, so a pass is never recorded as run on a version it did not run on. Nil names none. + Toolchain *ToolchainUsed `json:"toolchain,omitempty"` } // NodeNamesArg is the test tool's argument that carries a check's node names, and NodeNamesEnv the environment diff --git a/go/delivery/toolchains.go b/go/delivery/toolchains.go index 4fa2361..ecaebe4 100644 --- a/go/delivery/toolchains.go +++ b/go/delivery/toolchains.go @@ -99,22 +99,47 @@ type NodeToolchain struct { Toolchain } -// AgreedToolchain is the one Toolchain2 every node's entry of a language agrees on, or none and why: "toolchain not -// yet reported" with no entries, and "builders disagree" naming each differing version and recipe with the nodes -// reporting it when two differ (novox/hq ADR 0405 clause 2). The answer does not depend on the entries' order. The -// entries must all be of one language. -func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) { +// SittingOut is a node whose toolchain of a language is not the agreed one: it builds nothing of that language until +// it reports the agreed version (novox/hq ADR 0584 clause 2). +type SittingOut struct { + Node string + Version string + Recipe string +} + +// Agreement is what the nodes' entries of one language come to (novox/hq ADR 0584). Toolchain is the agreed one, set +// when Wait is empty. Wait is why nothing is agreed: "toolchain not yet reported" with no entries, or "builders +// disagree" naming each version and recipe with the nodes reporting it, when no version has a strict majority. +// SittingOut lists, sorted by node, the nodes that report another version than the agreed one. +type Agreement struct { + Toolchain Toolchain2 + SittingOut []SittingOut + Wait string +} + +// ToolchainAgreement is the agreed toolchain of a language: the version and recipe a strict majority of the +// reporting nodes report (novox/hq ADR 0584 clause 1). With no strict majority (two against two) or no entry it +// gives no toolchain and says why in Wait. The answer does not depend on the entries' order. The entries must all be +// of one language. +func ToolchainAgreement(entries []NodeToolchain) Agreement { if len(entries) == 0 { - return Toolchain2{}, ToolchainNoReport + return Agreement{Wait: ToolchainNoReport} } groups := map[Toolchain2][]string{} for _, e := range entries { k := Toolchain2{Language: e.Language, Version: e.Version, Recipe: e.Recipe} groups[k] = append(groups[k], e.Node) } - if len(groups) == 1 { - for k := range groups { - return k, "" + for k, nodes := range groups { + if 2*len(nodes) > len(entries) { + a := Agreement{Toolchain: k} + for _, e := range entries { + if e.Version != k.Version || e.Recipe != k.Recipe { + a.SittingOut = append(a.SittingOut, SittingOut{Node: e.Node, Version: e.Version, Recipe: e.Recipe}) + } + } + sort.Slice(a.SittingOut, func(i, j int) bool { return a.SittingOut[i].Node < a.SittingOut[j].Node }) + return a } } parts := make([]string, 0, len(groups)) @@ -123,5 +148,12 @@ func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) { parts = append(parts, fmt.Sprintf("%s recipe %s on %s", k.Version, k.Recipe, strings.Join(nodes, ", "))) } sort.Strings(parts) - return Toolchain2{}, "builders disagree: " + strings.Join(parts, "; ") + return Agreement{Wait: "builders disagree: " + strings.Join(parts, "; ")} +} + +// AgreedToolchain is ToolchainAgreement's toolchain, or none and why (its Wait): the version a strict majority of the +// nodes report (novox/hq ADR 0584, which changes ADR 0405 clause 2 and ADR 0477 clause 2). +func AgreedToolchain(entries []NodeToolchain) (tc Toolchain2, none string) { + a := ToolchainAgreement(entries) + return a.Toolchain, a.Wait } diff --git a/go/delivery/toolchains_test.go b/go/delivery/toolchains_test.go index 5b19b13..36fa002 100644 --- a/go/delivery/toolchains_test.go +++ b/go/delivery/toolchains_test.go @@ -55,36 +55,50 @@ func TestToolchainKeyAndValue(t *testing.T) { } } -// 1443/AC2 -func TestAgreedToolchain(t *testing.T) { - tc, none := AgreedToolchain([]NodeToolchain{tcEntry("ace", "go", "1.26.1", "3"), tcEntry("novox", "go", "1.26.1", "3")}) - if tc != (Toolchain2{Language: "go", Version: "1.26.1", Recipe: "3"}) || none != "" { - t.Errorf("%+v %q", tc, none) - } -} - -// 1443/AC3 -func TestAgreedToolchainNoReport(t *testing.T) { - tc, none := AgreedToolchain(nil) - if tc != (Toolchain2{}) || none != "toolchain not yet reported" { - t.Errorf("%+v %q", tc, none) - } -} - -// 1443/AC4 -func TestAgreedToolchainDisagree(t *testing.T) { - a, b, c := tcEntry("ace", "go", "1.26.1", "3"), tcEntry("novox", "go", "1.26.2", "3"), tcEntry("g14", "go", "1.26.1", "4") - _, first := AgreedToolchain([]NodeToolchain{a, b, c}) - _, second := AgreedToolchain([]NodeToolchain{c, b, a}) - if first == "" || first != second { - t.Errorf("order changed the answer: %q vs %q", first, second) - } - for _, want := range []string{"builders disagree", "1.26.1", "1.26.2", "ace", "novox", "g14"} { - if !strings.Contains(first, want) { - t.Errorf("%q does not name %q", first, want) +// 1443/AC2-4, ADR 0584: the agreed toolchain is the strict majority's; a minority node sits out; a tie waits. +func TestToolchainAgreement(t *testing.T) { + e := tcEntry + for _, c := range []struct { + name string + in []NodeToolchain + version string + out []SittingOut + wait []string // "" wait means agreed + }{ + {"all agree", []NodeToolchain{e("ace", "go", "1.26.1", "3"), e("novox", "go", "1.26.1", "3")}, "1.26.1", nil, nil}, + {"three against one", []NodeToolchain{e("ace", "typescript", "26.12.1", "3"), e("g14", "typescript", "26.13.0", "3"), e("novox", "typescript", "26.13.0", "3"), e("shanks", "typescript", "26.13.0", "3")}, + "26.13.0", []SittingOut{{"ace", "26.12.1", "3"}}, nil}, + {"two against two", []NodeToolchain{e("ace", "go", "1.26.1", "3"), e("novox", "go", "1.26.2", "3"), e("g14", "go", "1.26.1", "3"), e("shanks", "go", "1.26.2", "3")}, + "", nil, []string{"builders disagree", "1.26.1 recipe 3 on ace, g14", "1.26.2 recipe 3 on novox, shanks"}}, + {"one node alone", []NodeToolchain{e("ace", "go", "1.26.1", "3")}, "1.26.1", nil, nil}, + {"no report", nil, "", nil, []string{ToolchainNoReport}}, + } { + a := ToolchainAgreement(c.in) + rev := make([]NodeToolchain, len(c.in)) + for i, x := range c.in { + rev[len(c.in)-1-i] = x + } + if b := ToolchainAgreement(rev); b.Wait != a.Wait || b.Toolchain != a.Toolchain { + t.Errorf("%s: order changed the answer: %+v vs %+v", c.name, a, b) + } + if a.Toolchain.Version != c.version || len(a.SittingOut) != len(c.out) { + t.Errorf("%s: %+v", c.name, a) + } + for i := range c.out { + if a.SittingOut[i] != c.out[i] { + t.Errorf("%s: sitting out %+v, want %+v", c.name, a.SittingOut, c.out) + } + } + if (len(c.wait) == 0) != (a.Wait == "") { + t.Errorf("%s: wait %q", c.name, a.Wait) + } + for _, w := range c.wait { + if !strings.Contains(a.Wait, w) { + t.Errorf("%s: %q does not name %q", c.name, a.Wait, w) + } + } + if tc, none := AgreedToolchain(c.in); tc != a.Toolchain || none != a.Wait { + t.Errorf("%s: AgreedToolchain %+v %q", c.name, tc, none) } } - if tc, _ := AgreedToolchain([]NodeToolchain{a, b}); tc != (Toolchain2{}) { - t.Errorf("a disagreement gave %+v", tc) - } }