chunked: store compressed digest if validated

[giuseppe]

if the compressed digest was validated, as it happens when 'pull_options = {convert_images = "true"}' is set, then store it as well so that reusing the blob by its compressed digest works.

Previously, when an image converted to zstd:chunked was pulled a second time, it would not be recognized by its compressed digest, resulting in the need to re-pull the image again.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

I think this works for simple cases, but it is not optimal in general: A single Layer can have multiple compressed representations, and this only stores one.

For ordinary uncompressed layers, that is handled by putBlobToPendingFile (computing both digests and) calling RecordDigestUncompressedPair; that can fully record a M-to-1 mapping. And that mapping is then consulted in tryReusingBlobAsPending.

So, it seems to me that PutBlobPartial should call RecordDigestUncompressedPair if the uncompressed digest is available. It’s already making an assumption that the compressed digest has been validated … though having pkg/chunked explicitly return the value in DriverWithDifferOutput, as this PR does, and explicitly validating that CompressedDigest == blobDigest in TryReusingBlobAsPending, would be more obviously correct.

[mtrmac]

• This might overwrite a valid CompressedDigest value (hypothetically, from a template layer) with ""
• If this field is modified, a corresponding updateDigestMap call needs to happen as well.

I think the other parts of this PR are clearly valuable; this one doesn’t really hurt but seems unnecessary for the purpose. OTOH handling the DriverWithDifferOutput fields consistently is somewhat valuable.

[giuseppe]

thanks, I'll fix it and call updateDigestMap

[giuseppe]

I think this works for simple cases, but it is not optimal in general: A single Layer can have multiple compressed representations, and this only stores one.

The Layer object already knows about the CompressedDigest, so I think we can use for the case where we convert an image and validate the compressed digest.

when we pull a layer without any conversion happening, we store the following information in the overlay-layers/layers.json file:

[{"id":"94e5f06ff8e3d4441dc3cd8b090ff38dc911bfa8ebdb0dc28395bc98f82f983f","created":"2024-07-08T09:28:18.17830826Z","compressed-diff-digest":"sha256:ec99f8b99825a742d50fb3ce173d291378a46ab54b8ef7dd75e5654e2a296e99","compressed-size":3623844,"diff-digest":"sha256:94e5f06ff8e3d4441dc3cd8b090ff38dc911bfa8ebdb0dc28395bc98f82f983f","diff-size":8082944,"compression":2,"uidset":[0],"gidset":[0,42]}]

previously, without the current change we weren't storing the compressed-diff-digest:

[{"id":"d51af96cf93e225825efd484ea457f867cb2b967f7415b9a3b7e65a2f803838a","created":"2024-07-08T09:49:07.867152639Z","diff-digest":"sha256:d51af96cf93e225825efd484ea457f867cb2b967f7415b9a3b7e65a2f803838a","diff-size":4495360,"uidset":[65534,1,8,0],"gidset":[8,0,65534,1],"big-data-names":["zstd-chunked-manifest","zstd-chunked-layer-data"]}]

[mtrmac]

I think this works for simple cases, but it is not optimal in general: A single Layer can have multiple compressed representations, and this only stores one.

The Layer object already knows about the CompressedDigest

It knows one per deduplicated layer. That loses information if the same layer is compressed multiple ways. (And, also, the information is lost when the layer is deleted.)

The way this is expected to work is that is primarily recorded in the c/image BlobInfoCache.

[mtrmac]

@giuseppe containers/image#2478 is what I think code based on this PR can look like; that should work correctly even without the layerStoreapplyDiffFromStagingDirectory part of this PR.

Alternatively, WIP containers/image#2321 among various other changes, also updates BIC, without needing a new c/storage API (but hard-coding the same assumption about what c/storage validates).

[giuseppe]

It knows one per deduplicated layer. That loses information if the same layer is compressed multiple ways. (And, also, the information is lost when the layer is deleted.)

The way this is expected to work is that is primarily recorded in the c/image BlobInfoCache.

I am not opposing the change in c/image. I think we need that as well to deal with multiple compressed digests for the same uncompressed diff. I agree your change in containers/image#2478 is correct and we should have it. I still think though we need also the current change in c/storage so it can be used by APIs like LayersByCompressedDigest

[mtrmac]

LayersByCompressedDigest is fundamentally lossy … but, sure, as long as we have the API, it’s valuable to try to keep it as good as possible.

[giuseppe]

LayersByCompressedDigest is fundamentally lossy … but, sure, as long as we have the API, it’s valuable to try to keep it as good as possible.

so can we merge the current PR? :-)

[mtrmac]

I’m sorry, I screwed something up and this remained “pending” .

LGTM otherwise.

[rhatdan]

/lgtm

[mtrmac]

LGTM, for the record.