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

Install composer in a way that does not use COPY --from #5904

Merged

Conversation

deivid-rodriguez
Copy link
Contributor

I observed that many times buildkit cache is not being hit, triggering a rebuild of the whole dependabot-core image, in situations where it would not seem necessary.

One hypothesis is that this is due to the COPY --from usage when installing composer, since I recall issues with that not playing well with buildkit cache.

I want to try a different way of installing composer, in order to verify whether this is the culprit and this speeds up our CI.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/workaround-buikdkit-cache-not-hit branch from a29180a to f0bb467 Compare October 17, 2022 15:07
@deivid-rodriguez
Copy link
Contributor Author

I don’t think the problem is related to this after another look. When cache is not hit, I thought the first not hit line was the composer one, hence my hypothesis, but I see that I just got that wrong, the apt-get update line above does not hit the cache either.

However, each of the new ways of installing seems to take ~1second while the old way of pulling file installed file from external docker images seems considerably slower (~10 seconds), so maybe still worth doing. And both installations can be compressed to a single layer using this approach.

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review October 17, 2022 15:54
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner October 17, 2022 15:54
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

Glad the composer installations get cached with this change. Thanks!

I think COPY --from is slower for rebuilds because the Docker image is being built as a multistage build, and only the final image is cached since we don't use named stages

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/workaround-buikdkit-cache-not-hit branch 2 times, most recently from 026605d to eb50ce4 Compare October 19, 2022 19:28
I observed that many times buildkit cache is not being hit, triggering a
rebuild of the whole dependabot-core image, in situations where it would
not seem necessary.

One hypothesis is that this is due to the `COPY --from` usage when
installing composer, since I recall issues with that not playing well
with buildkit cache.

I'm not sure this will actually have any effect in caching, but seems to
be considerably faster anyways.
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/workaround-buikdkit-cache-not-hit branch from eb50ce4 to 94752da Compare October 20, 2022 11:14
@deivid-rodriguez deivid-rodriguez merged commit a20950f into main Oct 20, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/workaround-buikdkit-cache-not-hit branch October 20, 2022 12:38
@deivid-rodriguez
Copy link
Contributor Author

Maybe it's too early to celebrate but I've seen consistent CACHE hits when rebuilding on main image since I merged this PR 🎉

@jeffwidman
Copy link
Member

One potential downside of this PR is that the composer version is now sitting in a random bash command which Dependabot is unlikely to know how to monitor. Versus the docker image tags--while we don't process them today, we are more likely to process them in the future... I've seen a POC floating around on us eventually doing this.

If this allows us to have faster builds/hit caches better, than I'm still 👍 on this change, but wanted to mention it.

@deivid-rodriguez
Copy link
Contributor Author

That's a good point! Still I haven't seen any unepexted cache misses since merging this so I think this is worth keeping though it also makes things consistent because it's the only place in the Dockerfile where we use COPY --from, other ecosystems are installed in a way more similar way to what we do now.

@pavera pavera mentioned this pull request Oct 31, 2022
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.

None yet

3 participants