From 12f61998c5c692fe3cc25f34f9efb671ecb92a2d Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 10:33:52 +0200 Subject: [PATCH] Publish a merged candidate's image once; a retaken key builds only what is missing (review r1) --- .../mesh-delivery-builder/integration_test.go | 86 +++++++++++-------- .../internal/build/claim.go | 20 +++++ .../internal/build/run.go | 49 ++++++++++- .../internal/build/run_test.go | 76 ++++++++++++++++ 4 files changed, 191 insertions(+), 40 deletions(-) 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 0236e64..6dd6b42 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 @@ -595,38 +595,44 @@ func TestBuilderBuildsACandidatesFingerprintFromTheHeadMergedOntoTheTrunk(t *tes // TestAnImageIsBuiltOnOneNodeAndOnlyATrunkKeyPublishes: modules.imaged (one image artifact) is built on n1, where // mesh-build-image runs: a trunk record's key is done with the digest and the reference, its build call carrying publish; a -// candidate's key for the same module is done with no reference, its build call carrying no publish (ADR 0487 clause 12). +// candidate's key for the same module is built first with no publish and no reference; when the pull request is merged and the +// trunk's record names the candidate's fingerprint, the key is built again with publish and ends with a reference +// (ADR 0487 clauses 5 and 12). func TestAnImageIsBuiltOnOneNodeAndOnlyATrunkKeyPublishes(t *testing.T) { // 1705 b := startBed(t) + other := "src2:" + strings.Repeat("e", 64) digest := b.moduleCommit(t, "imaged", "catalogue-a67f0196-imaged.module.json", "modules/imaged", nil, commit) + // The trunk carries `other`; its key is built and published. + trunk := func(fp2 string) { + data, err := delivery.EncodeModule(delivery.Module{Repository: "novox/mesh-catalog", Commit: commit, FP: "src1:" + strings.Repeat("f", 64), FP2: fp2, + Manifest: digest, Path: "modules/imaged"}) + if err != nil { + t.Fatal(err) + } + if _, err := b.modules.Put(b.ctx, "imaged", data); err != nil { + t.Fatal(err) + } + } + trunk(other) b.start(t, "n1", nil) - v := b.await(t, "imaged", isState(delivery.BuildDone)) + okey, _ := delivery.BuilderBuildKey("imaged", other) + awaitKey := func(key string, pred func(delivery.BuilderBuild) bool) delivery.BuilderBuild { + var last delivery.BuilderBuild + for deadline := time.Now().Add(20 * time.Second); time.Now().Before(deadline); time.Sleep(50 * time.Millisecond) { + if e, err := b.builds.Get(b.ctx, key); err == nil { + if last, err = delivery.DecodeBuilderBuild(e.Value()); err == nil && pred(last) { + return last + } + } + } + t.Fatalf("builds.%s = %+v within 20 s", key, last) + return last + } + v := awaitKey(okey, isState(delivery.BuildDone)) if len(v.Artifacts) != 1 || v.Artifacts[0].Digest != digestOf("server") || !strings.HasSuffix(v.Artifacts[0].Reference, "@"+digestOf("server")) { t.Fatalf("trunk build = %+v, want the digest and the reference", v) } - if got := b.fake.count("mesh-build-image.build_image_build"); got != 1 { - t.Errorf("build_image_build called %d times, want once on one node", got) - } - b.fake.mu.Lock() - if len(b.fake.builds) != 1 || b.fake.builds[0]["publish"] != true { - t.Errorf("builds = %v, want one carrying publish", b.fake.builds) - } - b.fake.builds = nil - b.fake.mu.Unlock() - - // A candidate: the trunk's record moves to another fingerprint, the candidate carries fp. - other := "src2:" + strings.Repeat("e", 64) - data, err := delivery.EncodeModule(delivery.Module{Repository: "novox/mesh-catalog", Commit: commit, FP: "src1:" + strings.Repeat("f", 64), FP2: other, - Manifest: digest, Path: "modules/imaged"}) - if err != nil { - t.Fatal(err) - } - if err := b.builds.Delete(b.ctx, mustKey(t, "imaged")); err != nil { - t.Fatal(err) - } - if _, err := b.modules.Put(b.ctx, "imaged", data); err != nil { - t.Fatal(err) - } + // The candidate carries fp: built, not published. const head = "1111111111111111111111111111111111111111" pulls := []delivery.CandidatePull{{Head: delivery.Head{Repository: "novox/mesh-catalog", Number: 7, Head: head}, Modules: []string{"imaged"}, Fps: map[string]delivery.CandidateModule{"imaged": {FP2: fp, Manifest: digest}}}} @@ -639,26 +645,32 @@ func TestAnImageIsBuiltOnOneNodeAndOnlyATrunkKeyPublishes(t *testing.T) { // 170 if _, err := b.candidates.Put(b.ctx, ckey, cand); err != nil { t.Fatal(err) } + v = b.await(t, "imaged", isState(delivery.BuildDone)) + if len(v.Artifacts) != 1 || v.Artifacts[0].Reference != "" { + t.Fatalf("candidate build = %+v, want done with no reference", v) + } + b.fake.mu.Lock() + if len(b.fake.builds) != 2 || b.fake.builds[0]["publish"] != true || b.fake.builds[1]["publish"] != nil { + t.Errorf("builds = %v, want the trunk's with publish, then the candidate's with none", b.fake.builds) + } + b.fake.mu.Unlock() + // The pull request is merged: the trunk's record names fp, and its key ends published, built once more with publish. + trunk(fp) v = b.await(t, "imaged", func(v delivery.BuilderBuild) bool { - return v.State == delivery.BuildDone && len(v.Artifacts) == 1 && v.By != "" + return v.State == delivery.BuildDone && len(v.Artifacts) == 1 && v.Artifacts[0].Reference != "" }) - // The trunk's new fingerprint is built too, and published; the candidate's is built and not. - for deadline := time.Now().Add(10 * time.Second); time.Now().Before(deadline) && b.fake.count("mesh-build-image.build_image_build") < 3; time.Sleep(50 * time.Millisecond) { + if v.Artifacts[0].Digest != digestOf("server") { + t.Errorf("merged build = %+v", v) } b.fake.mu.Lock() defer b.fake.mu.Unlock() - if v.Artifacts[0].Reference != "" { - t.Errorf("candidate build = %+v, want no reference", v) - } - published, plain := 0, 0 + published := 0 for _, a := range b.fake.builds { - if _, has := a["publish"]; has { + if a["publish"] == true { published++ - } else { - plain++ } } - if published != 1 || plain != 1 { - t.Errorf("builds = %v, want one trunk call with publish and one candidate call with none", b.fake.builds) + if len(b.fake.builds) != 3 || published != 2 { + t.Errorf("builds = %v, want three, two of them with publish (the trunk's two fingerprints)", b.fake.builds) } } diff --git a/modules/mesh-delivery-builder/internal/build/claim.go b/modules/mesh-delivery-builder/internal/build/claim.go index 5acdf7d..cb5f74c 100644 --- a/modules/mesh-delivery-builder/internal/build/claim.go +++ b/modules/mesh-delivery-builder/internal/build/claim.go @@ -66,6 +66,23 @@ type ClaimOptions struct { // 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 + // Trunk is whether the trunk's record names this fingerprint. A done key whose image has no reference was built as a + // candidate's, which never pushes (ADR 0487 clause 12); on the trunk it is taken again to publish the image, when an image + // builder runs (ImageBuilder). + Trunk bool +} + +// Unpublished is whether a done build holds an image built without a push: it has no reference. +func Unpublished(b delivery.BuilderBuild) bool { + if b.State != delivery.BuildDone { + return false + } + for _, a := range b.Artifacts { + if a.Kind == "image" && a.Reference == "" { + return true + } + } + return false } // Claim takes the build of module at fingerprint, or answers nil when it is not this builder's to build now: @@ -101,6 +118,9 @@ func (c Claimer) Claim(ctx context.Context, module, fingerprint, manifest string 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 && Unpublished(cur) && opt.ImageBuilder != nil && opt.ImageBuilder() { + return &Claim{Key: key, Rev: rev, Build: cur}, nil + } return nil, nil case cur.State == delivery.BuildFailed && (strings.HasPrefix(cur.Error, RefusedPrefix) || strings.HasPrefix(cur.Error, buildrun.DigestsDifferPrefix)): return nil, nil diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index 1c4cbf4..eb60cf1 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -288,6 +288,7 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { } key, _ := delivery.BuilderBuildKey(module, m.FP2) b.mu.Lock() + trunk := b.trunkFP[key] first := !b.tried[key] b.tried[key] = true epoch := b.epoch @@ -297,7 +298,7 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { // 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}) + cl, err := b.Claimer.Claim(ctx, module, m.FP2, m.Manifest, ClaimOptions{TakeOver: first, MoreNodes: moreNodes, ImageBuilder: b.imageBuilderRuns, Trunk: trunk}) if err != nil { b.logf("%s: %v", module, err) return @@ -305,7 +306,20 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { if cl == nil { return } - result := b.build(ctx, module, m, plan, planErr) + // 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). + var prior *delivery.BuilderBuild + var only map[string]bool + if cl.Build.State == delivery.BuildDone { + prior, only = &cl.Build, map[string]bool{} + for _, bm := range OneNodeModules(cl.Build) { + only[bm] = true + } + if trunk && Unpublished(cl.Build) { + only[ImageBuildModule] = true + } + } + result := b.build(ctx, module, m, plan, planErr, prior, only) if ctx.Err() != nil { return } @@ -365,7 +379,7 @@ type lane struct { err error } -func (b *Builder) build(ctx context.Context, module string, m delivery.Module, plan Plan, planErr error) delivery.BuilderBuild { +func (b *Builder) build(ctx context.Context, module string, m delivery.Module, plan Plan, planErr error, prior *delivery.BuilderBuild, only map[string]bool) delivery.BuilderBuild { switch { case planErr != nil: return failed("%s", planErr) @@ -388,7 +402,12 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p b.Places.Cancel(l.node) } } + var kept []string // build modules whose artifacts stay as the prior done key has them for _, bm := range plan.Modules() { + if prior != nil && len(only) > 0 && !only[bm] { + kept = append(kept, bm) + continue + } language := buildrun.LanguageOf[bm] entries := b.Toolchains.Entries(language) qualifies := func(node string) bool { return b.qualifies(node, bm) } @@ -461,7 +480,23 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p return failed("%s", strings.Join(errs, "; ")) } var artifacts []delivery.BuiltArtifact + keptModule := map[string]bool{} + for _, bm := range kept { + keptModule[bm] = true + } for _, t := range plan.Tasks { + if keptModule[t.Module] { + found := false + for _, a := range prior.Artifacts { + if a.Name == t.Artifact { + artifacts, found = append(artifacts, a), true + } + } + if !found { + return failed("the done build holds no artifact %s to keep", t.Artifact) + } + continue + } byNode := map[string]string{} for _, l := range lanes { if d, ok := l.digests[t.Artifact]; ok && l.module == t.Module { @@ -480,6 +515,14 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p } artifacts = append(artifacts, built) } + for _, bm := range kept { + steps = append(steps, bm+" kept") + for lang, u := range prior.Toolchains { + if _, ok := used[lang]; !ok && buildrun.LanguageOf[bm] == lang { + used[lang] = u + } + } + } 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). diff --git a/modules/mesh-delivery-builder/internal/build/run_test.go b/modules/mesh-delivery-builder/internal/build/run_test.go index 489f174..251fe72 100644 --- a/modules/mesh-delivery-builder/internal/build/run_test.go +++ b/modules/mesh-delivery-builder/internal/build/run_test.go @@ -530,3 +530,79 @@ func TestANodeThatIsNotOnlineIsNotChosenAndComingBackWakesTheBuild(t *testing.T) t.Errorf("build = %+v, want done once a node is online", v) } } + +func TestACandidatesImageIsPublishedOnceWhenTheTrunkNamesItsFingerprint(t *testing.T) { // review r1, mesh-delivery 1; ADR 0487 clause 5 + r := newRig(t, imageArt) + r.b.Health = imageHave(t, "ace") + trunkModule(t, r) // the trunk carries another fingerprint; fp1 is the candidate's + r.b.Candidate(context.Background(), "feat%2Fx", candidateValue(t, "feat/x", 7, headX)) + r.b.Wait() + v := valueOf(t, r.st, r.key) + if v.State != delivery.BuildDone || v.Artifacts[0].Reference != "" { + t.Fatalf("candidate build = %+v, want done with no reference", v) + } + if n := len(r.bm.callsOf("build_image_build")); n != 1 { + t.Fatalf("%d image builds after the candidate, want 1", n) + } + // The pull request is merged: the trunk's record names the candidate's fingerprint. + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) + r.b.Wait() + v = valueOf(t, r.st, r.key) + if v.State != delivery.BuildDone || len(v.Artifacts) != 1 || v.Artifacts[0].Reference == "" { + t.Fatalf("trunk build = %+v, want done with a reference", v) + } + calls := r.bm.callsOf("build_image_build") + published := 0 + for _, c := range calls { + if c.body.(map[string]any)["publish"] == true { + published++ + } + } + if len(calls) != 2 || published != 1 { + t.Errorf("%d image builds, %d with publish, want 2 and exactly 1", len(calls), published) + } + // Written once more only: looking again builds nothing. + rev := r.st.revs[r.key] + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) + r.b.Wait() + if r.st.revs[r.key] != rev || len(r.bm.callsOf("build_image_build")) != 2 { + t.Error("a published key was built again") + } +} + +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) + } +}