Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz #2130

mtrmac · 2024-10-10T16:13:18Z

The current value obtained by summing the sizes of regular file contents does not match the size of the uncompressed layer tarball.

We don't have a convenient source to compute the correct size for estargz without pulling the full layer and defeating the point.

For recent zstd:chunked images, we have the full tar-split, so we can compute the correct size; ~~for now, this doesn't do that. That might slow down image size computation.~~

~~Absolutely untested, and we probably do want the tar-split-based computation to happen.~~

mtrmac · 2024-10-10T16:14:51Z

Cc: @giuseppe . This should fix containers/skopeo#2437 (comment) .

edsantiago · 2024-10-10T20:21:42Z

I picked this into my pet pr, and it works slightly better, but now it breaks in the podman additional-store test:

...
<+011ms> # # podman pull -q quay.io/libpod/testimage:20241010
<+070ms> # 9c6e6209f54a048342fd899e1e0885be64dfc836ed3664b33d6d07bcb4fc1c51
         # 263622a183c94c2433a43be5464a954b9c4e5b0a77cb177f6fe60dacfff66f80
         # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
         # #|     FAIL: pull -q quay.io/libpod/testimage:20241010, using storage.conf
         # #| expected: '263622a183c94c2433a43be5464a954b9c4e5b0a77cb177f6fe60dacfff66f80'
         # #|   actual: '9c6e6209f54a048342fd899e1e0885be64dfc836ed3664b33d6d07bcb4fc1c51'
         # #|         > '263622a183c94c2433a43be5464a954b9c4e5b0a77cb177f6fe60dacfff66f80'
         # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Basically, podman pull now spits out two hashes instead of one. Fails very reliably on my laptop too:

$ hack/bats -T --root 010
...
same failure, down to the same hashes

edsantiago · 2024-10-10T20:34:52Z

Skopeo is involved, though (for the copy to the store dir), and I haven't rebuilt skopeo, so it's possible that the bad thing is happening in that step.

mtrmac · 2024-10-10T21:05:25Z

Thanks! I can‘t see how the two-ID output can happen.

I’ll set up a VM and test (a better version of) this PR, as well as those tests, tomorrow.

mtrmac · 2024-10-11T23:02:20Z

PR updated to compute the size correctly for zstd:chunked with tar-split, manually tested per containers/skopeo#2437 (comment) (and inspecting the created layer metadata).

Ready for review.

mtrmac · 2024-10-12T00:36:32Z

$ hack/bats -T --root 010
...
same failure, down to the same hashes

Testing that on a slightly unclean VM, Podman commit 2aacd4e212525db4ee06be8e44e4405400d4df9d + this c/storage fix passes 010 successfully in make localsystem; the command above fails with a lot of unexpected, different, errors, e.g. about localhost/podman-pause. It’s very possible there’s something wrong about that environment, I know very little about Podman tests.

And, on the command line, podman pull -q quay.io/libpod/testimage:20241010 outputs just 9c6e6209f54a048342fd899e1e0885be64dfc836ed3664b33d6d07bcb4fc1c51

mtrmac · 2024-10-12T00:37:59Z

Scratch that last part, that’s not invoking the tested version.

edsantiago · 2024-10-12T00:40:13Z

Ugh. The podman-pause thing is my fault, I couldn't find a good solution to a nasty problem. Please try this:
containers/podman@9d8e3b0

cgwalters

What about adding the uncompressed size to the ToC? Seems like it'd possibly simplify things?

This looks sane to me but I only gave it a superficial review.

cgwalters · 2024-10-14T15:26:55Z

pkg/chunked/compression_linux.go

@@ -288,6 +288,36 @@ func ensureTOCMatchesTarSplit(toc *internal.TOC, tarSplit []byte) error {
 	return nil
 }

+// tarSizeFromTarSplit computes the total tarball size, computing only tarSplit


Suggested change

// tarSizeFromTarSplit computes the total tarball size, computing only tarSplit

