diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go index 3dcd257..cca65e1 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/integration_test.go @@ -56,10 +56,10 @@ type fakeBuildModules struct { failBuild atomic.Bool // refusePush makes every bundle and archive build fail as a push the artifact store refused (ADR 0498 clause 2). refusePush atomic.Bool - mu sync.Mutex - calls map[string]int // by `.` - fetches []map[string]any - builds []map[string]any // the arguments of every build call + mu sync.Mutex + calls map[string]int // by `.` + fetches []map[string]any + builds []map[string]any // the arguments of every build call } func (f *fakeBuildModules) count(tool string) int { @@ -121,7 +121,7 @@ func (f *fakeBuildModules) serve(t *testing.T, nc *nats.Conn, module string) { merge = `,"merge":"` + strings.Repeat("9", 40) + `"` } step.State, step.Result = delivery.StepPassed, json.RawMessage(`{"workspace":"w`+id+`","commit":"`+commit+`"`+merge+`}`) - case f.failBuild.Load() && node == "n1": + case f.failBuild.Load() && tool != "build_image_copy": // every node's build fails: one node builds a fingerprint step.State, step.Reason = delivery.StepFailed, fmt.Sprintf("go build of %v: boom", args["artifact"]) case tool == "build_image_copy": // the upstream copy answers the digest and the registry's reference (ADR 0498 clause 4) d := digestOf(fmt.Sprint(args["artifact"])) @@ -374,7 +374,7 @@ func isState(state string) func(delivery.BuilderBuild) bool { // TestBuilderMakesADoneKeyWithTheDigests: modules.netcheck (a Go and a TypeScript bundle) and its manifest are in the // indexer's buckets, and two nodes report each build module's toolchain; the program writes builds.netcheck. done, -// the digests of both bundles, both build modules' toolchains, after a fetch and a build on each of the two nodes. +// the digests of both bundles, both build modules' toolchains, after a fetch and a build on one node (ADR 0478 clause 4). func TestBuilderMakesADoneKeyWithTheDigests(t *testing.T) { // 1607/AC7 b := startBed(t) digest := b.module(t, "netcheck", "catalogue-a67f0196-netcheck.module.json", "modules/netcheck") @@ -391,9 +391,9 @@ func TestBuilderMakesADoneKeyWithTheDigests(t *testing.T) { // 1607/AC7 if v.Toolchains["go"].Version != "go1.27.1" || v.Toolchains["typescript"].Version != "tsc5" { t.Errorf("toolchains = %+v", v.Toolchains) } - for tool, want := range map[string]int{"mesh-build-go.build_go_fetch": 2, "mesh-build-go.build_go_build": 2, "mesh-build-node.build_node_fetch": 2, "mesh-build-node.build_node_build": 2} { + for tool, want := range map[string]int{"mesh-build-go.build_go_fetch": 1, "mesh-build-go.build_go_build": 1, "mesh-build-node.build_node_fetch": 1, "mesh-build-node.build_node_build": 1} { if got := b.fake.count(tool); got != want { - t.Errorf("%s called %d times, want %d (once on each of two nodes)", tool, got, want) + t.Errorf("%s called %d times, want %d (on one node, ADR 0478 clause 4)", tool, got, want) } } } @@ -410,8 +410,8 @@ func TestTwoBuilderInstancesStartingTogetherEndWithOneDoneKey(t *testing.T) { // if len(v.Artifacts) != 1 || v.Artifacts[0].Kind != "archive" || v.Artifacts[0].Digest != digestOf("files") { t.Fatalf("build = %+v, want the archive's digest", v) } - if n := b.fake.count("mesh-build-go.build_go_build"); n < 2 || n%2 != 0 { // each instance that built, on two nodes - t.Errorf("build_go_build was called %d times, want two for each instance that built", n) + if n := b.fake.count("mesh-build-go.build_go_build"); n < 1 { // each instance that built, on one node + t.Errorf("build_go_build was called %d times, want one for each instance that built", n) } } @@ -423,32 +423,7 @@ func TestAnArchiveOnlyModuleNamingNoToolchainIsBuilt(t *testing.T) { b.start(t, "n1", nil) v := b.await(t, "ca-trust", isState(delivery.BuildDone)) if len(v.Toolchains) != 0 || len(v.Artifacts) != 1 || strings.HasPrefix(v.Step, "one node (") { - t.Errorf("build = %+v, want done on two nodes naming no toolchain", v) - } -} - -// TestAKeyBuiltOnOneNodeIsBuiltAgainWhenASecondNodeComesOnline: n2 is offline, so the key is done on n1 alone and marked; -// n2 coming online makes the builder build it again on both nodes, as a new revision (ADR 0478 clause 4). -func TestAKeyBuiltOnOneNodeIsBuiltAgainWhenASecondNodeComesOnline(t *testing.T) { - b := startBed(t) - b.setOnline(t, "n2", false) - b.archive(t, "ca-trust", "modules/ca-trust", commit) - b.start(t, "n1", nil) - one := b.await(t, "ca-trust", isState(delivery.BuildDone)) - if !strings.HasPrefix(one.Step, "one node (") { - t.Fatalf("build = %+v, want it marked as built on one node", one) - } - key := mustKey(t, "ca-trust") - e, err := b.builds.Get(b.ctx, key) - if err != nil { - t.Fatal(err) - } - b.setOnline(t, "n2", true) - two := b.await(t, "ca-trust", func(v delivery.BuilderBuild) bool { - return v.State == delivery.BuildDone && !strings.HasPrefix(v.Step, "one node (") - }) - if e2, _ := b.builds.Get(b.ctx, key); e2.Revision() <= e.Revision() || !strings.Contains(two.Step, "n1 and n2") { - t.Errorf("build = %+v at revision %d (was %d), want a new revision built on n1 and n2", two, e2.Revision(), e.Revision()) + t.Errorf("build = %+v, want done naming no toolchain", v) } } @@ -459,7 +434,7 @@ func TestANodeWhoseBuildModuleIsUnhealthyIsNotUsed(t *testing.T) { b.archive(t, "ca-trust", "modules/ca-trust", commit) b.start(t, "n1", nil) v := b.await(t, "ca-trust", isState(delivery.BuildDone)) - if !strings.Contains(v.Step, "on one node, n1") { + if !strings.Contains(v.Step, "on n1") { t.Errorf("build = %+v, want it built on n1 alone", v) } } @@ -473,7 +448,7 @@ func TestAFailedKeyIsBuiltAgainAsANewRevisionFromANewModulesValueWithTheSameFing b.archive(t, "ca-trust", "modules/ca-trust", commit) b.start(t, "n1", nil) v := b.await(t, "ca-trust", isState(delivery.BuildFailed)) - if !strings.Contains(v.Error, "n1") || !strings.Contains(v.Error, "boom") { + if !strings.Contains(v.Error, " on n") || !strings.Contains(v.Error, "boom") { t.Errorf("build = %+v, want failed naming the node and the step's reason", v) } key, _ := delivery.BuilderBuildKey("ca-trust", fp) @@ -510,8 +485,8 @@ func TestAToolchainChangeDoesNotBuildTheOldFingerprint(t *testing.T) { // 1615/A if _, err := b.builds.Get(b.ctx, mustKey(t, "netcheck")); err == nil { t.Error("the old fingerprint, naming go1.27.1, was built with go1.27.2") } - if n := b.fake.count("mesh-build-go.build_go_build"); n != 2 { - t.Errorf("build_go_build was called %d times, want 2 (the new fingerprint on two nodes only)", n) + if n := b.fake.count("mesh-build-go.build_go_build"); n != 1 { + t.Errorf("build_go_build was called %d times, want 1 (the new fingerprint on one node only)", n) } } diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go index f2b61e6..40b6e29 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/main.go @@ -35,8 +35,6 @@ const ( // defaultStepBound is how long the builder waits for one step of a build module (a fetch downloads the Go modules, a // build compiles). defaultStepBound = 30 * time.Minute - // nodesPerFingerprint is how many build nodes build each fingerprint until release R4e (ADR 0478 clause 4). - nodesPerFingerprint = 2 ) func main() { @@ -178,7 +176,6 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro Health: have, Places: &buildrun.Places{Capacity: capacity}, Manifests: manifestReader{manifests}, - Nodes: nodesPerFingerprint, Logf: logger.Printf, } // The toolchains first, so a build that starts from the first modules value finds the nodes. diff --git a/modules/mesh-delivery-builder/internal/build/candidates_test.go b/modules/mesh-delivery-builder/internal/build/candidates_test.go index 7c26902..97ff67a 100644 --- a/modules/mesh-delivery-builder/internal/build/candidates_test.go +++ b/modules/mesh-delivery-builder/internal/build/candidates_test.go @@ -54,8 +54,8 @@ func TestACandidatesFingerprintIsBuiltOnceAsAKeyOfItsOwnFromTheHeadMergedOntoThe t.Fatalf("build = %+v, want the candidate's fingerprint built", v) } fetches := r.bm.callsOf("build_go_fetch") - if len(fetches) != 2 { // one on each of two nodes - t.Fatalf("%d fetches, want one per node", len(fetches)) + if len(fetches) != 1 { // one node per fingerprint (ADR 0478 clause 4) + t.Fatalf("%d fetches, want one", len(fetches)) } f := fetches[0].body.(map[string]any) if f["commit"] != headX || f["ref"] != "refs/pull/7/head" || f["base"] != "7a74cee83f88e6585a5511c3cc53bdf0f5303d20" || f["repository"] != "novox/mesh-catalog" { @@ -67,7 +67,7 @@ func TestACandidatesFingerprintIsBuiltOnceAsAKeyOfItsOwnFromTheHeadMergedOntoThe // Two heads carrying one fingerprint are one build. r.b.Candidate(context.Background(), "feat%2Fy", candidateValue(t, "feat/y", 8, headY)) r.b.Wait() - if n := len(r.bm.callsOf("build_go_fetch")); n != 2 { + if n := len(r.bm.callsOf("build_go_fetch")); n != 1 { t.Errorf("%d fetches after a second head with the same fingerprint, want no more", n) } } diff --git a/modules/mesh-delivery-builder/internal/build/claim.go b/modules/mesh-delivery-builder/internal/build/claim.go index 6a6659f..21b1316 100644 --- a/modules/mesh-delivery-builder/internal/build/claim.go +++ b/modules/mesh-delivery-builder/internal/build/claim.go @@ -39,32 +39,11 @@ type Claim struct { Build delivery.BuilderBuild } -// OneNodePrefix opens the Step of a done build that a build module built on one node only, because no second node -// qualified (ADR 0478 clause 4); the build modules that did so follow in parentheses, then ": ". Claim reads it to build -// the key again, as a new revision, once a second node qualifies. -const OneNodePrefix = "one node (" - -// OneNodeModules are the build modules a done build built on one node only, or none when it is not such a build. -func OneNodeModules(b delivery.BuilderBuild) []string { - if b.State != delivery.BuildDone || !strings.HasPrefix(b.Step, OneNodePrefix) { - return nil - } - list, _, ok := strings.Cut(strings.TrimPrefix(b.Step, OneNodePrefix), "): ") - if !ok || list == "" { - return nil - } - return strings.Split(list, ", ") -} - // ClaimOptions say what the builder knows about the moment of a claim. type ClaimOptions struct { // TakeOver takes a key found building whoever holds it: the builder has just started and looks at the key for the first // time, and a build left building by a builder that is gone, on this node or another, is nobody's (ADR 0478 clause 6). TakeOver bool - // MoreNodes says whether one of these build modules, which built a done key on one node only, has a second node that - // qualifies now and the key may be built again (it is not held back after a failed second build, issue 1664). Nil means - // none does. - MoreNodes func(modules []string) bool // ImageBuilder says whether a node runs a healthy image build module now: a key failed for NoImageBuilderPrefix is taken // again only then. Nil means none does. ImageBuilder func() bool @@ -135,9 +114,10 @@ func Unreferenced(b delivery.BuilderBuild) bool { // - no key: it is created building, by this builder; a create that finds the key made meanwhile gets nothing; // - a done key is not taken again (the same source is never built twice), except one whose bundle, archive or upstream // copy names no reference (Unreferenced, a key done before R4e: ADR 0498 clause 8), which is claimed building as a new -// revision and built whole, and one built on one node only (OneNodePrefix) -// when a second node qualifies now (ClaimOptions.MoreNodes): that is taken as it stands, with the done value, and its -// builder writes the new revision when the second build ends (ADR 0478 clause 4); +// revision and built whole, and one whose image or package was built unpublished as a candidate's once the trunk +// names it (ClaimOptions.Trunk): that is taken as it stands, with the done value, and its builder writes the new +// revision when the publishing build ends (ADR 0498 clause 6). From R4e one node builds each fingerprint (ADR 0478 +// clause 4), so a key built on one node is never built again for a second; // - a failed key is taken again as a new revision of the same key (ADR 0429 clause 18), except one failed for a kind no // builder builds, for an upstream pinned by no digest or a package version taken (delivery.FailedUntilChanged: ADR 0478 // clause 2, ADR 0498 clauses 4 and 5) or for digests that differ (buildrun.DigestsDifferPrefix, clause 4), @@ -169,9 +149,6 @@ func (c Claimer) Claim(ctx context.Context, module, fingerprint, manifest string if Unreferenced(cur) { return c.write(ctx, key, building, func(data []byte) (uint64, error) { return c.Store.Update(ctx, key, data, rev) }) } - if modules := OneNodeModules(cur); len(modules) > 0 && opt.MoreNodes != nil && opt.MoreNodes(modules) { - return &Claim{Key: key, Rev: rev, Build: cur}, nil - } if opt.Trunk && len(publishable(cur, opt.ImageBuilder)) > 0 { return &Claim{Key: key, Rev: rev, Build: cur}, nil } diff --git a/modules/mesh-delivery-builder/internal/build/followups_test.go b/modules/mesh-delivery-builder/internal/build/followups_test.go index 6128d0c..ce0efd6 100644 --- a/modules/mesh-delivery-builder/internal/build/followups_test.go +++ b/modules/mesh-delivery-builder/internal/build/followups_test.go @@ -8,43 +8,6 @@ import ( "git.novox.be/novox/mesh-sdk/go/delivery" ) -// 1664: a done key built on one node is not built again, over and over, while the second node's build keeps failing. -func TestASecondNodeBuildThatFailsIsNotRetriedUntilTheNodesChange(t *testing.T) { // issue 1664 - r := newRig(t, goBundle) - r.pres.Put("ace", presenceValue(t, false), false) // only novox qualifies - first := r.consider(t, "modules/m") - r.bm.fails["ace/build_go_fetch"] = "ace is out of disk" - r.pres.Put("ace", presenceValue(t, true), false) - r.b.NodesChanged(context.Background()) // ace came online: one more try - r.b.Wait() - tries := len(r.bm.callsOf("build_go_fetch")) - if tries < 2 { - t.Fatalf("%d fetches: the second node was never tried", tries) - } - if v := valueOf(t, r.st, r.key); !delivery.SameBuilderBuild(v, first) { - t.Fatalf("the done key changed: %+v", v) - } - // Nothing about the nodes changed: a modules value, a Reconsider and a start of looking again build nothing. - for i := 0; i < 3; i++ { - r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) - r.b.Reconsider(context.Background()) - r.b.Wait() - } - if n := len(r.bm.callsOf("build_go_fetch")); n != tries { - t.Errorf("%d fetches after looking again, was %d: the failed second build was retried in a loop", n, tries) - } - // A node changing (ace goes and comes back) is a reason to try again, and ace now builds. - delete(r.bm.fails, "ace/build_go_fetch") - r.pres.Put("ace", presenceValue(t, false), false) - r.b.NodesChanged(context.Background()) - r.pres.Put("ace", presenceValue(t, true), false) - r.b.NodesChanged(context.Background()) - r.b.Wait() - if v := valueOf(t, r.st, r.key); v.State != delivery.BuildDone || strings.HasPrefix(v.Step, OneNodePrefix) || !strings.Contains(v.Step, "ace and novox") { - t.Errorf("build = %+v, want it built on both nodes once ace could", v) - } -} - // 1660: at start the builder closes a key left building for a fingerprint the module no longer has. func TestAKeyLeftBuildingForAFingerprintTheModuleNoLongerHasIsClosedAtStart(t *testing.T) { // issue 1660 r := newRig(t, goBundle) diff --git a/modules/mesh-delivery-builder/internal/build/r4e_test.go b/modules/mesh-delivery-builder/internal/build/r4e_test.go index 9a03c7d..2998250 100644 --- a/modules/mesh-delivery-builder/internal/build/r4e_test.go +++ b/modules/mesh-delivery-builder/internal/build/r4e_test.go @@ -332,3 +332,31 @@ func TestTheFetchOfAModuleNamingItsPathsCarriesItsDirectoryAndThosePaths(t *test } func hex40(c byte) string { return strings.Repeat(string(c), 40) } + +func TestOneNodeBuildsEachFingerprint(t *testing.T) { // B9, ADR 0478 clause 4: "From R4e one node builds each fingerprint" + r := newRig(t, goBundle, archiveA, tsBundle) // ace and novox both qualify for both build modules + v := r.consider(t, "modules/m") + if v.State != delivery.BuildDone || strings.Contains(v.Step, " and ") || strings.HasPrefix(v.Step, "one node (") { + t.Fatalf("build = %+v, want done on one node of each build module, unmarked", v) + } + if f, b := len(r.bm.callsOf("build_go_fetch")), len(r.bm.callsOf("build_go_build")); f != 1 || b != 2 { + t.Errorf("go fetches %d, builds %d; want one fetch and one build of each artifact, on one node", f, b) + } + if f := len(r.bm.callsOf("build_node_fetch")); f != 1 { + t.Errorf("node fetches %d, want one", f) + } +} + +func TestAKeyDoneOnOneNodeIsNeverBuiltAgainForASecondNode(t *testing.T) { // B9: the second-node retake is gone + r := newRig(t, goBundle) + putBuild(t, r.st, r.key, delivery.BuilderBuild{State: delivery.BuildDone, By: "novox.builder", Manifest: manifestD, + Step: "one node (mesh-build-go): built mesh-build-go on one node, novox", + Artifacts: []delivery.BuiltArtifact{{Name: "tool", Kind: "bundle", Digest: digest, Reference: "artifact-store://m/tool/blobs/" + digest}}}) + writes := r.st.writes + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) + r.b.NodesChanged(context.Background()) + r.b.Wait() + if r.st.writes != writes || len(r.bm.calls) != 0 { + t.Errorf("a key done on one node was built again (%d writes, was %d; %d calls)", r.st.writes, writes, len(r.bm.calls)) + } +} diff --git a/modules/mesh-delivery-builder/internal/build/round4_test.go b/modules/mesh-delivery-builder/internal/build/round4_test.go index 24f962f..f2bcc51 100644 --- a/modules/mesh-delivery-builder/internal/build/round4_test.go +++ b/modules/mesh-delivery-builder/internal/build/round4_test.go @@ -34,8 +34,8 @@ func TestAnArchiveOnlyModuleIsBuiltAlthoughItsFingerprintNamesNoToolchain(t *tes if v.State != delivery.BuildDone || len(v.Artifacts) != 1 || v.Artifacts[0].Kind != "archive" || len(v.Toolchains) != 0 { t.Fatalf("build = %+v, want done with the archive and no toolchain named", v) } - if n := len(r.bm.callsOf("build_go_build")); n != 2 { - t.Errorf("%d go builds, want one on each of two nodes", n) + if n := len(r.bm.callsOf("build_go_build")); n != 1 { + t.Errorf("%d go builds, want one, on one node (ADR 0478 clause 4)", n) } } @@ -54,8 +54,8 @@ func TestAnArchiveOnlyModuleNeedsNoAgreedToolchainButNeedsNodesThatQualify(t *te r.b.Reconsider(context.Background()) r.b.Wait() v := valueOf(t, r.st, r.key) - if v.State != delivery.BuildDone || !strings.HasPrefix(v.Step, OneNodePrefix) { - t.Errorf("build = %+v, want done on novox, the one node that qualifies, marked as one node", v) + if v.State != delivery.BuildDone || !strings.Contains(v.Step, "on novox") { + t.Errorf("build = %+v, want done on novox, the one node that qualifies", v) } } @@ -70,25 +70,6 @@ func TestABundleAndAnArchiveOfOneBuildModuleStillNeedTheAgreedToolchain(t *testi } } -// 2. The digests-differ failure opens with its prefix, and nothing builds it again. -func TestDifferingDigestsFailTheKeyWithThePrefixAndNothingBuildsItAgain(t *testing.T) { // review round 4, 2 - r := newRig(t, goBundle) - r.bm.digests["ace/tool"] = "sha256:" + hex64('9') - v := r.consider(t, "modules/m") - if v.State != delivery.BuildFailed || !strings.HasPrefix(v.Error, "digests differ:") { - t.Fatalf("build = %+v, want a failure opening with exactly %q", v, "digests differ:") - } - calls, writes := len(r.bm.calls), r.st.writes - r.bm.digests = map[string]string{} // the nodes would now agree - r.b.Reconsider(context.Background()) - r.b.Wait() - r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) - r.b.Wait() - if len(r.bm.calls) != calls || r.st.writes != writes { - t.Errorf("a failed-for-digests key was built again: %d calls (was %d), %d writes (was %d)", len(r.bm.calls), calls, r.st.writes, writes) - } -} - func TestClaimLeavesAKeyFailedForDifferingDigestsAlone(t *testing.T) { // review round 4, 2 st := newMem() seed(t, st, delivery.BuilderBuild{State: delivery.BuildFailed, Error: buildrun.DigestsDifferPrefix + " the nodes built tool to different digests: ace x; novox y"}) @@ -99,67 +80,6 @@ func TestClaimLeavesAKeyFailedForDifferingDigestsAlone(t *testing.T) { // review } } -// 3. A done key built on one node is built again on a second node once one qualifies. -func TestADoneKeyOfOneNodeIsBuiltAgainOnASecondNodeAndTheDigestsCompared(t *testing.T) { // review round 4, 3 - r := newRig(t, goBundle) - r.pres.Put("ace", presenceValue(t, false), false) // only novox qualifies - v := r.consider(t, "modules/m") - if v.State != delivery.BuildDone || !strings.HasPrefix(v.Step, OneNodePrefix+buildrun.GoBuildModule+"): ") || !strings.Contains(v.Step, "on one node, novox") { - t.Fatalf("build = %+v, want done on one node, marked", v) - } - if got := OneNodeModules(v); len(got) != 1 || got[0] != buildrun.GoBuildModule { - t.Errorf("OneNodeModules = %v", got) - } - rev, calls := r.st.revs[r.key], len(r.bm.callsOf("build_go_build")) - // Nothing changes while one node qualifies: looking again builds nothing. - r.b.Reconsider(context.Background()) - r.b.Wait() - if r.st.revs[r.key] != rev || len(r.bm.callsOf("build_go_build")) != calls { - t.Fatal("a one-node key was built again with no second node") - } - // ace comes online: the key is built on ace and novox, as a new revision. - r.pres.Put("ace", presenceValue(t, true), false) - r.b.Reconsider(context.Background()) - r.b.Wait() - v = valueOf(t, r.st, r.key) - if v.State != delivery.BuildDone || strings.HasPrefix(v.Step, OneNodePrefix) || !strings.Contains(v.Step, "ace and novox") { - t.Errorf("build = %+v, want done on ace and novox", v) - } - if r.st.revs[r.key] <= rev { - t.Errorf("revision %d, was %d: not a new revision", r.st.revs[r.key], rev) - } - builds := r.bm.callsOf("build_go_build") - if len(builds) != calls+2 || builds[len(builds)-2].node == builds[len(builds)-1].node { - t.Errorf("%d builds (was %d), want one on each of two nodes", len(builds), calls) - } -} - -func TestASecondNodeThatDisagreesFailsTheDoneKeyWithTheDigestsPrefix(t *testing.T) { // review round 4, 3 - r := newRig(t, goBundle) - r.pres.Put("ace", presenceValue(t, false), false) - r.consider(t, "modules/m") - r.bm.digests["ace/tool"] = "sha256:" + hex64('9') - r.pres.Put("ace", presenceValue(t, true), false) - r.b.Reconsider(context.Background()) - r.b.Wait() - if v := valueOf(t, r.st, r.key); v.State != delivery.BuildFailed || !strings.HasPrefix(v.Error, buildrun.DigestsDifferPrefix) { - t.Errorf("build = %+v, want failed with digests differ", v) - } -} - -func TestASecondBuildThatFailsForAnotherReasonLeavesTheDoneKeyStanding(t *testing.T) { // review round 4, 3 - r := newRig(t, goBundle) - r.pres.Put("ace", presenceValue(t, false), false) - first := r.consider(t, "modules/m") - r.bm.fails["ace/build_go_fetch"] = "ace is out of disk" - r.pres.Put("ace", presenceValue(t, true), false) - r.b.Reconsider(context.Background()) - r.b.Wait() - if v := valueOf(t, r.st, r.key); !delivery.SameBuilderBuild(v, first) { - t.Errorf("build = %+v, want the first done build to stand: %+v", v, first) - } -} - // 4. A step never waits on a full node while another qualifying node has room. func TestChoosesMadeAtOnceSeeEachOthersReservationsSoNoLaneWaitsWhileANodeHasRoom(t *testing.T) { // review round 4, 4 p := &buildrun.Places{Capacity: map[string]int{"ace": 2, "g14": 1, "novox": 1, "shanks": 3}} @@ -252,7 +172,7 @@ func TestANodeWhoseBuildModuleIsNotHealthyIsNotChosen(t *testing.T) { // review have.Put("novox.mesh-build-go", haveValue(t, delivery.PhaseRunning, delivery.HealthOK), false) r.b.Health = have v := r.consider(t, "modules/m") - if v.State != delivery.BuildDone || !strings.Contains(v.Step, "on one node, novox") { + if v.State != delivery.BuildDone || !strings.Contains(v.Step, "on novox") { t.Fatalf("build = %+v, want done on novox alone", v) } for _, c := range r.bm.calls { diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index e725a4f..9c6f23e 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -42,8 +42,6 @@ type Builder struct { } // buildrun.Places holds a step back while its node is at its build capacity; a Builder with none runs one step at a time on each node. Places *buildrun.Places - // Nodes is how many build nodes build each fingerprint: two until release R4e (ADR 0478 clause 4). - Nodes int // Limit is how many builds run at once; four when 0. Limit int Logf func(format string, args ...any) @@ -60,10 +58,6 @@ type Builder struct { via map[string]via // by build key: a candidate's fetch (candidates.go) trunkFP map[string]bool // by build key: a trunk module value carries this fingerprint tried map[string]bool // by build key: looked at since this process started - // epoch counts the changes of the nodes (online, healthy, a toolchain) seen since this process started; a done key - // whose second-node build failed is held back until it moves (issue 1664). - epoch uint64 - secondAt map[string]uint64 // by build key: the epoch a second-node build of a one-node key failed at // published are the package versions `@` a done key of this bucket names as a package's reference: a key // waiting for one is built once it is here (ADR 0500 clause 6). published map[string]bool @@ -83,7 +77,7 @@ func (b *Builder) init() { b.sem = make(chan struct{}, limit) b.cands, b.via, b.trunkFP = map[string]delivery.Candidate{}, map[string]via{}, map[string]bool{} b.known, b.inflight, b.again, b.plans, b.tried = map[string]delivery.Module{}, map[string]bool{}, map[string]delivery.Module{}, map[string]planned{}, map[string]bool{} - b.secondAt, b.published = map[string]uint64{}, map[string]bool{} + b.published = map[string]bool{} if b.Places == nil { b.Places = &buildrun.Places{} } @@ -120,15 +114,8 @@ func (b *Builder) Module(ctx context.Context, key string, value []byte) { } // NodesChanged is Reconsider after a change of the nodes: one came online or went, a build module became healthy or -// stopped being, or a toolchain moved. A done key built on one node whose second-node build failed is tried again only -// after such a change, not at every modules value (issue 1664). -func (b *Builder) NodesChanged(ctx context.Context) { - b.init() - b.mu.Lock() - b.epoch++ - b.mu.Unlock() - b.Reconsider(ctx) -} +// stopped being, or a toolchain moved, so a build that waited for a node may go on. +func (b *Builder) NodesChanged(ctx context.Context) { b.Reconsider(ctx) } // Sweep reads, at start, every key of the bucket: it keeps the package versions done keys name as published (a key // waiting for one is then looked at again, ADR 0500 clause 6), and closes every key left building for a fingerprint its @@ -323,14 +310,8 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { trunk := b.trunkFP[key] first := !b.tried[key] b.tried[key] = true - epoch := b.epoch - failedAt, failedBefore := b.secondAt[key] b.mu.Unlock() - moreNodes := func(modules []string) bool { - // A second build that failed is not tried again until the nodes change (issue 1664). - return (!failedBefore || epoch > failedAt) && b.moreNodes(modules) - } - cl, err := b.Claimer.Claim(ctx, module, m.FP2, m.Manifest, ClaimOptions{TakeOver: first, MoreNodes: moreNodes, ImageBuilder: b.imageBuilderRuns, Trunk: trunk, Published: b.isPublished}) + cl, err := b.Claimer.Claim(ctx, module, m.FP2, m.Manifest, ClaimOptions{TakeOver: first, ImageBuilder: b.imageBuilderRuns, Trunk: trunk, Published: b.isPublished}) if err != nil { b.logf("%s: %v", module, err) return @@ -338,16 +319,13 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { if cl == nil { return } - // A done key taken again builds only what is missing: the build modules that ran on one node, and the image when it was - // built as a candidate's; the other artifacts stay as the done key has them (ADR 0487 clauses 5 and 8). + // A done key taken again builds only what is missing: the image or the package built unpublished as a candidate's; the + // other artifacts stay as the done key has them (ADR 0487 clauses 5 and 8, ADR 0498 clause 6). var prior *delivery.BuilderBuild var only map[string]bool publishing := false if cl.Build.State == delivery.BuildDone { prior, only = &cl.Build, map[string]bool{} - for _, bm := range OneNodeModules(cl.Build) { - only[bm] = true - } if trunk { for _, bm := range publishable(cl.Build, b.imageBuilderRuns) { only[bm] = true @@ -375,14 +353,6 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { // The trunk's image or package was built only as a candidate's and its push or publish failed: the key says so, so the builds view, the // checker and the failed-key retake see it, and a look does not build the image again for nothing. result.Error = keyError([]string{"publishing what the merged commit built failed: " + result.Error}) - } else if cl.Build.State == delivery.BuildDone && result.State == delivery.BuildFailed && !strings.HasPrefix(result.Error, buildrun.DigestsDifferPrefix) { - // A done key built again on a second node keeps its done value when the new build fails for another reason than - // the digests: the first build stands, and a later change of the nodes looks again. - b.logf("%s: built again on a second node and failed, the done build stands: %s", cl.Key, result.Error) - b.mu.Lock() - b.secondAt[cl.Key] = epoch - b.mu.Unlock() - return } if err := b.Claimer.Finish(ctx, cl, result); err != nil { b.logf("%s: the build could not be written: %v", cl.Key, err) @@ -444,7 +414,7 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p repository := delivery.RepositoryName(m.Repository) var lanes []*lane used := map[string]delivery.ToolchainUsed{} - var steps, oneNode []string + var steps []string // Every node chosen holds a reservation until its lane takes the place; a build that ends before its lanes start gives them up. giveUp := func(nodes ...string) { for _, n := range nodes { @@ -477,7 +447,7 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p } nodes = ns } else if plan.NeedsToolchain(bm) { - ns, tc, err := buildrun.ChooseNodes(language, entries, b.Places, qualifies, b.Nodes) + ns, tc, err := buildrun.ChooseNodes(language, entries, b.Places, qualifies, 1) if err != nil { giveUpLanes() return failed("%s", err) @@ -491,23 +461,15 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p nodes = ns } else { // An archive is packed with no toolchain (the fingerprint names none): any node of the build module that qualifies. - ns, err := buildrun.ChooseAnyNodes(language, entries, b.Places, qualifies, b.Nodes) + ns, err := buildrun.ChooseAnyNodes(language, entries, b.Places, qualifies, 1) if err != nil { giveUpLanes() return failed("%s", err) } nodes = ns } - if bm == ImageBuildModule { - steps = append(steps, fmt.Sprintf("%s on %s", bm, nodes[0])) - } else if len(nodes) == 1 { - oneNode = append(oneNode, bm) - steps = append(steps, fmt.Sprintf("%s on one node, %s", bm, nodes[0])) - } else { - sorted := append([]string(nil), nodes...) - sort.Strings(sorted) - steps = append(steps, fmt.Sprintf("%s on %s", bm, strings.Join(sorted, " and "))) - } + // One node builds each fingerprint (ADR 0478 clause 4, from R4e). + steps = append(steps, fmt.Sprintf("%s on %s", bm, nodes[0])) var tasks []Task for _, t := range plan.Tasks { if t.Module == bm { @@ -592,10 +554,6 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p } } step := "built " + strings.Join(steps, "; ") - if len(oneNode) > 0 { - // Marked so that Claim builds the key again once a second node qualifies (ADR 0478 clause 4). - step = OneNodePrefix + strings.Join(oneNode, ", ") + "): " + step - } if len(used) == 0 { used = nil } @@ -696,24 +654,6 @@ func (b *Builder) imageBuilderRuns() bool { return false } -// moreNodes is whether any of these build modules has two nodes that qualify now. -func (b *Builder) moreNodes(modules []string) bool { - for _, bm := range modules { - seen := map[string]bool{} - n := 0 - for _, e := range b.Toolchains.Entries(buildrun.LanguageOf[bm]) { - if !seen[e.Node] && b.qualifies(e.Node, bm) { - seen[e.Node] = true - n++ - } - } - if n >= 2 { - return true - } - } - return false -} - // step runs one step of a build module; the lane that runs it holds its node's place. func (b *Builder) step(ctx context.Context, bm, node, tool string, args map[string]any) (json.RawMessage, error) { return b.Runner.Run(ctx, bm, node, toolPrefix[bm]+tool, args) diff --git a/modules/mesh-delivery-builder/internal/build/run_test.go b/modules/mesh-delivery-builder/internal/build/run_test.go index aef2d5a..4a8524c 100644 --- a/modules/mesh-delivery-builder/internal/build/run_test.go +++ b/modules/mesh-delivery-builder/internal/build/run_test.go @@ -139,7 +139,7 @@ func newRig(t *testing.T, artifacts ...catalogue.Artifact) *rig { } r.b = &Builder{ Claimer: Claimer{Store: r.st, By: "novox.builder"}, Runner: buildrun.Runner{Caller: r.bm, Steps: r.bm.steps, Bound: time.Second}, - Toolchains: r.tools, Presence: r.pres, Manifests: r.mf, Nodes: 2, Logf: t.Logf, + Toolchains: r.tools, Presence: r.pres, Manifests: r.mf, Logf: t.Logf, } var err error if r.key, err = delivery.BuilderBuildKey("m", fp1); err != nil { @@ -180,7 +180,7 @@ var ( manifestD = mdigest('a') ) -func TestABundleAndAnArchiveAreBuiltOnTwoNodesAndDoneWhenTheDigestsAgree(t *testing.T) { // 1607/AC7, ADR 0478 clauses 1, 4 +func TestABundleAndAnArchiveAreBuiltOnOneNodeAndDone(t *testing.T) { // 1607/AC7, ADR 0478 clauses 1, 4 r := newRig(t, goBundle, archiveA, tsBundle) v := r.consider(t, "modules/m") if v.State != delivery.BuildDone || v.By != "novox.builder" || v.Manifest != manifestD || len(v.Artifacts) != 3 { @@ -200,15 +200,15 @@ func TestABundleAndAnArchiveAreBuiltOnTwoNodesAndDoneWhenTheDigestsAgree(t *test if got := v.Toolchains["typescript"]; got.Version != "tsc5" { t.Errorf("toolchains = %+v", v.Toolchains) } - // Two fetches per build module, one on each node; every build names the module's directory. - if n := len(r.bm.callsOf("build_go_fetch")); n != 2 { - t.Errorf("%d go fetches, want one per node", n) + // One fetch per build module, on one node (ADR 0478 clause 4); every build names the module's directory. + if n := len(r.bm.callsOf("build_go_fetch")); n != 1 { + t.Errorf("%d go fetches, want one", n) } - if n := len(r.bm.callsOf("build_go_build")); n != 4 { // tool and files, each on two nodes - t.Errorf("%d go builds, want 4", n) + if n := len(r.bm.callsOf("build_go_build")); n != 2 { // tool and files + t.Errorf("%d go builds, want 2", n) } - if n := len(r.bm.callsOf("build_node_build")); n != 2 { - t.Errorf("%d node builds, want 2", n) + if n := len(r.bm.callsOf("build_node_build")); n != 1 { + t.Errorf("%d node builds, want 1", n) } c := r.bm.callsOf("build_go_build")[0] if args := c.body.(map[string]any); args["module"] != "modules/m" || !strings.HasPrefix(fmt.Sprint(args["workspace"]), "w-") { @@ -220,21 +220,12 @@ func TestABundleAndAnArchiveAreBuiltOnTwoNodesAndDoneWhenTheDigestsAgree(t *test } } -func TestDifferingDigestsFailTheBuildNamingBothNodes(t *testing.T) { // 1607/AC4, ADR 0478 clause 4 - r := newRig(t, goBundle) - r.bm.digests["ace/tool"] = "sha256:" + hex64('9') - v := r.consider(t, "modules/m") - if v.State != delivery.BuildFailed || !strings.Contains(v.Error, "ace") || !strings.Contains(v.Error, "novox") || !strings.Contains(v.Error, digest) || !strings.Contains(v.Error, hex64('9')) { - t.Errorf("build = %+v, want failed naming both nodes and both digests", v) - } -} - -func TestOneQualifyingNodeBuildsOnceAndTheKeySaysSo(t *testing.T) { // 1607/AC4, ADR 0478 clause 4 +func TestOneQualifyingNodeBuildsOnce(t *testing.T) { // 1607/AC4, ADR 0478 clause 4 r := newRig(t, goBundle) r.tools.Put(buildrun.GoBuildModule, "ace.go", nil, true) v := r.consider(t, "modules/m") - if v.State != delivery.BuildDone || !strings.Contains(v.Step, "one node") || !strings.Contains(v.Step, "novox") { - t.Errorf("build = %+v, want done with its step saying one node, novox", v) + if v.State != delivery.BuildDone || !strings.Contains(v.Step, "on novox") { + t.Errorf("build = %+v, want done with its step saying novox", v) } if n := len(r.bm.callsOf("build_go_build")); n != 1 { t.Errorf("%d builds, want one", n) @@ -277,9 +268,6 @@ func TestAnImageGoesToOneNodeAndGetsOneDoneKeyWithItsReference(t *testing.T) { / if v.Artifacts[0].Reference == "" { t.Errorf("a trunk image has no reference: %+v", v.Artifacts[0]) } - if strings.HasPrefix(v.Step, OneNodePrefix) || len(OneNodeModules(v)) != 0 { - t.Errorf("step = %q, want no one-node mark: an image is never built again on a second node", v.Step) - } if n := len(r.bm.callsOf("build_image_fetch")); n != 1 { t.Errorf("%d image fetches, want one", n) } @@ -300,22 +288,19 @@ func TestAnImageIsBuiltOnlyWhereMeshBuildImageIsHealthy(t *testing.T) { // 1705a } } -func TestABundleAndAnImageOfOneModuleBuildOnTwoNodesAndOne(t *testing.T) { // 1705a +func TestABundleAndAnImageOfOneModuleBuildOnOneNodeEach(t *testing.T) { // 1705a r := newRig(t, goBundle, imageArt) r.b.Health = imageHave(t, "ace", "novox") v := r.consider(t, "modules/m") if v.State != delivery.BuildDone || len(v.Artifacts) != 2 { t.Fatalf("build = %+v", v) } - if n := len(r.bm.callsOf("build_go_build")); n != 2 { - t.Errorf("%d go builds, want 2", n) + if n := len(r.bm.callsOf("build_go_build")); n != 1 { + t.Errorf("%d go builds, want 1 (one node per fingerprint, ADR 0478 clause 4)", n) } if n := len(r.bm.callsOf("build_image_build")); n != 1 { t.Errorf("%d image builds, want 1", n) } - if modules := OneNodeModules(v); len(modules) != 0 { - t.Errorf("one-node modules = %v, want none", modules) - } } func TestWithNoImageNodeTheKeyFailsNamingTheProvisionAndIsNotRetakenUntilANodeReportsTheModule(t *testing.T) { // 1705b, lead's decision @@ -454,7 +439,7 @@ func TestAKeyTheBuilderLeftBuildingIsBuiltAtStartAndDoneKeysAreNotBuiltTwice(t * // whose compare-and-set comes first, and the other's write is refused, never overwriting it. func TestTwoInstancesStartingTogetherEndWithOneDoneKey(t *testing.T) { // 1607/AC7, review round 4 r := newRig(t, goBundle) - other := &Builder{Claimer: Claimer{Store: r.st, By: "ace.builder"}, Runner: r.b.Runner, Toolchains: r.tools, Presence: r.pres, Manifests: r.mf, Nodes: 2, Logf: t.Logf} + other := &Builder{Claimer: Claimer{Store: r.st, By: "ace.builder"}, Runner: r.b.Runner, Toolchains: r.tools, Presence: r.pres, Manifests: r.mf, Logf: t.Logf} value := moduleValue(t, "modules/m") var wg sync.WaitGroup for _, b := range []*Builder{r.b, other} { @@ -465,8 +450,8 @@ func TestTwoInstancesStartingTogetherEndWithOneDoneKey(t *testing.T) { // 1607/A if v := valueOf(t, r.st, r.key); v.State != delivery.BuildDone || len(v.Artifacts) != 1 { t.Errorf("build = %+v, want one done key", v) } - if n := len(r.bm.callsOf("build_go_build")); n < 2 || n%2 != 0 { - t.Errorf("%d builds, want two for each instance that built", n) + if n := len(r.bm.callsOf("build_go_build")); n < 1 { + t.Errorf("%d builds, want one for each instance that built", n) } } @@ -603,43 +588,6 @@ func TestACandidatesImageIsPublishedOnceWhenTheTrunkNamesItsFingerprint(t *testi } } -func TestASecondGoNodeDoesNotBuildOrPushTheImageAgain(t *testing.T) { // review r1, mesh-delivery 2; ADR 0487 clause 8 - r := newRig(t, goBundle, imageArt) - r.b.Health = imageHave(t, "ace", "novox") - r.pres.Put("ace", presenceValue(t, false), false) // only novox qualifies for Go (and the image) - v := r.consider(t, "modules/m") - if v.State != delivery.BuildDone || len(OneNodeModules(v)) != 1 { - t.Fatalf("build = %+v, want done with the Go bundle on one node", v) - } - var image delivery.BuiltArtifact - for _, a := range v.Artifacts { - if a.Kind == "image" { - image = a - } - } - if image.Reference == "" { - t.Fatalf("artifacts = %+v, want a published image", v.Artifacts) - } - r.pres.Put("ace", presenceValue(t, true), false) - r.b.Reconsider(context.Background()) - r.b.Wait() - v = valueOf(t, r.st, r.key) - if len(OneNodeModules(v)) != 0 || len(v.Artifacts) != 2 { - t.Fatalf("build = %+v, want done on two Go nodes", v) - } - if n := len(r.bm.callsOf("build_image_build")); n != 1 { - t.Errorf("%d image builds, want the one of the first build only", n) - } - for _, a := range v.Artifacts { - if a.Kind == "image" && a != image { - t.Errorf("image = %+v, was %+v: the done key was rewritten with another image", a, image) - } - } - if n := len(r.bm.callsOf("build_go_build")); n != 3 { // one, then two - t.Errorf("%d go builds, want 3", n) - } -} - func TestAFailedPublishOfACandidatesImageIsWrittenToTheKey(t *testing.T) { // review r2, mesh-delivery 1; ADR 0487 clause 5 r := newRig(t, imageArt) r.b.Health = imageHave(t, "ace")