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

Fix excessive memory and disk usage when sharing layers #2636

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Nov 18, 2024

Absolutely untested, filing for early review.

createNewLayer can add new entries to s.filenames for uncompressed versions of layers; these entries don't match manifest.LayerInfos, causing the existing logic to assume they are a config and to store them as ImageBigData.

That's both a potentially unlimited disk space waste (recording uncompressed versions of layers unnecessarily), and a memory pressure concern (ImageBigData is created by reading the whole blob into memory).

Instead, precisely track the digest of the config, and never include more than one file in ImageBigData.

Blobs added via PutBlob/TryReusingBlob but not referenced from the manifest are now ignored instead of being recorded inside the image.

This is intended to fix containers/podman#24527 . @nalind is there anything that depends on the current behavior that I’m missing?

Cc: @giuseppe , affects performance measurements.

}
for _, blob := range dataBlobs.Values() {
v, err := os.ReadFile(s.lockProtected.filenames[blob])
// Set up to save the cnofig as a data item. Since we only share layers, the config should be in a file.
Copy link
Member

Choose a reason for hiding this comment

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

typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@nalind
Copy link
Member

nalind commented Nov 19, 2024

This is intended to fix containers/podman#24527 . @nalind is there anything that depends on the current behavior that I’m missing?

I can't recall any places where we're using PutBlob() with isConfig set to true on a local storage destination for a blob that isn't mentioned in the manifest. I'd go so far as to say we don't have many locations that directly call PutBlob() for local storage destinations.

mtrmac added a commit to mtrmac/libpod that referenced this pull request Nov 20, 2024
@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 20, 2024

Podman CI run: containers/podman#24629

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 20, 2024

In manual testing, I have confirmed both the bug hypothesis and that this fixes it.

@rhatdan
Copy link
Member

rhatdan commented Nov 20, 2024

Can you remove Draft flag?

@edsantiago
Copy link
Member

Can this safely be merged? The skip F40, Go is too old commit suggests that this won't be able to be merged into podman or buildah, but it's not clear to me if it's "until F40 gets a new Golang update" or "never as long as F40 exists". go 1.22.6 -> .8 suggests the former but I recommend understanding this before merging.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 21, 2024

Can you remove Draft flag?

I want to see the Podman CI ~passing first.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 21, 2024

Can this safely be merged? The skip F40, Go is too old commit suggests that this won't be able to be merged into podman or buildah

That’s already the case before this PR, due to #2601 .

, but it's not clear to me if it's "until F40 gets a new Golang update" or "never as long as F40 exists". go 1.22.6 -> .8 suggests the former

https://bodhi.fedoraproject.org/updates/FEDORA-2024-9b94592629 means this should be resolved shortly.

In general, I‘ve been assuming that Go patch releases tend to include vulnerability fixes, and therefore tend to be included in Fedora quickly (… and given the static linking of Go binaries, our updated RPMs would be ~blocked on that Go update anyway). That’s not been the case with Go 1.22.{7,8}.

Separately, there’s an effort to change sigstore so that it doesn’t add the dependency on very newest Golang so quickly, around sigstore/sigstore#1879 .

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 21, 2024

containers/podman#24629 looks reasonable, rebased and ready for review.

@mtrmac mtrmac marked this pull request as ready for review November 21, 2024 18:40
@rhatdan
Copy link
Member

rhatdan commented Nov 21, 2024

LGTM
@nalind PTAL

@mtrmac mtrmac force-pushed the only-one-config branch 2 times, most recently from a48541d to 832134b Compare November 25, 2024 20:36
@cgwalters
Copy link
Contributor

So this was a regression from #2067 ? Which was motivated by containers/storage#595 ? Wow, a lot going on there.

Anyways, this code looks superficially sane to me...

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 25, 2024

No, that’s not related at all; #2067 only moves the dataBlobs code. This logic to select what is stored as ImageBigData has existed since the very first versions of the storage transport.

containers/podman#24527 (comment) points at the commit that introduced this bug.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
Merge at will

@cgwalters
Copy link
Contributor

Ahh, OK.

Tangential: just glancing at that code one thing that seems like it'd help is if we had the canonical uncompressed size too in the spec, then some of this could just be mapping between the blob descriptor and and uncompressed descriptor instead of having separate maps for digest and size or so?

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 26, 2024

Tangential: just glancing at that code one thing that seems like it'd help is if we had the canonical uncompressed size too in the spec, then some of this could just be mapping between the blob descriptor and and uncompressed descriptor instead of having separate maps for digest and size or so?

  • I don’t see the relationship to this PR
  • The code needs to handle all currently-existing images; spec extensions are worth considering but would not simplify the code
  • What if the manifest contains a wrong uncompressed size? A lot of the complexity in that file comes from the need to track untrusted / trusted values, and to ensure that a wrong or actively malicious layer in one image does not break using some other image where the layer shares an identifier. (Arguably, that’s 99% a self-imposed problem, when the key we use to deduplicate (uncompressed digest / TOC digest) and the key we use to identify inputs (compressed digests) differ. Maybe, if the key for all were a digest of the full layer metadata, and we gave up on some of the sharing, things could be much simpler to implement. But I’d first like to see a composefs-native c/storage design, supporting arbitrary combinations of independent layers instead of single parent links, before thinking what that would look like.)

createNewLayer can add new entries to s.filenames for
uncompressed versions of layers; these entries don't match
manifest.LayerInfos, causing the existing logic to assume
they are a config and to store them as ImageBigData.

That's both a potentially unlimited disk space waste (recording
uncompressed versions of layers unnecessarily), and a memory
pressure concern (ImageBigData is created by reading the whole
blob into memory).

Instead, precisely track the digest of the config, and never
include more than one file in ImageBigData.

Blobs added via PutBlob/TryReusingBlob but not referenced from
the manifest are now ignored instead of being recorded inside
the image.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac mtrmac merged commit 82bde79 into containers:main Nov 26, 2024
10 checks passed
@mtrmac mtrmac deleted the only-one-config branch November 26, 2024 17:14
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.

High memory usage when pulling large images with pre-existing layers.
6 participants