blobinfocache: cache compression types #1089

nalind · 2020-11-23T15:59:48Z

Extend the blob info cache to also cache the name of the type of compression used on a blob in a known location. (An earlier version of this patch cached the full MIME type, but required additional logic to map the MIME types in cases where we converted images during copying, so the patch was larger.)

Pass an updated MIME type and information about compression changes to PutBlob() in its input BlobInfo, so that it can populate the blob info cache with correct compression information when it succeeds. Some of the local tables we kept in manifest/common.go were moved to an internal package (internal/mediatypes) so that they could be reused there.

Make sure that when dockerImageDestination.TryReusingBlob() successfully uses a blob from the blob info cache, that it provides updated MIME type and compression information in the BlobInfo that it returns, so that converted manifests can use them to correctly update their LayerInfo fields.

This should fix #733.

mtrmac

Thanks for working on this!

(Not at all a review, just a minimal first skim. Making most of this ~unavoidably visible through the public API is the main worry.)

types/types.go

pkg/blobinfocache/boltdb/boltdb.go

docker/docker_image_dest.go

nalind · 2020-12-02T01:36:46Z

Dropped the "MediaType is always correct" goal because handling switching to zstd compression while updating a schema2 manifest wasn't looking doable without restructuring how the copy package was handling manifests.
The updated version makes TryReusingBlob() do most of the work, in that if it returns a non-nil CompressionAlgorithm in the returned BlobInfo, the existing manifest UpdateLayerInfos logic takes care of most of the rest. The version in the "docker" transport uses the blob info cache, while the "directory" and "oci" versions look at the blobs that they find.
The "docker" transport's GetBlob trusts the MediaType in the source BlobInfo that it's given. Pulling schema1 images could be a problem here -- the bits that had it supply a fake MediaType in its LayerInfos didn't look like they were needed for passing tests, so I dropped them out. They can come back if need be.
The "docker" transport's PutBlob just looks at the blob to make sure it's recording the correct information about how a blob is compressed.

nalind · 2020-12-03T21:21:16Z

The "docker" transport's GetBlob() implementation now detects compression on blobs that it retrieves for recording compression information instead of guessing based on the passed-in MIME type, so it no longer needs visibility into conversion information that the manifest package has.
The current revision drops the hunk that migrates compression type information into an internal/mediatypes package, because it's no longer necessary.

nalind · 2020-12-07T19:45:35Z

Rebased.

nalind · 2020-12-07T20:14:08Z

docker/docker_image_dest.go

+		var compressorName string
+		if info.CompressionAlgorithm != nil {
+			compressorName = info.CompressionAlgorithm.Name()
+		}


I'm not completely sure we end up recording the right value here.

Changed the TryReusingBlob() case where the registry indicates that it already has the blob so that we record the compression type as UnknownCompression. The MIME type we're passed from copy comes from the source image's manifest, so it should already be correct for the blob with the same digest without needing any fixup from us.

mtrmac

So far basically just looking at the API, I’ll continue reading the actual implementation tomorrow.

types/types.go

mtrmac · 2020-12-07T22:27:17Z

types/types.go

