Open edmorley opened 1 month ago
I presume this is related to:
This issue causes my CNB's integration test suite to take 9 minutes locally instead of 39 seconds.
It seems this issue has been known for some time (but unfortunately not documented in an easy to discover way) and that a solution here is currently blocked on upstream Docker API additions.
I think we should:
Thank you @edmorley for the analysis and suggested actions - I agree with what you've proposed here. Given that points 1-3 are low hanging fruit, I've marked this as a good first issue. If anyone is interested in taking it up, I'd be happy to guide.
Kindly to ask if there is somebody working for this problem? If not, I want to work on this. @natalieparellano Could you suggest something I can improve to make it work?
Hi @wuhenggongzi! I think nobody is working on this, thanks a lot for your help.
Let me try to help with task #1
- Have Pack CLI emit a performance warning if containerd is enabled, until this is resolved (I spent a non-zero amount of time tracking this down and writing an STR, it would be good to avoid others having to figure it out themselves)
imgutil
that validate if we the docker client use containerd as storage. I think we can do two things:
We can put the warning in imgutil
during the save operation of an image
imgutil
, notice you will need to update the constructor for the Store
too.We can put the warning in pack
when creating the docker client
imgutil
function usesContainerdStorage
that I mentioned above and use it here to validate if containerd is used and write the warningI think I prefer option 1, because at the end the issue must be solved in imgutil
and once is fixed we can remove the warning. but I would like to hear some thoughts from @edmorley and @natalieparellano
@jjbustamante I have been mulling this over, I think option 1 might be a bit clunky because then we'd need to remember to call something like image.Warnings()
after calling image.Save()
or else change the signature of that method which would be messy.
It might be easier to print the warning at the point where we construct the image, but that still requires us to call a separate method from NewImage
and at that point we might just go ahead and call usesContainerdStorage
.
I think we might expose something like IncursDaemonPerformancePenalty
in imgutil that we can always call, but when we fix the issue in imgutil we can just update that method to return false
.
WDYT?
Thanks for the feedback @natalieparellano, what I was thinking about option 1
was the following:
WithLogger()
option hereStore
constructor, to also receive a Logger
local.NewImage()
to check if we received the Logger
option, in such as case, update the creation of the Store
here to pass through that Logger instance. Store
has a valid logger instance to print the warning message during the Image.Save operation without having to change the signature. It will depend on imageutil consumer to set the logger and if the logger is not set, we skip trying to print the warning, this must avoid breaking things.pack
and lifecycle
to set the Logger
when local.NewImage()
is calledI see. The downside there is that imgutil would have full control over whether that message gets printed as debug / info / warn, etc - pack & the lifecycle have no say. That said I think we're pretty much in agreement that a warn message is the way to go. I'm still in favor of exposing something like IncursDaemonPerformancePenalty
, but I won't stand in the way of whatever is easiest to implement.
@natalieparellano , @jjbustamante Thanks for your help, please forgive my poor English. I tried to understand your instructions, this is my understanding:
The above are the changes made before fixing this problem.
Hi @wuhenggongzi,
You can start focusing on changes in imgutil repository, the first 4 bullets I mentioned above have the places where I think you can do the changes, BUT, I think we can also explore Natalie's idea IncursDaemonPerformancePenalty
but we can think on that later.
I will suggest, you create a draft PR on imgutil
and we will guide you from there. I will try to create an issue on imgutil
too
@wuhenggongzi I created the following issue on imgutil
@jjbustamante Thank you for your guidance and help. I have submitted a PR now, but it indicates that there is a problem with my coverage, but I don't know how I should deal with it. Personally, I think it points to code that should already be tested, what should I do to pass it?
Partially resolved by https://github.com/buildpacks/pack/pull/2284, but there is more to do, so I will leave this open and move it to the next milestone
Summary
When using the containerd image store (which is now the default for new installs for Docker for Desktop), along with an ephemeral builder image (ie: one where additional buildpacks have been added beyond the ones in the builder, or the group order has been overridden), there is a 20 second delay/hang before the "Analyzing" phase starts, plus image exporting takes 9 seconds instead of milliseconds. This occurs even if the build is forced to trusted mode using
--trust-extra-buildpacks
.Reproduction
Steps
Use containerd for pulling and storing images
is enabled.mkdir testcase && cd $_
touch Procfile
time pack build --builder heroku/builder:24 --pull-policy if-not-present --timestamps --verbose --buildpack heroku/procfile --trust-extra-buildpacks testcase
Current behavior
With containerd enabled:
With containerd disabled:
The majority of the delay occurs at these two places:
~20 second delay here:
~9 second delay when exporting the app image:
When containerd is disabled, the timings for the problematic sections shown above are instead:
~40ms delay (rather than 20 seconds):
~30ms delay when exporting the app image (rather than 9 seconds):
Full logs:
Expected behavior
pack build
duration is very similar regardless of whether containerd is enabled or not (eg is under 2 seconds for both).Environment
pack info
docker info