Skip to content

Improve build times by caching layers, when possible #44

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

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Aug 3, 2021

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

By using ghcr.io/laminas/laminas-continuous-integration-action:build-cache as a registry-based
docker image layer cache, we can avoid re-building any intermediate docker image layers for which
an exact SHA256 match exists.

This will dramatically speed up builds that happen to change little within the Dockerfile (or
any related sources), and it will reduce any network I/O failures caused by upstream package manager
hiccups.

Note that random users cannot just push to the cache: active caching can only happen when operating
within the boundaries of the laminas/* organization (direct repository pushes, operations performed
by maintainers).

To prevent stale images due to caching, we do a full re-build when the base image changes: it's an acceptable
cost, for something that changes less frequently, while we still retain caching advantages
when multiple pushes are performed close to each other.

By using `ghcr.io/laminas/laminas-continuous-integration-action:build-cache` as a registry-based
docker image layer cache, we can avoid re-building any intermediate docker image layers for which
an exact SHA256 match exists.

This will dramatically speed up builds that happen to change little within the `Dockerfile` (or
any related sources), and it will reduce any network I/O failures caused by upstream package manager
hiccups.

Note that random users cannot just push to the cache: active caching can only happen when operating
within the boundaries of the `laminas/*` organization (direct repository pushes, operations performed
by maintainers).
In a previous commit, we introduced `cache-from` and `cache-to`, which lead
to re-using docker image layers as much as possible. This is fine, but it also
means that, on the long term, we may run into stale base layers, where dependencies
do not get updated when the system shifts dramatically.

To prevent that, we do a full re-build when the base image changes: it's an acceptable
cost, for something that changes less frequently, while we still retain caching advantages
when multiple pushes are performed close to each other.
@Ocramius Ocramius added this to the 1.11.0 milestone Aug 3, 2021
@Ocramius
Copy link
Member Author

Ocramius commented Aug 3, 2021

Hmm, I'm unsure how to test this one: it seems to me that the Dockerfile is only used:

on:
  release:
    types: [published]

@weierophinney how do we verify this change? 🤔

@Ocramius Ocramius requested a review from weierophinney August 3, 2021 14:07
@Ocramius
Copy link
Member Author

Ocramius commented Aug 3, 2021

In fact, I think we should test the Dockerfile build at every change: not just on release 🤔

The idea would be to have push: false when outside the context of a release.

@weierophinney
Copy link
Member

In fact, I think we should test the Dockerfile build at every change: not just on release

We can add a workflow for that - it would be identical to the build-and-push-containers workflow, but with push:false, and operating on pull request instead of release publication. Can you add that to this PR?

@Ocramius
Copy link
Member Author

Ocramius commented Aug 3, 2021

Can you add that to this PR?

Yeah, didn't figure out how to do it without completely replicating the workflow file (which I'd like to avoid)

@weierophinney
Copy link
Member

Add the pull_request event to the workflow.

For the push value, you can use ${{ github.event_name == "release" }}.

@weierophinney weierophinney force-pushed the feature/use-docker-buildx-layer-caching-during-ci-builds branch 2 times, most recently from 0482c53 to ae74bc0 Compare August 19, 2021 21:14
@boesing boesing mentioned this pull request Aug 19, 2021
@weierophinney weierophinney force-pushed the feature/use-docker-buildx-layer-caching-during-ci-builds branch 2 times, most recently from b2c982d to 1bb112d Compare August 19, 2021 21:30
This change:

- adds "pull_request" as an event that will trigger the workflow.
- marks the "docker login" step as conditional, only to run if the "container_username" secret is present.
- alters the original "Build and push" step such that it only runs on a release
- adds two new "build and push" steps
  - one runs on pull_request if a container_username secret is present, and caches layers
  - one runs on pull_request if a container_username secret is NOT present, and DOES NOT cache layers

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney force-pushed the feature/use-docker-buildx-layer-caching-during-ci-builds branch from 1bb112d to a6f9b54 Compare August 19, 2021 21:31
@weierophinney
Copy link
Member

@Ocramius figured out how to make this happen.

Interestingly... it will be better in the future for TSC members to submit PRs as branches pushed directly to the repo, as otherwise repository secrets are not injected in the workflow, and we thus have no credentials for pushing to the registry.... and thus no ability to cache layers. (Release will always cache layers, however.)

Pushing a change to the README.md to add this note about contribution, and then can merge.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

🚢

@weierophinney weierophinney merged commit 0573cf3 into laminas:1.11.x Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants