Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

copy: implement instanceCopyClone for zstd compression #1987

Merged
merged 11 commits into from
Jul 26, 2023

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Jun 7, 2023

  • 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 Outdated Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 7, 2023

@mtrmac WDYT about this, Could you PTAL.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated
@@ -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 {
Copy link
Collaborator

@mtrmac mtrmac Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

mtrmac commented Jun 28, 2023

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
Copy link
Contributor Author

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.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 30, 2023
@flouthoc
Copy link
Contributor Author

flouthoc commented Jul 2, 2023

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

flouthoc added a commit to flouthoc/image that referenced this pull request Jul 6, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 6, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 7, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 7, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 7, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 7, 2023
`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 <[email protected]>
mtrmac pushed a commit to flouthoc/image that referenced this pull request Jul 7, 2023
`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 <[email protected]>
@flouthoc flouthoc force-pushed the copy-add-zstd-annotation branch 4 times, most recently from 67260a5 to 0ce8697 Compare July 13, 2023 08:27
@flouthoc
Copy link
Contributor Author

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

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One last nit?

copy/copy.go Outdated Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

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

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

copy/copy.go Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
* 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 <[email protected]>
Implement `instanceCopyClone` for multiple compression.

Signed-off-by: Aditya R <[email protected]>
While performing copy, set a custom compression passed generated from
prepareInstanceCopies.

Signed-off-by: Aditya R <[email protected]>
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 <[email protected]>
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 <[email protected]>
* test multiple copy requests of same compression

Signed-off-by: Aditya R <[email protected]>
EnsureCompressionVariantsExist is only valid when working with a
manifest list.

Signed-off-by: Aditya R <[email protected]>
@mtrmac
Copy link
Collaborator

mtrmac commented Jul 26, 2023

Thank you so much, again!

@mtrmac mtrmac merged commit 8c387a1 into containers:main Jul 26, 2023
9 checks passed
flouthoc added a commit to flouthoc/image that referenced this pull request Aug 4, 2023
`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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Aug 4, 2023
…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 <[email protected]>
flouthoc added a commit to flouthoc/image that referenced this pull request Aug 4, 2023
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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 4, 2023
`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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 4, 2023
…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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 4, 2023
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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 5, 2023
`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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 5, 2023
…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 <[email protected]>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 5, 2023
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants