client: add Import() and Export() for importing/exporting image in OCI format by AkihiroSuda · Pull Request #1013 · containerd/containerd

AkihiroSuda · 2017-06-15T10:41:30Z

Part of #831

Export as a tar (Note: "-" can be used for stdout):

$ ctr images export /tmp/oci-busybox.tar docker.io/library/busybox:latest
$ tar tvf /tmp/oci-busybox.tar
drwxr-xr-x 0/0               0 1970-01-01 00:00 blobs/
drwxr-xr-x 0/0               0 1970-01-01 00:00 blobs/sha256/
-r--r--r-- 0/0              30 1970-01-01 00:00 oci-layout
-r--r--r-- 0/0             527 1970-01-01 00:00 blobs/sha256/c79345819a6882c31b41bc771d9a94fc52872fa651b36771fbe0c8461d7ee558
-r--r--r-- 0/0            1507 1970-01-01 00:00 blobs/sha256/c75bebcdd211f41b3a460c7bf82970ed6c75acaab9cd4c9a4e125b03ca113798
-r--r--r-- 0/0          699311 1970-01-01 00:00 blobs/sha256/1cae461a1479c5a24dd38bd5f377ce65f531399a7db8c3ece891ac2197173f1d
-rw-r--r-- 0/0             257 1970-01-01 00:00 index.json

Import a tar (Note: "-" can be used for stdin):

$ ctr images import foo/new:latest /tmp/oci-busybox.tar
unpacking sha256:c79345819a6882c31b41bc771d9a94fc52872fa651b36771fbe0c8461d7ee558...done

Note:

