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

add support for pack to extend base images using daemon #1791

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

itsdarshankumar
Copy link
Contributor

@itsdarshankumar itsdarshankumar commented Jun 2, 2023

Summary

As pack already has access to a daemon, it can apply the dockerfiles directly, saving the extended build base image in the daemon which gives us great performance benefits. Thus it will not need to use the extender phase of lifecycle. Additionally it will drop the requirement that the image being extended must be published to a registry.

Output

image

image

Related

Resolves #1623

@itsdarshankumar itsdarshankumar requested review from a team as code owners June 2, 2023 06:25
@itsdarshankumar itsdarshankumar marked this pull request as draft June 2, 2023 06:25
@github-actions github-actions bot added this to the 0.30.0 milestone Jun 2, 2023
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jun 2, 2023
@itsdarshankumar itsdarshankumar force-pushed the run-extend branch 2 times, most recently from 12db41c to 28f6828 Compare June 2, 2023 06:30
Signed-off-by: Darshan Kumar <[email protected]>
@jjbustamante jjbustamante modified the milestones: 0.30.0, 0.31.0 Jun 7, 2023
@jjbustamante
Copy link
Member

Moving this one to milestone 0.31.0

@github-actions github-actions bot modified the milestones: 0.31.0, 0.30.0 Jun 11, 2023
@jjbustamante jjbustamante modified the milestones: 0.30.0, 0.30.1 Jun 13, 2023
@natalieparellano natalieparellano modified the milestones: 0.30.1, 0.31.0 Jun 14, 2023
Signed-off-by: Darshan Kumar <[email protected]>
@github-actions github-actions bot modified the milestones: 0.34.0, 0.33.0 Dec 20, 2023
@jjbustamante jjbustamante modified the milestones: 0.33.0, 0.34.0 Dec 20, 2023
@github-actions github-actions bot modified the milestones: 0.34.0, 0.33.0 Dec 25, 2023
@natalieparellano
Copy link
Member

@itsdarshankumar thank you for making all the updates to your branch. I tried to re-run the test mentioned in #1791 (comment), but I'm still getting the error: [extender (build)] {"errorDetail":{"message":"COPY failed: file not found in build context or excluded by .dockerignore: stat mvnw: file does not exist"},"error":"COPY failed: file not found in build context or excluded by .dockerignore: stat mvnw: file does not exist"}. When I rerun the command with released pack, the build succeeds.

Before we proceed further, it might be prudent to check with @loewenstein @c0d1ngm0nk3y and others to see if there is still appetite for this change, given forthcoming changes to the extend phase outlined in buildpacks/spec#384. To summarize, with #1791 we'd use the daemon to extend builder images only (effectively chaining docker build together using the generated build.Dockerfiles). This gives us a nice performance boost but introduces code complexity because we cannot extend run images this way as it makes performance worse (due to time spent saving extended layers from the daemon). If we merge this PR the changes from buildpacks/spec#384 might be more complicated to implement. Another possible consideration is that a future integration with BuildKit (if we ever get there) might make relying on the daemon for image extension a bit simpler. Please opine!

@loewenstein
Copy link

I will leave a thorough technical comment up to @pbusko @c0d1ngm0nk3y @modulo11 @phil9909 - but I am wondering how a pack optimisation could make a lifecycle change harder?

@jjbustamante jjbustamante modified the milestones: 0.33.0, 0.34.0 Jan 23, 2024
@natalieparellano
Copy link
Member

@loewenstein it wouldn't make the lifecycle change harder - but, some things would need to be re-implemented in pack since we aren't relying on the lifecycle for build image extension.

@loewenstein
Copy link

@loewenstein it wouldn't make the lifecycle change harder - but, some things would need to be re-implemented in pack since we aren't relying on the lifecycle for build image extension.

FWIW buildpacks/lifecycle#1276 is said lifecycle implementation ;)

@natalieparellano
Copy link
Member

@itsdarshankumar thank you for your work here. Given the evolution of the "Dockerfiles" feature and the introduction of configurable context directories (buildpacks/spec#384), it may no longer by prudent to have a separate implementation of build image extension that uses the daemon, as this would require changes to be implemented twice (once in the lifecycle for run image extension and non-pack platforms, and once in pack for build image extension). Also having build image extension (uses daemon) and run image extension (uses lifecycle) behave differently in pack might be confusing, especially for newer contributors.

However the work in this PR is still extremely valuable as it gives us an indication of the performance benefits we might see in relying on the daemon for image extension vs kaniko. I suggest we keep on eye on potential upcoming changes to the daemon (such as moby/moby#44369) that might run image extension with the daemon more performant. If build image extension and run image extension could both use the daemon, that would make things simpler in pack. That doesn't solve the need for changes to be implemented twice (lifecycle, pack) but we can still learn from this PR how to make that easier, if we decide the performance improvement is worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC #0105] pack should support build image extension with Docker
5 participants