From f9d466c5b4ed1517d1abdec9939918ad57306558 Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Tue, 7 Jul 2020 11:50:40 -0400 Subject: [PATCH] engine: roll forward of https://github.com/tilt-dev/tilt/pull/3362, re-using image builds from other resources (#3553) --- internal/engine/buildcontroller_test.go | 130 ++++++++++++++++++++++++ internal/engine/upper.go | 79 +++++++++++++- internal/engine/upper_test.go | 27 ++++- internal/store/engine_state.go | 27 ++++- internal/store/engine_state_test.go | 11 +- pkg/model/build_reason.go | 5 + pkg/model/manifest.go | 16 +++ 7 files changed, 285 insertions(+), 10 deletions(-) diff --git a/internal/engine/buildcontroller_test.go b/internal/engine/buildcontroller_test.go index cd05925fc9..c407c0270b 100644 --- a/internal/engine/buildcontroller_test.go +++ b/internal/engine/buildcontroller_test.go @@ -1265,6 +1265,136 @@ func TestErrorHandlingWithMultipleBuilds(t *testing.T) { f.assertAllBuildsConsumed() } +func TestManifestsWithSameTwoImages(t *testing.T) { + f := newTestFixture(t) + m1, m2 := NewManifestsWithSameTwoImages(f) + f.Start([]model.Manifest{m1, m2}) + + f.waitForCompletedBuildCount(2) + + call := f.nextCall("m1 build1") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + call = f.nextCall("m2 build1") + assert.Equal(t, m2.K8sTarget(), call.k8s()) + + aPath := f.JoinPath("common", "a.txt") + f.fsWatcher.Events <- watch.NewFileEvent(aPath) + + f.waitForCompletedBuildCount(4) + + // Make sure that both builds are triggered, and that they + // are triggered in a particular order. + call = f.nextCall("m1 build2") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + state := call.state[m1.ImageTargets[0].ID()] + assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet) + + // Make sure that when the second build is triggered, we did the bookkeeping + // correctly around marking the first and second image built and only deploying + // the k8s resources. + call = f.nextCall("m2 build2") + assert.Equal(t, m2.K8sTarget(), call.k8s()) + + id := m2.ImageTargets[0].ID() + result := f.b.resultsByID[id] + assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id]) + + id = m2.ImageTargets[1].ID() + result = f.b.resultsByID[id] + assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id]) + + err := f.Stop() + assert.NoError(t, err) + f.assertAllBuildsConsumed() +} + +func TestManifestsWithTwoCommonAncestors(t *testing.T) { + f := newTestFixture(t) + m1, m2 := NewManifestsWithTwoCommonAncestors(f) + f.Start([]model.Manifest{m1, m2}) + + f.waitForCompletedBuildCount(2) + + call := f.nextCall("m1 build1") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + call = f.nextCall("m2 build1") + assert.Equal(t, m2.K8sTarget(), call.k8s()) + + aPath := f.JoinPath("base", "a.txt") + f.fsWatcher.Events <- watch.NewFileEvent(aPath) + + f.waitForCompletedBuildCount(4) + + // Make sure that both builds are triggered, and that they + // are triggered in a particular order. + call = f.nextCall("m1 build2") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + state := call.state[m1.ImageTargets[0].ID()] + assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet) + + // Make sure that when the second build is triggered, we did the bookkeeping + // correctly around marking the first and second image built, and only + // rebuilding the third image and k8s deploy. + call = f.nextCall("m2 build2") + assert.Equal(t, m2.K8sTarget(), call.k8s()) + + id := m2.ImageTargets[0].ID() + result := f.b.resultsByID[id] + assert.Equal(t, + store.NewBuildState(result, nil, nil), + call.state[id]) + + id = m2.ImageTargets[1].ID() + result = f.b.resultsByID[id] + assert.Equal(t, + store.NewBuildState(result, nil, nil), + call.state[id]) + + id = m2.ImageTargets[2].ID() + result = f.b.resultsByID[id] + + // Assert the 3rd image was not reused from the previous build. + assert.NotEqual(t, result, call.state[id].LastResult) + assert.Equal(t, + map[model.TargetID]bool{m2.ImageTargets[1].ID(): true}, + call.state[id].DepsChangedSet) + + err := f.Stop() + assert.NoError(t, err) + f.assertAllBuildsConsumed() +} + +func TestManifestsWithCommonAncestorAndTrigger(t *testing.T) { + f := newTestFixture(t) + m1, m2 := NewManifestsWithCommonAncestor(f) + f.Start([]model.Manifest{m1, m2}) + + f.waitForCompletedBuildCount(2) + + call := f.nextCall("m1 build1") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + call = f.nextCall("m2 build1") + assert.Equal(t, m2.K8sTarget(), call.k8s()) + + f.store.Dispatch(server.AppendToTriggerQueueAction{Name: m1.Name}) + f.waitForCompletedBuildCount(3) + + // Make sure that only one build was triggered. + call = f.nextCall("m1 build2") + assert.Equal(t, m1.K8sTarget(), call.k8s()) + + f.assertNoCall("m2 should not be rebuilt") + + err := f.Stop() + assert.NoError(t, err) + f.assertAllBuildsConsumed() +} + func (f *testFixture) waitUntilManifestBuilding(name model.ManifestName) { msg := fmt.Sprintf("manifest %q is building", name) f.WaitUntilManifestState(msg, name, func(ms store.ManifestState) bool { diff --git a/internal/engine/upper.go b/internal/engine/upper.go index 50ce9cf953..726e06c6a4 100644 --- a/internal/engine/upper.go +++ b/internal/engine/upper.go @@ -234,22 +234,91 @@ func handleBuildResults(engineState *store.EngineState, isBuildSuccess := br.Error == nil ms := mt.State + mn := mt.Manifest.Name for id, result := range results { ms.MutableBuildStatus(id).LastResult = result } // Remove pending file changes that were consumed by this build. for _, status := range ms.BuildStatuses { - for file, modTime := range status.PendingFileChanges { - if store.BeforeOrEqual(modTime, br.StartTime) { - delete(status.PendingFileChanges, file) - } - } + status.ClearPendingChangesBefore(br.StartTime) } if isBuildSuccess { ms.LastSuccessfulDeployTime = br.FinishTime } + + // Update build statuses for duplicated image targets in other manifests. + // This ensures that those images targets aren't redundantly rebuilt. + for _, currentMT := range engineState.TargetsBesides(mn) { + // We only want to update image targets for Manifests that are already queued + // for rebuild and not currently building. This has two benefits: + // + // 1) If there's a bug in Tilt's image caching (e.g., + // https://github.com/tilt-dev/tilt/pull/3542), this prevents infinite + // builds. + // + // 2) If the current manifest build was kicked off by a trigger, we don't + // want to queue manifests with the same image. + if currentMT.NextBuildReason() == model.BuildReasonNone || + engineState.IsCurrentlyBuilding(currentMT.Manifest.Name) { + continue + } + + currentMS := currentMT.State + idSet := currentMT.Manifest.TargetIDSet() + updatedIDSet := make(map[model.TargetID]bool) + + for id, result := range results { + _, ok := idSet[id] + if !ok { + continue + } + + // We can only reuse image update, not live-updates or other kinds of + // deploys. + _, isImageResult := result.(store.ImageBuildResult) + if !isImageResult { + continue + } + + currentStatus := currentMS.MutableBuildStatus(id) + currentStatus.LastResult = result + currentStatus.ClearPendingChangesBefore(br.StartTime) + updatedIDSet[id] = true + } + + if len(updatedIDSet) == 0 { + continue + } + + // Suppose we built manifestA, which contains imageA depending on imageCommon. + // + // We also have manifestB, which contains imageB depending on imageCommon. + // + // We need to mark imageB as dirty, because it was not built in the manifestA + // build but its dependency was built. + // + // Note that this logic also applies to deploy targets depending on image + // targets. If we built manifestA, which contains imageX and deploy target + // k8sA, and manifestB contains imageX and deploy target k8sB, we need to mark + // target k8sB as dirty so that Tilt actually deploys the changes to imageX. + rDepsMap := currentMT.Manifest.ReverseDependencyIDs() + for updatedID := range updatedIDSet { + + // Go through each target depending on an image we just built. + for _, rDepID := range rDepsMap[updatedID] { + + // If that target was also built, it's up-to-date. + if updatedIDSet[rDepID] { + continue + } + + // Otherwise, we need to mark it for rebuild to pick up the new image. + currentMS.MutableBuildStatus(rDepID).PendingDependencyChanges[updatedID] = br.StartTime + } + } + } } func handleBuildCompleted(ctx context.Context, engineState *store.EngineState, cb buildcontrol.BuildCompleteAction) { diff --git a/internal/engine/upper_test.go b/internal/engine/upper_test.go index f63c8975d8..5854070427 100644 --- a/internal/engine/upper_test.go +++ b/internal/engine/upper_test.go @@ -2827,8 +2827,10 @@ func TestWatchManifestsWithCommonAncestor(t *testing.T) { call = f.nextCall("m2 build1") assert.Equal(t, m2.K8sTarget(), call.k8s()) - f.WriteFile("common/a.txt", "hello world") - f.fsWatcher.Events <- watch.NewFileEvent(f.JoinPath("common/a.txt")) + f.WriteFile(filepath.Join("common", "a.txt"), "hello world") + + aPath := f.JoinPath("common", "a.txt") + f.fsWatcher.Events <- watch.NewFileEvent(aPath) f.waitForCompletedBuildCount(4) @@ -2837,9 +2839,30 @@ func TestWatchManifestsWithCommonAncestor(t *testing.T) { call = f.nextCall("m1 build2") assert.Equal(t, m1.K8sTarget(), call.k8s()) + state := call.state[m1.ImageTargets[0].ID()] + assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet) + + // Make sure that when the second build is triggered, we did the bookkeeping + // correctly around reusing the image and propagating DepsChanged when + // we deploy the second k8s target. call = f.nextCall("m2 build2") assert.Equal(t, m2.K8sTarget(), call.k8s()) + id := m2.ImageTargets[0].ID() + result := f.b.resultsByID[id] + assert.Equal(t, store.NewBuildState(result, nil, nil), call.state[id]) + + id = m2.ImageTargets[1].ID() + result = f.b.resultsByID[id] + + // Assert the 2nd image was not re-used from the previous result. + assert.NotEqual(t, result, call.state[id].LastResult) + assert.Equal(t, map[model.TargetID]bool{m2.ImageTargets[0].ID(): true}, + call.state[id].DepsChangedSet) + + err := f.Stop() + assert.NoError(t, err) + f.assertAllBuildsConsumed() } func TestConfigChangeThatChangesManifestIsIncludedInManifestsChangedFile(t *testing.T) { diff --git a/internal/store/engine_state.go b/internal/store/engine_state.go index da69e754da..4e9e859ae9 100644 --- a/internal/store/engine_state.go +++ b/internal/store/engine_state.go @@ -169,7 +169,6 @@ func (e *EngineState) BuildStatus(id model.TargetID) BuildStatus { } } return BuildStatus{} - } func (e *EngineState) AvailableBuildSlots() int { @@ -246,6 +245,19 @@ func (e EngineState) Targets() []*ManifestTarget { return result } +func (e EngineState) TargetsBesides(mn model.ManifestName) []*ManifestTarget { + targets := e.Targets() + result := make([]*ManifestTarget, 0, len(targets)) + for _, mt := range targets { + if mt.Manifest.Name == mn { + continue + } + + result = append(result, mt) + } + return result +} + func (e *EngineState) ManifestInTriggerQueue(mn model.ManifestName) bool { for _, queued := range e.TriggerQueue { if queued == mn { @@ -340,6 +352,19 @@ func (s BuildStatus) IsEmpty() bool { s.LastResult == nil } +func (s *BuildStatus) ClearPendingChangesBefore(startTime time.Time) { + for file, modTime := range s.PendingFileChanges { + if BeforeOrEqual(modTime, startTime) { + delete(s.PendingFileChanges, file) + } + } + for file, modTime := range s.PendingDependencyChanges { + if BeforeOrEqual(modTime, startTime) { + delete(s.PendingDependencyChanges, file) + } + } +} + type ManifestState struct { Name model.ManifestName diff --git a/internal/store/engine_state_test.go b/internal/store/engine_state_test.go index 494ff6e58c..aa076cdbca 100644 --- a/internal/store/engine_state_test.go +++ b/internal/store/engine_state_test.go @@ -154,11 +154,18 @@ func TestNextBuildReason(t *testing.T) { iTargetID := model.ImageID(container.MustParseSelector("sancho")) status := mt.State.MutableBuildStatus(kTarget.ID()) + assert.Equal(t, "Initial Build", + mt.NextBuildReason().String()) + status.PendingDependencyChanges[iTargetID] = time.Now() - assert.Equal(t, "Initial Build | Dependency Updated", + assert.Equal(t, "Initial Build", + mt.NextBuildReason().String()) + + mt.State.AddCompletedBuild(model.BuildRecord{StartTime: time.Now(), FinishTime: time.Now()}) + assert.Equal(t, "Dependency Updated", mt.NextBuildReason().String()) status.PendingFileChanges["a.txt"] = time.Now() - assert.Equal(t, "Initial Build | Changed Files | Dependency Updated", + assert.Equal(t, "Changed Files | Dependency Updated", mt.NextBuildReason().String()) } diff --git a/pkg/model/build_reason.go b/pkg/model/build_reason.go index c26f5e7ea1..8df33bbff2 100644 --- a/pkg/model/build_reason.go +++ b/pkg/model/build_reason.go @@ -93,6 +93,11 @@ func (r BuildReason) String() string { } } + // The Init build reason should be listed alone too. + if r.Has(BuildReasonFlagInit) { + return translations[BuildReasonFlagInit] + } + // Use an array to iterate over the translations to ensure the iteration order // is consistent. for _, v := range allBuildReasons { diff --git a/pkg/model/manifest.go b/pkg/model/manifest.go index 4eb7919682..bf1933c2c9 100644 --- a/pkg/model/manifest.go +++ b/pkg/model/manifest.go @@ -63,6 +63,22 @@ func (m Manifest) DependencyIDs() []TargetID { return result } +// A map from each target id to the target IDs that depend on it. +func (m Manifest) ReverseDependencyIDs() map[TargetID][]TargetID { + result := make(map[TargetID][]TargetID) + for _, iTarget := range m.ImageTargets { + for _, depID := range iTarget.DependencyIDs() { + result[depID] = append(result[depID], iTarget.ID()) + } + } + if !m.deployTarget.ID().Empty() { + for _, depID := range m.deployTarget.DependencyIDs() { + result[depID] = append(result[depID], m.deployTarget.ID()) + } + } + return result +} + func (m Manifest) WithImageTarget(iTarget ImageTarget) Manifest { m.ImageTargets = []ImageTarget{iTarget} return m