Support reading any/all images from docker-archive: tarballs #991

mtrmac · 2020-07-25T21:01:52Z

Add support for choosing an image to read from a docker-archive:, either by reference (using the existing syntax) or by index (docker-archive:/some/path:@0).
Add docker/archive.Reader that allows listing all images in a docker-archive-formatted file, and reading any/all of them, only copying/extracting the archive once.

See individual commit messages for details.

vrothberg

Love the direction and embedding the index into the reference!

docker/internal/tarfile/src.go

docker/archive/multi_source.go

docs/containers-transports.5.md

mtrmac · 2020-07-28T13:47:53Z

docker/archive/multi_source.go

+// MultiImageSource manages a single Docker archive, allows listing its contents
+// and accessing individual images with less overhead than creating image references
+// individually (because the archive is, if necessary, copied or decompressed only once).
+type MultiImageSource struct {


I’m not entirely happy about this name - it’s not an ImageSource. docker/archive.Reader? MultiReader? MultiImageReader?

Trying docker/archive.Reader — it’s not just for multi-image readers, a caller that wants to open a single-image archive, extract the tag from the manifest, and read the single image, would benefit as well.

mtrmac · 2020-07-28T13:50:58Z

docker/internal/tarfile/readable.go

+)
+
+// ReadableArchive is a ((docker save)-formatted) tar archive that allows random access to any component.
+type ReadableArchive struct {


This name feels a bit weird, but so does internal/tarfile.Reader. I’m very much open to suggestions.

OTOH it’s internal, so it’s not as critical.

Trying tarfile.Reader, with variable name archive. The internal/tarfile.Reader/archive.Reader pair is somewhat symmetrical, but it might be too confusing; I’m not sure.

mtrmac · 2020-07-28T13:53:57Z

@vrothberg This should now be code-complete (full transport tests, leading to fixes in reference parsing), but I still need to rebase to move some changes across commits. And it remains mostly untested, hoping Podman’s tests will handle the basics.

mtrmac · 2020-07-29T17:37:10Z

@vrothberg Ready for review (but still basically untested).

vrothberg · 2020-07-30T09:02:51Z

Thanks, I'll test #998 and will comment there.

vrothberg

LGTM

rhatdan · 2020-08-11T21:24:09Z

Needs a rebase

mtrmac · 2020-08-12T00:06:50Z

Rebased, now includes all of #998.

vrothberg · 2020-08-12T08:27:22Z

@rhatdan ... merge me :)

vrothberg

Could we add means to figure out if a given reference refers to a file? Ultimately, if it's a file then we have to create a Reader and query for the reference list.

Currently, I don't find any other way than checking if (ref).StringWithinTransport() is on disk or not. However, that's not ideal as it may very well be a typo or the file really doesn't exit.

mtrmac · 2020-08-12T11:21:27Z

It wouldn’t be unprecedented… but I kind of think that the caller should know that in the first place. IIRC the only really ambiguous case is podman pull docker-archive:$path, where I’d argue “treat the transport:name syntax always as a single image” is closest to a consistent semantics.

Could we add means to figure out if a given reference refers to a file? Ultimately, if it's a file then we have to create a Reader and query for the reference list.

Currently, I don't find any other way than checking if (ref).StringWithinTransport() is on disk or not.

Well, if you want an ugly hack, the syntax is documented to split path:rest on a :. But I’d recommend against that.

mtrmac · 2020-08-12T13:08:42Z

Added another commit, “Add docker/archive.NewReaderForReference”. DO NOT MERGE until it is verified it is useful for Podman.

mtrmac · 2020-08-13T13:21:58Z

Also added Reader.ManifestTagsForReference to accommodate Podman’s handling of unqualified short names in the archives (which are only created by projectatomic/docker AFAIK). Ugh.

vrothberg

LGTM ... ready to merge from my side

rhatdan · 2020-08-19T18:43:22Z

Need @mtrmac to remove the "DO NOT Merge:" from the commit message.

It's a possible user error, not a supposedly-unreachable internal error. Signed-off-by: Miloslav Trmač <[email protected]>

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

... so that we can add more functionality to it without making it public API. Keep the original docker/tarfile.Source as a compatibility shim. Similarly, move docker/tarfile/src_test.go as well, to keep it close to the implementation being tested. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

…archive ... instead of the forwarding shims. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

…rfile This is a private API now, so we can just drop it; rename the ...With[System]Context variants to use the shorter names, and update all callers. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

