copy: implement `instanceCopyClone` for `zstd` compression #1987

flouthoc · 2023-06-07T06:54:36Z

copy.Options now contains a new field EnsureCompressionVariantExists map[string]int which allows users to specify if they want to clone images with specified compression.
copyMultipleImages now implements instanceCopyClone and extends function specifically for zstd compression.

copy/multiple.go

flouthoc · 2023-06-07T06:59:02Z

@mtrmac WDYT about this, Could you PTAL.

mtrmac

We are getting close, but it I think we need to build two more bits of infrastructure first. Also maybe revisit the original #1875 for a list of concerns to handle.

We need to add the internalManifest.OCI1InstanceAnnotationCompressionZSTD annotation even for non-…CopyClone copies (e.g. when users use the existing DestinationCtx.CompressionFormat option). (And that should not be hard-coded in the generic copy code, but something the image and/or manifest format handlers help with.) That will probably also make the new …CopyClone path simpler.
We need a way for copySingleImage to create a Zstd variant even if the destination contains a gzip one; right now the TryReusingBlob path would just use the gzip blobs.

copy/copy.go

copy/multiple.go

mtrmac · 2023-06-07T19:01:49Z

copy/multiple.go

@@ -39,8 +42,15 @@ func prepareInstanceCopies(instanceDigests []digest.Digest, options *Options) []
 			logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests))
 			continue
 		}