Media types are not automatically translated into OCI ones, but still valid OCI layout. I'll implement media type translation in another PR.
Importing an image from TAR might result in accessing unneeded blobs, because TAR does not support random access. So I suggest using directory representation. (I'll implement in follow-up PR) client: add Import() and Export() for importing/exporting image in OCI format #1013 (comment)

Signed-off-by: Akihiro Suda [email protected]

codecov-io · 2017-06-15T10:47:50Z

Codecov Report

Merging #1013 into master will decrease coverage by 1.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1013      +/-   ##
==========================================
- Coverage   24.69%   23.64%   -1.06%     
==========================================
  Files          38       40       +2     
  Lines        4317     4509     +192     
==========================================
  Hits         1066     1066              
- Misses       3049     3241     +192     
  Partials      202      202

Impacted Files	Coverage Δ
client.go	`2.55% <0%> (-0.49%)`	⬇️
import.go	`0% <0%> (ø)`
export.go	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a054c6...b518f11. Read the comment docs.

crosbymichael · 2017-06-15T17:23:02Z

Just don't add this until its implemented

crosbymichael · 2017-06-15T17:24:58Z

same here, don't add options until you are going to implement them

crosbymichael · 2017-06-15T17:27:06Z

Instead of taking a path, would it be better to have export work over a reader?

Export() (io.ReadCloser, error)?

This way, clients have more flexibility for how they handle the exported content, they can push it over a stream or write it to disk at their path. Right now this client is not very flexible when you take a path like this

If we are going to use TAR (correct me if misunderstanding) for Export(),
Import() would use TAR as well: Import(ctx context.Context, ref string, tarr io.ReadCloser, opts ...ImportOpt)(Image, error).

However, the implementation for Import() is likely to be inefficient, because TAR cannot be read randomly.

So my suggestion is to use directory by default across both Export() and Import().
We can still add WithOCITarExportation(tarr io.Reader) ExportOpt for tar support.

WDYT?

Also, directory representation might be useful for Export() as well, because we can export blobs in parallel. (Unlike generic filesystem IO operation, this would be CPU-intensive for digest computation and good for multi-threading, but I didn't do benchmark 😅 )

What is the usecase you are solving for because I think you are underestimating how inefficient the on disk format is.

Image distribution using NFS/S3/SSHFS/IPFS/whatever.

tar cf /mnt/nfs-or-whatever/some-image | dist import foo/some-image:some-ref-name --ref-name some-ref-name - would cause I/O for unneeded blobs, unless tar is aware of OCI format and the ref name being used.

Also, dist cannot read index.json without scanning whole the "tape".

AkihiroSuda · 2017-06-26T10:05:33Z

Implemented both Import/Export for both directory representation and TAR representation.

@crosbymichael
Could you please try?
Usage are added to the description text of this PR.

crosbymichael · 2017-06-26T17:33:00Z

Will review, thanks!

crosbymichael · 2017-06-27T23:00:54Z

In your changes you didn't address the feedback. You are still outputting files to disk instead of returning/consuming an io.Reader. Its usually bad design to have packages output files to disk that consumers are using. Sometimes you don't have a disk or are working via a remote API. Returning streams from the client is the best design to allow users to consume the results however they want. You can even just extract it to disk if you want once you have reader.

AkihiroSuda · 2017-06-28T02:09:30Z

@crosbymichael
I added dist export --representation oci+tar according to your feedback.
This oci+tar exportation works as you expect (though it receives an io.Writer rather than returning io.Reader).

I can remove extra dist export --representation oci+directory, but it might be asymmetric to dist import, which I think should still support oci+directory for efficiency #1013 (comment)

WDYT?

crosbymichael · 2017-06-28T16:54:45Z

That is fine for the cli tools but I meant the method signatures for the *Client should be.

func (c *Client) Import(context.Context, string, io.Reader) error
func (c *Client) Export(context.Context, Descriptor) (io.ReadCloser, error)

This way the client is useful by anyone, local, over a socket, etc and the CLI can save the tar, extract to a directory, etc.

crosbymichael · 2017-06-28T16:56:08Z

Its extremely hard to mock, test, use generally when packages write to files on disk internally.

AkihiroSuda · 2017-06-29T07:21:00Z

OK, changed API as follows:

// Import imports an image from a Tar stream using reader.
// OCI format is assumed by default.
//
// Note that unreferenced blobs are imported to the content store as well.
func (c *Client) Import(ctx context.Context, ref string, reader io.Reader, opts ...ImportOpt) (Image, error)

// ImportDirectory imports an image from a directory using opener.
// OCI format is assumed by default.
//
// Unlike Import(), only blobs referenced by the manifest are imported to the content store.
//
// opener is typically `func(relPath string)(io.ReadCloser, error){ return os.Open(filepath.Join(someDir, relPath)) }`,
// but it does not need to use real filesystem.
func (c *Client) ImportDirectory(ctx context.Context, ref string, opener func(string) (io.ReadCloser, error), opts ...ImportOpt) (Image, error)

// Export exports an image to a Tar stream.
// OCI format is used by default.
// It is up to caller to put "org.opencontainers.image.ref.name" annotation to desc.
func (c *Client) Export(ctx context.Context, desc ocispec.Descriptor, opts ...ExportOpt) (io.ReadCloser, error)

// No `ExportDirectory`

AkihiroSuda · 2017-06-29T07:27:37Z

+
+// Directory is ImageDriver for directory representation of an OCI image.
+// The directory must not exist before calling Init function of this structure.
+func Directory(path string) ImageDriver {


Just note that this directory ImageDriver is not used at all in containerd, but can be used for 3rd party projects.
(My https://github.com/AkihiroSuda/filegrain is going to vendor this)

I thought we were removing this.

It would be good to put together a proposal for directory based layouts. This use case really doesn't make sense. The OCI layout is meant for an archive.

stevvooe · 2017-06-29T20:50:12Z

-//
-// img directory must not exist before calling this function.
+// BlobWriter writes an OCI blob and returns a digest when closed.
+type BlobWriter interface {


This looks eerily similar to content.Writer. Let's avoid duplication.

The content package is internal detail of containerd while oci package is specific to OCI format.
So I'm not sure we should deduplicate, but may I try that in follow-up PR?

e.g. future content package may use non-filesystem blob store, or just use sharded directory (blobs/sha256/de/deadbeef)
Then it could not be deduplicated

@AkihiroSuda The content package is the premiere approach for doing io on OCI oriented content. Use the interface definition, don't define a new one that is slightly different.

stevvooe · 2017-06-29T20:54:02Z

+//
+// opener is typically `func(relPath string)(io.ReadCloser, error){ return os.Open(filepath.Join(someDir, relPath)) }`,
+// but it does not need to use real filesystem.
+func (c *Client) ImportDirectory(ctx context.Context, ref string, opener func(string) (io.ReadCloser, error), opts ...ImportOpt) (Image, error) {


I'm not sure that this directory layout format is optimal, nor am I sure that handling directories and tarfiles with the same code path is the best for consistent implementations.

Is there any way we could split Import/Export and ImportDirectory into two PRs?

AkihiroSuda · 2017-06-30T03:39:52Z

@stevvooe @crosbymichael

Eliminated ImportDirectory.
Now the PR solely works with tar stream.

I'll open follow-up PR for reviving ImportDirectory.
import-directory.patch.txt

AkihiroSuda · 2017-07-03T04:58:37Z

@stevvooe

Removed oci/dir.go and deduplicated BlobWriter interface. PTAL.

However, I do still think OCI is not limited to be used solely for archival purpose, let me discuss that in follow-up PR:
https://github.com/opencontainers/image-spec/blob/master/image-layout.md

This layout MAY be used in a variety of different transport mechanisms: archive formats (e.g. tar, zip), shared filesystem environments (e.g. nfs), or networked file fetching (e.g. http, ftp, rsync).

stevvooe · 2017-07-05T22:09:49Z

@AkihiroSuda Thanks!

That document is making a leap to say that the layout can be used for "shared filesystem environments (e.g. nfs), or networked file fetching (e.g. http, ftp, rsync)." The layout makes no provisions for concurrency, updates or security in such environments.

aaronlehmann · 2017-07-07T23:57:53Z

filepath.ToSlash?

aaronlehmann · 2017-07-08T00:10:12Z

unexpected size?

AkihiroSuda · 2017-07-11T08:11:51Z

@aaronlehmann
Thank you, didn't know that there is filepath.ToSlash.

Updated PR

stevvooe · 2017-07-14T22:49:57Z

@AkihiroSuda Looks like this needs another rebase.

AkihiroSuda · 2017-07-16T14:23:48Z

Rebased. Now CLI commands are ctr import and ctr export.
PTAL.

stevvooe · 2017-07-17T20:33:23Z

@AkihiroSuda Should be image import/export?

AkihiroSuda · 2017-07-18T10:12:50Z

done @stevvooe

stevvooe · 2017-07-18T20:21:14Z

Shouldn't format be resolved by the incoming file?

It could be easily resolved if we can add support for directories.
However, for tar, we can't resolve the format until reading whole the "tape" to find oci-layout.

I don't think directories solves that problem. Would it make sense to pre-unpack the format then resolve the version?

Does it work with a foreign format other than OCI v1?

For the most part, any archival format should be safe for pre-unpack, to some degree. However, what happens when there is a new oci layout format?

Future OCI would need to be specified manually with WithOCIv2ImportFormat(), if it is incompatible to v1

stevvooe · 2017-07-18T20:21:34Z

Docker save/load support?

As you know Docker will very soonly support save/load in OCI format. moby/moby#33355

Do you want the classic save/load support in containerd?
(Can be another PR?)

Do you want the classic save/load support in containerd?

Yes. OCI save/load support may not be available for a few versions.

OK, but let me do that in a follow-up PR 😅

File an issue?

stevvooe · 2017-07-21T18:08:06Z

LGTM

PTAL @dmcgowan

AkihiroSuda · 2017-07-27T10:31:58Z

rebased, @dmcgowan @crosbymichael @mlaventure PTAL?

…I format Export as a tar (Note: "-" can be used for stdout): $ ctr images export /tmp/oci-busybox.tar docker.io/library/busybox:latest Import a tar (Note: "-" can be used for stdin): $ ctr images import foo/new:latest /tmp/oci-busybox.tar Note: media types are not converted at the moment: e.g. application/vnd.docker.image.rootfs.diff.tar.gzip -> application/vnd.oci.image.layer.v1.tar+gzip Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2017-07-28T04:59:47Z

CI green

crosbymichael

LGTM

AkihiroSuda mentioned this pull request Jun 15, 2017

Client API for importing/exporting OCI images #831

Closed

crosbymichael requested changes Jun 15, 2017

View reviewed changes

AkihiroSuda force-pushed the oci-export branch from 4f89882 to 01a6ab2 Compare June 26, 2017 09:57

AkihiroSuda changed the title ~~client: add Export() for exporting image in OCI format~~ client: add Import() and Export() for importing/exporting image in OCI format Jun 26, 2017

AkihiroSuda force-pushed the oci-export branch 4 times, most recently from 18a3168 to e5482bd Compare June 29, 2017 06:57

AkihiroSuda commented Jun 29, 2017

View reviewed changes

stevvooe reviewed Jun 29, 2017

View reviewed changes

AkihiroSuda force-pushed the oci-export branch from e5482bd to 054fee3 Compare June 30, 2017 03:37

AkihiroSuda force-pushed the oci-export branch from 054fee3 to 84a6019 Compare July 3, 2017 04:56

aaronlehmann reviewed Jul 7, 2017

View reviewed changes

Comment thread oci/tar.go Outdated

Copy link
Copy Markdown

aaronlehmann Jul 7, 2017

Choose a reason for hiding this comment

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

filepath.ToSlash?

aaronlehmann reviewed Jul 8, 2017

View reviewed changes

Comment thread oci/oci.go Outdated

Copy link
Copy Markdown

aaronlehmann Jul 8, 2017

Choose a reason for hiding this comment

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

unexpected size?

AkihiroSuda force-pushed the oci-export branch from 84a6019 to a618b49 Compare July 11, 2017 08:11

AkihiroSuda force-pushed the oci-export branch from a618b49 to 5904aa1 Compare July 16, 2017 14:22

AkihiroSuda force-pushed the oci-export branch from 5904aa1 to a557fd4 Compare July 18, 2017 10:12

stevvooe reviewed Jul 18, 2017

View reviewed changes

AkihiroSuda mentioned this pull request Jul 21, 2017

client: add more formats for Import/Export #1229

Closed

AkihiroSuda force-pushed the oci-export branch from a557fd4 to 7d937dc Compare July 27, 2017 10:31

AkihiroSuda force-pushed the oci-export branch from 7d937dc to b518f11 Compare July 28, 2017 04:48

crosbymichael approved these changes Jul 28, 2017

View reviewed changes

crosbymichael merged commit 183a6ca into containerd:master Jul 28, 2017

Conversation

AkihiroSuda commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael Jun 15, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 26, 2017

Uh oh!

crosbymichael commented Jun 26, 2017

Uh oh!

crosbymichael commented Jun 27, 2017

Uh oh!

AkihiroSuda commented Jun 28, 2017

Uh oh!

crosbymichael commented Jun 28, 2017

Uh oh!

crosbymichael commented Jun 28, 2017

Uh oh!

AkihiroSuda commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 30, 2017

Uh oh!

AkihiroSuda commented Jul 3, 2017

Uh oh!

stevvooe commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jul 11, 2017

Uh oh!

stevvooe commented Jul 14, 2017

Uh oh!

AkihiroSuda commented Jul 16, 2017

Uh oh!

stevvooe commented Jul 17, 2017

Uh oh!

AkihiroSuda commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 15, 2017 •

edited

Loading

codecov-io commented Jun 15, 2017 •

edited

Loading

crosbymichael Jun 15, 2017 •

edited

Loading