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

Ephemeral builder tars are missing base image layers #925

Closed
matejvasek opened this issue Oct 26, 2020 · 14 comments
Closed

Ephemeral builder tars are missing base image layers #925

matejvasek opened this issue Oct 26, 2020 · 14 comments
Assignees
Labels
type/bug Issue that reports an unexpected behaviour.
Milestone

Comments

@matejvasek
Copy link
Contributor

When creating ephemeral builder the pack binary uses hack optimization when creating tar stream. The tar pushed to repo is missing some layers, the layers are however already present as they are downloaded in advance so it's still somehow working. But that is not a valid format. This causes issues for instance for podman which has more strict validation.

@matejvasek matejvasek added status/triage Issue or PR that requires contributor attention. type/bug Issue that reports an unexpected behaviour. labels Oct 26, 2020
@matejvasek
Copy link
Contributor Author

For more info containers/podman#8132.

@matejvasek
Copy link
Contributor Author

This will block #564.

@matejvasek matejvasek changed the title Ephemeral builder are missing base image layers Ephemeral builder tars are missing base image layers Oct 26, 2020
@jromero jromero self-assigned this Oct 27, 2020
@jromero
Copy link
Member

jromero commented Oct 27, 2020

Hi @matejvasek, thanks for the report. If it's what I'm thinking about, yes we are using an optimization hack specific to the docker daemon implementation. @ekcasey was the mastermind IIRC.

The "simple fix" as mentioned by @mtrmac is something we've recently contemplated but will take a bit longer to implement in the way we've previously discussed it. We can investigate a little further and determine smaller changes to resolve the issue in a shorter timeframe.

@ekcasey
Copy link
Member

ekcasey commented Oct 30, 2020

Yes this is a hack that we added to improve performance against the docker daemon. It is crucial that we keep this hack in place when using the docker daemon because the performance implications are enormous.

We could likely make the hack configurable in imgutil and toggle it on/off for docker/podman, but performance may suffer for pack+podman users.

@dfreilich
Copy link
Member

@ekcasey You had run an experiment somewhere to see what those performance implications were, right? Any chance you could repost those?

@ekcasey
Copy link
Member

ekcasey commented Oct 30, 2020

@dfreilich We did this a looong time ago (sometime in 2018) so suffice it to say I don't have original comparison data handy, but we can do some back of the envelope math. The time saved is the time it takes to docker save and then docker load the base image.

For example, if I am creating an ephemeral builder with a single additional buildpack the "base image" is the original builder. It just took me 29.646s to docker save -o builder.tar paketobuildpacks/builder:full and 26.103s to docker load -i builder.tar. So for this (admittedly larger) builder the hack is saving users close to one minute for the command pack build --builder paketobuildpacks/builder:full --buildpack <buildpack>. And that is just for the ephemeral builder part. If we got rid of the hack, the same problem would arise during export adding another 30-60s most likely.

@jromero
Copy link
Member

jromero commented Dec 4, 2020

@matejvasek,

@ekcasey started working on this but found a separate issue, reported by @abitrolly, that looks like might stem from the podman vs docker implementation. Are you able to look at the possible cause for podman not providing platform information as detailed in #966?

@matejvasek
Copy link
Contributor Author

matejvasek commented Dec 4, 2020

@jromero
I'll look at it. It might be caused by missing implementation for /containers/{id}/archive endpoint in podman. I recently implemented that containers/podman#8126, but it's probably not released yet.

@matejvasek
Copy link
Contributor Author

matejvasek commented Dec 4, 2020

Also 2.1 doesn't contain this fix of mine: containers/podman#8109. Could you please try build from master?

@matejvasek
Copy link
Contributor Author

matejvasek commented Dec 4, 2020

After pack update I can reproduce the issue #966 it on master. I just used older version of pack before.
Edit: actually I cannot reproduce it on latest master as it's been fixed. I just forgot to fetch correct remote 🤦‍♂️ .
Edit2: or maybe I fetched correct remote, but the fix has been merged like 20min ago, so I might miss it in first fetch 😄 .

@matejvasek
Copy link
Contributor Author

Hmm the OS is empty string in response JSON. I'll try to fix that.

@matejvasek
Copy link
Contributor Author

@jromero After some digging I found out it's been fixed here containers/podman#8494. So podman master should work.

@jromero
Copy link
Member

jromero commented Mar 5, 2021

Hi @matejvasek, just trying to button up everything here. Given that buildpacks/lifecycle#485 was released in lifecycle 0.10.0, this should be addresses when using a builder with a lifecycle version >= 0.10.0. I'm assuming it's all tied to #966.

Please reopen if I'm mistaken.

EDIT: I misassociated where the fix would be coming from. In this particular case, it's not the lifecycle but instead the buildpacks/imgutil library. This issue was addressed in commit buildpacks/imgutil@339e186 (Dec 9, 2020), pack updated the library (1b6730e) in release v0.16.0 to buildpacks/imgutil@8581300 (Dec 11, 2020).

@jromero jromero closed this as completed Mar 5, 2021
@jromero jromero removed the status/triage Issue or PR that requires contributor attention. label Mar 5, 2021
@jromero jromero added this to the 0.16.0 milestone Mar 5, 2021
@vlk-charles
Copy link

I don't know if it's still relevant to this issue but the hack can be made to work with Podman too. You just have to reference something in manifest.json instead of leaving the paths empty for layers. See containers/podman#8132 (comment). This shouldn't break Docker compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Issue that reports an unexpected behaviour.
Projects
None yet
Development

No branches or pull requests

5 participants