From 564e6bc7802b606d829a498eee0c2bb8ce4032e1 Mon Sep 17 00:00:00 2001 From: Victor Vieux Date: Thu, 16 Jan 2014 18:40:33 -0800 Subject: [PATCH 1/3] Move docker rmi to a job Docker-DCO-1.1-Signed-off-by: Victor Vieux (github: vieux) --- api.go | 32 ++++++++---- commands.go | 13 +++-- integration/commands_test.go | 6 +-- integration/runtime_test.go | 2 +- integration/server_test.go | 20 ++++---- server.go | 99 ++++++++++++++++++++++-------------- 6 files changed, 101 insertions(+), 71 deletions(-) diff --git a/api.go b/api.go index 13397e6b92..89d4561130 100644 --- a/api.go +++ b/api.go @@ -668,19 +668,31 @@ func deleteImages(srv *Server, version float64, w http.ResponseWriter, r *http.R if vars == nil { return fmt.Errorf("Missing parameter") } - name := vars["name"] - imgs, err := srv.ImageDelete(name, version > 1.1) - if err != nil { + var ( + buffer = bytes.NewBuffer(nil) + job = srv.Eng.Job("image_delete", vars["name"]) + ) + job.Stdout.Add(buffer) + job.SetenvBool("autoPrune", version > 1.1) + if err := job.Run(); err != nil { return err } - if imgs != nil { - if len(imgs) != 0 { - return writeJSON(w, http.StatusOK, imgs) - } - return fmt.Errorf("Conflict, %s wasn't deleted", name) + + outs := engine.NewTable("", 0) + if _, err := outs.ReadFrom(buffer); err != nil { + return err } - w.WriteHeader(http.StatusNoContent) - return nil + + if len(outs.Data) != 0 { + var err error + if version < 1.9 { + _, err = outs.WriteTo(w) + } else { + _, err = outs.WriteListTo(w) + } + return err + } + return fmt.Errorf("Conflict, %s wasn't deleted", vars["name"]) } func postContainersStart(srv *Server, version float64, w http.ResponseWriter, r *http.Request, vars map[string]string) error { diff --git a/commands.go b/commands.go index 84466c204a..d918efdace 100644 --- a/commands.go +++ b/commands.go @@ -828,18 +828,17 @@ func (cli *DockerCli) CmdRmi(args ...string) error { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") } else { - var outs []APIRmi - err = json.Unmarshal(body, &outs) - if err != nil { + outs := engine.NewTable("Created", 0) + if _, err := outs.ReadFrom(bytes.NewReader(body)); err != nil { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") continue } - for _, out := range outs { - if out.Deleted != "" { - fmt.Fprintf(cli.out, "Deleted: %s\n", out.Deleted) + for _, out := range outs.Data { + if out.Get("Deleted") != "" { + fmt.Fprintf(cli.out, "Deleted: %s\n", out.Get("Deleted")) } else { - fmt.Fprintf(cli.out, "Untagged: %s\n", out.Untagged) + fmt.Fprintf(cli.out, "Untagged: %s\n", out.Get("Untagged")) } } } diff --git a/integration/commands_test.go b/integration/commands_test.go index 48819670e7..a0fc4b9523 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -1030,11 +1030,11 @@ func TestContainerOrphaning(t *testing.T) { buildSomething(template2, imageName) // remove the second image by name - resp, err := srv.ImageDelete(imageName, true) + resp, err := srv.DeleteImage(imageName, true) // see if we deleted the first image (and orphaned the container) - for _, i := range resp { - if img1 == i.Deleted { + for _, i := range resp.Data { + if img1 == i.Get("Deleted") { t.Fatal("Orphaned image with container") } } diff --git a/integration/runtime_test.go b/integration/runtime_test.go index 008be9ef38..e08643b36d 100644 --- a/integration/runtime_test.go +++ b/integration/runtime_test.go @@ -61,7 +61,7 @@ func cleanup(eng *engine.Engine, t *testing.T) error { } for _, image := range images.Data { if image.Get("ID") != unitTestImageID { - mkServerFromEngine(eng, t).ImageDelete(image.Get("ID"), false) + mkServerFromEngine(eng, t).DeleteImage(image.Get("ID"), false) } } return nil diff --git a/integration/server_test.go b/integration/server_test.go index 1347d54b3a..4efe478f9b 100644 --- a/integration/server_test.go +++ b/integration/server_test.go @@ -35,7 +35,7 @@ func TestImageTagImageDelete(t *testing.T) { t.Errorf("Expected %d images, %d found", nExpected, nActual) } - if _, err := srv.ImageDelete("utest/docker:tag2", true); err != nil { + if _, err := srv.DeleteImage("utest/docker:tag2", true); err != nil { t.Fatal(err) } @@ -47,7 +47,7 @@ func TestImageTagImageDelete(t *testing.T) { t.Errorf("Expected %d images, %d found", nExpected, nActual) } - if _, err := srv.ImageDelete("utest:5000/docker:tag3", true); err != nil { + if _, err := srv.DeleteImage("utest:5000/docker:tag3", true); err != nil { t.Fatal(err) } @@ -56,7 +56,7 @@ func TestImageTagImageDelete(t *testing.T) { nExpected = len(initialImages.Data[0].GetList("RepoTags")) + 1 nActual = len(images.Data[0].GetList("RepoTags")) - if _, err := srv.ImageDelete("utest:tag1", true); err != nil { + if _, err := srv.DeleteImage("utest:tag1", true); err != nil { t.Fatal(err) } @@ -358,7 +358,7 @@ func TestRmi(t *testing.T) { t.Fatalf("Expected 2 new images, found %d.", images.Len()-initialImages.Len()) } - _, err = srv.ImageDelete(imageID, true) + _, err = srv.DeleteImage(imageID, true) if err != nil { t.Fatal(err) } @@ -472,18 +472,16 @@ func TestDeleteTagWithExistingContainers(t *testing.T) { } // Try to remove the tag - imgs, err := srv.ImageDelete("utest:tag1", true) + imgs, err := srv.DeleteImage("utest:tag1", true) if err != nil { t.Fatal(err) } - if len(imgs) != 1 { - t.Fatalf("Should only have deleted one untag %d", len(imgs)) + if len(imgs.Data) != 1 { + t.Fatalf("Should only have deleted one untag %d", len(imgs.Data)) } - untag := imgs[0] - - if untag.Untagged != unitTestImageID { - t.Fatalf("Expected %s got %s", unitTestImageID, untag.Untagged) + if untag := imgs.Data[0].Get("Untagged"); untag != unitTestImageID { + t.Fatalf("Expected %s got %s", unitTestImageID, untag) } } diff --git a/server.go b/server.go index 49beeb5fb4..d88dc16e82 100644 --- a/server.go +++ b/server.go @@ -336,6 +336,10 @@ func (srv *Server) ImageExport(job *engine.Job) engine.Status { job.Error(err) return engine.StatusErr } + if err := job.Eng.Register("image_delete", srv.ImageDelete); err != nil { + job.Error(err) + return engine.StatusErr + } return engine.StatusOK } @@ -1813,7 +1817,7 @@ func (srv *Server) ContainerDestroy(job *engine.Job) engine.Status { var ErrImageReferenced = errors.New("Image referenced by a repository") -func (srv *Server) deleteImageAndChildren(id string, imgs *[]APIRmi, byParents map[string][]*Image) error { +func (srv *Server) deleteImageAndChildren(id string, imgs *engine.Table, byParents map[string][]*Image) error { // If the image is referenced by a repo, do not delete if len(srv.runtime.repositories.ByID()[id]) != 0 { return ErrImageReferenced @@ -1845,14 +1849,16 @@ func (srv *Server) deleteImageAndChildren(id string, imgs *[]APIRmi, byParents m if err != nil { return err } - *imgs = append(*imgs, APIRmi{Deleted: id}) + out := &engine.Env{} + out.Set("Deleted", id) + imgs.Add(out) srv.LogEvent("delete", id, "") return nil } return nil } -func (srv *Server) deleteImageParents(img *Image, imgs *[]APIRmi) error { +func (srv *Server) deleteImageParents(img *Image, imgs *engine.Table) error { if img.Parent != "" { parent, err := srv.runtime.graph.Get(img.Parent) if err != nil { @@ -1871,12 +1877,42 @@ func (srv *Server) deleteImageParents(img *Image, imgs *[]APIRmi) error { return nil } -func (srv *Server) deleteImage(img *Image, repoName, tag string) ([]APIRmi, error) { +func (srv *Server) DeleteImage(name string, autoPrune bool) (*engine.Table, error) { var ( - imgs = []APIRmi{} - tags = []string{} + repoName, tag string + img, err = srv.runtime.repositories.LookupImage(name) + imgs = engine.NewTable("", 0) + tags = []string{} ) + if err != nil { + return nil, fmt.Errorf("No such image: %s", name) + } + + // FIXME: What does autoPrune mean ? + if !autoPrune { + if err := srv.runtime.graph.Delete(img.ID); err != nil { + return nil, fmt.Errorf("Cannot delete image %s: %s", name, err) + } + return nil, nil + } + + if !strings.Contains(img.ID, name) { + repoName, tag = utils.ParseRepositoryTag(name) + } + + // If we have a repo and the image is not referenced anywhere else + // then just perform an untag and do not validate. + // + // i.e. only validate if we are performing an actual delete and not + // an untag op + if repoName != "" && len(srv.runtime.repositories.ByID()[img.ID]) == 1 { + // Prevent deletion if image is used by a container + if err := srv.canDeleteImage(img.ID); err != nil { + return nil, err + } + } + //If delete by id, see if the id belong only to one repository if repoName == "" { for _, repoAndTag := range srv.runtime.repositories.ByID()[img.ID] { @@ -1903,17 +1939,19 @@ func (srv *Server) deleteImage(img *Image, repoName, tag string) ([]APIRmi, erro return nil, err } if tagDeleted { - imgs = append(imgs, APIRmi{Untagged: img.ID}) + out := &engine.Env{} + out.Set("Untagged", img.ID) + imgs.Add(out) srv.LogEvent("untag", img.ID, "") } } if len(srv.runtime.repositories.ByID()[img.ID]) == 0 { - if err := srv.deleteImageAndChildren(img.ID, &imgs, nil); err != nil { + if err := srv.deleteImageAndChildren(img.ID, imgs, nil); err != nil { if err != ErrImageReferenced { return imgs, err } - } else if err := srv.deleteImageParents(img, &imgs); err != nil { + } else if err := srv.deleteImageParents(img, imgs); err != nil { if err != ErrImageReferenced { return imgs, err } @@ -1922,39 +1960,22 @@ func (srv *Server) deleteImage(img *Image, repoName, tag string) ([]APIRmi, erro return imgs, nil } -func (srv *Server) ImageDelete(name string, autoPrune bool) ([]APIRmi, error) { - var ( - repository, tag string - img, err = srv.runtime.repositories.LookupImage(name) - ) +func (srv *Server) ImageDelete(job *engine.Job) engine.Status { + if n := len(job.Args); n != 1 { + job.Errorf("Usage: %s IMAGE", job.Name) + return engine.StatusErr + } + + imgs, err := srv.DeleteImage(job.Args[0], job.GetenvBool("autoPrune")) if err != nil { - return nil, fmt.Errorf("No such image: %s", name) + job.Error(err) + return engine.StatusErr } - - // FIXME: What does autoPrune mean ? - if !autoPrune { - if err := srv.runtime.graph.Delete(img.ID); err != nil { - return nil, fmt.Errorf("Cannot delete image %s: %s", name, err) - } - return nil, nil + if _, err := imgs.WriteTo(job.Stdout); err != nil { + job.Error(err) + return engine.StatusErr } - - if !strings.Contains(img.ID, name) { - repository, tag = utils.ParseRepositoryTag(name) - } - - // If we have a repo and the image is not referenced anywhere else - // then just perform an untag and do not validate. - // - // i.e. only validate if we are performing an actual delete and not - // an untag op - if repository != "" && len(srv.runtime.repositories.ByID()[img.ID]) == 1 { - // Prevent deletion if image is used by a container - if err := srv.canDeleteImage(img.ID); err != nil { - return nil, err - } - } - return srv.deleteImage(img, repository, tag) + return engine.StatusOK } func (srv *Server) canDeleteImage(imgID string) error { From 177f6588824de5a489f0b31a2cf053c3cdf0bb0e Mon Sep 17 00:00:00 2001 From: Victor Vieux Date: Mon, 20 Jan 2014 14:10:23 -0800 Subject: [PATCH 2/3] remove buffer Docker-DCO-1.1-Signed-off-by: Victor Vieux (github: vieux) --- api.go | 26 +++----------------------- commands.go | 4 ++-- server.go | 11 ++++++----- 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/api.go b/api.go index 89d4561130..698e5b429a 100644 --- a/api.go +++ b/api.go @@ -668,31 +668,11 @@ func deleteImages(srv *Server, version float64, w http.ResponseWriter, r *http.R if vars == nil { return fmt.Errorf("Missing parameter") } - var ( - buffer = bytes.NewBuffer(nil) - job = srv.Eng.Job("image_delete", vars["name"]) - ) - job.Stdout.Add(buffer) + var job = srv.Eng.Job("image_delete", vars["name"]) + job.Stdout.Add(w) job.SetenvBool("autoPrune", version > 1.1) - if err := job.Run(); err != nil { - return err - } - outs := engine.NewTable("", 0) - if _, err := outs.ReadFrom(buffer); err != nil { - return err - } - - if len(outs.Data) != 0 { - var err error - if version < 1.9 { - _, err = outs.WriteTo(w) - } else { - _, err = outs.WriteListTo(w) - } - return err - } - return fmt.Errorf("Conflict, %s wasn't deleted", vars["name"]) + return job.Run() } func postContainersStart(srv *Server, version float64, w http.ResponseWriter, r *http.Request, vars map[string]string) error { diff --git a/commands.go b/commands.go index d918efdace..5578455761 100644 --- a/commands.go +++ b/commands.go @@ -823,13 +823,13 @@ func (cli *DockerCli) CmdRmi(args ...string) error { var encounteredError error for _, name := range cmd.Args() { - body, _, err := readBody(cli.call("DELETE", "/images/"+name, nil, false)) + stream, _, err := cli.call("DELETE", "/images/"+name, nil, false) if err != nil { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") } else { outs := engine.NewTable("Created", 0) - if _, err := outs.ReadFrom(bytes.NewReader(body)); err != nil { + if _, err := outs.ReadListFrom(stream); err != nil { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") continue diff --git a/server.go b/server.go index d88dc16e82..95b8433676 100644 --- a/server.go +++ b/server.go @@ -99,6 +99,7 @@ func jobInitApi(job *engine.Job) engine.Status { "build": srv.Build, "pull": srv.ImagePull, "import": srv.ImageImport, + "image_delete": srv.ImageDelete, } { if err := job.Eng.Register(name, handler); err != nil { job.Error(err) @@ -336,10 +337,6 @@ func (srv *Server) ImageExport(job *engine.Job) engine.Status { job.Error(err) return engine.StatusErr } - if err := job.Eng.Register("image_delete", srv.ImageDelete); err != nil { - job.Error(err) - return engine.StatusErr - } return engine.StatusOK } @@ -1971,7 +1968,11 @@ func (srv *Server) ImageDelete(job *engine.Job) engine.Status { job.Error(err) return engine.StatusErr } - if _, err := imgs.WriteTo(job.Stdout); err != nil { + if len(imgs.Data) == 0 { + job.Errorf("Conflict, %s wasn't deleted", job.Args[0]) + return engine.StatusErr + } + if _, err := imgs.WriteListTo(job.Stdout); err != nil { job.Error(err) return engine.StatusErr } From f41e0cf0485eac21d65c1af19a732b350292d200 Mon Sep 17 00:00:00 2001 From: Victor Vieux Date: Tue, 21 Jan 2014 17:56:09 -0800 Subject: [PATCH 3/3] fix error handling Docker-DCO-1.1-Signed-off-by: Victor Vieux (github: vieux) --- commands.go | 43 +++++++++++++++-------------------------- engine/env.go | 12 +++--------- engine/streams.go | 7 ++++++- integration/api_test.go | 10 +++++----- 4 files changed, 30 insertions(+), 42 deletions(-) diff --git a/commands.go b/commands.go index 5578455761..98be8b6edf 100644 --- a/commands.go +++ b/commands.go @@ -823,13 +823,13 @@ func (cli *DockerCli) CmdRmi(args ...string) error { var encounteredError error for _, name := range cmd.Args() { - stream, _, err := cli.call("DELETE", "/images/"+name, nil, false) + body, _, err := readBody(cli.call("DELETE", "/images/"+name, nil, false)) if err != nil { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") } else { outs := engine.NewTable("Created", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { fmt.Fprintf(cli.err, "%s\n", err) encounteredError = fmt.Errorf("Error: failed to remove one or more images") continue @@ -859,16 +859,13 @@ func (cli *DockerCli) CmdHistory(args ...string) error { return nil } - stream, _, err := cli.call("GET", "/images/"+cmd.Arg(0)+"/history", nil, false) - if stream != nil { - defer stream.Close() - } + body, _, err := readBody(cli.call("GET", "/images/"+cmd.Arg(0)+"/history", nil, false)) if err != nil { return err } outs := engine.NewTable("Created", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { return err } @@ -1139,16 +1136,13 @@ func (cli *DockerCli) CmdImages(args ...string) error { filter := cmd.Arg(0) if *flViz || *flTree { - stream, _, err := cli.call("GET", "/images/json?all=1", nil, false) - if stream != nil { - defer stream.Close() - } + body, _, err := readBody(cli.call("GET", "/images/json?all=1", nil, false)) if err != nil { return err } outs := engine.NewTable("Created", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { return err } @@ -1211,16 +1205,14 @@ func (cli *DockerCli) CmdImages(args ...string) error { v.Set("all", "1") } - stream, _, err := cli.call("GET", "/images/json?"+v.Encode(), nil, false) - if stream != nil { - defer stream.Close() - } + body, _, err := readBody(cli.call("GET", "/images/json?"+v.Encode(), nil, false)) + if err != nil { return err } outs := engine.NewTable("Created", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { return err } @@ -1532,16 +1524,14 @@ func (cli *DockerCli) CmdDiff(args ...string) error { return nil } - stream, _, err := cli.call("GET", "/containers/"+cmd.Arg(0)+"/changes", nil, false) - if stream != nil { - defer stream.Close() - } + body, _, err := readBody(cli.call("GET", "/containers/"+cmd.Arg(0)+"/changes", nil, false)) + if err != nil { return err } outs := engine.NewTable("", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { return err } for _, change := range outs.Data { @@ -1674,15 +1664,14 @@ func (cli *DockerCli) CmdSearch(args ...string) error { v := url.Values{} v.Set("term", cmd.Arg(0)) - stream, _, err := cli.call("GET", "/images/search?"+v.Encode(), nil, true) - if stream != nil { - defer stream.Close() - } + + body, _, err := readBody(cli.call("GET", "/images/search?"+v.Encode(), nil, false)) + if err != nil { return err } outs := engine.NewTable("star_count", 0) - if _, err := outs.ReadListFrom(stream); err != nil { + if _, err := outs.ReadListFrom(body); err != nil { return err } w := tabwriter.NewWriter(cli.out, 10, 1, 3, ' ', 0) diff --git a/engine/env.go b/engine/env.go index 37ba2ddb4c..2df303c9e1 100644 --- a/engine/env.go +++ b/engine/env.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "io/ioutil" "sort" "strconv" "strings" @@ -325,15 +324,10 @@ func (t *Table) WriteTo(dst io.Writer) (n int64, err error) { return n, nil } -func (t *Table) ReadListFrom(src io.Reader) (n int64, err error) { +func (t *Table) ReadListFrom(src []byte) (n int64, err error) { var array []interface{} - content, err := ioutil.ReadAll(src) - if err != nil { - return -1, err - } - - if err := json.Unmarshal(content, &array); err != nil { + if err := json.Unmarshal(src, &array); err != nil { return -1, err } @@ -347,7 +341,7 @@ func (t *Table) ReadListFrom(src io.Reader) (n int64, err error) { } } - return int64(len(content)), nil + return int64(len(src)), nil } func (t *Table) ReadFrom(src io.Reader) (n int64, err error) { diff --git a/engine/streams.go b/engine/streams.go index 4b1f172b49..48f031de8f 100644 --- a/engine/streams.go +++ b/engine/streams.go @@ -5,6 +5,7 @@ import ( "container/ring" "fmt" "io" + "io/ioutil" "sync" ) @@ -228,7 +229,11 @@ func (o *Output) AddListTable() (dst *Table, err error) { o.tasks.Add(1) go func() { defer o.tasks.Done() - if _, err := dst.ReadListFrom(src); err != nil { + content, err := ioutil.ReadAll(src) + if err != nil { + return + } + if _, err := dst.ReadListFrom(content); err != nil { return } }() diff --git a/integration/api_test.go b/integration/api_test.go index 6fac656c99..ec9e4acb7c 100644 --- a/integration/api_test.go +++ b/integration/api_test.go @@ -170,7 +170,7 @@ func TestGetImagesJSON(t *testing.T) { assertHttpNotError(r, t) images := engine.NewTable("Created", 0) - if _, err := images.ReadListFrom(r.Body); err != nil { + if _, err := images.ReadListFrom(r.Body.Bytes()); err != nil { t.Fatal(err) } @@ -205,7 +205,7 @@ func TestGetImagesJSON(t *testing.T) { assertHttpNotError(r2, t) images2 := engine.NewTable("ID", 0) - if _, err := images2.ReadListFrom(r2.Body); err != nil { + if _, err := images2.ReadListFrom(r2.Body.Bytes()); err != nil { t.Fatal(err) } @@ -238,7 +238,7 @@ func TestGetImagesJSON(t *testing.T) { assertHttpNotError(r3, t) images3 := engine.NewTable("ID", 0) - if _, err := images3.ReadListFrom(r3.Body); err != nil { + if _, err := images3.ReadListFrom(r3.Body.Bytes()); err != nil { t.Fatal(err) } @@ -264,7 +264,7 @@ func TestGetImagesHistory(t *testing.T) { assertHttpNotError(r, t) outs := engine.NewTable("Created", 0) - if _, err := outs.ReadListFrom(r.Body); err != nil { + if _, err := outs.ReadListFrom(r.Body.Bytes()); err != nil { t.Fatal(err) } if len(outs.Data) != 1 { @@ -409,7 +409,7 @@ func TestGetContainersChanges(t *testing.T) { } assertHttpNotError(r, t) outs := engine.NewTable("", 0) - if _, err := outs.ReadListFrom(r.Body); err != nil { + if _, err := outs.ReadListFrom(r.Body.Bytes()); err != nil { t.Fatal(err) }