engine: better handling for image builds in multiple manifests. Fixes https://github.com/windmilleng/tilt/issues/2992 by nicks · Pull Request #3362 · tilt-dev/tilt

nicks · 2020-05-21T15:11:59Z

Please review the following commits I made in branch nicks/ch6738:

c995054 (2020-05-21 11:07:44 -0400)
engine: better handling for image builds in multiple manifests. Fixes #2992

Code review reminders, by giving a LGTM you attest that:

Commits are adequately tested
Code is easy to understand and conforms to style guides
Incomplete code is marked with TODOs
Code is suitably instrumented with logging and metrics

nicks · 2020-05-21T16:10:15Z

I think this isn't too complex a solution, and sets us up pretty well to make things better incrementally. The most important bit of the PR is this comment:

        // Stores the times that dependencies were marked dirty, so we can prioritize
	// the oldest one first.
	//
	// Long-term, we want to process all dependencies as a build graph rather than
	// a list of manifests. Specifically, we'll build one Target at a time.  One
	// the build completes, we'll look at all the targets that depend on it, and
	// mark PendingDependencyChanges to indicate that they need a rebuild.
	//
	// Short-term, we only use this for cases where two manifests share a common
	// image. This only handles cross-manifest dependencies.
	//
	// This approach allows us to start working on the bookkeeping and
	// dependency-tracking in the short-term, without having to switch over to a
	// full dependency graph in one swoop.

nicks · 2020-05-21T20:29:57Z

had a good convo with Maia offlist about this; I'm going to write some graph accessors to make it more clear what all the nested loops are doing

nicks · 2020-05-21T21:34:07Z

ok, i moved all the for loops in handleBuildResults into graph traversal accessors. ptal?

landism · 2020-05-22T18:15:09Z

hoo boy this is a big PR!

I'm still going through it, but while I do, some tests for these cases could be good:

