Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dockercompose: fix spurious full rebuild instead of Live Update #5242

Merged
merged 4 commits into from
Dec 2, 2021

Conversation

milas
Copy link
Contributor

@milas milas commented Dec 2, 2021

Occasionally, when using Live Update with a Docker Compose project,
a file change would trigger a full rebuild instead of Live Update
despite matching a sync path.

The root cause here is that the Docker Compose target watched files
in addition to the image target, so there was a race: if the Docker
Compose target saw the file but the image target hadn't yet, the
Docker Compose builder would take over and do a full rebuild. This
has probably been exacerbated by the API transition as there's now
greater potential for timing-related issues like this to manifest.

The reason the Docker Compose target watches for files is because
in some cases, there IS no image target for a Docker Compose service.
If there's no docker_build or custom_build configured, and the
Docker Compose service has a build: section in its YAML, the Docker
Compose build and deployer watched for file changes and then triggered
a build via passing the --build flag to the docker-compose up
call.

To address this, the Docker Compose target now behaves more like a
Kubernetes target. It does not watch for any files. Instead, image
targets are created for both services that will be managed by Tilt
(via docker_build or custom_build) as well as those that will
be built by Docker Compose. (NOTE: If a Docker Compose service has
no build: section and there's no corresponding docker_build or
custom_build call for it, there will be no target, as no image
builds will occur. This is commonly seen for infra deps that are
pulled from a registry.)

The image targets for Docker Compose managed builds have a new
BuildDetails type of DockerComposeBuild to go along with the
pre-existing DockerBuild and CustomBuild types. This ensures
that all file changes are tied to the image, so it will pass through
the LiveUpdateBuildAndDeployer first, and fallback to the
DockerComposeBuildAndDeployer if not Live Update-eligible. (This
is always the case for DockerComposeBuild targets currently!)
The DockerComposeBuildAndDeployer looks at the image targets and
either builds the set of Tilt-managed images for the service OR
delegates to docker-compose up by including the --build flag.

The DockerComposeBuild targets are created during Docker Compose
assembly during Tiltfile loading if no Tilt-managed builder exists.
This is kinda icky - it'd probably be nicer to create these as
stubs when docker_compose() is called and allow docker_build
and custom_build to override them, but that's a bigger yak shave
than practical for this fix.

Fixes #5196.

Occasionally, when using Live Update with a Docker Compose project,
a file change would trigger a full rebuild instead of Live Update
despite matching a sync path.

The root cause here is that the Docker Compose target watched files
in addition to the image target, so there was a race: if the Docker
Compose target saw the file but the image target hadn't yet, the
Docker Compose builder would take over and do a full rebuild. This
has probably been exacerbated by the API transition as there's now
greater potential for timing-related issues like this to manifest.

The reason the Docker Compose target watches for files is because
in some cases, there IS no image target for a Docker Compose service.
If there's no `docker_build` or `custom_build` configured, and the
Docker Compose service has a `build:` section in its YAML, the Docker
Compose build and deployer watched for file changes and then triggered
a build via passing the `--build` flag to the `docker-compose up`
call.

To address this, the Docker Compose target now behaves more like a
Kubernetes target. It does not watch for any files. Instead, image
targets are created for both services that will be managed by Tilt
(via `docker_build` or `custom_build`) as well as those that will
be built by Docker Compose. (NOTE: If a Docker Compose service has
no `build:` section and there's no corresponding `docker_build` or
`custom_build` call for it, there will be no target, as no image
builds will occur. This is commonly seen for infra deps that are
pulled from a registry.)

The image targets for Docker Compose managed builds have a new
`BuildDetails` type of `DockerComposeBuild` to go along with the
pre-existing `DockerBuild` and `CustomBuild` types. This ensures
that all file changes are tie to the image, so it will pass through
the `LiveUpdateBuildAndDeployer` first, and fallback to the
`DockerComposeBuildAndDeployer` if not Live Update-eligible. (This
is always the case for `DockerComposeBuild` targets currently!)
The `DockerComposeBuildAndDeployer` looks at the image targets and
either builds the set of Tilt-managed images for the service OR
delegates to `docker-compose up` by including the `--build` flag.

The `DockerComposeBuild` targets are created during Docker Compose
assembly during Tiltfile loading if no Tilt-managed builder exists.
This is kinda icky - it'd probably be nicer to create these as
stubs when `docker_compose()` is called and allow `docker_build`
and `custom_build` to override them, but that's a bigger yak shave
than practical for this fix.
@milas milas added the bug Something isn't working label Dec 2, 2021
@milas
Copy link
Contributor Author

milas commented Dec 2, 2021

Apologies for the number of changes here - there's unfortunately lots of tiny ripples [especially in tests], and I couldn't figure out a great way to chunk it because it's a pretty atomic cut-over.

Also, there is currently a regression, which is that local/bind mount volumes aren't ignored for rebuilds - I need to re-introduce LocalIgnoredDirectories and use that for DockerComposeBuild targets. Fixed in 18e7159. (Note: ignoring local volumes doesn't work if you are using docker_build or custom_build, which was already the case, so isn't a regression, but is probably worth noting in the docs somewhere, so I will add that as a caveat.)

Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup! Explanation makes sense to me, code and functionality are in better shape than before even with the caveats.

@nicks
Copy link
Member

nicks commented Dec 2, 2021

high-level question: how do you think we should represent this in the api server?

i was originally assuming that the DockerComposeUp object would know which images come from a DockerImage (handled by tilt) and which images are handled by the DockerComposeUp object (handled by docker-compose). But this PR introduces a second possibility - that we have some sort of DockerComposeImage object in the system. But I'm not sure. what do you think?

@milas
Copy link
Contributor Author

milas commented Dec 2, 2021

Good question! FWIW my decision for approach of modeling this as close to the image builds for K8s was based on hopefully making API transitions easier.

I actually see a few possibilities:

  • Create a DockerComposeImage API type as mentioned
    • In practice, this would really just be shelling out to docker-compose build ... which sounds suspiciously like a custom build, so see ⬇️
  • Auto create CustomBuild objects that shell out to docker-compose build ... for services w/o explicit build directives in Tiltfile
    • This could be done now and would Just Work ™️ - we'd never use the --build flag for up calls, knowing that we've always pre-built everything
  • Eliminate Docker Compose managed builds entirely - create full DockerBuild image targets from DC services
    • This would probably be a "breaking" change just because of likelihood of subtle differences in some potential esoteric cases
    • Users could always define a custom_build that calls docker-compose build <service> if they want DC to manage the build
    • Bonus: we could start injecting Tilt immutable refs into the DC YAML now that we're using compose-go so have an easily mutable version of the model handy!

The common thread here is that I believe it makes sense to split the build from the deploy long-term, so that we can have consistent semantics across K8s + DC.

@milas milas merged commit 14e1b56 into master Dec 2, 2021
@milas milas deleted the milas/docker-compose-live-update branch December 2, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Live Sync Sporadically Does Full Rebuild
3 participants