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

decouple composefs and zstd:chunked #2016

Open
cgwalters opened this issue Jul 12, 2024 · 6 comments
Open

decouple composefs and zstd:chunked #2016

cgwalters opened this issue Jul 12, 2024 · 6 comments
Labels
area/composefs composefs related changes area/zstd:chunked Issues relating to zstd:chunked

Comments

@cgwalters
Copy link
Contributor

cgwalters commented Jul 12, 2024

Today,

[storage.options.overlay]
use_composefs = "true"

Actually also requires:

[storage.options]
pull_options = {convert_images = "true"} 

And it's especially the convert_images = true that adds huge latency to pulls of images that aren't already in zstd:chunked format.

I am probably willing to put some time in code behind decoupling these two things, as I do not believe at a technical level they need to be coupled.

If someone feels really strongly, it's OK to close this issue though.

In the meantime at a practical level, this issue will just be a reference point for what is likely to be a FAQ of "why does enabling composefs slow down my image pulls"?

@cgwalters cgwalters added area/composefs composefs related changes area/zstd:chunked Issues relating to zstd:chunked labels Jul 12, 2024
@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2024

@giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2024

@alexlarsson PTAL

@giuseppe
Copy link
Member

we have convert_images so we can reuse all the existing infra to deduplicate files across layers.

We could generate a composefs blob directly from the extracted tarball content, but we'd lose the other advantages we have with pkg/chunked.

I think we still need to create a TOC (even in a simpler format without offsets to the payloads in the tarball stream) but we can avoid the temporary conversion to zstd

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 12, 2024

we have convert_images so we can reuse all the existing infra to deduplicate files across layers.

So… the deduplication / lookup code can, in principle, be called from drivers/overlay outside of the zstd call stack. And convert_images should probably not even trigger the PutBlobPartial code path in c/image, it should be “ordinary application of a non-chunked layer”. I do think that would be a better design; but the change would be invasive.

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 12, 2024

[… and I have a self-imposed moratorium on reviewing any changes that add options or complexity to drivers/overlay because I just can’t keep up.]

@cgwalters
Copy link
Contributor Author

I do think that would be a better design; but the change would be invasive.

Yeah...I didn't look more than superficially at this but I am sure you're right.

[… and I have a self-imposed moratorium on reviewing any changes that add options or complexity to drivers/overlay because I just can’t keep up.]

I guess as far as options, #2017 is actually higher priority from my PoV and obviously way simpler (and I'm curious if you agree with that option).

But just to back up on this and help level set: right now I maintain a whole separate stack in bootc (ostree-rs-ext) that is today mapping non-zstd:chunked images ultimately into composefs (indirectly via ostree). My interest is in reducing this duplication, and that's partly where this issue came from. People are today booting via bootc into composefs from OS containers that are gzip, and I don't want to break that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/composefs composefs related changes area/zstd:chunked Issues relating to zstd:chunked
Projects
None yet
Development

No branches or pull requests

4 participants