resources r1 and r2 both depend on image i1 - user manually rebuilds r1 - does the image rebuild trigger a rebuild of r2?
resource r1 uses image i3, which is FROM image i2, which is FROM image i1. Is IsRecycled transitive? I'm still trying to understand this PR, so I'm not sure if this is the right way to test this, but maybe a construction here would be add resource r2 that uses image i1, rebuild r2 - do i2 and i3 get recycled when rebuilding r1? (also that's just an optimization so maybe not super important)

maiamcc

Broad feedback:

the second loop in handleBuildResults, where we're RE-dirtying anything that depends on something that we just recycled, seems superfluous -- what if when we recycled a result we DIDN'T scrub its file changes, and instead in the build controller said "oh, this result is recycled and no files have changed since it was marked recycled, so it doesn't need to be built" and it were a no-op in the builder that way? (would have to store "recycledAt" as a timestamp instead of a boolean.)
@landism and I are both having a lot of trouble wrapping our heads around this. If you don't take the above suggestion, (i.e. if recycling at build time and recycling on build complete remain distinct), consider breaking it into two PRs for each of those parts?
what I was trying to articulate in slack is that from the naming, I expect targetQueue.ShouldRecycle to check the Recycled flag we set on the result when handling build results. I.e. I understand "recycled" to mean "we took this result from a different manifest's build", which is distinct from "nothing that directly affects this target has changed" (both of which fall under the umbrella of "we don't need to build this target right now"). I can see how this distinction will be less useful in the future world. It's only questionably relevant now but at minimum the transition from the old world is confusing.

GH won't let me comment on this cuz it's not a line you changed but in targetQueue.RunBuilds:

func (q *TargetQueue) RunBuilds(handler BuildHandler) error {
	for _, target := range q.sortedTargets {
		id := target.ID()
		if q.depsNeedBuild[id] {
			// If the dependencies are dirty, we can't use any state from the previous build.
			result, err := handler(target, store.BuildState{}, q.dependencyResults(target))
			if err != nil {
				return err
			}
			q.results[id] = result
		} else if q.needsOwnBuild[id] {
			// If only files are dirty, we can try to do an incremental build.
			result, err := handler(target, q.state[id], q.dependencyResults(target))
			if err != nil {
				return err
			}
		...

I think these two cases are not functionally different anymore: i can't find anything in the build handler for either ibad or dcbad that actually does anything with state. could you merge these two cases / remove (or leave a note to remove) the state arg from the handler if we're not using it? would make this code easier to read and reason about.

maiamcc · 2020-05-21T18:26:10Z

-		ms.MutableBuildStatus(id).LastResult = result
-	}
-
 	// Remove pending file changes that were consumed by this build.


this comment is no longer accurate

maiamcc · 2020-05-21T18:27:19Z

+// across multiple manifests.
+//
+// If an image target succeeds, we want to record that success
+// in all manifests that depend on that image.


this isn't all this function does -- this comment should probably go somewhere below, and possibly add a higher-level docstring here that explains the purpose of the function as a whole

maiamcc · 2020-05-21T18:50:28Z

+	brManifestName model.ManifestName, br model.BuildRecord, results store.BuildResultSet) {
+	isBuildSuccess := br.Error == nil
+
+	for _, mt := range engineState.Targets() {


fwiw, this code is gnarly enough that i'd appreciate comments for each paragraph

maiamcc · 2020-05-22T17:50:39Z

-		} else {
+		} else if q.shouldRecycle(id) {
 			// Otherwise, we can re-use results from the previous build.
 			// If needsOwnBuild is false, then LastSuccessfulResult must exist if it's empty.


if what's empty?

maiamcc · 2020-05-22T17:51:20Z

-			q.results[id] = lastResult
+			q.results[id] = lastResult.Recycle()
+		} else {
+			return fmt.Errorf("Internal error: build marked both clean and not recyclable")


Suggested change

return fmt.Errorf("Internal error: build marked both clean and not recyclable")

return fmt.Errorf("Internal error: target marked both clean and not recyclable")

nicks · 2020-05-22T19:57:15Z

cool, let me send out two independent prs:

a PR that isolates the TargetQueue changes we want
a PR that isolates the types of graph structures that we want to be able to support, and adds tests for them
to make it clearer what's going on in this pr

maiamcc · 2020-05-22T20:00:31Z

FYI here are notes i took inline while going through this PR with Matt -- some suggestions for how to clarify comments, and some questions/comments about implementation. If that's a helpful format, feel free to use, otherwise I can submit them all as PR comments when this is ready for re-review.

…ilt, and 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.

nicks · 2020-05-22T22:10:33Z

ok, i sent out

engine: remove last build state from the target queue build handlers #3372
engine: build and deployers should return results for targets they built, and #3374
for some of the cleanup tasks your comments inspired

after those PRs go in, the whole concept of recycling will magically go away, since it's really just working around ImageBuildAndDeployers return results for things they didn't build

…ilt, and 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.

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

nicks

OK, I added a more tests, and removed the notion of recycling.

All the other comments should be addressed or moot

landism · 2020-06-08T20:35:52Z

Is this waiting for review or changes?

nicks · 2020-06-08T21:51:15Z

waiting on review! haven't been pushing on it too hard since there's a lot of other things in the air

maiamcc

cool, looks good behavior-wise. Most of my comments are cosmetic things about position/wording of comments, variable naming etc.--I think they'll help comprehensibility but ofc feel free to take or leave them.

I'd also suggest doing a search thru the code for "recycle"/"recycling" since I think it still appears in a few places but we've moved away from that terminology

maiamcc · 2020-06-15T16:25:55Z

+			return
+		}
+
+		_, isImageResult := result.(store.ImageBuildResult)


hmm, would we ever hit this point in the code with something that WASN'T an image result? like, can there exist two k8s targets of the same name across two different manifests?

live update results aren't reusable, added a comment about this

maiamcc · 2020-06-15T16:52:02Z

+	// build but its dependency was built.
+	engineState.ForEachMutableBuildStatusDependingOn(results, func(id, reverseDepID model.TargetID, mn model.ManifestName, reverseDepStatus *store.BuildStatus) {
+		if mn == mt.Manifest.Name {
+			// We're only worried about manifests that we didn't build.


Suggested change

// We're only worried about manifests that we didn't build.

// We're only worried about manifests that we didn't just build.

maiamcc · 2020-06-15T17:06:42Z

+	// 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.


(idk if this comment is quite right but i DO think it's useful to mention the deploy-targets-depending-on-image-targets case--when this comment only talks about inter-image dependencies, it seems kinda special-case-y, but really this bit of code is much more generic than that!)

Suggested change

// build but its dependency was built.

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

maiamcc · 2020-06-15T17:07:52Z

+
+		_, ok := results[reverseDepID]
+		if ok {
+			// The reverseDep is fresh! Skip it.


Suggested change

// The reverseDep is fresh! Skip it.

// The reverseDep is fresh (i.e. was just built)! Skip it.

maiamcc · 2020-06-15T17:10:27Z

+			return
+		}
+
+		_, depIsNew := results[id]


doesn't the iterator guarantee that we'll only ever get id if it's in results, making this check redundant?

yep! this is vestigial

maiamcc · 2020-06-15T17:39:47Z

 }

+func updateBuildStatus(status *store.BuildStatus, br model.BuildRecord, result store.BuildResult, isBuildSuccess bool) {
+	// Record the last result


Suggested change

// Record the last result

// Record the result

(otherwise it seems like you're talking about the result BEFORE the one you're currently processing)

maiamcc · 2020-06-15T17:40:06Z

+	//
+	// This has a minor bug: if one of the images in a set failed, but another one
+	// succeeded, this doesn't mark any of them as successful. We should fix this,
+	// though personally, I think LastSuccessfulResult has problematic semantics


Suggested change

// though personally, I think LastSuccessfulResult has problematic semantics

// though personally, I (Nick) think LastSuccessfulResult has problematic semantics

maiamcc · 2020-06-15T17:41:01Z

+	}
+
+	// Update build statuses for duplicated image targets in other manifests.
+	// This ensures that those images targets aren't rebuilt.


Suggested change

// This ensures that those images targets aren't rebuilt.

// This ensures that those images targets aren't redundantly rebuilt.

maiamcc · 2020-06-15T17:44:42Z

 	BuildReasonFlagTriggerCLI:     "CLI Trigger",
 	BuildReasonFlagTriggerUnknown: "Unknown Trigger",
 	BuildReasonFlagTiltfileArgs:   "Tilt Args",
+	BuildReasonFlagChangedDep:     "New Image",


I'm not sure I as a user would understand what this meant... why not just "dependency updated" or something?

i changed it to "image updated", since we don't really use the "dependency" language in other user-facing parts of tilt

maiamcc · 2020-06-15T17:45:42Z

 	call = f.nextCall("m1 build2")
 	assert.Equal(t, m1.K8sTarget(), call.k8s())

+	imageStateM1I0 := call.state[m1.ImageTargets[0].ID()]


this test seems of a piece with the tests above in buildcontroller_test.go, any reason it's here instead of there? nbd, just seems odd.

ya, probably. i think a lot of these need to be reorganized anyway

…2992

Fixes #2992 (#3362)" This reverts commit dad9d4e.

Fixes #2992 (#3362)" (#3505) This reverts commit dad9d4e.

this is the source of a lot of the infinite rebuild problems in #3362 There are a lot of weird cases where the original image existence check would fail, and so we would deliberately rebuild the image. This makes the check more robust, and adds logging when it does fail so that we have some hope of figuring out the problem.

…rces

…rces (#3553)

nicks requested review from landism and maiamcc May 21, 2020 15:12

nicks force-pushed the nicks/ch6738 branch from c995054 to a16db2e Compare May 21, 2020 15:59

nicks force-pushed the nicks/ch6738 branch from a16db2e to 1b473f8 Compare May 21, 2020 16:11

nicks force-pushed the nicks/ch6738 branch 2 times, most recently from 1ba3c54 to 473896d Compare May 21, 2020 21:32

nicks force-pushed the nicks/ch6738 branch from 473896d to 4781c38 Compare May 21, 2020 21:48

maiamcc suggested changes May 22, 2020

View reviewed changes

nicks mentioned this pull request May 22, 2020

engine: build and deployers should return results for targets they built, and #3374

Merged

nicks mentioned this pull request May 27, 2020

engine: print out results from previous builds #3384

Merged

nicks force-pushed the nicks/ch6738 branch 4 times, most recently from eed6c70 to 0ef22a0 Compare May 27, 2020 21:56

nicks changed the base branch from master to nicks/reused May 27, 2020 22:13

nicks commented May 27, 2020

View reviewed changes

Base automatically changed from nicks/reused to master May 28, 2020 15:14

nicks force-pushed the nicks/ch6738 branch from 0ef22a0 to 78769fa Compare May 28, 2020 15:15

nicks requested a review from maiamcc May 28, 2020 17:48

nicks force-pushed the nicks/ch6738 branch from 78769fa to f3fd65e Compare June 8, 2020 21:31

maiamcc approved these changes Jun 15, 2020

View reviewed changes

engine: better handling for image builds in multiple manifests. Fixes #…

79169ed

…2992

nicks force-pushed the nicks/ch6738 branch from f3fd65e to 79169ed Compare June 15, 2020 22:22

nicks merged commit dad9d4e into master Jun 16, 2020

nicks deleted the nicks/ch6738 branch June 16, 2020 13:12

This was referenced Jun 16, 2020

image in multiple deployments built multiple times #2992

Closed

Parallel build does not wait for base image #2855

Closed

nicks added a commit that referenced this pull request Jun 24, 2020

Revert "engine: better handling for image builds in multiple manifests.

4ba3a9f

Fixes #2992 (#3362)" This reverts commit dad9d4e.

nicks mentioned this pull request Jun 24, 2020

Revert "engine: better handling for image builds in multiple manifests. Fixes https://github.com/windmilleng/tilt/issues/2992 (#3362)" #3505

Merged

landism pushed a commit that referenced this pull request Jun 24, 2020

Revert "engine: better handling for image builds in multiple manifests.

ee10a50

Fixes #2992 (#3362)" (#3505) This reverts commit dad9d4e.

nicks mentioned this pull request Jul 2, 2020

engine: fix bugs in image build caching #3542

Merged

nicks added a commit that referenced this pull request Jul 6, 2020

engine: roll forward of #3362, re-using image builds from other resou…

d5bfb3f

…rces

nicks added a commit that referenced this pull request Jul 6, 2020

engine: roll forward of #3362, re-using image builds from other resou…

24351f9

…rces

nicks mentioned this pull request Jul 6, 2020

engine: roll forward of https://github.com/tilt-dev/tilt/pull/3362, re-using image builds from other resources #3553

Merged

nicks added a commit that referenced this pull request Jul 7, 2020

engine: roll forward of #3362, re-using image builds from other resou…

9ee95d7

…rces

nicks added a commit that referenced this pull request Jul 7, 2020

engine: roll forward of #3362, re-using image builds from other resou…

f9d466c

…rces (#3553)

	return fmt.Errorf("Internal error: build marked both clean and not recyclable")
	return fmt.Errorf("Internal error: target marked both clean and not recyclable")

	// We're only worried about manifests that we didn't build.
	// We're only worried about manifests that we didn't just build.

	// The reverseDep is fresh! Skip it.
	// The reverseDep is fresh (i.e. was just built)! Skip it.

	// though personally, I think LastSuccessfulResult has problematic semantics
	// though personally, I (Nick) think LastSuccessfulResult has problematic semantics

	// This ensures that those images targets aren't rebuilt.
	// This ensures that those images targets aren't redundantly rebuilt.

Uh oh!

Conversation

nicks commented May 21, 2020

Uh oh!

nicks commented May 21, 2020

Uh oh!

nicks commented May 21, 2020

Uh oh!

nicks commented May 21, 2020

Uh oh!

landism commented May 22, 2020

Uh oh!

maiamcc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicks commented May 22, 2020

Uh oh!

maiamcc commented May 22, 2020

Uh oh!

nicks commented May 22, 2020

Uh oh!

nicks left a comment

Choose a reason for hiding this comment

Uh oh!

landism commented Jun 8, 2020

Uh oh!

nicks commented Jun 8, 2020

Uh oh!

maiamcc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers