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

Efficiently cache docker build #21914

Merged
merged 6 commits into from
Mar 1, 2024
Merged

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Feb 22, 2024

relates to : mozilla/addons#1992

Description

Using more efficient caching within our docker build to speed up successive builds.

Context

This opens up faster build times both locally and in CI and makes the idea of building images more frequently a lot more palletable.

Testing

This PR adds a make command to build a local docker image

make build_docker_image

This will build an image. If you want to really see the cache benefit, then first prune your local layer cache

docker system prune -a --volumes

If you have already run make build_docker_image you should prune the builder cache as well

docker buildx prune --all

This is like runnig for the first time. It will take a while.

Then modify the package.json or one of the requirements files.

Run the build again.

You will see that pip is using "cached" versions of dependencies which have already been installed. That is the more aggressive layer caching kicking in.

You'll also see that if you change other files in the code base, it won't necessarily reinstall dependencies. You can compare that behavior to running the same commands on master.

@KevinMind KevinMind changed the base branch from verify-docker-build-ci to master February 22, 2024 18:06
@KevinMind KevinMind requested review from a team and diox and removed request for a team February 22, 2024 18:07
Makefile-docker Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
locale/requirements.txt Outdated Show resolved Hide resolved
@KevinMind KevinMind force-pushed the ADDSRV-720-cache-docker-mounts branch 2 times, most recently from 9dcd191 to 89b4e82 Compare February 26, 2024 20:55
@KevinMind
Copy link
Contributor Author

@KevinMind KevinMind force-pushed the ADDSRV-720-cache-docker-mounts branch 2 times, most recently from e162495 to ad43992 Compare February 26, 2024 21:02
@KevinMind KevinMind requested a review from diox February 26, 2024 21:04
@KevinMind KevinMind force-pushed the ADDSRV-720-cache-docker-mounts branch from ad43992 to bc57245 Compare February 26, 2024 21:05
@KevinMind
Copy link
Contributor Author

KevinMind commented Feb 26, 2024

Comparison of master to this branch with different scenarios, changing different types of files that would invalidate different stages of the build.

Running make build_docker_image with that commit cherry-picked onto master.

Operation Before After % Difference Absolute Difference
Cold: (empty docker cache) 398.9s 386.8s -3.0% 12.1s
Warm: Modify prod.txt (add a single dep.) 179.1s 88.7s -50.4% 90.4s
Warm: Modify src/** (add a comment to file) 170.2s 34.5s -79.7% 135.7s
Hot: (filled cache no changes) 6.8s 6.2s -8.8% 0.6s

@diox
Copy link
Member

diox commented Feb 26, 2024

So trying to use this... After running make build_docker_image, you get:

WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load

What am I supposed to run ? I tried docker build --cache-from addons-server-test:latest -t test_amo . but got:

=> ERROR importing cache manifest from addons-server-test:latest

And it looks like it's rebuilding from the beginning - with layers caching, but it's not as fast as I'd expect given there are no changes (it's rebuilding dependencies, re-running locale compilation, redoing the copy and assets, taking 60 sec total)

@KevinMind
Copy link
Contributor Author

You can ignore the warning as the make command is using the cache type=locale and saving to your local file system instead of pushing to a registry.

You can look at what the make command executes to see how you'd repeat with raw docker commands. TLDR; you have to make sure to use buildkit and you have to make sure to use the docker-container driver to enable the full scope of caching.

docker build as you have it will likely miss out on several opportunities because of one or the other or both things. part of the reason why I added the make command is to normalize that flow. I don't want to have to remember what exact magic to pass, just make it work.

Though it is probably a good thing to add --load so that the built image is added to the docker daemon, since we are using a custom builder I'm not 100% sure if it is automatically available or we have to pass load. I can check that and update if it doesn't pass it up the chain.

@KevinMind
Copy link
Contributor Author

Also, to clarify the error you got, when you run--cache-from addons-server-test:latest you are asking docker to pull a cache manifest from an image called addons-server-test:latest` if that image doesn't exist on your machine or in a hub docker is setup to look in, then it will fail. In the make file we specify --cache-from... that points to the ./docker-cache directory in the local file system which you should see after running.

@diox
Copy link
Member

diox commented Feb 26, 2024

Cool, adding --load to the build command is what I was looking for. Now I can manipulate my built image normally, I see it in docker images etc.

In the make file we specify --cache-from... that points to the ./docker-cache directory in the local file system which you should see after running.

Ah right, I figured I needed to point it to the right place but forgot the directory was entirely custom, that makes sense now, thanks.

@KevinMind
Copy link
Contributor Author

There needs to be some docks written and mostly pointing to other places for this I think :)

@KevinMind KevinMind force-pushed the ADDSRV-720-cache-docker-mounts branch 2 times, most recently from 78e9d8e to 7200896 Compare February 27, 2024 10:06
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

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

Please update the PR title to something more descriptive before merging.

Makefile-docker Show resolved Hide resolved
@KevinMind
Copy link
Contributor Author

I'd like to merge this first #21930 The actions will be super useful in testing this. I would like to do a bit more testing before merging.

- cache npm/pip dependencies across builds
- add clear logging to npm install for cache hit/miss
- move copy npm files to update_assets
- splitting to stages offers better caching of layers and more efficient use of disk/time
- The initial gains will be with better caching of locale compilation, but will expand as we can move more logic from the final stage

TODO: split up apt depedencies to specific stages, move update_assets to pre-final stage to prevent re-running on every build with a * file change.
@KevinMind KevinMind force-pushed the ADDSRV-720-cache-docker-mounts branch 22 times, most recently from 780197d to b5e7153 Compare March 1, 2024 10:16
@KevinMind KevinMind changed the title ADDSRV-720-cache-docker-mounts Efficiently cache docker build Mar 1, 2024
@KevinMind KevinMind marked this pull request as ready for review March 1, 2024 10:28
@KevinMind KevinMind merged commit 823e825 into master Mar 1, 2024
13 checks passed
@KevinMind KevinMind deleted the ADDSRV-720-cache-docker-mounts branch March 1, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants