engine: parallize builds with common re-usable images. Fixes #3468 (#…

…3566)

internal/engine/buildcontrol/build_control.go

@@ -95,27 +95,71 @@ func isWaitingOnDependencies(state store.EngineState, mt *store.ManifestTarget)
 	return false
 }
 
+// Check to see if this is an ImageTarget where the built image
+// can be potentially reused.
+//
+// Note that this is a quick heuristic check for making parallelization decisions.
+//
+// The "correct" decision about whether an image can be re-used is more complex
+// and expensive, and includes:
+//
+// 1) Checks of dependent images
+// 2) Live-update sync checks
+// 3) Checks that the image still exists on the image store
+//
+// But in this particular context, we can cheat a bit.
+func canReuseImageTargetHeuristic(spec model.TargetSpec, status store.BuildStatus) bool {
+	id := spec.ID()
+	if id.Type != model.TargetTypeImage {
+		return false
+	}
+
+	// NOTE(nick): A more accurate check might see if the pending file changes
+	// are potentially live-updatable, but this is OK for the case of a base image.
+	if len(status.PendingFileChanges) > 0 || len(status.PendingDependencyChanges) > 0 {
+		return false
+	}
+
+	result := status.LastResult
+	if result == nil {
+		return false
+	}
+
+	switch result.(type) {
+	case store.ImageBuildResult, store.LiveUpdateBuildResult:
+		return true
+	}
+	return false
+}
+
 func RemoveTargetsWithBuildingComponents(mts []*store.ManifestTarget) []*store.ManifestTarget {
 	building := make(map[model.TargetID]bool)
 
 	for _, mt := range mts {
 		if mt.State.IsBuilding() {
 			building[mt.Manifest.ID()] = true
 
-			// TODO(nick): This logic isn't quite right.  A manifest can re-use image
-			// results from another manifest's build.
 			for _, spec := range mt.Manifest.TargetSpecs() {
+				if canReuseImageTargetHeuristic(spec, mt.State.BuildStatus(spec.ID())) {
+					continue
+				}
+
 				building[spec.ID()] = true
 			}
 		}
 	}
 
-	isBuilding := func(m model.Manifest) bool {
+	hasBuildingComponent := func(mt *store.ManifestTarget) bool {
+		m := mt.Manifest
 		if building[m.ID()] {
 			return true
 		}
 
 		for _, spec := range m.TargetSpecs() {
+			if canReuseImageTargetHeuristic(spec, mt.State.BuildStatus(spec.ID())) {
+				continue
+			}
+
 			if building[spec.ID()] {
 				return true
 			}
@@ -125,7 +169,7 @@ func RemoveTargetsWithBuildingComponents(mts []*store.ManifestTarget) []*store.M
 
 	var ret []*store.ManifestTarget
 	for _, mt := range mts {
-		if !isBuilding(mt.Manifest) {
+		if !hasBuildingComponent(mt) {
 			ret = append(ret, mt)
 		}
 	}

internal/engine/buildcontrol/build_control_test.go

@@ -108,6 +108,36 @@ func TestTwoK8sTargetsWithBaseImage(t *testing.T) {
 	f.assertNextTargetToBuild("sancho-two")
 }
 
+func TestTwoK8sTargetsWithBaseImagePrebuilt(t *testing.T) {
+	f := newTestFixture(t)
+	defer f.TearDown()
+
+	baseImage := model.MustNewImageTarget(container.MustParseSelector("sancho-base"))
+	sanchoOneImage := model.MustNewImageTarget(container.MustParseSelector("sancho-one")).
+		WithDependencyIDs([]model.TargetID{baseImage.ID()})
+	sanchoTwoImage := model.MustNewImageTarget(container.MustParseSelector("sancho-two")).
+		WithDependencyIDs([]model.TargetID{baseImage.ID()})
+
+	sanchoOne := f.upsertManifest(manifestbuilder.New(f, "sancho-one").
+		WithImageTargets(baseImage, sanchoOneImage).
+		WithK8sYAML(testyaml.SanchoYAML).
+		Build())
+	sanchoTwo := f.upsertManifest(manifestbuilder.New(f, "sancho-two").
+		WithImageTargets(baseImage, sanchoTwoImage).
+		WithK8sYAML(testyaml.SanchoYAML).
+		Build())
+
+	sanchoOne.State.MutableBuildStatus(baseImage.ID()).LastResult = store.ImageBuildResult{}
+	sanchoTwo.State.MutableBuildStatus(baseImage.ID()).LastResult = store.ImageBuildResult{}
+
+	f.assertNextTargetToBuild("sancho-one")
+
+	sanchoOne.State.CurrentBuild = model.BuildRecord{StartTime: time.Now()}
+
+	// Make sure sancho-two can start while sanchoOne is still pending.
+	f.assertNextTargetToBuild("sancho-two")
+}
+
 type testFixture struct {
 	*tempdir.TempDirFixture
 	t  *testing.T