From 04ef69a1e963e0c9bc9b685188f2c584ef275e45 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Fri, 9 Oct 2015 18:51:15 +0200 Subject: [PATCH 1/4] integration-cli: docker_cli_build_test: check error before defer Signed-off-by: Antonio Murdaca --- integration-cli/docker_cli_build_test.go | 64 ++++++++++++------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 00498541ff..e7696fd72b 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -819,10 +819,10 @@ RUN [ $(ls -l /exists/exists_file | awk '{print $3":"$4}') = 'dockerio:dockerio' "test_file3": "test3", "test_file4": "test4", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err := buildImageFromContext(name, ctx, true); err != nil { c.Fatal(err) @@ -839,10 +839,10 @@ func (s *DockerSuite) TestBuildAddMultipleFilesToFile(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using ADD with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -861,10 +861,10 @@ func (s *DockerSuite) TestBuildJSONAddMultipleFilesToFile(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using ADD with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -883,10 +883,10 @@ func (s *DockerSuite) TestBuildAddMultipleFilesToFileWild(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using ADD with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -905,10 +905,10 @@ func (s *DockerSuite) TestBuildJSONAddMultipleFilesToFileWild(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using ADD with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -927,10 +927,10 @@ func (s *DockerSuite) TestBuildCopyMultipleFilesToFile(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using COPY with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -949,10 +949,10 @@ func (s *DockerSuite) TestBuildJSONCopyMultipleFilesToFile(c *check.C) { "file1.txt": "test1", "file2.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using COPY with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -987,10 +987,10 @@ RUN [ $(cat "/test dir/test_file6") = 'test6' ]`, "test_dir/test_file5": "test5", "test dir/test_file6": "test6", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err := buildImageFromContext(name, ctx, true); err != nil { c.Fatal(err) @@ -1023,10 +1023,10 @@ RUN [ $(cat "/test dir/test_file6") = 'test6' ]`, "test_dir/test_file5": "test5", "test dir/test_file6": "test6", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err := buildImageFromContext(name, ctx, true); err != nil { c.Fatal(err) @@ -1043,10 +1043,10 @@ func (s *DockerSuite) TestBuildAddMultipleFilesToFileWithWhitespace(c *check.C) "test file1": "test1", "test file2": "test2", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using ADD with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -1065,10 +1065,10 @@ func (s *DockerSuite) TestBuildCopyMultipleFilesToFileWithWhitespace(c *check.C) "test file1": "test1", "test file2": "test2", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "When using COPY with more than one source file, the destination must be a directory and end with a /" if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -1134,10 +1134,10 @@ func (s *DockerSuite) TestBuildCopyWildcardNoFind(c *check.C) { ctx, err := fakeContext(`FROM busybox COPY file*.txt /tmp/ `, nil) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() _, err = buildImageFromContext(name, ctx, true) if err == nil { @@ -1182,10 +1182,10 @@ func (s *DockerSuite) TestBuildCopyWildcardCache(c *check.C) { map[string]string{ "file1.txt": "test1", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { @@ -2214,10 +2214,10 @@ func (s *DockerSuite) TestBuildRelativeCopy(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() _, err = buildImageFromContext(name, ctx, false) if err != nil { c.Fatal(err) @@ -2699,10 +2699,10 @@ func (s *DockerSuite) TestBuildAddLocalFileWithCache(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -2728,10 +2728,10 @@ func (s *DockerSuite) TestBuildAddMultipleLocalFileWithCache(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -2759,10 +2759,10 @@ func (s *DockerSuite) TestBuildAddLocalFileWithoutCache(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -2786,10 +2786,10 @@ func (s *DockerSuite) TestBuildCopyDirButNotFile(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "dir/foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -2820,10 +2820,10 @@ func (s *DockerSuite) TestBuildAddCurrentDirWithCache(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -2876,10 +2876,10 @@ func (s *DockerSuite) TestBuildAddCurrentDirWithoutCache(c *check.C) { ctx, err := fakeContext(dockerfile, map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() id1, err := buildImageFromContext(name, ctx, true) if err != nil { c.Fatal(err) @@ -3065,10 +3065,10 @@ CMD ["cat", "/foo"]`, "foo": "bar", }, ) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() context, err := archive.Tar(ctx.Dir, compression) if err != nil { c.Fatalf("failed to build context tar: %v", err) @@ -3186,10 +3186,10 @@ func (s *DockerSuite) TestBuildEntrypointRunCleanup(c *check.C) { map[string]string{ "foo": "hello", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err := buildImageFromContext(name, ctx, true); err != nil { c.Fatal(err) } @@ -3213,10 +3213,10 @@ func (s *DockerSuite) TestBuildForbiddenContextPath(c *check.C) { "test.txt": "test1", "other.txt": "other", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() expected := "Forbidden path outside the build context: ../../ " if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) { @@ -3231,10 +3231,10 @@ func (s *DockerSuite) TestBuildAddFileNotFound(c *check.C) { ctx, err := fakeContext(`FROM scratch ADD foo /usr/local/bar`, map[string]string{"bar": "hello"}) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err := buildImageFromContext(name, ctx, true); err != nil { if !strings.Contains(err.Error(), "foo: no such file or directory") { c.Fatalf("Wrong error %v, must be about missing foo file or directory", err) @@ -3631,10 +3631,10 @@ func (s *DockerSuite) TestBuildDockerignoringDockerignore(c *check.C) { "Dockerfile": dockerfile, ".dockerignore": ".dockerignore\n", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if _, err = buildImageFromContext(name, ctx, true); err != nil { c.Fatalf("Didn't ignore .dockerignore correctly:%s", err) } @@ -3653,10 +3653,10 @@ func (s *DockerSuite) TestBuildDockerignoreTouchDockerfile(c *check.C) { "Dockerfile": dockerfile, ".dockerignore": "Dockerfile\n", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() if id1, err = buildImageFromContext(name, ctx, true); err != nil { c.Fatalf("Didn't build it correctly:%s", err) @@ -4926,10 +4926,10 @@ func (s *DockerSuite) TestBuildRenamedDockerfile(c *check.C) { "dFile": "FROM busybox\nRUN echo from dFile", "files/dFile2": "FROM busybox\nRUN echo from files/dFile2", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() out, _, err := dockerCmdInDir(c, ctx.Dir, "build", "-t", "test1", ".") if err != nil { @@ -5028,10 +5028,10 @@ func (s *DockerSuite) TestBuildFromMixedcaseDockerfile(c *check.C) { map[string]string{ "dockerfile": "FROM busybox\nRUN echo from dockerfile", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() out, _, err := dockerCmdInDir(c, ctx.Dir, "build", "-t", "test1", ".") if err != nil { @@ -5053,10 +5053,10 @@ RUN echo from Dockerfile`, map[string]string{ "dockerfile": "FROM busybox\nRUN echo from dockerfile", }) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() out, _, err := dockerCmdInDir(c, ctx.Dir, "build", "-t", "test1", ".") if err != nil { @@ -5084,10 +5084,10 @@ RUN find /tmp/`}) ctx, err := fakeContext(`FROM busybox RUN echo from Dockerfile`, map[string]string{}) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() // Make sure that -f is ignored and that we don't use the Dockerfile // that's in the current dir @@ -5109,10 +5109,10 @@ func (s *DockerSuite) TestBuildFromStdinWithF(c *check.C) { ctx, err := fakeContext(`FROM busybox RUN echo from Dockerfile`, map[string]string{}) - defer ctx.Close() if err != nil { c.Fatal(err) } + defer ctx.Close() // Make sure that -f is ignored and that we don't use the Dockerfile // that's in the current dir @@ -5669,8 +5669,8 @@ func (s *DockerSuite) TestBuildNullStringInAddCopyVolume(c *check.C) { "nullfile": "test2", }, ) - defer ctx.Close() c.Assert(err, check.IsNil) + defer ctx.Close() _, err = buildImageFromContext(name, ctx, true) c.Assert(err, check.IsNil) From 56f5e3459f8d7477d2aa60dee02bc7cd8a8731ad Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Fri, 9 Oct 2015 19:13:45 +0200 Subject: [PATCH 2/4] graph: add parent img refcount for faster rmi also fix a typo in pkg/truncindex package comment Signed-off-by: Antonio Murdaca --- daemon/daemonbuilder/builder.go | 2 +- daemon/image_delete.go | 4 +- graph/graph.go | 64 ++++++++++++++++++----- integration-cli/docker_cli_images_test.go | 40 +++++++++++++- integration-cli/docker_cli_rmi_test.go | 12 +++++ 5 files changed, 105 insertions(+), 17 deletions(-) diff --git a/daemon/daemonbuilder/builder.go b/daemon/daemonbuilder/builder.go index b8ff702119..6786401631 100644 --- a/daemon/daemonbuilder/builder.go +++ b/daemon/daemonbuilder/builder.go @@ -193,7 +193,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, // GetCachedImage returns a reference to a cached image whose parent equals `parent` // and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error. func (d Docker) GetCachedImage(imgID string, cfg *runconfig.Config) (string, error) { - cache, err := d.Daemon.ImageGetCached(string(imgID), cfg) + cache, err := d.Daemon.ImageGetCached(imgID, cfg) if cache == nil || err != nil { return "", err } diff --git a/daemon/image_delete.go b/daemon/image_delete.go index efe6362812..070ec0b1d4 100644 --- a/daemon/image_delete.go +++ b/daemon/image_delete.go @@ -279,7 +279,7 @@ func (daemon *Daemon) checkImageDeleteHardConflict(img *image.Image) *imageDelet } // Check if the image has any descendent images. - if daemon.Graph().HasChildren(img) { + if daemon.Graph().HasChildren(img.ID) { return &imageDeleteConflict{ hard: true, imgID: img.ID, @@ -337,5 +337,5 @@ func (daemon *Daemon) checkImageDeleteSoftConflict(img *image.Image) *imageDelet // that there are no repository references to the given image and it has no // child images. func (daemon *Daemon) imageIsDangling(img *image.Image) bool { - return !(daemon.repositories.HasReferences(img) || daemon.Graph().HasChildren(img)) + return !(daemon.Repositories().HasReferences(img) || daemon.Graph().HasChildren(img.ID)) } diff --git a/graph/graph.go b/graph/graph.go index cbdfeee1a1..7ae1807608 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -104,6 +104,7 @@ type Graph struct { tarSplitDisabled bool uidMaps []idtools.IDMap gidMaps []idtools.IDMap + parentRefs map[string]int } // file names for ./graph// @@ -141,12 +142,13 @@ func NewGraph(root string, driver graphdriver.Driver, uidMaps, gidMaps []idtools } graph := &Graph{ - root: abspath, - idIndex: truncindex.NewTruncIndex([]string{}), - driver: driver, - retained: &retainedLayers{layerHolders: make(map[string]map[string]struct{})}, - uidMaps: uidMaps, - gidMaps: gidMaps, + root: abspath, + idIndex: truncindex.NewTruncIndex([]string{}), + driver: driver, + retained: &retainedLayers{layerHolders: make(map[string]map[string]struct{})}, + uidMaps: uidMaps, + gidMaps: gidMaps, + parentRefs: make(map[string]int), } // Windows does not currently support tarsplit functionality. @@ -174,6 +176,20 @@ func (graph *Graph) restore() error { for _, v := range dir { id := v.Name() if graph.driver.Exists(id) { + pth := filepath.Join(graph.root, id, "json") + jsonSource, err := os.Open(pth) + if err != nil { + return err + } + defer jsonSource.Close() + decoder := json.NewDecoder(jsonSource) + var img *image.Image + if err := decoder.Decode(img); err != nil { + return err + } + graph.imageMutex.Lock(img.Parent) + graph.parentRefs[img.Parent]++ + graph.imageMutex.Unlock(img.Parent) ids = append(ids, id) } } @@ -326,7 +342,13 @@ func (graph *Graph) register(im image.Descriptor, layerData io.Reader) (err erro if err := os.Rename(tmp, graph.imageRoot(imgID)); err != nil { return err } - graph.idIndex.Add(imgID) + + graph.idIndex.Add(img.ID) + + graph.imageMutex.Lock(img.Parent) + graph.parentRefs[img.Parent]++ + graph.imageMutex.Unlock(img.Parent) + return nil } @@ -393,6 +415,10 @@ func (graph *Graph) Delete(name string) error { if err != nil { return err } + img, err := graph.Get(id) + if err != nil { + return err + } tmp, err := graph.mktemp() graph.idIndex.Delete(id) if err == nil { @@ -407,6 +433,14 @@ func (graph *Graph) Delete(name string) error { } // Remove rootfs data from the driver graph.driver.Remove(id) + + graph.imageMutex.Lock(img.Parent) + graph.parentRefs[img.Parent]-- + if graph.parentRefs[img.Parent] == 0 { + delete(graph.parentRefs, img.Parent) + } + graph.imageMutex.Unlock(img.Parent) + // Remove the trashed image directory return os.RemoveAll(tmp) } @@ -424,9 +458,11 @@ func (graph *Graph) Map() map[string]*image.Image { // The walking order is undetermined. func (graph *Graph) walkAll(handler func(*image.Image)) { graph.idIndex.Iterate(func(id string) { - if img, err := graph.Get(id); err != nil { + img, err := graph.Get(id) + if err != nil { return - } else if handler != nil { + } + if handler != nil { handler(img) } }) @@ -453,8 +489,11 @@ func (graph *Graph) ByParent() map[string][]*image.Image { } // HasChildren returns whether the given image has any child images. -func (graph *Graph) HasChildren(img *image.Image) bool { - return len(graph.ByParent()[img.ID]) > 0 +func (graph *Graph) HasChildren(imgID string) bool { + graph.imageMutex.Lock(imgID) + count := graph.parentRefs[imgID] + graph.imageMutex.Unlock(imgID) + return count > 0 } // Retain keeps the images and layers that are in the pulling chain so that @@ -472,11 +511,10 @@ func (graph *Graph) Release(sessionID string, layerIDs ...string) { // A head is an image which is not the parent of another image in the graph. func (graph *Graph) Heads() map[string]*image.Image { heads := make(map[string]*image.Image) - byParent := graph.ByParent() graph.walkAll(func(image *image.Image) { // If it's not in the byParent lookup table, then // it's not a parent -> so it's a head! - if _, exists := byParent[image.ID]; !exists { + if !graph.HasChildren(image.ID) { heads[image.ID] = image } }) diff --git a/integration-cli/docker_cli_images_test.go b/integration-cli/docker_cli_images_test.go index 3f639a5a16..f85c84ee83 100644 --- a/integration-cli/docker_cli_images_test.go +++ b/integration-cli/docker_cli_images_test.go @@ -51,7 +51,6 @@ func (s *DockerSuite) TestImagesEnsureImageWithBadTagIsNotListed(c *check.C) { if strings.Contains(out, "busybox") { c.Fatal("images should not have listed busybox") } - } func (s *DockerSuite) TestImagesOrderedByCreationDate(c *check.C) { @@ -204,3 +203,42 @@ func (s *DockerSuite) TestImagesWithIncorrectFilter(c *check.C) { c.Assert(err, check.NotNil) c.Assert(out, checker.Contains, "Invalid filter") } + +func (s *DockerSuite) TestImagesEnsureOnlyHeadsImagesShown(c *check.C) { + testRequires(c, DaemonIsLinux) + + dockerfile := ` + FROM scratch + MAINTAINER docker + ENV foo bar` + + head, out, err := buildImageWithOut("scratch-image", dockerfile, false) + c.Assert(err, check.IsNil) + + split := strings.Split(out, "\n") + intermediate := strings.TrimSpace(split[5][7:]) + + out, _ = dockerCmd(c, "images") + if strings.Contains(out, intermediate) { + c.Fatalf("images shouldn't show non-heads images, got %s in %s", intermediate, out) + } + if !strings.Contains(out, head[:12]) { + c.Fatalf("images should contain final built images, want %s in out, got %s", head[:12], out) + } +} + +func (s *DockerSuite) TestImagesEnsureImagesFromScratchShown(c *check.C) { + testRequires(c, DaemonIsLinux) + + dockerfile := ` + FROM scratch + MAINTAINER docker` + + id, _, err := buildImageWithOut("scratch-image", dockerfile, false) + c.Assert(err, check.IsNil) + + out, _ := dockerCmd(c, "images") + if !strings.Contains(out, id[:12]) { + c.Fatalf("images should contain images built from scratch (e.g. %s), got %s", id[:12], out) + } +} diff --git a/integration-cli/docker_cli_rmi_test.go b/integration-cli/docker_cli_rmi_test.go index 8a60ec7fe1..19914315ee 100644 --- a/integration-cli/docker_cli_rmi_test.go +++ b/integration-cli/docker_cli_rmi_test.go @@ -284,3 +284,15 @@ RUN echo 2 #layer2 // should be allowed to untag with the -f flag c.Assert(out, checker.Contains, fmt.Sprintf("Untagged: %s:latest", newTag)) } + +func (*DockerSuite) TestRmiParentImageFail(c *check.C) { + testRequires(c, DaemonIsLinux) + + parent, err := inspectField("busybox", "Parent") + c.Assert(err, check.IsNil) + out, _, err := dockerCmdWithError("rmi", parent) + c.Assert(err, check.NotNil) + if !strings.Contains(out, "image has dependent child images") { + c.Fatalf("rmi should have failed because it's a parent image, got %s", out) + } +} From f9e81b40f4065e3d0851172759ef58fda6572cce Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Fri, 9 Oct 2015 19:14:45 +0200 Subject: [PATCH 3/4] daemon: faster image cache miss detection Lookup the graph parent reference to detect a builder cache miss before looping the whole graph image index to build a parent-children tree. Signed-off-by: Antonio Murdaca --- daemon/daemon.go | 8 ++++++++ daemon/image_delete.go | 2 +- graph/graph.go | 3 +-- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 91af20ac66..3dda37ff34 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -1147,6 +1147,14 @@ func (daemon *Daemon) GetRemappedUIDGID() (int, int) { // created. nil is returned if a child cannot be found. An error is // returned if the parent image cannot be found. func (daemon *Daemon) ImageGetCached(imgID string, config *runconfig.Config) (*image.Image, error) { + // for now just exit if imgID has no children. + // maybe parentRefs in graph could be used to store + // the Image obj children for faster lookup below but this can + // be quite memory hungry. + if !daemon.Graph().HasChildren(imgID) { + return nil, nil + } + // Retrieve all images images := daemon.Graph().Map() diff --git a/daemon/image_delete.go b/daemon/image_delete.go index 070ec0b1d4..b28072103a 100644 --- a/daemon/image_delete.go +++ b/daemon/image_delete.go @@ -337,5 +337,5 @@ func (daemon *Daemon) checkImageDeleteSoftConflict(img *image.Image) *imageDelet // that there are no repository references to the given image and it has no // child images. func (daemon *Daemon) imageIsDangling(img *image.Image) bool { - return !(daemon.Repositories().HasReferences(img) || daemon.Graph().HasChildren(img.ID)) + return !(daemon.repositories.HasReferences(img) || daemon.Graph().HasChildren(img.ID)) } diff --git a/graph/graph.go b/graph/graph.go index 7ae1807608..9704ede6bd 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -512,8 +512,7 @@ func (graph *Graph) Release(sessionID string, layerIDs ...string) { func (graph *Graph) Heads() map[string]*image.Image { heads := make(map[string]*image.Image) graph.walkAll(func(image *image.Image) { - // If it's not in the byParent lookup table, then - // it's not a parent -> so it's a head! + // if it has no children, then it's not a parent, so it's an head if !graph.HasChildren(image.ID) { heads[image.ID] = image } From f6577be1c93150149c291f9d18375d7bcae9ebb1 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sun, 11 Oct 2015 09:32:52 +0200 Subject: [PATCH 4/4] graph: ensure _tmp dir is always removed Also remove unused func `newTempFile` and prevent a possible deadlock between pull_v2 `attemptIDReuse` and graph `register` Signed-off-by: Antonio Murdaca --- graph/graph.go | 49 ++++++++++------------- graph/pull_v2.go | 10 ++--- integration-cli/docker_cli_images_test.go | 3 ++ 3 files changed, 29 insertions(+), 33 deletions(-) diff --git a/graph/graph.go b/graph/graph.go index 9704ede6bd..deed4043b3 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -99,12 +99,16 @@ type Graph struct { root string idIndex *truncindex.TruncIndex driver graphdriver.Driver + imagesMutex sync.Mutex imageMutex imageMutex // protect images in driver. retained *retainedLayers tarSplitDisabled bool uidMaps []idtools.IDMap gidMaps []idtools.IDMap - parentRefs map[string]int + + // access to parentRefs must be protected with imageMutex locking the image id + // on the key of the map (e.g. imageMutex.Lock(img.ID), parentRefs[img.ID]...) + parentRefs map[string]int } // file names for ./graph// @@ -176,17 +180,10 @@ func (graph *Graph) restore() error { for _, v := range dir { id := v.Name() if graph.driver.Exists(id) { - pth := filepath.Join(graph.root, id, "json") - jsonSource, err := os.Open(pth) + img, err := graph.loadImage(id) if err != nil { return err } - defer jsonSource.Close() - decoder := json.NewDecoder(jsonSource) - var img *image.Image - if err := decoder.Decode(img); err != nil { - return err - } graph.imageMutex.Lock(img.Parent) graph.parentRefs[img.Parent]++ graph.imageMutex.Unlock(img.Parent) @@ -278,6 +275,10 @@ func (graph *Graph) Register(im image.Descriptor, layerData io.Reader) (err erro return err } + // this is needed cause pull_v2 attemptIDReuse could deadlock + graph.imagesMutex.Lock() + defer graph.imagesMutex.Unlock() + // We need this entire operation to be atomic within the engine. Note that // this doesn't mean Register is fully safe yet. graph.imageMutex.Lock(imgID) @@ -318,10 +319,10 @@ func (graph *Graph) register(im image.Descriptor, layerData io.Reader) (err erro graph.driver.Remove(imgID) tmp, err := graph.mktemp() - defer os.RemoveAll(tmp) if err != nil { - return fmt.Errorf("mktemp failed: %s", err) + return err } + defer os.RemoveAll(tmp) parent := im.Parent() @@ -343,11 +344,11 @@ func (graph *Graph) register(im image.Descriptor, layerData io.Reader) (err erro return err } - graph.idIndex.Add(img.ID) + graph.idIndex.Add(imgID) - graph.imageMutex.Lock(img.Parent) - graph.parentRefs[img.Parent]++ - graph.imageMutex.Unlock(img.Parent) + graph.imageMutex.Lock(parent) + graph.parentRefs[parent]++ + graph.imageMutex.Unlock(parent) return nil } @@ -371,6 +372,7 @@ func (graph *Graph) TempLayerArchive(id string, sf *streamformatter.StreamFormat if err != nil { return nil, err } + defer os.RemoveAll(tmp) a, err := graph.TarLayer(image) if err != nil { return nil, err @@ -401,14 +403,6 @@ func (graph *Graph) mktemp() (string, error) { return dir, nil } -func (graph *Graph) newTempFile() (*os.File, error) { - tmp, err := graph.mktemp() - if err != nil { - return nil, err - } - return ioutil.TempFile(tmp, "") -} - // Delete atomically removes an image from the graph. func (graph *Graph) Delete(name string) error { id, err := graph.idIndex.Get(name) @@ -419,17 +413,16 @@ func (graph *Graph) Delete(name string) error { if err != nil { return err } - tmp, err := graph.mktemp() graph.idIndex.Delete(id) - if err == nil { + tmp, err := graph.mktemp() + if err != nil { + tmp = graph.imageRoot(id) + } else { if err := os.Rename(graph.imageRoot(id), tmp); err != nil { // On err make tmp point to old dir and cleanup unused tmp dir os.RemoveAll(tmp) tmp = graph.imageRoot(id) } - } else { - // On err make tmp point to old dir for cleanup - tmp = graph.imageRoot(id) } // Remove rootfs data from the driver graph.driver.Remove(id) diff --git a/graph/pull_v2.go b/graph/pull_v2.go index 680c2434f6..346b7ee58b 100644 --- a/graph/pull_v2.go +++ b/graph/pull_v2.go @@ -7,7 +7,6 @@ import ( "io" "io/ioutil" "os" - "sync" "github.com/Sirupsen/logrus" "github.com/docker/distribution" @@ -359,6 +358,9 @@ func (p *v2Puller) pullV2Tag(out io.Writer, tag, taggedName string) (tagUpdated Action: "Extracting", }) + p.graph.imagesMutex.Lock() + defer p.graph.imagesMutex.Unlock() + p.graph.imageMutex.Lock(d.img.id) defer p.graph.imageMutex.Unlock(d.img.id) @@ -549,8 +551,6 @@ func (p *v2Puller) getImageInfos(m *manifest.Manifest) ([]contentAddressableDesc return imgs, nil } -var idReuseLock sync.Mutex - // attemptIDReuse does a best attempt to match verified compatibilityIDs // already in the graph with the computed strongIDs so we can keep using them. // This process will never fail but may just return the strongIDs if none of @@ -561,8 +561,8 @@ func (p *v2Puller) attemptIDReuse(imgs []contentAddressableDescriptor) { // This function needs to be protected with a global lock, because it // locks multiple IDs at once, and there's no good way to make sure // the locking happens a deterministic order. - idReuseLock.Lock() - defer idReuseLock.Unlock() + p.graph.imagesMutex.Lock() + defer p.graph.imagesMutex.Unlock() idMap := make(map[string]struct{}) for _, img := range imgs { diff --git a/integration-cli/docker_cli_images_test.go b/integration-cli/docker_cli_images_test.go index f85c84ee83..27fbfb721f 100644 --- a/integration-cli/docker_cli_images_test.go +++ b/integration-cli/docker_cli_images_test.go @@ -215,6 +215,9 @@ func (s *DockerSuite) TestImagesEnsureOnlyHeadsImagesShown(c *check.C) { head, out, err := buildImageWithOut("scratch-image", dockerfile, false) c.Assert(err, check.IsNil) + // this is just the output of docker build + // we're interested in getting the image id of the MAINTAINER instruction + // and that's located at output, line 5, from 7 to end split := strings.Split(out, "\n") intermediate := strings.TrimSpace(split[5][7:])