// tarSizeFromTarSplit computes the total tarball size using only the tarSplit metadata

Thanks, I don’t know what I was thinking.

cgwalters · 2024-10-14T15:27:33Z

pkg/chunked/compression_linux.go

+		case storage.FileType:
+			// entry.Size is the “logical size”, which might not be the physical size for sparse entries;
+			// but the way tar-split/tar/asm.WriteOutputTarStream combines FileType entries and returned files contents,
+			// sparse files are not supported.


But we don't error out today?

If we should be erroring out, that’s something that should happen at tar-split construction.

The physical size outright isn’t available in the tar-split format, so here this consumer is only documenting the impact of that assumption.

(As is, sparse files are barely supported by archive/tar or the tar-split fork: they are indicated either by a special file type, or as a regular file a special PAX record; and there is no API to expose the data / hole segments. Also, c/storage/pkg/archive does not special-case them at all.)

mtrmac · 2024-10-14T18:09:43Z

What about adding the uncompressed size to the ToC? Seems like it'd possibly simplify things?

It might be a bit late for that. The format is documented “as normal” with no warning that we might want to change the format and break images. IIRC we sort of did that when introducing the tar-split element, but already at that time we had to relent and re-introduce at least the previous level of support of the no-tar-split format. Still, if this were the only concern, we should at least document the idea around pkg/chunked/internal.TOC for future consideration.
If we did include the size in the TOC, we would have to worry about producers recording incorrect sizes. I mean, we have to worry about that anyway… but the tar-split metadata must be correct exactly for the operation which primarily relies on the uncompressed size value, so that is not introducing a new assumption. Compare also how we have 3 different sources of metadata in zstd:chunked files — it seems to me that reducing redundancy is a better trade-off here anyway.

Anyway, I’m open to adding a comment to the TOC type.

The current value obtained by summing the sizes of regular file contents does not match the size of the uncompressed layer tarball. We don't have a convenient source to compute the correct size for estargz without pulling the full layer and defeating the point; so we must allow for the size being unknown. For recent zstd:chunked images, we have the full tar-split, so we can compute the correct size; that will happen in the following commits. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Empty tar-split shouldn't ever happen, but being precise here doesn't hurt. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2024-10-14T21:21:30Z

@edsantiago

On a VM manually building things:

All of:

Podman main commit 11ab0b72465432d93c5421d566ad0501f072a954 without this PR
containers/podman@9d8e3b0 without this PR
Podman main commit 11ab0b72465432d93c5421d566ad0501f072a954 with this PR
containers/podman@9d8e3b0 with this PR

pass make localsystem, apart from [500] podman networking: port with --userns=keep-id for rootless or --uidmap=* for rootful failing to find rootlessport, very likely unrelated.

Similarly, containers/podman@9d8e3b0 passes hack/bats -T --root 010, with or without this PR; and the podman main commit fails, with or without this PR.

Either way, I haven’t been able to reproduce this:

Basically, podman pull now spits out two hashes instead of one.

I have now filed containers/podman#24265 to exercise that in the usual environment.

Is there anything else I should be looking at?

> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@chunked-size > go mod tidy && go mod vendor + a HACK to override the bloat check Signed-off-by: Miloslav Trmač <mitr@redhat.com>

edsantiago · 2024-10-14T21:29:13Z

@mtrmac please forgive me, I've switched gears for today and have commitments I must attend to. The crucial element I don't see in your comment is running the full CI suite using testimage:20241010 (or 1009). Those two images were pushed with zstd and are the ones that cause all the problems. Thing is, the failing tests use skopeo for one step, and I'm pretty sure you might also need to patch that. I never was able to figure that out, and can't look into it now.

mtrmac · 2024-10-14T21:31:14Z

Thanks, I was looking for references to testimage:20241010 and I couldn’t find any. I’ll revisit tomorrow.

edsantiago · 2024-10-15T20:05:04Z

Thank you. That's really pretty huge.

