Improve support for manifest lists and image indexes by nalind · Pull Request #400 · containers/image

nalind · 2018-01-13T05:44:11Z

This patch series adds a ManifestList interface, implemented by Schema2List and OCI1Index types, to the manifest package.
It then adds an instanceDigest parameter to the ImageSource.LayerInfosForCopy(), ImageDestination.PutManifest(), and ImageDestination.PutSignatures() methods, so that we can store multiple manifests to transports that can support them, and read them from sources which need to update the layer digest list.
It uses those parameters and updates copy.Image() to accept a MultipleImages boolean option which directs it to copy all instances if the source reference produces an ImageSource that represents multiple images.
It then finishes updating the containers-storage, directory, docker, and oci transports to be able to read and write multiple images at a time.

nalind · 2018-01-13T05:47:36Z

(There's a PutManifest() call in skopeo's cmd/skopeo/layers.go that we can't update here, so the make test-skopeo step will fail in Travis.)

mtrmac

WIP, reviewed only the easy docker:/atomic:/dir: transport updates so far.

mtrmac

Makes sense overall.

Highlights are the determination of instance digests in destinations, and how OCI is supposed to work.

TomSweeneyRedHat · 2018-02-02T14:30:47Z

+		}
+	}
+	// ... but if we're forcing it, we prefer the forced value a bit more.
+	if forcedManifestListMIMEType != "" {


can you do the for loop from 251-255 as part of an else for this if statement?

nalind · 2018-02-02T15:49:55Z

The linter's complaining about stuttering with the ManifestList and ManifestListUpdate types. Any preferences over renaming them to List and ListUpdate?

TomSweeneyRedHat · 2018-02-02T18:05:06Z

+	// we just need to screen out the ones that are actually layers to get the list of non-layers.
+	if err = s.commitDataBlobs(img.ID); err != nil {
+		if _, err2 := s.imageRef.transport.store.DeleteImage(img.ID, true); err2 != nil {
+			logrus.Debugf("error deleting incomplete image %q: %v", img.ID, err2)


On the fence here, but if you've an error deleting an incomplete image, should it just go to debug or should you wrap up err2 also and return with the return below?

Ditto with the next few delete with a debug on error.

copy.Image does that in a few (very few) cases, see if err := dest.Close(); err != nil {. All too often we do defer something.Close() and ignore the possible failure.

Either way it’s problematic. On one hand, yes, throwing away failure information may make diagnosing failures difficult. On the other, the copy.Image attempt to parenthesize the secondary failure has already confused at least one user, who focused on the secondary failure instead of the primary one.

TomSweeneyRedHat · 2018-02-02T18:09:18Z

+			logrus.Debugf("error deleting incomplete image %q: %v", img.ID, err2)
+		}
+		logrus.Debugf("error saving manifest for image %q: %v", img.ID, err)
+		return err


The errors prior to this you wrapped with a little verbiage. This one and below you did a debug then straight error. I'd vote continuing on with the error wrap using the veribage from the debugs, but your call.

Changed them as you described.

mtrmac · 2018-02-02T22:05:34Z

The linter's complaining about stuttering with the ManifestList and ManifestListUpdate types. Any preferences over renaming them to List and ListUpdate?

manifest.List works for me. Definitely better than manifest.MultiArchImage :)

rhatdan · 2018-06-01T13:03:56Z

@nalind is this something you are still interested in or should we close this?

mtrmac · 2018-06-01T22:35:18Z

@nalind is this something you are still interested in or should we close this?

The feature is definitely useful and eventually we would like to have it; this PR is about 90% there, so it seems more useful to keep it open and noticed once in a few months than to close it and have it forgotten forever.

In order to support copying multi-arch containers and Flatpaks, we need to be able to copy manifest lists and OCI image indexes from registry to registry. Work is underway to add such support to skopeo (containers/image#400), but as a temporary workaround add 'bodhi-skopeo-lite', which implements the subset of 'skopeo copy' we need, but with manifest list/image index support. Use of this needs to be specifically configured.

In order to support copying multi-arch containers and Flatpaks, we need to be able to copy manifest lists and OCI image indexes from registry to registry. Work is underway to add such support to skopeo (containers/image#400), but as a temporary workaround add 'bodhi-skopeo-lite', which implements the subset of 'skopeo copy' we need, but with manifest list/image index support. Use of this needs to be specifically configured. Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>

bowlofeggs · 2018-06-14T20:49:18Z

I would also really like OCI and manifest list support in Skopeo for https://github.com/fedora-infra/bodhi

nalind · 2019-10-03T14:23:37Z

Rebased.

nalind · 2019-10-07T13:30:36Z

Rebased.

nalind · 2019-10-14T20:03:57Z

Tweaked the description for CopyOnlyInstances.

mrunalp · 2019-10-15T18:06:11Z

@vrothberg ptal

vrothberg

First review. It's looking really good so far. I plan to get my hands dirty a bit and play with the code.

nalind · 2019-10-18T06:03:10Z

Updated.

vrothberg · 2019-10-18T11:57:12Z

+	// Copy each image, or just the ones we want to copy, in turn.
+	instanceDigests := list.Instances()
+	updates := make([]manifest.ListUpdate, len(instanceDigests))
+	for i, instanceDigest := range instanceDigests {


This is still valid but I am cool to defer beautifying the copy reports to a future PR.

vrothberg

Just very minor nits. After that LGTM.

Add the manifest.List interface, and implementations for OCIv1 Index and Docker Schema2List documents. Add an instanceDigest parameter to PutManifest(), PutSignatures(), and LayerInfosForCopy, for symmetry with GetManifest() and GetSignatures(). Return an error if the instanceDigest is supplied to destinations which don't support them, and add stubs that do so even to the transports which would support it, so that we don't break compilation here. Add a MultipleImages flag to copy.Options, and if the source for a copy operation contains multiple images, copy all of the images if we can. If we can't copy them all, but we were told to, return an error. Use the generic manifest list API to select a single image to copy from a list, so that we aren't just limited to the Docker manifest list format for those cases. When guessing at the type of a manifest, if the manifest contains a list of manifests, use its declared MIME type if it included one, else assume it's an OCI index, because an OCI index doesn't include its MIME type. When copying, switch from using an encode-then-compare of the original and updated versions of the list to checking if the instance list was changed (one of the things we might have changed) or if its type has changed due to conversion (the other change we might have made). If neither has changed, then we don't need to change the encoded value of the manifest. When copying, when checking for a digest mismatch in a target image reference, ignore a mismatch between the digest in the reference and the digest of the main manifest if we're copying one element from a list, and the digest in the reference matches the digest of the manifest list. When copying, if conversion of manifests for single images is being forced, convert manifest lists to the corresponding list types. When copying, supply the unparsed top level to Commit() by attaching the value to the context.Context. Support manifest lists in the directory transport by using the instance digest as a prefix of the filename used to store a manifest or a piece of signature data. Support manifest lists in the oci-layout transport by accepting indexes as we do images, and stop guessing about Platform values to add to the top-level index. Support storing manifest lists to registries in the docker: transport by using the manifest digest when we're writing one image as part of pushing a list of them, and by using the instance digest when reading or writing signature data, when one is specified, or the cached digest of the non-instanced digest when one is not specified. Add partial support for manifest lists to the storage transport: when committing one image from a list into storage, also add a copy of the manifest list by extracting it from the context.Context. The logic is already in place to enable locating an image using any of multiple manifest digests. When writing an image that has an instanceDigest value (meaning it's a secondary image), don't try to generate a canonical reference to add to the image's list of names if the reference for the primary image doesn't contain a name. That should only happen if we're writing using just an image ID, which is unlikely, but we still need to handle it. Avoid computing the digest of the manifest, or retrieving the either-a-tag-or-a-digest value from the target reference, if we're given an instanceDigest, which would override them anyway. Move the check for non-nil instanceDigest values up into the main PutSignatures() method instead of duplicating it in the per-strategy helpers. Add mention of the instanceDigest parameter and its use to various PutManifest, PutSignatures, and LayerInfosForCopy implementations and their declarations in interfaces. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind · 2019-10-21T18:28:25Z

Dropped the patch that redirected to containers/skopeo#741 for testing, so I expect the test-skopeo target to fail.

vrothberg

LGTM

rhatdan · 2019-10-21T19:58:50Z

LGTM
As expected without the DONOTMERGE patch, tests fail.
Merging.

grooverdan · 2019-10-22T23:05:02Z

Thank you all for writing and reviewing this feature. 🍰

nalind commented Jan 13, 2018

View reviewed changes

Comment thread manifest/list.go

nalind commented Jan 13, 2018

View reviewed changes

Comment thread copy/copy.go Outdated

nalind force-pushed the manifest-lists branch 3 times, most recently from dde2ae2 to e6a48b4 Compare January 16, 2018 15:45

mtrmac reviewed Jan 20, 2018

View reviewed changes

mtrmac reviewed Jan 22, 2018

View reviewed changes

nalind commented Jan 24, 2018

View reviewed changes

Comment thread storage/storage_image.go Outdated

mtrmac mentioned this pull request Jan 30, 2018

Add support for containers to the masher fedora-infra/bodhi#2028

Closed

nalind force-pushed the manifest-lists branch from e6a48b4 to ef30284 Compare February 1, 2018 22:25

TomSweeneyRedHat reviewed Feb 2, 2018

View reviewed changes

mtrmac reviewed Feb 2, 2018

View reviewed changes

Comment thread copy/copy.go Outdated

Comment thread copy/copy.go Outdated

Comment thread copy/copy.go Outdated

mtrmac reviewed Feb 2, 2018

View reviewed changes

Comment thread manifest/list_test.go Outdated

Comment thread manifest/manifest.go Outdated

mtrmac reviewed Feb 5, 2018

View reviewed changes

mtrmac mentioned this pull request May 7, 2018

[RFE] add manifest subcommand containers/podman#713

Closed

owtaylor mentioned this pull request Jun 8, 2018

Add bodhi-skopeo-lite - a skopeo-workalike with manifest list support fedora-infra/bodhi#2430

Merged

nalind force-pushed the manifest-lists branch from 4d513f5 to b1afe26 Compare October 3, 2019 14:23

nalind force-pushed the manifest-lists branch 2 times, most recently from f5eaa59 to 8f8d9c4 Compare October 7, 2019 13:30

vrothberg mentioned this pull request Oct 8, 2019

Introduce the sync command containers/skopeo#524

Merged

nalind mentioned this pull request Oct 10, 2019

containers/image v5; support manifest lists; add a "manifest" command containers/buildah#1902

Closed

nalind force-pushed the manifest-lists branch from 8f8d9c4 to f956c29 Compare October 14, 2019 19:55

nalind mentioned this pull request Oct 14, 2019

copy: add a --all/-a flag containers/skopeo#741

Merged

nalind force-pushed the manifest-lists branch from f956c29 to 9f81195 Compare October 14, 2019 22:02

vrothberg reviewed Oct 16, 2019

View reviewed changes

nalind force-pushed the manifest-lists branch from 9f81195 to 0c63aa8 Compare October 18, 2019 05:47

vrothberg reviewed Oct 18, 2019

View reviewed changes

Comment thread copy/manifest.go Outdated

Comment thread docker/docker_image_dest.go Outdated

nalind force-pushed the manifest-lists branch from 0c63aa8 to d4632a5 Compare October 18, 2019 19:01

nalind force-pushed the manifest-lists branch from d4632a5 to 1836b4d Compare October 18, 2019 19:15

nalind mentioned this pull request Oct 21, 2019

Move to containers/image v5, support manifest lists containers/podman#4310

Merged

nalind force-pushed the manifest-lists branch from 1836b4d to ca5fe04 Compare October 21, 2019 18:26

vrothberg reviewed Oct 21, 2019

View reviewed changes

rhatdan merged commit 6934023 into containers:master Oct 21, 2019

nalind mentioned this pull request Oct 22, 2019

copy.Image(): select the CopySystemImage image using the source context #734

Merged

mtrmac mentioned this pull request Mar 22, 2021

manifest: GuessMIMEType: support OCI artifacts #1186

Merged

mtrmac mentioned this pull request Jun 19, 2021

Manifest list support nits #1268

Merged

Conversation

nalind commented Jan 13, 2018

Uh oh!

nalind commented Jan 13, 2018

Uh oh!

Uh oh!

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomSweeneyRedHat Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

nalind commented Feb 2, 2018

Uh oh!

TomSweeneyRedHat Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

mtrmac Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

nalind Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac commented Feb 2, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhatdan commented Jun 1, 2018

Uh oh!

mtrmac commented Jun 1, 2018

Uh oh!

bowlofeggs commented Jun 14, 2018

Uh oh!

nalind commented Oct 3, 2019

Uh oh!

nalind commented Oct 7, 2019

Uh oh!

nalind commented Oct 14, 2019

Uh oh!