Implement daemon.Write #114

nkubala · 2018-04-26T21:16:35Z

This adds support for loading a v1.Image into a local docker daemon, by first writing it in docker save format using tarball.Write, and then feeding that to the docker client.

Previously tarball.Write only supported writing to disk, so to avoid this I changed the signature to return an io.ReadCloser by default. tarball.WriteToFile can be used in its place to write the contents of this reader to disk.

jonjohnsonjr · 2018-04-26T21:21:20Z

v1/tarball/write.go

+	defer tf.Close()
+
+	if img == nil {
+		return fmt.Errorf("wtf we have a serious problem")


I want a keyboard shortcut for this.

imjasonh · 2018-04-27T00:30:31Z

v1/tarball/write.go

+
+	go func() {
+		// Close the writer with any errors encountered during
+		// extraction. These errors will be returned by the reader end


This comment mentions extraction.

imjasonh · 2018-04-27T00:31:58Z

v1/tarball/write.go

@@ -49,6 +59,33 @@ func Write(p string, tag name.Tag, img v1.Image, wo *WriteOptions) error {
 	tf := tar.NewWriter(w)
 	defer tf.Close()

+	tr := tar.NewReader(Write(tag, img, wo))


You should be able to just io.Copy(f, r) I think right?

I thought so at first, but the copy doesn't seem to read correctly. Not sure if this is because it's a tar reader or if it's coming from a pipe, but if you have suggestions I'm all ears

You're wrapping these in extra layers of tar read/writers (they are... tarnished? :P), something like this should work:

func WriteToFile(p string, tag name.Tag, img v1.Image, wo *WriteOptions) error { f, err := os.Create(p) if err != nil { return err } defer f.Close() r := Write(tag, img, wo) defer r.Close() _, err = io.Copy(f, r) return err }

But this feels like going around your elbow to get to your ear. See my comment on tarball.Write.

-1 for the pun 😆

you're right about the extra read/write wrapping, but yeah this definitely wasn't the best construction. fixed up to make a little bit more sense

imjasonh · 2018-04-27T12:33:10Z

v1/daemon/write.go

 // WriteOptions are used to expose optional information to guide or
 // control the image write.
 type WriteOptions struct {
 	// TODO(dlorenc): What kinds of knobs does the daemon expose?
 }

 // Write saves the image into the daemon as the given reference.
-func Write(ref name.Reference, img v1.Image, wo WriteOptions) error {
-	return fmt.Errorf("NYI: daemon.Write(%v)", ref)
+func Write(ref name.Reference, img v1.Image, wo WriteOptions) (types.ImageLoadResponse, error) {


This library's API shouldn't return some other library's types. Is there another way to express this?

ah yeah that's a great point. I still want to expose the client's response to the user, so I'll change this to return (string, error)

dlorenc · 2018-04-27T20:03:53Z

v1/daemon/write.go

+	if err != nil {
+		return "", err
+	}
+	resp.Body.Close()


nit: move this up as a defer before you do the ReadAll. Also check the err from ImageLoad separately.

jonjohnsonjr · 2018-04-28T06:14:28Z

v1/daemon/write.go

+		return "", err
+	}
+
+	tag, err := name.NewTag(ref.Name(), name.WeakValidation)


If this can only be a tag, just have daemon.Write only accept a name.Tag?

FWIW I've seen RepoDigests somewhere before, so maybe this should be handling digests as well?

not sure why I did this....maybe I was just modeling after remote.Write? now that you mention it this probably should handle digests, but then shouldn't tarball.Write as well?

I think something about the tarball format expects a tag (or used to?). Not sure.

jonjohnsonjr · 2018-04-28T06:44:03Z

v1/tarball/write.go

@@ -49,6 +59,33 @@ func Write(p string, tag name.Tag, img v1.Image, wo *WriteOptions) error {
 	tf := tar.NewWriter(w)
 	defer tf.Close()

+	tr := tar.NewReader(Write(tag, img, wo))


You're wrapping these in extra layers of tar read/writers (they are... tarnished? :P), something like this should work:

func WriteToFile(p string, tag name.Tag, img v1.Image, wo *WriteOptions) error { f, err := os.Create(p) if err != nil { return err } defer f.Close() r := Write(tag, img, wo) defer r.Close() _, err = io.Copy(f, r) return err }

But this feels like going around your elbow to get to your ear. See my comment on tarball.Write.

jonjohnsonjr · 2018-04-28T06:51:49Z

v1/tarball/write.go

-	// One manifest.json file at the top level containing information about several images.
-	// One file for each layer, named after the layer's SHA.
-	// One file for the config blob, named after its SHA.
+// Write writes the image at the given tag to a tarball,


Nit: "at the given tag" feels like it might pull the image first... this wasn't super obvious to me from the method signature. It might be useful to call out that this is a local operation (i.e. just adds it to RepoTags).

yeah fair enough. I would have clarified but I removed this altogether :)

jonjohnsonjr · 2018-04-28T07:01:15Z

v1/daemon/write_test.go

+	"github.com/google/go-containerregistry/v1/tarball"
+)
+
+func TestWriteImage(t *testing.T) {


This doesn't override getImageLoader, so it has a dependency on docker, which I don't have installed, so I can't run the tests :)

write_test.go:36: Error writing image tar: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

(you probably have some test_image_2:latests in your daemon 😂 )

this is testing loading an image into the local daemon, so if you don't have docker installed this will definitely fail :)

the test is just doing a tarball.ImageFromPath on one of the tars checked into testdata and then retagging it as test_image_2:latest.

I won't fight this too hard, but it would be nice if I could bazel test ... this without installing docker :) can you mock out the docker interaction for this test and add an integration test that exercises the actual daemon?

I'm not a huge fan of implicit dependencies, but if nobody else cares, it's nbd.

no that's a good point, if we were relying on docker to always be present then what would be the point of this library :) fixed to mock out so we don't need docker to test

jonjohnsonjr · 2018-04-28T07:14:29Z

v1/tarball/write.go

-	// One file for the config blob, named after its SHA.
+// Write writes the image at the given tag to a tarball,
+// and returns it through a reader to be consumed
+func Write(tag name.Tag, img v1.Image, wo *WriteOptions) io.ReadCloser {


It's weird to me that something called Write returns a ReadCloser. After spending a few minutes trying to come up with a name that made more sense, I think I realized the issue.

This would be a lot cleaner if tarball.Write was instead:

func Write(w io.Writer, tag name.Tag, img v1.Image, wo *WriteOptions) error

(Hey look, that's tarball.write ;P)

Then tarball.WriteToFile could just be sugar to create the file for you:

func WriteToFile(p string, tag name.Tag, img v1.Image, wo *WriteOptions) error { f, err := os.Create(p) if err != nil { return err } defer f.Close() return Write(f, tag, img, wo) }

And then you can move the io.Pipe from tarball.Write into daemon.Write, which would just pipe tarball.Write into cli.ImageLoad.

@imjasonh thoughts? I think this would be a bit more idiomatic/composable.

Yes! I was bumping up on that too and just didn't think about it enough. I think we could probably port that same change to mutate.Extract too (why is Extract in mutate?)

But yeah that's definitely a better API

+1 to giving mutate.Extract the same treatment.

Disclaimer: the rest of this comment is unrelated to this PR.

tl;dr I think we need fs.Write and fs.Image (or fs.Layer)

(why is Extract in mutate?)

I'll bring up my comment again -- if we can change mutate.Extract to:

func Flatten(v1.Image) (v1.Image, error)

... I think it makes sense to leave it in mutate. Otherwise, I agree it doesn't really fit the style of mutations in there (maybe it belongs in v1util or tarball? or a new fs.Write?). Unfortunately, the change I want makes things a bit complicated and potentially a lot slower...

I'm wondering if we can abuse this to get what I want without much code... e.g.:

func Flatten(i v1.Image) (v1.Image, error) { // pseudo-go, will likely need lots of plumbing to avoid excessive buffering return mutate.AppendLayers(empty.Image(), layer.Uncompressed(mutate.Extract(i))) }

Then implementing {remote,tarball,daemon} | flatten | {remote,tarball,daemon} is trivial :)

Thanks for the feedback here, this is definitely a better construction. Passing in a writer rather than returning a reader puts the onus on the caller to do the IO, which I think is a lot cleaner.

re Extract being in mutate: I originally had this in tarball, but wasn't sure it made sense in there since it's not technically limited to tarballs (though they do get converted before doing the actual extraction).

The actual motivation for this was twofold: I wanted to have a cleaner way of doing filesystem comparisons in container-diff (extracting the flattened fs is an obvious choice), but then I also wanted to implement Flatten in its original state because it's cool :)

I tried to make it as extensible as possible, and I guess mutate was the only neutral package that made sense. I'm fine moving it around though.

jonjohnsonjr · 2018-04-28T07:47:18Z

v1/daemon/write.go

+	// write the image in docker save format first, then load it
+	reader := tarball.Write(tag, img, &tarball.WriteOptions{})
+	resp, err := cli.ImageLoad(context.Background(), reader, false)
+	b, err := ioutil.ReadAll(resp.Body)


It might be better to just make daemon.Write return an io.ReadCloser (i.e. resp.Body) instead of a string? I'm not sure how this will be used, so it's hard to say.

nkubala · 2018-05-08T18:20:26Z

this should be RFAL

jonjohnsonjr · 2018-05-08T18:28:10Z

v1/daemon/write_test.go

+	"github.com/google/go-containerregistry/v1/tarball"
+)
+
+func TestWriteImage(t *testing.T) {


I won't fight this too hard, but it would be nice if I could bazel test ... this without installing docker :) can you mock out the docker interaction for this test and add an integration test that exercises the actual daemon?

I'm not a huge fan of implicit dependencies, but if nobody else cares, it's nbd.

jonjohnsonjr · 2018-05-08T18:32:53Z

v1/daemon/write.go

+		return "", err
+	}
+
+	tag, err := name.NewTag(ref.Name(), name.WeakValidation)


I think something about the tarball format expects a tag (or used to?). Not sure.

jonjohnsonjr · 2018-05-08T18:33:37Z

v1/daemon/write.go

+	defer resp.Body.Close()
+	b, readErr := ioutil.ReadAll(resp.Body)
+	response := string(b)
+	if err != nil {


I think you need to check this before defer resp.Body.Close() , since resp might be nil.

ah yeah good catch, resp can't be nil but resp.Body can

jonjohnsonjr

LGTM other than some comment nits.

jonjohnsonjr · 2018-05-09T17:38:04Z

v1/tarball/write.go

+}
+
+// Write in the compressed format.
+// Return a reader for contents of a tarball, containing:


Update this comment.

jonjohnsonjr · 2018-05-09T17:38:55Z

v1/daemon/write.go

+var getImageLoader = func() (ImageLoader, error) {
+	return client.NewEnvClient()
+}
+
 // WriteOptions are used to expose optional information to guide or
 // control the image write.
 type WriteOptions struct {
 	// TODO(dlorenc): What kinds of knobs does the daemon expose?
 }

 // Write saves the image into the daemon as the given reference.


s/reference/tag/

jonjohnsonjr · 2018-05-09T17:40:34Z

v1/daemon/write_test.go

+	"github.com/google/go-containerregistry/v1/tarball"
+)
+
+func TestWriteImage(t *testing.T) {


jonjohnsonjr reviewed Apr 26, 2018

View reviewed changes

nkubala force-pushed the daemon_write branch 2 times, most recently from 5f113c9 to b20d438 Compare April 26, 2018 21:56

jonjohnsonjr assigned dlorenc Apr 26, 2018

imjasonh requested changes Apr 27, 2018

View reviewed changes

nkubala force-pushed the daemon_write branch from dc0250a to cbd40f5 Compare April 27, 2018 18:25

dlorenc reviewed Apr 27, 2018

View reviewed changes

nkubala mentioned this pull request Apr 27, 2018

Switch from containers/image to go-containerregistry GoogleContainerTools/container-diff#229

Merged

jonjohnsonjr requested changes Apr 28, 2018

View reviewed changes

nkubala force-pushed the daemon_write branch from 423ac6f to ad2bfad Compare May 8, 2018 18:15

jonjohnsonjr requested changes May 8, 2018

View reviewed changes

jonjohnsonjr approved these changes May 9, 2018

View reviewed changes

implement daemon.write

395c6f8

nkubala force-pushed the daemon_write branch from ffcefcb to 395c6f8 Compare May 9, 2018 20:27

imjasonh approved these changes May 9, 2018

View reviewed changes

jonjohnsonjr merged commit 1968f30 into google:master May 9, 2018

Implement daemon.Write #114

Implement daemon.Write #114

Uh oh!

Conversation

nkubala commented Apr 26, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr Apr 28, 2018 • 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!

nkubala commented May 8, 2018

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!

jonjohnsonjr left a comment

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

jonjohnsonjr Apr 28, 2018 •

edited

Loading