... to eventually allow creating multiple Sources from a single Reader (= a temporary file containing a seekable/uncompressed copy of the archive). Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

At least docker/archive will need to deal with the two optionally separately, so such a separate constructor must exist; and the other callers are not much more complex if they separate it as well. This will also allow us to add reference lookup without having to duplicate the API. As a concession to the simpler callers, add a closeArchive parameter (currently always true) that allows them not to worry about the lifetime of the tarfile.Reader. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Every caller will need that data; this way it can be shared across several consumers, and we don't need to synchronize access/creation of the parsed data structure. Should not change behavior, except that errors are now reported earlier. Signed-off-by: Miloslav Trmač <[email protected]>

We can drop the unused error return value now. Signed-off-by: Miloslav Trmač <[email protected]>

We already accept the syntax for docker-archive: references, now implement the lookup instead of warning and ignoring the value. Implement the lookup in tarfile.Reader, not tarfile.Source, because we will want to provide an API to obtain tags from a Reader+Reference, without constructing a Source. Signed-off-by: Miloslav Trmač <[email protected]>

@0

Add support for path:@index (e.g. path:@0, path:@1 ...) reference syntax to docker-archive. This will allow reading even untagged images from multi-image archives. Signed-off-by: Miloslav Trmač <[email protected]>

with only two operations: Close(), and List() which returns a set of ImageReference objects that allow accessing the individual images. For now, use of every reference triggers creation of a new tarfile.Reader; that will be fixed momentarily. Signed-off-by: Miloslav Trmač <[email protected]>

In archive.Reader, embed a reference to tarfile.Reader to the created ImageReference objects, and use them in NewImageSource. Signed-off-by: Miloslav Trmač <[email protected]>

To allow accessing the same image multiple times, if given a (probably user-specified) ImageReference. Signed-off-by: Miloslav Trmač <[email protected]>

to allow Podman default to localhost/$tag for unqualified $tag values in docker-archive tarnballs. (Note that only projectatomic/docker creates such tarballs; almost no-one else should use this.) Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2020-08-19T19:24:52Z

OK, rebased and “DO NOT MERGE” dropped (although I haven’t had time to fully review the Podman PR)

rhatdan · 2020-08-19T19:41:27Z

LGTM
Merge once it passes the tests.

vrothberg

LGTM, thanks a lot for the great work and your support, @mtrmac !

mtrmac mentioned this pull request Jul 25, 2020

WIP - support multi-image docker archives #975

Closed

vrothberg reviewed Jul 27, 2020

View reviewed changes

docker/internal/tarfile/src.go Outdated Show resolved Hide resolved

docker/archive/multi_source.go Outdated Show resolved Hide resolved

docs/containers-transports.5.md Show resolved Hide resolved

mtrmac force-pushed the tarfile-read branch from 6486de6 to f75bbd0 Compare July 28, 2020 13:45

mtrmac commented Jul 28, 2020

View reviewed changes

mtrmac force-pushed the tarfile-read branch 4 times, most recently from 4d46560 to dcfdda0 Compare July 29, 2020 17:34

mtrmac marked this pull request as ready for review July 29, 2020 17:37

mtrmac changed the title ~~WIP: Support reading any/all images from docker-archive: tarballs~~ Support reading any/all images from docker-archive: tarballs Jul 29, 2020

mtrmac mentioned this pull request Jul 29, 2020

docker-archive: read+write #998

Closed

mtrmac force-pushed the tarfile-read branch 4 times, most recently from 171a2b6 to be68ec7 Compare August 7, 2020 00:15

mtrmac mentioned this pull request Aug 7, 2020

Add --daemon-host parameter to inspect containers/skopeo#559

Closed

vrothberg reviewed Aug 11, 2020

View reviewed changes

mtrmac force-pushed the tarfile-read branch from be68ec7 to 3f5ca89 Compare August 11, 2020 23:18

vrothberg reviewed Aug 12, 2020

View reviewed changes

mtrmac changed the title ~~Support reading any/all images from docker-archive: tarballs~~ DO NOT MERGE: Support reading any/all images from docker-archive: tarballs Aug 12, 2020

mtrmac force-pushed the tarfile-read branch from 9eaa05c to 92ff589 Compare August 13, 2020 13:21

vrothberg approved these changes Aug 19, 2020

View reviewed changes

