engine: build and deployers should return results for targets they bu…

…ilt, and (#3374)

not return results for targets they didn't build.

This should make #3362
much simpler and easy to reason about.

Also adds test harnesses for different build graph configurations.

internal/engine/build_and_deployer.go

@@ -18,11 +18,15 @@ import (
 
 type BuildAndDeployer interface {
 	// BuildAndDeploy builds and deployed the specified target specs.
-	//
-	// Returns a BuildResult that expresses the outputs(s) of the build.
+
+	// Returns a BuildResultSet containing output (build result and associated
+	// file changes) for each target built in this call. The BuildResultSet only
+	// contains results for newly built targets--if a target was clean and didn't
+	// need to be built, it doesn't appear in the result set.
 	//
 	// BuildResult can be used to construct a set of BuildStates, which contain
-	// the last successful builds of each target and the files changed since that build.
+	// the last successful builds of each target and the files changed since that
+	// build.
 	BuildAndDeploy(ctx context.Context, st store.RStore, specs []model.TargetSpec, currentState store.BuildStateSet) (store.BuildResultSet, error)
 }
 

internal/engine/buildcontrol/target_queue.go

@@ -90,18 +90,37 @@ func NewImageTargetQueue(ctx context.Context, iTargets []model.ImageTarget, stat
 	}, nil
 }
 
-func (q *TargetQueue) Results() store.BuildResultSet {
-	return q.results
+// New results that were built with the current queue. Omits results
+// that were re-used previous builds.
+//
+// Returns results that the BuildAndDeploy contract expects.
+func (q *TargetQueue) NewResults() store.BuildResultSet {
+	newResults := store.BuildResultSet{}
+	for id, result := range q.results {
+		if q.isBuilding(id) {
+			newResults[id] = result
+		}
+	}
+	return newResults
+}
+
+// All results for targets in the current queue.
+func (q *TargetQueue) AllResults() store.BuildResultSet {
+	allResults := store.BuildResultSet{}
+	for id, result := range q.results {
+		allResults[id] = result
+	}
+	return allResults
 }
 
-func (q *TargetQueue) IsDirty(id model.TargetID) bool {
+func (q *TargetQueue) isBuilding(id model.TargetID) bool {
 	return q.needsOwnBuild[id] || q.depsNeedBuild[id]
 }
 
-func (q *TargetQueue) CountDirty() int {
+func (q *TargetQueue) CountBuilds() int {
 	result := 0
 	for _, target := range q.sortedTargets {
-		if q.IsDirty(target.ID()) {
+		if q.isBuilding(target.ID()) {
 			result++
 		}
 	}
@@ -111,7 +130,7 @@ func (q *TargetQueue) CountDirty() int {
 func (q *TargetQueue) RunBuilds(handler BuildHandler) error {
 	for _, target := range q.sortedTargets {
 		id := target.ID()
-		if q.IsDirty(id) {
+		if q.isBuilding(id) {
 			result, err := handler(target, q.dependencyResults(target))
 			if err != nil {
 				return err

internal/engine/docker_compose_build_and_deployer.go

@@ -73,7 +73,7 @@ func (bd *DockerComposeBuildAndDeployer) BuildAndDeploy(ctx context.Context, st
 		return store.BuildResultSet{}, err
 	}
 
-	numStages := q.CountDirty()
+	numStages := q.CountBuilds()
 	haveImage := len(iTargets) > 0
 
 	ps := build.NewPipelineState(ctx, numStages, bd.clock)
@@ -109,22 +109,23 @@ func (bd *DockerComposeBuildAndDeployer) BuildAndDeploy(ctx context.Context, st
 		return store.NewImageBuildResultSingleRef(iTarget.ID(), ref), nil
 	})
 
+	newResults := q.NewResults()
 	if err != nil {
-		return store.BuildResultSet{}, err
+		return newResults, err
 	}
 
 	stdout := logger.Get(ctx).Writer(logger.InfoLvl)
 	stderr := logger.Get(ctx).Writer(logger.InfoLvl)
 	err = bd.dcc.Up(ctx, dcTarget.ConfigPaths, dcTarget.Name, !haveImage, stdout, stderr)
 	if err != nil {
-		return store.BuildResultSet{}, err
+		return newResults, err
 	}
 
 	// NOTE(dmiller): right now we only need this the first time. In the future
 	// it might be worth it to move this somewhere else
 	cid, err := bd.dcc.ContainerID(ctx, dcTarget.ConfigPaths, dcTarget.Name)
 	if err != nil {
-		return store.BuildResultSet{}, err
+		return newResults, err
 	}
 
 	// grab the initial container state
@@ -138,9 +139,8 @@ func (bd *DockerComposeBuildAndDeployer) BuildAndDeploy(ctx context.Context, st
 		containerState = containerJSON.ContainerJSONBase.State
 	}
 
-	results := q.Results()
-	results[dcTarget.ID()] = store.NewDockerComposeDeployResult(dcTarget.ID(), cid, containerState)
-	return results, nil
+	newResults[dcTarget.ID()] = store.NewDockerComposeDeployResult(dcTarget.ID(), cid, containerState)
+	return newResults, nil
 }
 
 // tagWithExpected tags the given ref as whatever Docker Compose expects, i.e. as

internal/engine/image_build_and_deployer.go

@@ -124,7 +124,7 @@ func (ibd *ImageBuildAndDeployer) BuildAndDeploy(ctx context.Context, st store.R
 	}
 
 	// each image target has two stages: one for build, and one for push
-	numStages := q.CountDirty()*2 + 1
+	numStages := q.CountBuilds()*2 + 1
 
 	ps := build.NewPipelineState(ctx, numStages, ibd.clock)
 	defer func() { ps.End(ctx, err) }()
@@ -156,14 +156,20 @@ func (ibd *ImageBuildAndDeployer) BuildAndDeploy(ctx context.Context, st store.R
 		anyLiveUpdate = anyLiveUpdate || !iTarget.LiveUpdateInfo().Empty()
 		return store.NewImageBuildResult(iTarget.ID(), refs.LocalRef, refs.ClusterRef), nil
 	})
+
+	newResults := q.NewResults()
 	if err != nil {
-		return store.BuildResultSet{}, buildcontrol.WrapDontFallBackError(err)
+		return newResults, buildcontrol.WrapDontFallBackError(err)
 	}
 
 	// (If we pass an empty list of refs here (as we will do if only deploying
 	// yaml), we just don't inject any image refs into the yaml, nbd.
-	brs, err := ibd.deploy(ctx, st, ps, iTargetMap, kTarget, q.Results(), anyLiveUpdate)
-	return brs, buildcontrol.WrapDontFallBackError(err)
+	k8sResult, err := ibd.deploy(ctx, st, ps, iTargetMap, kTarget, q.AllResults(), anyLiveUpdate)
+	if err != nil {
+		return newResults, buildcontrol.WrapDontFallBackError(err)
+	}
+	newResults[kTarget.ID()] = k8sResult
+	return newResults, nil
 }
 
 func (ibd *ImageBuildAndDeployer) push(ctx context.Context, ref reference.NamedTagged, ps *build.PipelineState, iTarget model.ImageTarget, kTarget model.K8sTarget) error {
@@ -223,7 +229,7 @@ func (ibd *ImageBuildAndDeployer) shouldUseKINDLoad(ctx context.Context, iTarg m
 
 // Returns: the entities deployed and the namespace of the pod with the given image name/tag.
 func (ibd *ImageBuildAndDeployer) deploy(ctx context.Context, st store.RStore, ps *build.PipelineState,
-	iTargetMap map[model.TargetID]model.ImageTarget, kTarget model.K8sTarget, results store.BuildResultSet, needsSynclet bool) (store.BuildResultSet, error) {
+	iTargetMap map[model.TargetID]model.ImageTarget, kTarget model.K8sTarget, results store.BuildResultSet, needsSynclet bool) (store.BuildResult, error) {
 	ps.StartPipelineStep(ctx, "Deploying")
 	defer ps.EndPipelineStep(ctx)
 
@@ -266,9 +272,8 @@ func (ibd *ImageBuildAndDeployer) deploy(ctx context.Context, st store.RStore, p
 		}
 		podTemplateSpecHashes = append(podTemplateSpecHashes, hs...)
 	}
-	results[kTarget.ID()] = store.NewK8sDeployResult(kTarget.ID(), uids, podTemplateSpecHashes, deployed)
 
-	return results, nil
+	return store.NewK8sDeployResult(kTarget.ID(), uids, podTemplateSpecHashes, deployed), nil
 }
 
 func (ibd *ImageBuildAndDeployer) indentLogger(ctx context.Context) context.Context {

internal/engine/image_build_and_deployer_test.go

@@ -4,13 +4,14 @@ import (
 	"archive/tar"
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
-	"github.com/opencontainers/go-digest"
+	digest "github.com/opencontainers/go-digest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tilt-dev/wmclient/pkg/dirs"
@@ -150,8 +151,6 @@ func TestImageIsClean(t *testing.T) {
 	iTargetID1 := manifest.ImageTargets[0].ID()
 	result1 := store.NewImageBuildResultSingleRef(iTargetID1, container.MustParseNamedTagged("sancho-base:tilt-prebuilt1"))
 
-	f.docker.ImageListCount = 1
-
 	stateSet := store.BuildStateSet{
 		iTargetID1: store.NewBuildState(result1, []string{}),
 	}
@@ -311,8 +310,6 @@ func TestMultiStageDockerBuildWithSecondImageDirty(t *testing.T) {
 
 	newFile := f.WriteFile("sancho/message.txt", "message")
 
-	f.docker.ImageListCount = 1
-
 	stateSet := store.BuildStateSet{
 		iTargetID1: store.NewBuildState(result1, nil),
 		iTargetID2: store.NewBuildState(result2, []string{newFile}),
@@ -794,6 +791,77 @@ func TestDockerBuildTargetStage(t *testing.T) {
 	assert.Equal(t, "stage", f.docker.BuildOptions.Target)
 }
 
+func TestTwoManifestsWithCommonImage(t *testing.T) {
+	f := newIBDFixture(t, k8s.EnvGKE)
+	defer f.TearDown()
+
+	m1, m2 := NewManifestsWithCommonAncestor(f)
+	results1, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m1), store.BuildStateSet{})
+	require.NoError(t, err)
+	assert.Equal(t,
+		[]string{"image:gcr.io/common", "image:gcr.io/image-1", "k8s:image-1"},
+		resultKeys(results1))
+
+	stateSet := f.resultsToNextState(results1)
+
+	results2, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m2), stateSet)
+	require.NoError(t, err)
+	assert.Equal(t,
+		// We did not return image-common because it didn't need a rebuild.
+		[]string{"image:gcr.io/image-2", "k8s:image-2"},
+		resultKeys(results2))
+}
+
+func TestTwoManifestsWithTwoCommonAncestors(t *testing.T) {
+	f := newIBDFixture(t, k8s.EnvGKE)
+	defer f.TearDown()
+
+	m1, m2 := NewManifestsWithTwoCommonAncestors(f)
+	results1, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m1), store.BuildStateSet{})
+	require.NoError(t, err)
+	assert.Equal(t,
+		[]string{"image:gcr.io/base", "image:gcr.io/common", "image:gcr.io/image-1", "k8s:image-1"},
+		resultKeys(results1))
+
+	stateSet := f.resultsToNextState(results1)
+
+	results2, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m2), stateSet)
+	require.NoError(t, err)
+	assert.Equal(t,
+		// We did not return image-common because it didn't need a rebuild.
+		[]string{"image:gcr.io/image-2", "k8s:image-2"},
+		resultKeys(results2))
+}
+
+func TestTwoManifestsWithSameTwoImages(t *testing.T) {
+	f := newIBDFixture(t, k8s.EnvGKE)
+	defer f.TearDown()
+
+	m1, m2 := NewManifestsWithSameTwoImages(f)
+	results1, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m1), store.BuildStateSet{})
+	require.NoError(t, err)
+	assert.Equal(t,
+		[]string{"image:gcr.io/common", "image:gcr.io/image-1", "k8s:dep-1"},
+		resultKeys(results1))
+
+	stateSet := f.resultsToNextState(results1)
+
+	results2, err := f.ibd.BuildAndDeploy(f.ctx, f.st, buildTargets(m2), stateSet)
+	require.NoError(t, err)
+	assert.Equal(t,
+		[]string{"k8s:dep-2"},
+		resultKeys(results2))
+}
+
+func resultKeys(result store.BuildResultSet) []string {
+	keys := []string{}
+	for id := range result {
+		keys = append(keys, id.String())
+	}
+	sort.Strings(keys)
+	return keys
+}
+
 type ibdFixture struct {
 	*tempdir.TempDirFixture
 	ctx    context.Context
@@ -807,7 +875,13 @@ type ibdFixture struct {
 func newIBDFixture(t *testing.T, env k8s.Env) *ibdFixture {
 	f := tempdir.NewTempDirFixture(t)
 	dir := dirs.NewWindmillDirAt(f.Path())
+
 	docker := docker.NewFakeClient()
+
+	// Setting ImageListCount makes the fake ImageExists always return true,
+	// which is the behavior we want when testing the ImageBuildAndDeployer.
+	docker.ImageListCount = 1
+
 	ctx, _, ta := testutils.CtxAndAnalyticsForTest()
 	kClient := k8s.NewFakeK8sClient()
 	kl := &fakeKINDLoader{}
@@ -832,6 +906,14 @@ func (f *ibdFixture) TearDown() {
 	f.TempDirFixture.TearDown()
 }
 
+func (f *ibdFixture) resultsToNextState(results store.BuildResultSet) store.BuildStateSet {
+	stateSet := store.BuildStateSet{}
+	for id, result := range results {
+		stateSet[id] = store.NewBuildState(result, nil)
+	}
+	return stateSet
+}
+
 func (f *ibdFixture) replaceRegistry(defaultReg string, sel container.RefSelector) reference.Named {
 	reg := container.MustNewRegistry(defaultReg)
 	named, err := reg.ReplaceRegistryForLocalRef(sel)

internal/engine/testdata_test.go

@@ -288,11 +288,11 @@ func NewManifestsWithCommonAncestor(fixture Fixture) (model.Manifest, model.Mani
 	target1 := model.MustNewImageTarget(ref1).WithBuildDetails(model.DockerBuild{
 		Dockerfile: `FROM ` + refCommon.String(),
 		BuildPath:  fixture.JoinPath("image-1"),
-	})
+	}).WithDependencyIDs([]model.TargetID{targetCommon.ID()})
 	target2 := model.MustNewImageTarget(ref2).WithBuildDetails(model.DockerBuild{
 		Dockerfile: `FROM ` + refCommon.String(),
 		BuildPath:  fixture.JoinPath("image-2"),
-	})
+	}).WithDependencyIDs([]model.TargetID{targetCommon.ID()})
 
 	m1 := manifestbuilder.New(fixture, "image-1").
 		WithK8sYAML(testyaml.Deployment("image-1", ref1.String())).
@@ -305,6 +305,72 @@ func NewManifestsWithCommonAncestor(fixture Fixture) (model.Manifest, model.Mani
 	return m1, m2
 }
 
+func NewManifestsWithTwoCommonAncestors(fixture Fixture) (model.Manifest, model.Manifest) {
+	refBase := container.MustParseSelector("gcr.io/base")
+	refCommon := container.MustParseSelector("gcr.io/common")
+	ref1 := container.MustParseSelector("gcr.io/image-1")
+	ref2 := container.MustParseSelector("gcr.io/image-2")
+
+	fixture.MkdirAll("base")
+	fixture.MkdirAll("common")
+	fixture.MkdirAll("image-1")
+	fixture.MkdirAll("image-2")
+
+	targetBase := model.MustNewImageTarget(refBase).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM golang:1.10`,
+		BuildPath:  fixture.JoinPath("base"),
+	})
+	targetCommon := model.MustNewImageTarget(refCommon).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM ` + refBase.String(),
+		BuildPath:  fixture.JoinPath("common"),
+	}).WithDependencyIDs([]model.TargetID{targetBase.ID()})
+	target1 := model.MustNewImageTarget(ref1).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM ` + refCommon.String(),
+		BuildPath:  fixture.JoinPath("image-1"),
+	}).WithDependencyIDs([]model.TargetID{targetCommon.ID()})
+	target2 := model.MustNewImageTarget(ref2).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM ` + refCommon.String(),
+		BuildPath:  fixture.JoinPath("image-2"),
+	}).WithDependencyIDs([]model.TargetID{targetCommon.ID()})
+
+	m1 := manifestbuilder.New(fixture, "image-1").
+		WithK8sYAML(testyaml.Deployment("image-1", ref1.String())).
+		WithImageTargets(targetBase, targetCommon, target1).
+		Build()
+	m2 := manifestbuilder.New(fixture, "image-2").
+		WithK8sYAML(testyaml.Deployment("image-2", ref2.String())).
+		WithImageTargets(targetBase, targetCommon, target2).
+		Build()
+	return m1, m2
+}
+
+func NewManifestsWithSameTwoImages(fixture Fixture) (model.Manifest, model.Manifest) {
+	refCommon := container.MustParseSelector("gcr.io/common")
+	ref1 := container.MustParseSelector("gcr.io/image-1")
+
+	fixture.MkdirAll("common")
+	fixture.MkdirAll("image-1")
+
+	targetCommon := model.MustNewImageTarget(refCommon).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM golang:1.10`,
+		BuildPath:  fixture.JoinPath("common"),
+	})
+	target1 := model.MustNewImageTarget(ref1).WithBuildDetails(model.DockerBuild{
+		Dockerfile: `FROM ` + refCommon.String(),
+		BuildPath:  fixture.JoinPath("image-1"),
+	}).WithDependencyIDs([]model.TargetID{targetCommon.ID()})
+
+	m1 := manifestbuilder.New(fixture, "dep-1").
+		WithK8sYAML(testyaml.Deployment("dep-1", ref1.String())).
+		WithImageTargets(targetCommon, target1).
+		Build()
+	m2 := manifestbuilder.New(fixture, "dep-2").
+		WithK8sYAML(testyaml.Deployment("dep-2", ref1.String())).
+		WithImageTargets(targetCommon, target1).
+		Build()
+	return m1, m2
+}
+
 func assembleLiveUpdate(syncs []model.LiveUpdateSyncStep, runs []model.LiveUpdateRunStep, shouldRestart bool, fallBackOn []string, f Fixture) model.LiveUpdate {
 	var steps []model.LiveUpdateStep
 	if len(fallBackOn) > 0 {