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

Improve docker image build time through caches #5316

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

sansmoraxz
Copy link
Contributor

@sansmoraxz sansmoraxz commented Dec 25, 2023

Add build cache for incremental builds.

As per this guide we get significant performance boost for incremental builds.

Since the docker files are already in one place, I suspect that the cache is being shared. Anyway we will get a huge performance boost while building locally. Personal observations show around 5x improvements for minute changes although this may vary.

Checklist

Relates to #5313

@sansmoraxz sansmoraxz requested a review from a team as a code owner December 25, 2023 17:41
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

@JorTurFer
Copy link
Member

Nice improvement! This will help for sure with local builds. Have you checked if we can integrate this with GH Caches? I mean, workflows executed on GH Hosted runners won't have anything cached. We added tasks for exporting the cache:

- name: Go modules cache
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3.3.2
with:
path: ${{ steps.go-paths.outputs.mod_cache }}
key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }}
- name: Go build cache
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3.3.2
with:
path: ${{ steps.go-paths.outputs.build_cache }}
key: ${{ runner.os }}-go-build-cache-${{ hashFiles('**/go.sum') }}

Is there any known path that we can include to cache across CI executions?

@sansmoraxz
Copy link
Contributor Author

As I understand, when we mount it using the cache mounts the cache volume is maintained separately by docker. The entire feature seems to be poorly documented and while following the breadcrumbs ended up to this issue.

I think the GHA compatibility is not yet there. Buildkit doesn't seem to export cache directory, and ability to control storage location requires some weird hacks.

- name: Go modules cache
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3.3.2
with:
path: ${{ steps.go-paths.outputs.mod_cache }}
key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }}
- name: Go build cache
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3.3.2
with:
path: ${{ steps.go-paths.outputs.build_cache }}
key: ${{ runner.os }}-go-build-cache-${{ hashFiles('**/go.sum') }}

I suspect bind mounts while building would be easier to pull off for these.

@JorTurFer
Copy link
Member

Honestly, improving developer experience is mire than enough for me, as generating images is done by a CI check, where 10 minutes more or less doesn't make the difference at all. If the local experience has been improved, I think we can merge it as it is.
WDYT @kedacore/keda-core-contributors ?

@JorTurFer
Copy link
Member

JorTurFer commented Dec 26, 2023

/run-e2e internal
Update: You can check the progress here

@sansmoraxz
Copy link
Contributor Author

I might have figured something out for the pipeline.

Swapping the mount type before building the image might work.

Something like:

sed -i '/ARG GOCACHE/a ARG LOCALCACHE=/root/.cache/go-build' Dockerfile*
sed -i 's|--mount=type=cache,target=${GOCACHE}|--mount=type=bind,source=${LOCALCACHE},target=${GOCACHE}|' Dockerfile*

WDYT?

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, nice improvement

@JorTurFer
Copy link
Member

nah, don't worry @sansmoraxz .
Improving developer experience is always nice, but for CI I think that we aren't earning so much because a few minutes more or less don't make the difference during a CI check.

@JorTurFer
Copy link
Member

Cloud you resolve the Changelog conflict? then we can merge the PR 😄

Signed-off-by: Souyama <souyama.debnath@atos.net>
Signed-off-by: Souyama <souyama.debnath@atos.net>
@sansmoraxz
Copy link
Contributor Author

Pretty much the reason I delayed adding to the changelog.

Ahh well, rebased.

@JorTurFer JorTurFer enabled auto-merge (squash) December 29, 2023 10:27
@JorTurFer
Copy link
Member

/skip-e2e

@JorTurFer JorTurFer merged commit 614eafb into kedacore:main Dec 29, 2023
20 checks passed
@sansmoraxz sansmoraxz deleted the ft/dkr-build-cache branch December 29, 2023 11:15
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
* Improve build time with build caches

Signed-off-by: Souyama <souyama.debnath@atos.net>

* Add to Changelog

Signed-off-by: Souyama <souyama.debnath@atos.net>

---------

Signed-off-by: Souyama <souyama.debnath@atos.net>
Signed-off-by: anton.lysina <alysina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants