copy/multiple: instanceCopyCopy honor `UpdateCompressionAlgorithms` #2047

flouthoc · 2023-07-18T07:53:48Z

Needed By: containers/skopeo#2047

Users can set compression formats while performing copy in such cases
instanceCopyCopy must honor UpdateCompressionAlgorithms.
copy/single: set requiredCompression if compressionFormat is changed

Modifies copy/single to correctly use the feature from
blob: TryReusingBlobWithOptions consider RequiredCompression if set #2023, that is if compression is
changed blob must be resued only if it matches required compression.

oci_index: init Annotations if nil and required

If UpdateCompressionAlgorithms is set then Annotations map must be
initialized if its not set.

Integration tests for e2e feature added here: containers/skopeo#2047 (comment)

flouthoc · 2023-07-18T07:54:29Z

@mtrmac PTAL once merged I will vendor this into containers/skopeo#2047

mtrmac

Consider a simple skopeo copy --format oci of an existing image: In that case we should preserve the original blobs (or reuse blobs at destination, if possible), and add the OCI annotations.

So while copy --dest-compress-format zstd is the most salient use case, I think this should work by collecting compression algorithms from all copies.

IIRC the BlobInfo returned from copyLayer always contains an algorithm, or Unknown/Uncompressed.
Then, copyLayers can deduplicate the values into a set; and return that value.
copySingleImage can return the relevant algorithms (optionally add a copySingleImageResult struct so that we don’t have that many unnamed return values)
and that can then be used for ListEdit

copy/single.go

internal/manifest/oci_index.go

mtrmac · 2023-07-18T22:16:32Z

Consider a simple skopeo copy --format oci of an existing image: In that case we should preserve the original blobs (or reuse blobs at destination, if possible), and add the OCI annotations.

(If the source is not already OCI, it is probably v2s1 or v2s2, which only support gzip, so that seems like a non-issue. But the copy can reuse zstd blobs at the destination, creating an OCI image that is mixed gzip + Zstd, or possibly Zstd-only.)

mtrmac

Also I think the shortcut in OptimizeDestinationImageAlreadyExists must not be taken if require…Match.

mtrmac · 2023-07-19T01:13:33Z

Ugh, the OptimizeDestinationImageAlreadyExists path really is a pain… When we do a full copy, we do get algorithm data from all blobs as a side-effect. When we skip that, all of that is missing and needs to be obtained elsewhere. Options I can see:

Teach internal/image.SourcedImage (and manifest implementations) to return a list of algorithms from a manifest. That’s quite possible (we have compressionMIMETypeSet tables already), probably cleanest, but it’s another new API addition, and the data we would get that way comes from a different source than from ordinary copies.
Take the list.Instance().ReadOnly.CompressionAlgorithms data, which was recently computed, pass it from the outside down to copySingleImage, and have copySingleImage return that to the caller on the Optimize… path. Technically that would work, except that the value would not be available in the non-multi-image callers of copySingleImage, i.e. that would introduce an inconsistency in behavior.
Take the conceptually-misplaced heuristic determining CompressionAlgorithm at the start of copyLayer; extract it to a separate helper function; and call that in compareImageDestinationManifestEqual to build the data. That way the algorithm values we return is determined exactly the same way on the Optimize… path and on the other one, and always available.

I think 3. is the cleanest way to do this right now; and if we ever replace the “conceptually-misplaced heuristic” (which serves as a nice example that technical debt never goes away on its own) with a proper manifest API addition, that would naturally handle both code paths.

flouthoc · 2023-07-19T10:07:26Z

@mtrmac I have not updated tests yet but this is what I am thinking. WDYT ?

mtrmac

A quick pass, not a careful review — so far all the primary design points look correct.

copy/multiple.go

mtrmac · 2023-07-19T15:12:24Z

copy/single.go

 }

+func removeDuplicateCompression(algos []compressiontypes.Algorithm) []compressiontypes.Algorithm {


I think this can be built on internal/set.Set instead of manually.

… and probably inlined in the two loops that collect the data.

Set.Add( ensures that no duplicates are there ( I have added a test to verify that ) however compression.Algorithm is not a comparable which is required by Set hence I have created a Set using string i.e compression.Algorithm.Name() and its working fine.

Later I'm using AlgorithmsByNames to retrieve a slice of Algorithms.

copy/single.go

mtrmac

A full review now — ~nothing new.

copy/single.go

mtrmac

Thanks!

Last comments on a debug log and a misplaced comment.

internal/set/set_test.go

copy/single.go

pkg/compression/compression.go

If `UpdateCompressionAlgorithms` is set then `Annotations` map must be initialized if its not set. Signed-off-by: Aditya R <[email protected]>

Modifies `copy/single` to correctly use the feature from containers#2023, that is if compression is changed blob must be resued only if it matches required compression. Signed-off-by: Aditya R <[email protected]>

imagecopier implementes feature for requireCompressionFormatMatch refactor `copySingleImage` to accept that as an argument. Signed-off-by: Aditya R <[email protected]>

mtrmac

LGTM otherwise, feel free to merge without another round.

copy/single.go

Users can set compression formats while performing `copy` in such cases `instanceCopyCopy` must honor UpdateCompressionAlgorithms. Signed-off-by: Aditya R <[email protected]>

Verifies if no duplicates are kept when `.Values()` is used to return slice. Signed-off-by: Aditya R <[email protected]>

flouthoc · 2023-07-21T03:05:04Z

I don't have merge access, @containers/image-maintainers PTAL

mtrmac

Thanks!

flouthoc mentioned this pull request Jul 18, 2023

copy: add support for --format zstd containers/skopeo#2047

Closed

flouthoc mentioned this pull request Jul 18, 2023

copy: implement instanceCopyClone for zstd compression #1987

Merged

mtrmac reviewed Jul 18, 2023

View reviewed changes

copy/single.go Outdated Show resolved Hide resolved

internal/manifest/oci_index.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 19, 2023

View reviewed changes

flouthoc force-pushed the instancecopycopy-compression branch from 6988b8e to 6571477 Compare July 19, 2023 09:57

flouthoc requested a review from mtrmac July 19, 2023 10:07

mtrmac reviewed Jul 19, 2023

View reviewed changes

copy/single.go Outdated Show resolved Hide resolved

flouthoc force-pushed the instancecopycopy-compression branch 5 times, most recently from f9f8cc5 to 0b7e0b5 Compare July 20, 2023 05:47

flouthoc requested a review from mtrmac July 20, 2023 05:51

mtrmac reviewed Jul 20, 2023

View reviewed changes

internal/set/set_test.go Outdated Show resolved Hide resolved

copy/single.go Show resolved Hide resolved

copy/single.go Outdated Show resolved Hide resolved

copy/single.go Show resolved Hide resolved

pkg/compression/compression.go Outdated Show resolved Hide resolved

flouthoc added 3 commits July 21, 2023 07:10

oci_index: init Annotations if nil and required

70a7208

If `UpdateCompressionAlgorithms` is set then `Annotations` map must be initialized if its not set. Signed-off-by: Aditya R <[email protected]>

copy/single: set requiredCompression if compressionFormat is changed

cce2d37

Modifies `copy/single` to correctly use the feature from containers#2023, that is if compression is changed blob must be resued only if it matches required compression. Signed-off-by: Aditya R <[email protected]>

copy/single: accept requireCompressionFormatMatch for imagecopier

538b8a4

imagecopier implementes feature for requireCompressionFormatMatch refactor `copySingleImage` to accept that as an argument. Signed-off-by: Aditya R <[email protected]>

flouthoc force-pushed the instancecopycopy-compression branch from 0b7e0b5 to a40cb55 Compare July 21, 2023 01:40

flouthoc requested a review from mtrmac July 21, 2023 01:41

mtrmac reviewed Jul 21, 2023

View reviewed changes

copy/single.go Show resolved Hide resolved

flouthoc added 2 commits July 21, 2023 08:10

copy/multiple: instanceCopyCopy honor UpdateCompressionAlgorithms

83b0cc5

Users can set compression formats while performing `copy` in such cases `instanceCopyCopy` must honor UpdateCompressionAlgorithms. Signed-off-by: Aditya R <[email protected]>

internal/set: verify if no duplicates in set

bb006b3

Verifies if no duplicates are kept when `.Values()` is used to return slice. Signed-off-by: Aditya R <[email protected]>

flouthoc force-pushed the instancecopycopy-compression branch from a40cb55 to bb006b3 Compare July 21, 2023 02:44

flouthoc requested a review from mtrmac July 21, 2023 02:47

mtrmac approved these changes Jul 21, 2023

View reviewed changes

mtrmac merged commit 80390f7 into containers:main Jul 21, 2023

flouthoc mentioned this pull request Jul 25, 2023

Buildah reuse gzip layers when "--compression-format zstd" assigned containers/buildah#4613

Closed

flouthoc mentioned this pull request Aug 4, 2023

[release-5.26] backport API changes copy and list components from upstream. #2071

Closed

crazy-max mentioned this pull request Sep 15, 2023

copy: fix nil pointer dereference when checking compression algorithm #2120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copy/multiple: instanceCopyCopy honor `UpdateCompressionAlgorithms` #2047

copy/multiple: instanceCopyCopy honor `UpdateCompressionAlgorithms` #2047

flouthoc commented Jul 18, 2023 •

edited

Loading

flouthoc commented Jul 18, 2023 •

edited

Loading

mtrmac left a comment •

edited

Loading

mtrmac commented Jul 18, 2023

mtrmac left a comment

mtrmac commented Jul 19, 2023

flouthoc commented Jul 19, 2023

mtrmac left a comment

mtrmac Jul 19, 2023

mtrmac Jul 19, 2023

flouthoc Jul 20, 2023 •

edited

Loading

mtrmac left a comment

mtrmac left a comment

mtrmac left a comment

flouthoc commented Jul 21, 2023

mtrmac left a comment

		}

		func removeDuplicateCompression(algos []compressiontypes.Algorithm) []compressiontypes.Algorithm {

copy/multiple: instanceCopyCopy honor UpdateCompressionAlgorithms #2047

copy/multiple: instanceCopyCopy honor UpdateCompressionAlgorithms #2047

Conversation

flouthoc commented Jul 18, 2023 • edited Loading

flouthoc commented Jul 18, 2023 • edited Loading

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

mtrmac commented Jul 18, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Jul 19, 2023

flouthoc commented Jul 19, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Jul 19, 2023

Choose a reason for hiding this comment

mtrmac Jul 19, 2023

Choose a reason for hiding this comment

flouthoc Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Jul 21, 2023

mtrmac left a comment

Choose a reason for hiding this comment

copy/multiple: instanceCopyCopy honor `UpdateCompressionAlgorithms` #2047

copy/multiple: instanceCopyCopy honor `UpdateCompressionAlgorithms` #2047

flouthoc commented Jul 18, 2023 •

edited

Loading

flouthoc commented Jul 18, 2023 •

edited

Loading

mtrmac left a comment •

edited

Loading

flouthoc Jul 20, 2023 •

edited

Loading