+		operation := instanceCopyCopy
+		forceZstd := false
+		if _, ok := options.EnsureCompressionVariantExists["zstd"]; ok {


Some code somewhere, maybe here, needs to reject requests for any formats that we don’t currently support.

We also should fail if this is set while copying a non-index image; or, eventually, implement that. (Notably that matters for podman build && podman push.)

copy/multiple.go

flouthoc · 2023-06-12T06:31:08Z

We need to add the internalManifest.OCI1InstanceAnnotationCompressionZSTD annotation even for non-…CopyClone copies (e.g. when users use the existing DestinationCtx.CompressionFormat option). (And that should not be hard-coded in the generic copy code, but something the image and/or manifest format handlers help with.) That will probably also make the new …CopyClone path simpler.

@mtrmac Created a new PR for this #1992

flouthoc · 2023-06-28T11:59:17Z

I am re-working on this but there is overlapping part with #1875 , should i just close this one and work on a single #1875 ? @mtrmac WDYT ?

mtrmac · 2023-06-28T18:54:25Z

I think the immediate next step is to ensure that skopeo copy --format zstd, or a simple copy that reuses existing Zstd blobs, sets the annotation; I think it would be a bit weird if we had a separate “clone” feature that understood the annotation, but the simpler basic copy didn’t work.

I imagine (without having done the work) that would require some changes to the interface of TryReusingBlobWithOptions/ReusedBlob — and that touches enough files to probably be worth a separate PR.

The next bit of infrastructure before actually triggering the copy is to ensure that layer reuse only reuses blobs with the desired compression algorithm, per #1987 (comment) . (and perhaps to expose that as an user option, per some of the Buildah bug reports — that could well be separate later.)

That’s also somewhere around TryReusingBlobWithOptions — conceptually it’s separate (and with two very independent design discussions); the two could even be done in parallel. OTOH the logistics and merge conflicts might well make it much easier to do these two parts in a single PR.

Once those pieces of infrastructure are built, I think the remaining part would really be the top-level caller in copy/multiple.go. And maybe a very last step to convert a single-arch image into one with two instances, so that buildah push can create the two-instance image.

As to whether to do that in this PR, or in the original #1875, I think if we identify a separate concern that can be implemented and discussed mostly independently, it’s a bit more convenient to file a fresh PR so that there isn’t a history of a previous discussion (and GitHub “Load more” placeholders) to wade through. So I’d very weakly prefer all work to happen in PRs other than #1875, and for #1875 to be used more or less just as a tracker of identified issues / concerns — but I don‘t feel strongly about that at all.

flouthoc · 2023-06-29T08:52:57Z

SGTM I think then I'll keep this PR seperate than #1875 and will open new ones for other tasks its easier to review that way.

flouthoc · 2023-07-02T15:38:11Z

This will need #2023 to get merged first.
While adding instance we also need Platform hence: #2029

`c/image` uses `Instance(` API to get the required details of an instance while performing replication `Platform` of instance is needed hence `ListUpdate` must include platform. Needed by: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2023-07-13T08:57:09Z

@mtrmac PTAL, I am using this PR directly in draft/POC PR of buildah and its working correctly for me: containers/buildah#4912

mtrmac

Thanks! One last nit?

copy/copy.go

flouthoc · 2023-07-26T05:15:12Z

@containers/image-maintainers Could you merge me pls.

vrothberg

LGTM

copy/copy.go

copy/multiple.go

* copy.Options now contains a new field `EnsureCompressionVariantExists map[string]int` which allows users to specify if they want to clone images with specified `compression`. Signed-off-by: Aditya R <arajan@redhat.com>

Signed-off-by: Aditya R <arajan@redhat.com>

Implement `instanceCopyClone` for multiple compression. Signed-off-by: Aditya R <arajan@redhat.com>

While performing copy, set a custom compression passed generated from prepareInstanceCopies. Signed-off-by: Aditya R <arajan@redhat.com>

Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: containers#1987 (comment) Signed-off-by: Aditya R <arajan@redhat.com>

After implementing `instanceCopyClone` now instance to be copied can exceed the original number of instances in the source, so amend report message to make it more meaningful. Signed-off-by: Aditya R <arajan@redhat.com>

Signed-off-by: Aditya R <arajan@redhat.com>

* test multiple copy requests of same compression Signed-off-by: Aditya R <arajan@redhat.com>

Signed-off-by: Aditya R <arajan@redhat.com>

EnsureCompressionVariantsExist is only valid when working with a manifest list. Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac · 2023-07-26T14:23:04Z

Thank you so much, again!

`c/image` uses `Instance(` API to get the required details of an instance while performing replication `Platform` of instance is needed hence `ListUpdate` must include platform. Needed by: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

…gorithmNames There is a need to read annotations of a particular instance to get its compression. Expose `Annotations` as a read-only field. Needed By: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: containers#1987 (comment) Signed-off-by: Aditya R <arajan@redhat.com>

`c/image` uses `Instance(` API to get the required details of an instance while performing replication `Platform` of instance is needed hence `ListUpdate` must include platform. Needed by: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

…gorithmNames There is a need to read annotations of a particular instance to get its compression. Expose `Annotations` as a read-only field. Needed By: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: containers#1987 (comment) Signed-off-by: Aditya R <arajan@redhat.com>

`c/image` uses `Instance(` API to get the required details of an instance while performing replication `Platform` of instance is needed hence `ListUpdate` must include platform. Needed by: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

…gorithmNames There is a need to read annotations of a particular instance to get its compression. Expose `Annotations` as a read-only field. Needed By: containers#1987 Signed-off-by: Aditya R <arajan@redhat.com>

Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: containers#1987 (comment) Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the copy-add-zstd-annotation branch from 35a0a29 to 805cb83 Compare June 7, 2023 06:55

flouthoc commented Jun 7, 2023

View reviewed changes

copy/multiple.go Outdated Show resolved Hide resolved

mtrmac reviewed Jun 7, 2023

View reviewed changes

copy/multiple.go Outdated Show resolved Hide resolved

flouthoc force-pushed the copy-add-zstd-annotation branch from 5e1499b to 31c95ce Compare June 12, 2023 06:27

mtrmac mentioned this pull request Jun 23, 2023

compression can't be done when specify option --compression-format in push command. containers/podman#18660

Closed

flouthoc force-pushed the copy-add-zstd-annotation branch from 31c95ce to 97d07ec Compare June 30, 2023 06:57

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 30, 2023

flouthoc mentioned this pull request Jul 2, 2023

blob: TryReusingBlobWithOptions consider RequiredCompression if set #2023

Merged

flouthoc mentioned this pull request Jul 6, 2023

manifest: ListUpdate add imgspecv1.Platform field #2029

Merged

flouthoc mentioned this pull request Jul 6, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/buildah#4912

Merged

flouthoc force-pushed the copy-add-zstd-annotation branch 4 times, most recently from 67260a5 to 0ce8697 Compare July 13, 2023 08:27

flouthoc force-pushed the copy-add-zstd-annotation branch from 0ce8697 to e96cf0f Compare July 13, 2023 08:59

mtrmac reviewed Jul 25, 2023

View reviewed changes

copy/copy.go Outdated Show resolved Hide resolved

flouthoc force-pushed the copy-add-zstd-annotation branch from 03fe2ca to ea4b49e Compare July 26, 2023 03:59

flouthoc requested a review from mtrmac July 26, 2023 04:03

flouthoc mentioned this pull request Jul 26, 2023

manifests,push: add support for AddCompression containers/common#1585

Merged

vrothberg approved these changes Jul 26, 2023

View reviewed changes

copy/copy.go Show resolved Hide resolved

copy/multiple.go Outdated Show resolved Hide resolved

flouthoc added 11 commits July 26, 2023 17:29

copy: add EnsureCompressionVariantExist for instanceCopyClone

9f13c15

* copy.Options now contains a new field `EnsureCompressionVariantExists map[string]int` which allows users to specify if they want to clone images with specified `compression`. Signed-off-by: Aditya R <arajan@redhat.com>

internal/set: implement AddSlice for easier syntax

db7bbfe

Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple: implement instanceCopyClone

fc4b590

Implement `instanceCopyClone` for multiple compression. Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple: instanceCopyClone set custom compression

74bd78e

While performing copy, set a custom compression passed generated from prepareInstanceCopies. Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple: error on EnsureCompressionVariant and CopySpecificImages

7c835f8

Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: containers#1987 (comment) Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple: report more meaningful copy msg

18ec467

After implementing `instanceCopyClone` now instance to be copied can exceed the original number of instances in the source, so amend report message to make it more meaningful. Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple_test: retrofit tests for instanceCopyCopy

d8b3936

Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple_test: implement test for instanceCopyClone with helpers

a3e6b6c

Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple_test: multiple copy requests of same compression

c84a3fa

* test multiple copy requests of same compression Signed-off-by: Aditya R <arajan@redhat.com>

copy/multiple_test: clone must happen once for duplicate entries

f3a5f6a

Signed-off-by: Aditya R <arajan@redhat.com>

copy/copy: fail copySingleImage cases on EnsureCompressionVariantsExist

74e7265

EnsureCompressionVariantsExist is only valid when working with a manifest list. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the copy-add-zstd-annotation branch from ea4b49e to 74e7265 Compare July 26, 2023 12:00

mtrmac merged commit 8c387a1 into containers:main Jul 26, 2023

flouthoc mentioned this pull request Aug 1, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/podman#19468

Merged

copy: implement instanceCopyClone for zstd compression #1987

copy: implement instanceCopyClone for zstd compression #1987

Uh oh!

Conversation

flouthoc commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

flouthoc commented Jun 7, 2023

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flouthoc commented Jun 12, 2023

Uh oh!

flouthoc commented Jun 28, 2023

Uh oh!

mtrmac commented Jun 28, 2023

Uh oh!

flouthoc commented Jun 29, 2023

Uh oh!

flouthoc commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flouthoc commented Jul 13, 2023

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flouthoc commented Jul 26, 2023

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mtrmac commented Jul 26, 2023

Uh oh!

Uh oh!

copy: implement `instanceCopyClone` for `zstd` compression #1987

copy: implement `instanceCopyClone` for `zstd` compression #1987

flouthoc commented Jun 7, 2023 •

edited

Loading

mtrmac Jun 7, 2023 •

edited

Loading

flouthoc commented Jul 2, 2023 •

edited

Loading