Publish a merged candidate's image once; a retaken key builds only what is missing (review r1)

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)
 	}
 }

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

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).

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)
+	}
+}