buildcontrol: don't build services if one of their deps is building

[nicks]

Hello @maiamcc,

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

de2bd0d (2020-01-24 16:10:16 -0500)
buildcontrol: don't build services if one of their deps is building

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

[maiamcc]

Nick, what's the story behind this PR?

I think (at least for v1 of resource deps) Matt deliberately didn't enforce dependencies beyond the very first build. I know we'd kicked around the idea of, when service B depends on service A, any update of service A triggers an update of service B--while this PR isn't precisely that, it's at least a step in that direction (i.e. in respecting dependency beyond the first build).

Matt, can you explain your thought process here? I.e. was it just a "keep the scope narrow" consideration, or is there a reason to NOT go down this road (or at least, offer the option to not)?

[nicks]

i was responding to this bug: https://app.clubhouse.io/windmill/story/4318/parallel-builds-can-lead-to-unwanted-live-updates

as linked to in the branch name

one alternative idea is to have very different semantics when the resource dep is a local_resource vs a k8s_resource

[nicks]

and fwiw: when we were designing resource deps, parallel builds weren't even possible, so I would be surprised if this bug was considered

[nicks]

so i can think of a couple different ways to resolve this, and i'm curious about @landism 's thoughts.

• Make resource_deps behave very differently when you depend on a build-only local_resource (i.e., no serve_cmd)
• one way is to introduce a new kind of deps (e.g., build_deps). Make it illegal 🚓 for resource_deps on a build-only local_resource
• add a output_files to local_resource, and auto-infer the build deps
• something else?

[landism]

Matt, can you explain your thought process here? I.e. was it just a "keep the scope narrow" consideration, or is there a reason to NOT go down this road (or at least, offer the option to not)?

So yeah, as Nick said, parallel builds weren't a thing, so I didn't really have a thought process around that specifically, but I did consider making it much more feature-rich (e.g., different kinds of resource_deps, like "after X builds, rebuild Y") and decided to just address what seemed to be the most common use case - "don't start Y until after X is running, because Y generates lots of annoying errors until X comes up".

The main downside I see to the solution in this PR is that there are many cases where you want Y to block on X at startup, but once you have an X running, there's no reason X and Y can't build at the same time.

Make resource_deps behave very differently when you depend on a build-only local_resource (i.e., no serve_cmd)

There's probably some design term for this, but I generally prefer keeping behavior consistent across the intersections of features, rather than making users learn a bunch of special cases to understand how features interact.

One option we threw around was allowing resource_deps to take a dict where the name is resource name and the value is the dep type (e.g., "don't start Y until X has started", "don't deploy Y while X is unhealthy" (maybe this is a strictly better rule than what we've got?), "don't build Y while X is building", "always build Y after X finishes deploying", "call this function to determine whether to build and/or deploy Y (and then we expose resource status to their starlark function)"). I haven't gotten much further in my thoughts on this (I mostly decided that was a can of worms and the existing implementation had a much better bang:buck for what user feedback we'd actually gotten)

add a output_files to local_resource, and auto-infer the build deps

This sounds good, but a thing I like about the idea of making builds mutually exclusive is that we could use that to ensure we're not doing redundant builds of the same layers in tilt.build.

[maiamcc]

The main downside I see to the solution in this PR is that there are many cases where you want Y to block on X at startup, but once you have an X running, there's no reason X and Y can't build at the same time.

yeah that was my thought as well. happy to try this and see if ppl complain, tho.

but also, if X runs and changes files which trigger Y, how sure are we that users will have explicitly specified X as a dep of Y?

[maiamcc]

fwiw it wasn't apparent to me from this test name what the test does

[maiamcc]

imo RemoveCurrentlyBuildingTargetsAndDeps and RemoveTargetsWaitingOnDependencies aren't very distinct, the difference between them isn't apparent at a glance (tho this might just be me)

[nicks]

Matt and I talked about this a bunch offline. Totally agree it should be consistent, but not totally clear what the mental model is for what you expect resource_deps to do, and that the definition of consistency depends on what your mental model is.

We both agree that the current resource_deps behavior doesn't make any sense when you're depending on a local_resource that produces files for other resources.

In the short term, I'm going to:

1. Withdraw this PR
2. Update the parallel build logic so that we don't never schedule local_resource() commands in parallel with anything else

And then at some later time, we're going to spend more time thinking thru what the parallelism / dependency story is for local_resources.

@maiamcc does that sound right to you?