From efb454486253bf2da044b3acde74b4ce2e3e9384 Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Sun, 11 Oct 2015 09:32:52 +0200 Subject: [PATCH] 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 Upstream-commit: f6577be1c93150149c291f9d18375d7bcae9ebb1 Component: engine --- components/engine/graph/graph.go | 49 ++++++++----------- components/engine/graph/pull_v2.go | 10 ++-- .../integration-cli/docker_cli_images_test.go | 3 ++ 3 files changed, 29 insertions(+), 33 deletions(-) diff --git a/components/engine/graph/graph.go b/components/engine/graph/graph.go index 9704ede6bd..deed4043b3 100644 --- a/components/engine/graph/graph.go +++ b/components/engine/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/components/engine/graph/pull_v2.go b/components/engine/graph/pull_v2.go index 680c2434f6..346b7ee58b 100644 --- a/components/engine/graph/pull_v2.go +++ b/components/engine/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/components/engine/integration-cli/docker_cli_images_test.go b/components/engine/integration-cli/docker_cli_images_test.go index f85c84ee83..27fbfb721f 100644 --- a/components/engine/integration-cli/docker_cli_images_test.go +++ b/components/engine/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:])