mtrmac changed the title ~~DO NOT MERGE: Support reading any/all images from docker-archive: tarballs~~ Support reading any/all images from docker-archive: tarballs Aug 19, 2020

mtrmac added 15 commits August 19, 2020 21:23

Fix an error message on docker-archive:path:name@sha256:$digest

ad1aec9

It's a possible user error, not a supposedly-unreachable internal error. Signed-off-by: Miloslav Trmač <[email protected]>

Remove an obsolete FIXME.

e48f30f

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Use the docker/internal/tarfile.Source from docker/daemon and docker/…

46b22aa

…archive ... instead of the forwarding shims. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Split docker/internal/tarfile.Reader from Source

9335376

... to eventually allow creating multiple Sources from a single Reader (= a temporary file containing a seekable/uncompressed copy of the archive). Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Turn tarfile.Source.LoadTarManifest into a TarManifest

227c7dc

We can drop the unused error return value now. Signed-off-by: Miloslav Trmač <[email protected]>

Introduce docker-archive:path:@index syntax for reading untagged images

5aa1680

Add support for path:@index (e.g. path:@0, path:@1 ...) reference syntax to docker-archive. This will allow reading even untagged images from multi-image archives. Signed-off-by: Miloslav Trmač <[email protected]>

Finally, share a tarfile.Reader across archiveSource objects

a831aa9

In archive.Reader, embed a reference to tarfile.Reader to the created ImageReference objects, and use them in NewImageSource. Signed-off-by: Miloslav Trmač <[email protected]>

Add docker/archive.NewReaderForReference

c62a5a3

To allow accessing the same image multiple times, if given a (probably user-specified) ImageReference. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the tarfile-read branch from 92ff589 to 84a8611 Compare August 19, 2020 19:24

vrothberg reviewed Aug 20, 2020

View reviewed changes

vrothberg merged commit 2de4843 into containers:master Aug 20, 2020

mtrmac deleted the tarfile-read branch August 20, 2020 11:46

mtrmac mentioned this pull request Jan 31, 2022

Add support for docker-archive: to skopeo list-tags containers/skopeo#1029

Closed

jwhb mentioned this pull request Apr 13, 2022

docker load fails on Windows rancher/rke2#2732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reading any/all images from docker-archive: tarballs #991

Support reading any/all images from docker-archive: tarballs #991

mtrmac commented Jul 25, 2020 •

edited

Loading

vrothberg left a comment

mtrmac Jul 28, 2020

mtrmac Jul 29, 2020

mtrmac Jul 28, 2020

mtrmac Jul 29, 2020

mtrmac commented Jul 28, 2020

mtrmac commented Jul 29, 2020

vrothberg commented Jul 30, 2020

vrothberg left a comment

rhatdan commented Aug 11, 2020

mtrmac commented Aug 12, 2020

vrothberg commented Aug 12, 2020

vrothberg left a comment •

edited

Loading

mtrmac commented Aug 12, 2020

mtrmac commented Aug 12, 2020

mtrmac commented Aug 13, 2020

vrothberg left a comment

rhatdan commented Aug 19, 2020

mtrmac commented Aug 19, 2020

rhatdan commented Aug 19, 2020

vrothberg left a comment

Support reading any/all images from docker-archive: tarballs #991

Support reading any/all images from docker-archive: tarballs #991

Conversation

mtrmac commented Jul 25, 2020 • edited Loading

vrothberg left a comment

Choose a reason for hiding this comment

mtrmac Jul 28, 2020

Choose a reason for hiding this comment

mtrmac Jul 29, 2020

Choose a reason for hiding this comment

mtrmac Jul 28, 2020

Choose a reason for hiding this comment

mtrmac Jul 29, 2020

Choose a reason for hiding this comment

mtrmac commented Jul 28, 2020

mtrmac commented Jul 29, 2020

vrothberg commented Jul 30, 2020

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Aug 11, 2020

mtrmac commented Aug 12, 2020

vrothberg commented Aug 12, 2020

vrothberg left a comment • edited Loading

Choose a reason for hiding this comment

mtrmac commented Aug 12, 2020

mtrmac commented Aug 12, 2020

mtrmac commented Aug 13, 2020

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Aug 19, 2020

mtrmac commented Aug 19, 2020

rhatdan commented Aug 19, 2020

vrothberg left a comment

Choose a reason for hiding this comment

mtrmac commented Jul 25, 2020 •

edited

Loading

vrothberg left a comment •

edited

Loading