rhatdan · 2024-10-15T21:58:16Z

/lgtm

kolyshkin · 2024-10-15T22:03:16Z

layers.go

+	// - If UncompressedDigest is set, this must be set to a valid value.
+	// - Otherwise, if TOCDigest is set, this is either valid or -1.
+	// - If neither of this digests is set, this should be treated as if it were
+	//   an uninitialized value.


You need indentation for list items (same as for code blocks, i.e. an extra space).

Suggested change

// - If UncompressedDigest is set, this must be set to a valid value.

// - Otherwise, if TOCDigest is set, this is either valid or -1.

// - If neither of this digests is set, this should be treated as if it were

// an uninitialized value.

// - If UncompressedDigest is set, this must be set to a valid value.

// - Otherwise, if TOCDigest is set, this is either valid or -1.

// - If neither of this digests is set, this should be treated as if it were

// an uninitialized value.

Yeah, gofmt didn’t want to help with this (and struct field comments are formatted in HTML just as an uninterpreted code block), so I punted.

You’re right, this is the right thing to do. Fixed in #2136 .

kolyshkin · 2024-10-15T22:12:09Z

pkg/chunked/storage_linux.go

+	if tarSplit != nil {
+		uncompressedTarSize, err = tarSizeFromTarSplit(tarSplit)
+		if err != nil {
+			return nil, fmt.Errorf("computing size from tar-split")


nit: errors.New, or fmt.Errorf("computing site from tar-split: %w", err), or just err.

Thanks! Fixed in #2136 .

mtrmac · 2024-10-15T22:29:49Z

So, for your purposes, change 20241011 above to 20240123, and import the 11-12-13 changes to 320-system-df, and skip (or ignore) the apiv2 tests.

Note to self (and in case it is useful to others): Commit “DO NOT MERGE: Test with a zstd:chunked testimage” in containers/podman#24287 .

Follow-ups to #2130

cgwalters · 2024-10-16T12:46:35Z

The zstd:chunked implementation changes how image IDs are computed: So far, (for schema2 and OCI), the image ID == config digest. With zstd:chunked, partially-pulled images and fully-pulled images (depending on the code path and other options) have different IDs, there can be 2^(layers) different IDs for “the same” image.

Is there more info on that? Sounds worth adding to docs/containers-storage-composefs.md perhaps.

mtrmac · 2024-10-16T18:56:24Z

@cgwalters That’s not directly a c/storage property; it’s a c/image choice of a a deterministic image ID, in https://github.com/containers/image/blob/cba49408c0ea237a6aa6dba5e81b74f4a8f23480/storage/storage_dest.go#L671-L684 .

Yes, we might eventually need a user-facing explanation; I’m not sure where is a good place for it, the internal c/* projects are not too likely to be known to users. Maybe a Podman blog post.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

openshift-ci bot added do-not-merge/work-in-progress approved labels Oct 10, 2024

mtrmac force-pushed the chunked-size branch from 96b0ad1 to 451245f Compare October 10, 2024 16:14

mtrmac mentioned this pull request Oct 10, 2024

writing blob: Size mismatch containers/skopeo#2437

Closed

mtrmac force-pushed the chunked-size branch from 451245f to 92824f9 Compare October 11, 2024 22:55

mtrmac changed the title ~~Don't set UncompressedSize on chunked pull~~ Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz Oct 11, 2024

mtrmac marked this pull request as ready for review October 11, 2024 23:02

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 11, 2024

mtrmac force-pushed the chunked-size branch from 92824f9 to 992fa3e Compare October 11, 2024 23:37

mtrmac added the kind/bug label Oct 11, 2024

cgwalters reviewed Oct 14, 2024

View reviewed changes

mtrmac force-pushed the chunked-size branch from 992fa3e to 2d22674 Compare October 14, 2024 17:43

mtrmac added 3 commits October 14, 2024 20:10

Explicitly differentiate between empty and missing tar-split

7eb4a10

Empty tar-split shouldn't ever happen, but being precise here doesn't hurt. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Compute the layer size from tar-split for zstd:chunked layers

f979bad

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the chunked-size branch from 2d22674 to f979bad Compare October 14, 2024 18:10

This was referenced Oct 15, 2024

Only return one image ID, and hopefully a more precise one, from pulling. containers/common#2202

Merged

Update system tests to handle zstd:chunked images containers/podman#24286

Merged

openshift-ci bot assigned rhatdan Oct 15, 2024

openshift-ci bot added the lgtm label Oct 15, 2024

openshift-merge-bot bot merged commit 14d1fce into containers:main Oct 15, 2024
19 checks passed

kolyshkin reviewed Oct 15, 2024

View reviewed changes

mtrmac deleted the chunked-size branch October 15, 2024 22:31

mtrmac mentioned this pull request Oct 15, 2024

Follow-ups to #2130 #2136

Merged

vrothberg added a commit that referenced this pull request Oct 16, 2024

Merge pull request #2136 from mtrmac/chunked-size-2

b417e8d

Follow-ups to #2130

mtrmac mentioned this pull request Oct 16, 2024

DO NOT MERGE: Testing https://github.com/containers/storage/pull/2130 containers/podman#24265

Closed

cgwalters mentioned this pull request Oct 17, 2024

zstd:chunked issues containers/bootc#509

Open

mtrmac mentioned this pull request Oct 17, 2024

Consistently use a string type for expectedLayerDiffIDFlag containers/image#2603

Merged

mtrmac added a commit to mtrmac/libpod that referenced this pull request Oct 18, 2024

Update c/storage after containers/storage#2130

d296cb6

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz #2130

Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz #2130

mtrmac commented Oct 10, 2024 •

edited

Loading

mtrmac commented Oct 10, 2024

edsantiago commented Oct 10, 2024

edsantiago commented Oct 10, 2024

mtrmac commented Oct 10, 2024

mtrmac commented Oct 11, 2024

mtrmac commented Oct 12, 2024

mtrmac commented Oct 12, 2024

edsantiago commented Oct 12, 2024

cgwalters left a comment

cgwalters Oct 14, 2024

mtrmac Oct 14, 2024

cgwalters Oct 14, 2024

mtrmac Oct 14, 2024

mtrmac commented Oct 14, 2024

mtrmac commented Oct 14, 2024

edsantiago commented Oct 14, 2024

mtrmac commented Oct 14, 2024

edsantiago commented Oct 15, 2024

rhatdan commented Oct 15, 2024

kolyshkin Oct 15, 2024

mtrmac Oct 15, 2024

kolyshkin Oct 15, 2024

mtrmac Oct 15, 2024

mtrmac commented Oct 15, 2024

cgwalters commented Oct 16, 2024

mtrmac commented Oct 16, 2024

	// tarSizeFromTarSplit computes the total tarball size, computing only tarSplit
	// tarSizeFromTarSplit computes the total tarball size using only the tarSplit metadata

Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz #2130

Correctly compute UncompressedSize on zstd:chunked pull, don’t set it on estargz #2130

Conversation

mtrmac commented Oct 10, 2024 • edited Loading

mtrmac commented Oct 10, 2024

edsantiago commented Oct 10, 2024

edsantiago commented Oct 10, 2024

mtrmac commented Oct 10, 2024

mtrmac commented Oct 11, 2024

mtrmac commented Oct 12, 2024

mtrmac commented Oct 12, 2024

edsantiago commented Oct 12, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Oct 14, 2024

mtrmac commented Oct 14, 2024

edsantiago commented Oct 14, 2024

mtrmac commented Oct 14, 2024

edsantiago commented Oct 15, 2024

rhatdan commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Oct 15, 2024

cgwalters commented Oct 16, 2024

mtrmac commented Oct 16, 2024

mtrmac commented Oct 10, 2024 •

edited

Loading