+// BICReplacementCandidate2 is an item returned by BlobInfoCache2.CandidateLocations2.
+type BICReplacementCandidate2 struct {
+	Digest         digest.Digest
+	CompressorName string


Can this be “uncompressed” or “unknown”? If so, that should be documented and distinct.

And do we just soldier on when getting “unknown”, or prefer correctness and don’t reuse? Just by asking that question I’m somewhat suggesting to prefer correctness, OTOH that would make third-party cache implementations half-ignored. Something for me to think about while I continue reading tomorrow.

Yes, I think it makes sense to mark cases where we don't know because we retrieved the data from a cache that original methods. Added constants for uncompressed and unknown to avoid ambiguity.

Distinguishing cases where the compression type isn't known from uncompressed cases would keep us from actively breaking things, so I'm leaning that way as well. Updated dockerImageDestination.TryReusingBlob() to skip candidate locations where the compression type isn't known.

Okay, I think we don't need to skip these any more.

What happens with unknown compression now? If that happens (i.e. an old BoltDB cache that knows a location but no compression, or a third-party BlobInfoCache implementation), don’t we fall back to the old behavior of using that blob but not updating the MIME type to match?

OperationAndAlgorithmForCompressor() gets passed UnknownCompression and returns PreserveOriginal with no specified compressor, and dockerImageDestination.TryReusingBlob() returns them in a BlobInfo with the original media type and no compression changes. When that eventually gets to manifest.updatedMIMEType(), it doesn't make any changes. So until we actually read that blob again, we will continue to behave as before when reusing that blob.

Isn’t “operate as before” #733? Some version of this PR skipped the blobs with UnknownCompression: #1089 (comment) .

That might force an extra unnecessary upload for schema1 (where the MIME type does not matter), but with schema1 disabled by docker/distribution by default, fixing #733 more reliably seems preferable.

pkg/blobinfocache/memory/memory_test.go

pkg/blobinfocache/memory/memory.go

mtrmac · 2020-12-07T22:44:39Z

pkg/blobinfocache/internal/test/test.go

@@ -103,6 +116,14 @@ func assertCandidatesMatch(t *testing.T, scopeName string, expected []candidate,
 	assert.Equal(t, e, actual)
 }

+func assertCandidatesMatch2(t *testing.T, scopeName string, expected []candidate, actual []types.BICReplacementCandidate2) {


If the set of new tests is the same as the old tests (is it?), maybe instead move the cache.CandidateLocations{,2} calls inside the assert… helper, have a single helper call test both methods, and we can avoid the duplication.

(This can clearly wait until after the interface is settled.)

pkg/blobinfocache/internal/test/test.go

pkg/blobinfocache/internal/prioritize/prioritize_test.go

nalind · 2020-12-09T17:49:50Z

Rebased, and fixed cases where we incorrectly used the io.Reader returned by pkg/compression.DetectCompressionFormat().

mtrmac

Just some half-baked thoughts, hopefully useful a bit.

mtrmac · 2020-12-09T22:23:03Z

directory/directory_dest.go

+		return false, types.BlobInfo{}, err
+	}
+
+	compressionOperation, compressionAlgorithm, _, _, err := blobinfocache.DetectCompressionFormat(f)


Both in this and the OCI/layout case, the code only looks for exactly the specified digest and no substitution happens. So this should never cause a change in the MIME type, and the two calls to DetectCompressionFormat seem unnecessary.

OTOH what about the canSubstitute case in storageImageDestination.TryReusingBlob? That one might need a MIME type update.

storage is a bit of a weird beast here. It uses LayersInfosForCopy(), so it's never on the hook for being able to reproduce the blobs that it's given. It's also in the business of trying to avoid causing changes to the manifest even when they're allowed, so it wants to only ever behave as though canSubstitute is false.

Yes, OTOH we already need to provide an uncompressed size so that the generic copy code is consistent, IIRC, so it makes sense for that rare code path to be fully consistent instead of partially.

Let’s see how the other paths pan out, at this point it’s probably just something to eventually verify.

docker/docker_image_dest.go

internal/blobinfocache/types.go

copy/copy.go

docker/docker_image_dest.go

types/types.go

mtrmac

We could change CandidateLocations2() to ignore entries for which the compression type isn't known - pkg/blobinfocache/boltdb/cache.appendReplacementCandidates() would just return if the compressionBucket wasn't there or if it failed to return a compressorNameValue for digestKey.

That’s actually a good point: a previous version had the UnknownCompression check in dockerImageDestination.TryReusingBlob, but it’s not conceptually the transports’ responsibility to care. Defining CandidateLocations2 to never include UnknownCompression entries would get that knowledge out of the transports, at least, and simplify the 2 cache implementations + the compatibility wrapper a bit — OTOH it would be three places responsible for the same check, and probably require test changes.

(Conceptually the decision ideally belongs to the copy pipeline around copyLayer, OTOH that would require the TryReusingBlob change we’re trying to avoid, and it would be non-trivial — a callback from TryReusingBlob back into the copy code to determine which candidate locations are acceptable?) So now I don’t have a preference WRT locating the check in dockerImageDestination vs. CandidateLocations2.

As for having the UnknownCompression check or not — I’d prefer to include it, so that users can just upgrade without having to manually clear caches. Is it blocking? I guess not (it can be added in a follow-up, after all), but it’s very close.

nalind · 2021-01-15T19:04:27Z

As for having the UnknownCompression check or not — I’d prefer to include it, so that users can just upgrade without having to manually clear caches. Is it blocking? I guess not (it can be added in a follow-up, after all), but it’s very close.

I lean that way myself, so I can change it.

nalind · 2021-01-15T21:21:41Z

OTOH it would be three places responsible for the same check, and probably require test changes.

Yes, it did. Hopefully I got them all.

mtrmac

LGTM, some very last nits.

Thanks for all your work!

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/memory/memory.go

internal/blobinfocache/types.go

internal/blobinfocache/blobinfocache.go

nalind · 2021-01-15T22:12:57Z

Okay, updated and back in the CI queue.

copy/copy.go

Fix a compile warning in a test. Signed-off-by: Nalin Dahyabhai <[email protected]>

Add some tests for manifest.updatedMIMEType. Signed-off-by: Nalin Dahyabhai <[email protected]>

Extend the blob info cache to also cache the name of the type of compression used on a blob that we've seen, or specific values that indicate that we know the blob was not compressed, or that we don't know whether or not it was compressed. New methods for adding known blob-compression pairs and reading candidate locations including compression information are part of a new internal BlobInfoCache2 interface which the library's BlobInfoCache implementors also implement. When we copy a blob, try to record the state of compression for the source blob, and if we applied any changes, the blob we produced. Make sure that when TryReusingBlob successfully uses a blob from the blob info cache, that it provides compression information in the BlobInfo that it returns, so that manifests can be updated to describe layers using the correct MIME types. When attempting to write a manifest, if a manifest can't be written because layers were compressed using an algorithm which can't be expressed using that manifest type, continue on to trying other manifest formats. Signed-off-by: Nalin Dahyabhai <[email protected]>

mtrmac

LGTM. Thanks again!

Romain-Geissler-1A · 2021-01-27T14:29:42Z

Hi,

Sorry for jumping into this pull request. I think I may suffer from this issue using podman + Artifactory. I wanted to try with the latest fedora rawhide which apparently uses podman from master branch everyday, but it looks like I still have the issue. Checking podman source code, apparently the version of containers/image used there is still 5.9.0 from last december. Would you know if the container team in Red Hat plans on updating containers/image in podman in January or February, and start pushing updated podman packages with this fix in fedora rawhide ?

Cheers,
Romain

rhatdan · 2021-01-27T15:43:14Z

We should be cutting a new release of containers/image soon, and then this will get vendored into podman Most likely podman 3.1 or later.

jomeier · 2021-01-27T18:04:37Z

Yes could you cut a new release, please? A pluriversum of developers is waiting for that.

jomeier · 2021-01-27T18:21:16Z

Skopeo and all images mirrored to quay for OKD and the community operators, ... are also dependent on that.

mtrmac · 2021-01-27T18:52:44Z

#1128 is ongoing.

rhatdan · 2021-01-27T20:10:49Z

Yes this fix with be in podman 3.0 and newer versions of skopeo and buildah which will show up in rhel8.4.

mtrmac reviewed Nov 23, 2020

View reviewed changes

types/types.go Outdated Show resolved Hide resolved

pkg/blobinfocache/boltdb/boltdb.go Outdated Show resolved Hide resolved

docker/docker_image_dest.go Outdated Show resolved Hide resolved

nalind force-pushed the blob-info-cache-media-types branch 3 times, most recently from af75978 to 2f89fba Compare November 24, 2020 14:51

mtrmac mentioned this pull request Nov 24, 2020

podman build should support building V2.2 manifests containers/buildah#2317

Closed

vrothberg mentioned this pull request Nov 30, 2020

invalid tar header: unknown containers/buildah#1589

Closed

nalind force-pushed the blob-info-cache-media-types branch 2 times, most recently from fd2134a to 2767594 Compare December 1, 2020 23:19

nalind force-pushed the blob-info-cache-media-types branch 5 times, most recently from 1f26e4b to 54b1d26 Compare December 3, 2020 19:13

nalind force-pushed the blob-info-cache-media-types branch from 54b1d26 to 89621de Compare December 7, 2020 14:45

nalind mentioned this pull request Dec 7, 2020

020-copy.bats: check that we set the manifest type correctly containers/skopeo#1124

Merged

nalind force-pushed the blob-info-cache-media-types branch from 89621de to 9d239cb Compare December 7, 2020 19:45

nalind commented Dec 7, 2020

View reviewed changes

mtrmac reviewed Dec 7, 2020

View reviewed changes

nalind force-pushed the blob-info-cache-media-types branch 3 times, most recently from d97bf66 to f8c84ef Compare December 9, 2020 17:49

nalind force-pushed the blob-info-cache-media-types branch 2 times, most recently from fa20fe8 to 8113d28 Compare December 9, 2020 20:30

mtrmac reviewed Dec 9, 2020

View reviewed changes

nalind force-pushed the blob-info-cache-media-types branch 2 times, most recently from 6299052 to 32d0532 Compare December 15, 2020 21:21

mtrmac reviewed Jan 15, 2021

View reviewed changes

nalind force-pushed the blob-info-cache-media-types branch from e310c82 to 5e6ae45 Compare January 15, 2021 21:21

mtrmac reviewed Jan 15, 2021

View reviewed changes

pkg/blobinfocache/boltdb/boltdb.go Show resolved Hide resolved

pkg/blobinfocache/memory/memory.go Show resolved Hide resolved

internal/blobinfocache/types.go Show resolved Hide resolved

internal/blobinfocache/blobinfocache.go Show resolved Hide resolved

nalind force-pushed the blob-info-cache-media-types branch from 5e6ae45 to 705ae93 Compare January 15, 2021 22:06

mtrmac reviewed Jan 15, 2021

View reviewed changes

copy/copy.go Show resolved Hide resolved

mtrmac mentioned this pull request Jan 15, 2021

Clarify the canModifyBlob condition in copyBlobFromStream #1119

Merged

nalind added 3 commits January 16, 2021 13:56

ostree.TestReferenceSignaturePath: fix a compiler warning in a test

53de4b6

Fix a compile warning in a test. Signed-off-by: Nalin Dahyabhai <[email protected]>

manifest: add a test for UpdatedMIMEType

de300ab

Add some tests for manifest.updatedMIMEType. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the blob-info-cache-media-types branch from 705ae93 to 5364600 Compare January 16, 2021 19:01

mtrmac approved these changes Jan 18, 2021

View reviewed changes

mtrmac merged commit f90eda4 into containers:master Jan 18, 2021

nalind deleted the blob-info-cache-media-types branch January 18, 2021 14:55

LorbusChris mentioned this pull request Jan 18, 2021

quay.io images have invalid v1 manifest mimetype okd-project/okd#402

Closed

nalind mentioned this pull request Feb 3, 2021

copy: compute blob compression on reused blobs based on source MediaType #1138

Merged

mtrmac mentioned this pull request Feb 10, 2021

--dest-compress doesn't change layers mediaType in manifest containers/skopeo#1192

Closed

This was referenced Feb 22, 2021

bump buildah to v1.16.7 and imagebuilder to v1.1.8 openshift/builder#183

Closed

[release-5.10] copy: compute blob compression on reused blobs based on source MediaType #1162

Merged

nalind mentioned this pull request Mar 9, 2021

Bug 1905095: bump github.com/containers/image openshift/builder#221

Merged

mtrmac mentioned this pull request Nov 22, 2021

Remove support for pulling v2 schema1 moby/moby#42300

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blobinfocache: cache compression types #1089

blobinfocache: cache compression types #1089

nalind commented Nov 23, 2020

mtrmac left a comment

nalind commented Dec 2, 2020

nalind commented Dec 3, 2020

nalind commented Dec 7, 2020

nalind Dec 7, 2020

nalind Dec 9, 2020

mtrmac left a comment

mtrmac Dec 7, 2020

nalind Dec 9, 2020

nalind Dec 9, 2020

nalind Dec 15, 2020

mtrmac Jan 14, 2021

nalind Jan 15, 2021

mtrmac Jan 15, 2021

mtrmac Dec 7, 2020

nalind commented Dec 9, 2020

mtrmac left a comment

mtrmac Dec 9, 2020

nalind Dec 10, 2020

mtrmac Dec 10, 2020

mtrmac left a comment

nalind commented Jan 15, 2021

nalind commented Jan 15, 2021

mtrmac left a comment

nalind commented Jan 15, 2021

mtrmac left a comment

Romain-Geissler-1A commented Jan 27, 2021

rhatdan commented Jan 27, 2021

jomeier commented Jan 27, 2021

jomeier commented Jan 27, 2021

mtrmac commented Jan 27, 2021

rhatdan commented Jan 27, 2021

blobinfocache: cache compression types #1089

blobinfocache: cache compression types #1089

Conversation

nalind commented Nov 23, 2020

mtrmac left a comment

Choose a reason for hiding this comment

nalind commented Dec 2, 2020

nalind commented Dec 3, 2020

nalind commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac 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

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

nalind commented Dec 9, 2020

mtrmac 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

mtrmac left a comment

Choose a reason for hiding this comment

nalind commented Jan 15, 2021

nalind commented Jan 15, 2021

mtrmac left a comment

Choose a reason for hiding this comment

nalind commented Jan 15, 2021

mtrmac left a comment

Choose a reason for hiding this comment

Romain-Geissler-1A commented Jan 27, 2021

rhatdan commented Jan 27, 2021

jomeier commented Jan 27, 2021

jomeier commented Jan 27, 2021

mtrmac commented Jan 27, 2021

rhatdan commented Jan 27, 2021