From bbc690539b4c468e4889ff62b4080e300ed10106 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 10:19:26 +0200 Subject: [PATCH 1/3] Build an image on one node through mesh-build-image; only a trunk key publishes (issue 1705) --- .../mesh-delivery-builder/integration_test.go | 80 +++++++++++ .../catalogue-a67f0196-imaged.module.json | 14 ++ .../internal/build/claim.go | 8 +- .../internal/build/claim_test.go | 16 +++ .../internal/build/plan.go | 15 +- .../internal/build/plan_test.go | 16 ++- .../internal/build/run.go | 81 +++++++++-- .../internal/build/run_test.go | 129 +++++++++++++++++- .../mesh-delivery-builder/manifest_test.go | 6 +- modules/mesh-delivery-builder/module.json | 3 + 10 files changed, 346 insertions(+), 22 deletions(-) create mode 100644 modules/mesh-delivery-builder/cmd/mesh-delivery-builder/testdata/catalogue-a67f0196-imaged.module.json 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 7affcfc..0236e64 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 @@ -57,6 +57,7 @@ type fakeBuildModules struct { 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 { @@ -88,6 +89,10 @@ func (f *fakeBuildModules) serve(t *testing.T, nc *nats.Conn, module string) { f.mu.Lock() f.fetches = append(f.fetches, args) f.mu.Unlock() + } else { + f.mu.Lock() + f.builds = append(f.builds, args) + f.mu.Unlock() } id := fmt.Sprintf("s%d", f.n.Add(1)) key, _ := delivery.StepKey(node, id) @@ -116,6 +121,9 @@ func (f *fakeBuildModules) serve(t *testing.T, nc *nats.Conn, module string) { step.State, step.Result = delivery.StepPassed, json.RawMessage(`{"workspace":"w`+id+`","commit":"`+commit+`"`+merge+`}`) case f.failBuild.Load() && node == "n1": step.State, step.Reason = delivery.StepFailed, fmt.Sprintf("go build of %v: boom", args["artifact"]) + case tool == "build_image_build" && args["publish"] == true: // a trunk image is pushed: the digest and the reference + d := digestOf(fmt.Sprint(args["artifact"])) + step.State, step.Result = delivery.StepPassed, json.RawMessage(`{"digest":"`+d+`","reference":"registry.example/imaged/server@`+d+`"}`) default: step.State, step.Result = delivery.StepPassed, json.RawMessage(`{"digest":"`+digestOf(fmt.Sprint(args["artifact"]))+`"}`) } @@ -218,6 +226,8 @@ func startBed(t *testing.T) *bed { b.fake = &fakeBuildModules{js: js, ctx: ctx, calls: map[string]int{}} b.fake.serve(t, ctl, "mesh-build-go") b.fake.serve(t, ctl, "mesh-build-node") + b.fake.serve(t, ctl, "mesh-build-image") + b.setHealth(t, "n1", "mesh-build-image", delivery.HealthOK) // the image build module runs on n1 only b.report(t, "mesh-build-go", "n1", "go", "go1.27.1", "1") b.report(t, "mesh-build-go", "n2", "go", "go1.27.1", "1") b.report(t, "mesh-build-node", "n1", "typescript", "tsc5", "1") @@ -582,3 +592,73 @@ func TestBuilderBuildsACandidatesFingerprintFromTheHeadMergedOntoTheTrunk(t *tes t.Errorf("fetches = %v, want one of the candidate's head with its ref and the trunk commit", b.fake.fetches) } } + +// 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). +func TestAnImageIsBuiltOnOneNodeAndOnlyATrunkKeyPublishes(t *testing.T) { // 1705 + b := startBed(t) + digest := b.moduleCommit(t, "imaged", "catalogue-a67f0196-imaged.module.json", "modules/imaged", nil, commit) + b.start(t, "n1", nil) + v := b.await(t, "imaged", 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) + } + 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}}}} + hash, _ := delivery.HeadsHash([]delivery.Head{pulls[0].Head}) + cand, err := delivery.EncodeCandidate(delivery.Candidate{V: delivery.Version, Group: "feat/x", HeadsHash: hash, Pulls: pulls, FPSet: delivery.FingerprintSet(pulls)}) + if err != nil { + t.Fatal(err) + } + ckey, _ := delivery.CandidatesKey("feat/x") + if _, err := b.candidates.Put(b.ctx, ckey, cand); err != nil { + t.Fatal(err) + } + v = b.await(t, "imaged", func(v delivery.BuilderBuild) bool { + return v.State == delivery.BuildDone && len(v.Artifacts) == 1 && v.By != "" + }) + // 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) { + } + 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 + for _, a := range b.fake.builds { + if _, has := a["publish"]; has { + 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) + } +} diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/testdata/catalogue-a67f0196-imaged.module.json b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/testdata/catalogue-a67f0196-imaged.module.json new file mode 100644 index 0000000..eaf4606 --- /dev/null +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/testdata/catalogue-a67f0196-imaged.module.json @@ -0,0 +1,14 @@ +{ + "module": "imaged", + "version": "1", + "slug": "imaged", + "build": { + "artifacts": [ + { + "name": "server", + "kind": "image", + "from": "Dockerfile" + } + ] + } +} diff --git a/modules/mesh-delivery-builder/internal/build/claim.go b/modules/mesh-delivery-builder/internal/build/claim.go index 9ab0285..5acdf7d 100644 --- a/modules/mesh-delivery-builder/internal/build/claim.go +++ b/modules/mesh-delivery-builder/internal/build/claim.go @@ -63,6 +63,9 @@ type ClaimOptions struct { // 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 } // Claim takes the build of module at fingerprint, or answers nil when it is not this builder's to build now: @@ -72,7 +75,8 @@ type ClaimOptions struct { // builder writes the new revision when the second build ends (ADR 0478 clause 4); // - 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 yet (RefusedPrefix, ADR 0478 clause 2) or for digests that differ (buildrun.DigestsDifferPrefix, clause 4), -// which wait for another fingerprint; +// which wait for another fingerprint, and one failed for want of an image builder (NoImageBuilderPrefix), which waits for +// another fingerprint or a node that runs the image build module (ClaimOptions.ImageBuilder); // - a key left queued or building by this builder is taken as it stands (ADR 0478 clause 5: a stopped builder takes its // own again when it starts), and so is one left by another builder when ClaimOptions.TakeOver (clause 6). func (c Claimer) Claim(ctx context.Context, module, fingerprint, manifest string, opt ClaimOptions) (*Claim, error) { @@ -100,6 +104,8 @@ func (c Claimer) Claim(ctx context.Context, module, fingerprint, manifest string return nil, nil case cur.State == delivery.BuildFailed && (strings.HasPrefix(cur.Error, RefusedPrefix) || strings.HasPrefix(cur.Error, buildrun.DigestsDifferPrefix)): return nil, nil + case cur.State == delivery.BuildFailed && strings.HasPrefix(cur.Error, NoImageBuilderPrefix) && (opt.ImageBuilder == nil || !opt.ImageBuilder()): + return nil, nil case cur.State == delivery.BuildFailed: return c.write(ctx, key, building, func(data []byte) (uint64, error) { return c.Store.Update(ctx, key, data, rev) }) case cur.By == c.By || opt.TakeOver: diff --git a/modules/mesh-delivery-builder/internal/build/claim_test.go b/modules/mesh-delivery-builder/internal/build/claim_test.go index a5b2cb9..cab71e3 100644 --- a/modules/mesh-delivery-builder/internal/build/claim_test.go +++ b/modules/mesh-delivery-builder/internal/build/claim_test.go @@ -155,6 +155,22 @@ func TestAFailedKeyIsTakenAgainAsANewRevisionExceptARefusedKind(t *testing.T) { } } +func TestAKeyFailedForNoImageBuilderIsTakenAgainOnlyWhenANodeHasTheModule(t *testing.T) { // 1705b + st := newMem() + key := seed(t, st, delivery.BuilderBuild{State: delivery.BuildFailed, By: "novox.a", Error: NoImageBuilderPrefix + " no node has mesh-build-image"}) + writes := st.writes + c := Claimer{Store: st, By: "novox.a"} + for _, opt := range []ClaimOptions{{}, {ImageBuilder: func() bool { return false }}} { + if got, err := c.Claim(context.Background(), "m", fp1, "", opt); err != nil || got != nil || st.writes != writes { + t.Errorf("claim = %v, %v, writes %d (was %d), want none and no write", got, err, st.writes, writes) + } + } + got, err := c.Claim(context.Background(), "m", fp1, hex64('a'), ClaimOptions{ImageBuilder: func() bool { return true }}) + if err != nil || got == nil || valueOf(t, st, key).State != delivery.BuildBuilding { + t.Errorf("claim = %v, %v, want a claim with the key building", got, err) + } +} + func TestFinishMovesTheKeyByCompareAndSetAndWritesOnlyADifferentValue(t *testing.T) { // 1607/AC3 st := newMem() c := Claimer{Store: st, By: "novox.a"} diff --git a/modules/mesh-delivery-builder/internal/build/plan.go b/modules/mesh-delivery-builder/internal/build/plan.go index edebb4e..dc1f6b7 100644 --- a/modules/mesh-delivery-builder/internal/build/plan.go +++ b/modules/mesh-delivery-builder/internal/build/plan.go @@ -15,6 +15,15 @@ import ( // a key is not built again as a new revision. const RefusedPrefix = "refused kind:" +// ImageBuildModule is the build module that builds an image artifact (novox/hq ADR 0487): buildah, rootless, on the nodes that +// run it. It has no toolchain language; the builder finds its nodes from the node-engine's have. +const ImageBuildModule = "mesh-build-image" + +// NoImageBuilderPrefix opens the error of a build key failed because no node runs ImageBuildModule (ADR 0487 clause 5): such a +// key is not built again as a new revision, only when a node reports the module (ClaimOptions.ImageBuilder) or the fingerprint +// changes. +const NoImageBuilderPrefix = "no image builder:" + // Task is one artifact to build: its name and kind in the manifest, the build module that builds it and the language // of the toolchain it is built with. type Task struct { @@ -45,7 +54,7 @@ func (p Plan) Modules() []string { // (the indexer gives an archive no language, so a fingerprint names no toolchain for it; ADR 0478 clause 1). func (p Plan) NeedsToolchain(module string) bool { for _, t := range p.Tasks { - if t.Module == module && t.Kind != catalogue.ArtifactArchive { + if t.Module == module && t.Kind != catalogue.ArtifactArchive && t.Kind != catalogue.ArtifactImage { return true } } @@ -61,7 +70,7 @@ func (r *Refused) Error() string { } // PlanOf is the plan of a manifest (ADR 0478 clauses 1 and 2): a Go bundle and an archive are built by the Go build -// module, a TypeScript bundle by the Node one, each in the order of the artifacts' names. A manifest with an image, +// module, an image by the image build module (ADR 0487), a TypeScript bundle by the Node one, each in the order of the artifacts' names. A manifest with an // upstream or package artifact, or a bundle of another language, is refused as a whole, naming the first such artifact // by name. A manifest with no artifact has an empty plan. func PlanOf(m catalogue.Manifest) (Plan, error) { @@ -79,6 +88,8 @@ func PlanOf(m catalogue.Manifest) (Plan, error) { p.Tasks = append(p.Tasks, Task{Artifact: a.Name, Kind: a.Kind, Module: buildrun.GoBuildModule, Language: "go"}) case a.Kind == catalogue.ArtifactBundle && a.Language == "typescript": p.Tasks = append(p.Tasks, Task{Artifact: a.Name, Kind: a.Kind, Module: buildrun.NodeBuildModule, Language: "typescript"}) + case a.Kind == catalogue.ArtifactImage: + p.Tasks = append(p.Tasks, Task{Artifact: a.Name, Kind: a.Kind, Module: ImageBuildModule}) case a.Kind == catalogue.ArtifactBundle: return Plan{}, &Refused{Artifact: a.Name, What: fmt.Sprintf("bundle of language %q", a.Language)} default: diff --git a/modules/mesh-delivery-builder/internal/build/plan_test.go b/modules/mesh-delivery-builder/internal/build/plan_test.go index 3760917..d6b18b2 100644 --- a/modules/mesh-delivery-builder/internal/build/plan_test.go +++ b/modules/mesh-delivery-builder/internal/build/plan_test.go @@ -41,12 +41,11 @@ func TestPlanRoutesEachKindToItsBuildModule(t *testing.T) { // 1607/AC2 } } -func TestPlanRefusesImageUpstreamAndPackageByName(t *testing.T) { // 1607/AC2, ADR 0478 clause 2 +func TestPlanRefusesUpstreamAndPackageByName(t *testing.T) { // 1607/AC2, ADR 0478 clause 2 for _, c := range []struct { artifact catalogue.Artifact kind string }{ - {catalogue.Artifact{Name: "img", Kind: "image"}, "image"}, {catalogue.Artifact{Name: "up", Kind: "upstream"}, "upstream"}, {catalogue.Artifact{Name: "pkg", Kind: "package", Language: "typescript"}, "package"}, } { @@ -74,3 +73,16 @@ func TestPlanRefusesABundleOfAnotherLanguageAndAModuleWithNoArtifact(t *testing. } func asRefused(err error, target **Refused) bool { return errors.As(err, target) } + +func TestPlanSendsAnImageToMeshBuildImageWithNoToolchain(t *testing.T) { // 1705a, ADR 0487 clause 5 + p, err := PlanOf(manifestOf(catalogue.Artifact{Name: "img", Kind: "image"}, catalogue.Artifact{Name: "files", Kind: "archive"})) + if err != nil || len(p.Tasks) != 2 { + t.Fatalf("plan %+v, error %v", p, err) + } + if p.Tasks[1].Artifact != "img" || p.Tasks[1].Module != ImageBuildModule { + t.Errorf("task = %+v, want the image on %s", p.Tasks[1], ImageBuildModule) + } + if p.NeedsToolchain(ImageBuildModule) { + t.Error("an image needs no toolchain") + } +} diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index 5f7782d..1c4cbf4 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -10,6 +10,7 @@ import ( "strings" "sync" + "git.novox.be/novox/mesh-sdk/go/catalogue" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -19,7 +20,7 @@ type ToolchainEntries interface { } // toolPrefix is the start of each build module's tool names: fetch and build. -var toolPrefix = map[string]string{buildrun.GoBuildModule: "build_go_", buildrun.NodeBuildModule: "build_node_"} +var toolPrefix = map[string]string{buildrun.GoBuildModule: "build_go_", buildrun.NodeBuildModule: "build_node_", ImageBuildModule: "build_image_"} // Builder builds, dark, every new-form fingerprint the indexer writes (novox/hq ADR 0453 clause 4, ADR 0478): it is // handed each modules value as the watch of the indexer's bucket sees it, and for each module with a fingerprint and an @@ -296,7 +297,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}) + cl, err := b.Claimer.Claim(ctx, module, m.FP2, m.Manifest, ClaimOptions{TakeOver: first, MoreNodes: moreNodes, ImageBuilder: b.imageBuilderRuns}) if err != nil { b.logf("%s: %v", module, err) return @@ -360,6 +361,7 @@ type lane struct { module, node string tasks []Task digests map[string]string // artifact -> digest + references map[string]string // artifact -> the reference an image was pushed to (a trunk build only) err error } @@ -391,7 +393,17 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p entries := b.Toolchains.Entries(language) qualifies := func(node string) bool { return b.qualifies(node, bm) } var nodes []string - if plan.NeedsToolchain(bm) { + if bm == ImageBuildModule { + // An image is built on one node, chosen from the nodes whose image build module runs healthy (ADR 0487 clause 8): + // the module has no toolchain language, so no toolchain entry names its nodes, and a second build would only + // push the image again. + ns, err := buildrun.ChooseAnyNodes("image", b.imageEntries(), b.Places, qualifies, 1) + if err != nil { + giveUpLanes() + return failed("%s no node has %s, so the image cannot be built: the provision rootless-image-build is held by the module buildah; assign buildah and %s to a node (%v)", NoImageBuilderPrefix, ImageBuildModule, ImageBuildModule, err) + } + nodes = ns + } else if plan.NeedsToolchain(bm) { ns, tc, err := buildrun.ChooseNodes(language, entries, b.Places, qualifies, b.Nodes) if err != nil { giveUpLanes() @@ -413,7 +425,9 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p } nodes = ns } - if len(nodes) == 1 { + 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 { @@ -428,7 +442,7 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p } } for _, n := range nodes { - lanes = append(lanes, &lane{module: bm, node: n, tasks: tasks, digests: map[string]string{}}) + lanes = append(lanes, &lane{module: bm, node: n, tasks: tasks, digests: map[string]string{}, references: map[string]string{}}) } } var wg sync.WaitGroup @@ -458,7 +472,13 @@ func (b *Builder) build(ctx context.Context, module string, m delivery.Module, p if err != nil { return failed("%s", err) } - artifacts = append(artifacts, delivery.BuiltArtifact{Name: t.Artifact, Kind: t.Kind, Digest: d}) + built := delivery.BuiltArtifact{Name: t.Artifact, Kind: t.Kind, Digest: d} + for _, l := range lanes { + if r := l.references[t.Artifact]; r != "" && l.module == t.Module { + built.Reference = r + } + } + artifacts = append(artifacts, built) } step := "built " + strings.Join(steps, "; ") if len(oneNode) > 0 { @@ -478,6 +498,39 @@ func (b *Builder) qualifies(node, bm string) bool { return b.Places.CapacityOf(node) > 0 && b.online(node) && (b.Health == nil || b.Health.Healthy(node, bm)) } +// imageEntries are the nodes an image may be built on, as toolchain entries for the chooser: every node the builder has a +// capacity for or a build module toolchain from. Whether the image build module runs there is qualifies', from the have. +func (b *Builder) imageEntries() []delivery.NodeToolchain { + seen := map[string]bool{} + var entries []delivery.NodeToolchain + add := func(n string) { + if !seen[n] { + seen[n] = true + entries = append(entries, delivery.NodeToolchain{Node: n}) + } + } + for n := range b.Places.Capacity { + add(n) + } + for _, language := range []string{"go", "typescript"} { + for _, e := range b.Toolchains.Entries(language) { + add(e.Node) + } + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Node < entries[j].Node }) + return entries +} + +// imageBuilderRuns is whether a node can build an image now. +func (b *Builder) imageBuilderRuns() bool { + for _, e := range b.imageEntries() { + if b.qualifies(e.Node, ImageBuildModule) { + return true + } + } + 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 { @@ -534,7 +587,8 @@ func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m del } // A candidate is built from its head merged onto the trunk. A head that does not merge ends the fetch passed with the tree at // the trunk, and building that would store the trunk's build under the candidate's fingerprint. - if _, candidate := fetchArgs["base"]; candidate { + _, candidate := fetchArgs["base"] + if candidate { if len(fetched.Conflicts) > 0 { l.err = fmt.Errorf("the head does not merge into the trunk on %s: files in conflict: %s", l.node, strings.Join(fetched.Conflicts, ", ")) return @@ -545,18 +599,27 @@ func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m del } } for _, t := range l.tasks { - raw, err := b.step(ctx, l.module, l.node, "build", map[string]any{"workspace": fetched.Workspace, "module": m.Path, "artifact": t.Artifact}) + args := map[string]any{"workspace": fetched.Workspace, "module": m.Path, "artifact": t.Artifact} + if t.Kind == catalogue.ArtifactImage && !candidate { + // Only a trunk key pushes the image to the registry; a candidate's build never does (ADR 0487 clause 12). + args["publish"] = true + } + raw, err := b.step(ctx, l.module, l.node, "build", args) if err != nil { l.err = err return } var built struct { - Digest string `json:"digest"` + Digest string `json:"digest"` + Reference string `json:"reference"` } if err := json.Unmarshal(raw, &built); err != nil || built.Digest == "" { l.err = fmt.Errorf("the build of %s of %s on %s answered no digest: %s", t.Artifact, module, l.node, raw) return } l.digests[t.Artifact] = built.Digest + if built.Reference != "" && !candidate { + l.references[t.Artifact] = built.Reference + } } } diff --git a/modules/mesh-delivery-builder/internal/build/run_test.go b/modules/mesh-delivery-builder/internal/build/run_test.go index 73812c5..489f174 100644 --- a/modules/mesh-delivery-builder/internal/build/run_test.go +++ b/modules/mesh-delivery-builder/internal/build/run_test.go @@ -67,7 +67,11 @@ func (b *buildModules) CallOn(node, key string, body any) (json.RawMessage, erro if v, ok := b.digests[node+"/"+fmt.Sprint(args["artifact"])]; ok { d = v } - step.Result = json.RawMessage(`{"digest":"` + d + `"}`) + if tool == "build_image_build" && args["publish"] == true { + step.Result = json.RawMessage(`{"digest":"` + d + `","reference":"registry.example/m/img@` + d + `"}`) + } else { + step.Result = json.RawMessage(`{"digest":"` + d + `"}`) + } } sk, _ := delivery.StepKey(node, id) b.steps.mu.Lock() @@ -238,11 +242,126 @@ func TestAFailingStepFailsTheBuildWithItsReasonAndNodeAndIsTriedAgainAsANewRevis } } -func TestAModuleWithAnImageGetsARefusedKeyNeverBuiltAgain(t *testing.T) { // 1607/AC2, ADR 0478 clause 2 - r := newRig(t, archiveA, imageArt) +// imageHave marks mesh-build-image running and healthy on these nodes. +func imageHave(t *testing.T, nodes ...string) *buildrun.HaveSet { + t.Helper() + have := &buildrun.HaveSet{} + for _, n := range nodes { + have.Put(n+".mesh-build-go", haveValue(t, delivery.PhaseRunning, delivery.HealthOK), false) + have.Put(n+"."+ImageBuildModule, haveValue(t, delivery.PhaseRunning, delivery.HealthOK), false) + } + return have +} + +func TestAnImageGoesToOneNodeAndGetsOneDoneKeyWithItsReference(t *testing.T) { // 1705a, ADR 0487 clause 5 + r := newRig(t, imageArt) + r.b.Health = imageHave(t, "ace", "novox") // two nodes qualify; an image still builds on one v := r.consider(t, "modules/m") - if v.State != delivery.BuildFailed || !strings.HasPrefix(v.Error, RefusedPrefix) || !strings.Contains(v.Error, "image") || !strings.Contains(v.Error, "build-agent") { - t.Fatalf("build = %+v, want failed 'refused kind: image' naming build-agent", v) + if v.State != delivery.BuildDone || len(v.Artifacts) != 1 || v.Artifacts[0].Kind != "image" || v.Artifacts[0].Digest != digest { + t.Fatalf("build = %+v, want done with one image artifact and its digest", v) + } + 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) + } + writes := r.st.writes + r.consider(t, "modules/m") + if r.st.writes != writes || len(r.bm.callsOf("build_image_build")) != 1 { + t.Errorf("a done image key was built again") + } +} + +func TestAnImageIsBuiltOnlyWhereMeshBuildImageIsHealthy(t *testing.T) { // 1705a + r := newRig(t, imageArt) + r.b.Health = imageHave(t, "novox") + r.consider(t, "modules/m") + c := r.bm.callsOf("build_image_build") + if len(c) != 1 || c[0].node != "novox" { + t.Errorf("calls = %+v, want one on novox", c) + } +} + +func TestABundleAndAnImageOfOneModuleBuildOnTwoNodesAndOne(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_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 + r := newRig(t, imageArt) + r.b.Health = imageHave(t) // mesh-build-image runs nowhere + v := r.consider(t, "modules/m") + if v.State != delivery.BuildFailed || !strings.HasPrefix(v.Error, NoImageBuilderPrefix) || !strings.Contains(v.Error, "rootless-image-build") || !strings.Contains(v.Error, "buildah") || !strings.Contains(v.Error, ImageBuildModule) { + t.Fatalf("build = %+v, want failed naming rootless-image-build, buildah and mesh-build-image", v) + } + if len(r.bm.calls) != 0 { + t.Errorf("%d calls, want none", len(r.bm.calls)) + } + writes := r.st.writes + r.consider(t, "modules/m") + r.consider(t, "modules/m") + if r.st.writes != writes { + t.Errorf("the failed key was written again (%d writes, was %d)", r.st.writes, writes) + } + // A node reports the module: the key is taken again. + r.b.Health = imageHave(t, "ace") + v = r.consider(t, "modules/m") + if v.State != delivery.BuildDone { + t.Errorf("build = %+v, want done once a node reports mesh-build-image", v) + } +} + +func TestACandidatesImageCallCarriesNoPublishAndItsKeyNoReference(t *testing.T) { // 1705b, lead's decision + r := newRig(t, imageArt) + r.b.Health = imageHave(t, "ace") + trunkModule(t, r) + 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 || len(v.Artifacts) != 1 || v.Artifacts[0].Reference != "" { + t.Fatalf("build = %+v, want done with no reference", v) + } + calls := r.bm.callsOf("build_image_build") + if len(calls) != 1 { + t.Fatalf("%d image builds, want 1", len(calls)) + } + if _, has := calls[0].body.(map[string]any)["publish"]; has { + t.Errorf("a candidate's image call carries publish: %+v", calls[0].body) + } +} + +func TestATrunkImageCallCarriesPublish(t *testing.T) { // 1705b + r := newRig(t, imageArt) + r.b.Health = imageHave(t, "ace") + r.consider(t, "modules/m") + calls := r.bm.callsOf("build_image_build") + if len(calls) != 1 || calls[0].body.(map[string]any)["publish"] != true { + t.Errorf("calls = %+v, want publish true", calls) + } +} + +func TestAnUpstreamArtifactIsStillRefusedKeyNeverBuiltAgain(t *testing.T) { // 1607/AC2, ADR 0478 clause 2 + r := newRig(t, archiveA, catalogue.Artifact{Name: "up", Kind: "upstream"}) + v := r.consider(t, "modules/m") + if v.State != delivery.BuildFailed || !strings.HasPrefix(v.Error, RefusedPrefix) || !strings.Contains(v.Error, "upstream") || !strings.Contains(v.Error, "build-agent") { + t.Fatalf("build = %+v, want failed 'refused kind: upstream' naming build-agent", v) } if len(r.bm.calls) != 0 { t.Errorf("%d calls, want none for a refused kind", len(r.bm.calls)) diff --git a/modules/mesh-delivery-builder/manifest_test.go b/modules/mesh-delivery-builder/manifest_test.go index 70f4ca4..28d9afa 100644 --- a/modules/mesh-delivery-builder/manifest_test.go +++ b/modules/mesh-delivery-builder/manifest_test.go @@ -86,12 +86,12 @@ func TestManifestDeclarations(t *testing.T) { // 1607/AC1 if want := []state{{Name: "builds", History: 5}}; !reflect.DeepEqual(m.State, want) { // delivery.BuilderBuildsHistory t.Errorf("state = %+v, want %+v", m.State, want) } - if want := []string{"mesh-build-go.steps", "mesh-build-go.toolchains", "mesh-build-node.steps", "mesh-build-node.toolchains", + if want := []string{"mesh-build-go.steps", "mesh-build-go.toolchains", "mesh-build-image.steps", "mesh-build-node.steps", "mesh-build-node.toolchains", "mesh-delivery-indexer.candidates", "mesh-delivery-indexer.manifests", "mesh-delivery-indexer.modules", "mesh-delivery-watchman.presence", "node-engine.have"}; !reflect.DeepEqual(sorted(m.Reads), want) { t.Errorf("reads = %v, want %v", m.Reads, want) } - if want := []string{"mesh-build-go.build_go_build", "mesh-build-go.build_go_fetch", "mesh-build-node.build_node_build", "mesh-build-node.build_node_fetch"}; !reflect.DeepEqual(sorted(m.Invokes), want) { - t.Errorf("invokes = %v, want only the build modules' fetch and build", m.Invokes) + if want := []string{"mesh-build-go.build_go_build", "mesh-build-go.build_go_fetch", "mesh-build-image.build_image_build", "mesh-build-image.build_image_fetch", "mesh-build-node.build_node_build", "mesh-build-node.build_node_fetch"}; !reflect.DeepEqual(sorted(m.Invokes), want) { + t.Errorf("invokes = %v, want only the build modules' fetch and build (go, image, node)", m.Invokes) } // The builder is woken by a build module's report of a toolchain, as the indexer is: the bucket may not exist when it starts. if want := []string{"mesh-build-go.toolchain.reported", "mesh-build-node.toolchain.reported"}; !reflect.DeepEqual(sorted(m.Consumes), want) { diff --git a/modules/mesh-delivery-builder/module.json b/modules/mesh-delivery-builder/module.json index cb976f3..6d36190 100644 --- a/modules/mesh-delivery-builder/module.json +++ b/modules/mesh-delivery-builder/module.json @@ -12,6 +12,7 @@ "reads": [ "mesh-build-go.steps", "mesh-build-go.toolchains", + "mesh-build-image.steps", "mesh-build-node.steps", "mesh-build-node.toolchains", "mesh-delivery-indexer.candidates", @@ -23,6 +24,8 @@ "invokes": [ "mesh-build-go.build_go_build", "mesh-build-go.build_go_fetch", + "mesh-build-image.build_image_build", + "mesh-build-image.build_image_fetch", "mesh-build-node.build_node_build", "mesh-build-node.build_node_fetch" ], From 12f61998c5c692fe3cc25f34f9efb671ecb92a2d Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 10:33:52 +0200 Subject: [PATCH 2/3] 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) + } +} From db62e82e26d7f3423d115b9fe6739437cae6c007 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 10:37:21 +0200 Subject: [PATCH 3/3] Write a failed publish of a merged image to the key; read mesh-build-image's captured steps (review r2) --- .../internal/build/fixtures_test.go | 41 +++++++++++++++++++ .../internal/build/run.go | 31 +++++++++----- .../internal/build/run_test.go | 25 +++++++++++ .../internal/build/testdata/captured/README | 4 ++ .../testdata/captured/build-candidate.json | 14 +++++++ .../build/testdata/captured/build-trunk.json | 15 +++++++ .../build/testdata/captured/fetch.json | 13 ++++++ 7 files changed, 133 insertions(+), 10 deletions(-) create mode 100644 modules/mesh-delivery-builder/internal/build/fixtures_test.go create mode 100644 modules/mesh-delivery-builder/internal/build/testdata/captured/README create mode 100644 modules/mesh-delivery-builder/internal/build/testdata/captured/build-candidate.json create mode 100644 modules/mesh-delivery-builder/internal/build/testdata/captured/build-trunk.json create mode 100644 modules/mesh-delivery-builder/internal/build/testdata/captured/fetch.json diff --git a/modules/mesh-delivery-builder/internal/build/fixtures_test.go b/modules/mesh-delivery-builder/internal/build/fixtures_test.go new file mode 100644 index 0000000..b5a6566 --- /dev/null +++ b/modules/mesh-delivery-builder/internal/build/fixtures_test.go @@ -0,0 +1,41 @@ +package build + +import ( + "encoding/json" + "os" + "testing" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// result reads a captured step value of mesh-build-image (testdata/captured/README) and returns its result. +func captured(t *testing.T, name string) json.RawMessage { + t.Helper() + raw, err := os.ReadFile("testdata/captured/" + name) + if err != nil { + t.Fatal(err) + } + var step delivery.Step + if err := json.Unmarshal(raw, &step); err != nil { + t.Fatalf("%s: %v", name, err) + } + if step.State != delivery.StepPassed || len(step.Result) == 0 { + t.Fatalf("%s: state %q, result %s, want a passed step with a result", name, step.State, step.Result) + } + return step.Result +} + +func TestCapturedImageStepsDecodeWithTheBuildersOwnTypes(t *testing.T) { // review r2, mesh-catalog 2; ADR 0487 clause 1 + var f fetchResult + if err := json.Unmarshal(captured(t, "fetch.json"), &f); err != nil || f.Workspace == "" { + t.Errorf("fetch = %+v, %v, want a workspace (the field the builder reads)", f, err) + } + var trunk buildResult + if err := json.Unmarshal(captured(t, "build-trunk.json"), &trunk); err != nil || trunk.Digest == "" || trunk.Reference == "" { + t.Errorf("trunk build = %+v, %v, want a digest and a reference", trunk, err) + } + var cand buildResult + if err := json.Unmarshal(captured(t, "build-candidate.json"), &cand); err != nil || cand.Digest == "" || cand.Reference != "" { + t.Errorf("candidate build = %+v, %v, want a digest and no reference", cand, err) + } +} diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index eb60cf1..ea67174 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -310,6 +310,7 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { // 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 + publishing := false if cl.Build.State == delivery.BuildDone { prior, only = &cl.Build, map[string]bool{} for _, bm := range OneNodeModules(cl.Build) { @@ -317,6 +318,7 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { } if trunk && Unpublished(cl.Build) { only[ImageBuildModule] = true + publishing = true } } result := b.build(ctx, module, m, plan, planErr, prior, only) @@ -328,7 +330,11 @@ func (b *Builder) try(ctx context.Context, module string, m delivery.Module) { return } result.By, result.Manifest = b.Claimer.By, m.Manifest - if cl.Build.State == delivery.BuildDone && result.State == delivery.BuildFailed && !strings.HasPrefix(result.Error, buildrun.DigestsDifferPrefix) { + if publishing && result.State == delivery.BuildFailed { + // The trunk's image was built only as a candidate's and its push 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 = "publishing the image of the merged commit 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) @@ -597,6 +603,18 @@ func (b *Builder) step(ctx context.Context, bm, node, tool string, args map[stri return b.Runner.Run(ctx, bm, node, toolPrefix[bm]+tool, args) } +// fetchResult and buildResult are the results of a build module's fetch and build steps as the builder reads them. +type fetchResult struct { + Workspace string `json:"workspace"` + Merge string `json:"merge"` + Conflicts []string `json:"conflicts"` +} + +type buildResult struct { + Digest string `json:"digest"` + Reference string `json:"reference"` +} + // runLane takes a place on the node, fetches the commit there and builds each of the lane's artifacts, one step at a time. func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m delivery.Module, module string) { release, err := b.Places.Acquire(ctx, l.node) @@ -619,11 +637,7 @@ func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m del l.err = err return } - var fetched struct { - Workspace string `json:"workspace"` - Merge string `json:"merge"` - Conflicts []string `json:"conflicts"` - } + var fetched fetchResult if err := json.Unmarshal(raw, &fetched); err != nil || fetched.Workspace == "" { l.err = fmt.Errorf("the fetch on %s answered no workspace: %s", l.node, raw) return @@ -652,10 +666,7 @@ func (b *Builder) runLane(ctx context.Context, l *lane, repository string, m del l.err = err return } - var built struct { - Digest string `json:"digest"` - Reference string `json:"reference"` - } + var built buildResult if err := json.Unmarshal(raw, &built); err != nil || built.Digest == "" { l.err = fmt.Errorf("the build of %s of %s on %s answered no digest: %s", t.Artifact, module, l.node, raw) return diff --git a/modules/mesh-delivery-builder/internal/build/run_test.go b/modules/mesh-delivery-builder/internal/build/run_test.go index 251fe72..6da1ebe 100644 --- a/modules/mesh-delivery-builder/internal/build/run_test.go +++ b/modules/mesh-delivery-builder/internal/build/run_test.go @@ -606,3 +606,28 @@ func TestASecondGoNodeDoesNotBuildOrPushTheImageAgain(t *testing.T) { // review 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") + trunkModule(t, r) + r.b.Candidate(context.Background(), "feat%2Fx", candidateValue(t, "feat/x", 7, headX)) + r.b.Wait() + if v := valueOf(t, r.st, r.key); v.State != delivery.BuildDone { + t.Fatalf("candidate build = %+v", v) + } + r.bm.fails["ace/build_image_build"] = "push to the registry: connection refused" + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) // the merge: the trunk names the fingerprint + r.b.Wait() + v := valueOf(t, r.st, r.key) + if v.State != delivery.BuildFailed || !strings.Contains(v.Error, "connection refused") || !strings.Contains(v.Error, "publish") { + t.Fatalf("build = %+v, want failed naming the publish and its reason", v) + } + // The failed key is retaken as any failed key is, and ends published once the registry answers. + delete(r.bm.fails, "ace/build_image_build") + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) + r.b.Wait() + if v := valueOf(t, r.st, r.key); v.State != delivery.BuildDone || v.Artifacts[0].Reference == "" { + t.Errorf("build = %+v, want done with a reference", v) + } +} diff --git a/modules/mesh-delivery-builder/internal/build/testdata/captured/README b/modules/mesh-delivery-builder/internal/build/testdata/captured/README new file mode 100644 index 0000000..087dcbf --- /dev/null +++ b/modules/mesh-delivery-builder/internal/build/testdata/captured/README @@ -0,0 +1,4 @@ +Captured step values of mesh-build-image (fetch, a candidate's build, a trunk's build), copied from +novox/mesh-catalog modules/mesh-build-image/cmd/mesh-build-image/testdata/captured/. Both copies must change together: the +module's capture test writes them, and the builder's TestCapturedImageStepsDecodeWithTheBuildersOwnTypes (internal/build, +fixtures_test.go) decodes these with the types the builder reads results with and fails on a missing field. diff --git a/modules/mesh-delivery-builder/internal/build/testdata/captured/build-candidate.json b/modules/mesh-delivery-builder/internal/build/testdata/captured/build-candidate.json new file mode 100644 index 0000000..ee22661 --- /dev/null +++ b/modules/mesh-delivery-builder/internal/build/testdata/captured/build-candidate.json @@ -0,0 +1,14 @@ +{ + "ended": "2026-10-22T08:00:01Z", + "id": "STEPID", + "node": "ace", + "result": { + "artifact": "server", + "digest": "sha256:1111111111111111111111111111111111111111111111111111111111111111", + "pushed": false + }, + "started": "2026-10-22T08:00:00Z", + "state": "passed", + "tool": "build_image_build", + "v": 1 +} diff --git a/modules/mesh-delivery-builder/internal/build/testdata/captured/build-trunk.json b/modules/mesh-delivery-builder/internal/build/testdata/captured/build-trunk.json new file mode 100644 index 0000000..db897d3 --- /dev/null +++ b/modules/mesh-delivery-builder/internal/build/testdata/captured/build-trunk.json @@ -0,0 +1,15 @@ +{ + "ended": "2026-10-22T08:00:01Z", + "id": "STEPID", + "node": "ace", + "result": { + "artifact": "server", + "digest": "sha256:1111111111111111111111111111111111111111111111111111111111111111", + "pushed": true, + "reference": "REGISTRY/fixture-image/server@sha256:1111111111111111111111111111111111111111111111111111111111111111" + }, + "started": "2026-10-22T08:00:00Z", + "state": "passed", + "tool": "build_image_build", + "v": 1 +} diff --git a/modules/mesh-delivery-builder/internal/build/testdata/captured/fetch.json b/modules/mesh-delivery-builder/internal/build/testdata/captured/fetch.json new file mode 100644 index 0000000..6f893f4 --- /dev/null +++ b/modules/mesh-delivery-builder/internal/build/testdata/captured/fetch.json @@ -0,0 +1,13 @@ +{ + "ended": "2026-10-22T08:00:01Z", + "id": "STEPID", + "node": "ace", + "result": { + "commit": "COMMIT", + "workspace": "WORKSPACE" + }, + "started": "2026-10-22T08:00:00Z", + "state": "passed", + "tool": "build_image_fetch", + "v": 1 +}