Fix copy when used with scratch and images with empty RootFS

Commit the rwLayer to get the correct DiffID
Refacator copy in thebuilder
move more code into exportImage
cleanup some windows tests
Release the newly commited layer.
Set the imageID on the buildStage after exporting a new image.
Move archiver to BuildManager.
Have ReleaseableLayer.Commit return a layer
and store the Image from exportImage in the local imageSources cache
Remove NewChild from image interface.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

api/server/backend/build/backend.go

@@ -8,6 +8,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile"
 	"github.com/docker/docker/image"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
@@ -26,8 +27,8 @@ type Backend struct {
 }
 
 // NewBackend creates a new build backend from components
-func NewBackend(components ImageComponent, builderBackend builder.Backend) *Backend {
-	manager := dockerfile.NewBuildManager(builderBackend)
+func NewBackend(components ImageComponent, builderBackend builder.Backend, idMappings *idtools.IDMappings) *Backend {
+	manager := dockerfile.NewBuildManager(builderBackend, idMappings)
 	return &Backend{imageComponent: components, manager: manager}
 }
 

builder/builder.go

@@ -11,9 +11,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	containerpkg "github.com/docker/docker/container"
-	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
-	"github.com/docker/docker/pkg/idtools"
 	"golang.org/x/net/context"
 )
 
@@ -45,9 +43,7 @@ type Backend interface {
 	// ContainerCreateWorkdir creates the workdir
 	ContainerCreateWorkdir(containerID string) error
 
-	CreateImage(config []byte, parent string) (string, error)
-
-	IDMappings() *idtools.IDMappings
+	CreateImage(config []byte, parent string) (Image, error)
 
 	ImageCacheBuilder
 }
@@ -98,12 +94,12 @@ type Image interface {
 	ImageID() string
 	RunConfig() *container.Config
 	MarshalJSON() ([]byte, error)
-	NewChild(child image.ChildConfig) *image.Image
 }
 
 // ReleaseableLayer is an image layer that can be mounted and released
 type ReleaseableLayer interface {
 	Release() error
 	Mount() (string, error)
+	Commit() (ReleaseableLayer, error)
 	DiffID() layer.DiffID
 }

builder/dockerfile/builder.go

@@ -17,6 +17,7 @@ import (
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/streamformatter"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
@@ -39,15 +40,17 @@ var validCommitCommands = map[string]bool{
 
 // BuildManager is shared across all Builder objects
 type BuildManager struct {
+	archiver  *archive.Archiver
 	backend   builder.Backend
 	pathCache pathCache // TODO: make this persistent
 }
 
 // NewBuildManager creates a BuildManager
-func NewBuildManager(b builder.Backend) *BuildManager {
+func NewBuildManager(b builder.Backend, idMappings *idtools.IDMappings) *BuildManager {
 	return &BuildManager{
 		backend:   b,
 		pathCache: &syncmap.Map{},
+		archiver:  chrootarchive.NewArchiver(idMappings),
 	}
 }
 
@@ -75,6 +78,7 @@ func (bm *BuildManager) Build(ctx context.Context, config backend.BuildConfig) (
 		ProgressWriter: config.ProgressWriter,
 		Backend:        bm.backend,
 		PathCache:      bm.pathCache,
+		Archiver:       bm.archiver,
 	}
 	return newBuilder(ctx, builderOptions).build(source, dockerfile)
 }
@@ -85,6 +89,7 @@ type builderOptions struct {
 	Backend        builder.Backend
 	ProgressWriter backend.ProgressWriter
 	PathCache      pathCache
+	Archiver       *archive.Archiver
 }
 
 // Builder is a Dockerfile builder
@@ -124,7 +129,7 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder {
 		Aux:              options.ProgressWriter.AuxFormatter,
 		Output:           options.ProgressWriter.Output,
 		docker:           options.Backend,
-		archiver:         chrootarchive.NewArchiver(options.Backend.IDMappings()),
+		archiver:         options.Archiver,
 		buildArgs:        newBuildArgs(config.BuildArgs),
 		buildStages:      newBuildStages(),
 		imageSources:     newImageSources(clientCtx, options),

builder/dockerfile/copy.go

@@ -368,7 +368,7 @@ type copyFileOptions struct {
 	archiver   *archive.Archiver
 }
 
-func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error {
+func performCopyForInfo(dest copyInfo, source copyInfo, options copyFileOptions) error {
 	srcPath, err := source.fullPath()
 	if err != nil {
 		return err
@@ -379,36 +379,63 @@ func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error {
 	}
 
 	archiver := options.archiver
-	rootIDs := archiver.IDMappings.RootPair()
 
 	src, err := os.Stat(srcPath)
 	if err != nil {
 		return errors.Wrapf(err, "source path not found")
 	}
 	if src.IsDir() {
-		if err := archiver.CopyWithTar(srcPath, destPath); err != nil {
-			return err
-		}
-		return fixPermissions(srcPath, destPath, rootIDs)
+		return copyDirectory(archiver, srcPath, destPath)
 	}
-
 	if options.decompress && archive.IsArchivePath(srcPath) {
-		// To support the untar feature we need to clean up the path a little bit
-		// because tar is not very forgiving
-		tarDest := dest.path
-		// TODO: could this be just TrimSuffix()?
-		if strings.HasSuffix(tarDest, string(os.PathSeparator)) {
-			tarDest = filepath.Dir(dest.path)
-		}
-		return archiver.UntarPath(srcPath, tarDest)
+		return archiver.UntarPath(srcPath, destPath)
 	}
 
-	if err := idtools.MkdirAllAndChownNew(filepath.Dir(destPath), 0755, rootIDs); err != nil {
+	destExistsAsDir, err := isExistingDirectory(destPath)
+	if err != nil {
 		return err
 	}
-	if err := archiver.CopyFileWithTar(srcPath, destPath); err != nil {
-		return err
+	// dest.path must be used because destPath has already been cleaned of any
+	// trailing slash
+	if endsInSlash(dest.path) || destExistsAsDir {
+		// source.path must be used to get the correct filename when the source
+		// is a symlink
+		destPath = filepath.Join(destPath, filepath.Base(source.path))
+	}
+	return copyFile(archiver, srcPath, destPath)
+}
+
+func copyDirectory(archiver *archive.Archiver, source, dest string) error {
+	if err := archiver.CopyWithTar(source, dest); err != nil {
+		return errors.Wrapf(err, "failed to copy directory")
+	}
+	return fixPermissions(source, dest, archiver.IDMappings.RootPair())
+}
+
+func copyFile(archiver *archive.Archiver, source, dest string) error {
+	rootIDs := archiver.IDMappings.RootPair()
+
+	if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest), 0755, rootIDs); err != nil {
+		return errors.Wrapf(err, "failed to create new directory")
+	}
+	if err := archiver.CopyFileWithTar(source, dest); err != nil {
+		return errors.Wrapf(err, "failed to copy file")
+	}
+	return fixPermissions(source, dest, rootIDs)
+}
+
+func endsInSlash(path string) bool {
+	return strings.HasSuffix(path, string(os.PathSeparator))
+}
+
+// isExistingDirectory returns true if the path exists and is a directory
+func isExistingDirectory(path string) (bool, error) {
+	destStat, err := os.Stat(path)
+	switch {
+	case os.IsNotExist(err):
+		return false, nil
+	case err != nil:
+		return false, err
 	}
-	// TODO: do I have to change destPath to the filename?
-	return fixPermissions(srcPath, destPath, rootIDs)
+	return destStat.IsDir(), nil
 }

builder/dockerfile/copy_test.go

@@ -0,0 +1,45 @@
+package dockerfile
+
+import (
+	"testing"
+
+	"github.com/docker/docker/pkg/testutil/tempfile"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIsExistingDirectory(t *testing.T) {
+	tmpfile := tempfile.NewTempFile(t, "file-exists-test", "something")
+	defer tmpfile.Remove()
+	tmpdir := tempfile.NewTempDir(t, "dir-exists-test")
+	defer tmpdir.Remove()
+
+	var testcases = []struct {
+		doc      string
+		path     string
+		expected bool
+	}{
+		{
+			doc:      "directory exists",
+			path:     tmpdir.Path,
+			expected: true,
+		},
+		{
+			doc:      "path doesn't exist",
+			path:     "/bogus/path/does/not/exist",
+			expected: false,
+		},
+		{
+			doc:      "file exists",
+			path:     tmpfile.Name(),
+			expected: false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		result, err := isExistingDirectory(testcase.path)
+		if !assert.NoError(t, err) {
+			continue
+		}
+		assert.Equal(t, testcase.expected, result, testcase.doc)
+	}
+}

builder/dockerfile/copy_unix.go

@@ -1,3 +1,5 @@
+// +build !windows
+
 package dockerfile
 
 import (
@@ -7,20 +9,8 @@ import (
 	"github.com/docker/docker/pkg/idtools"
 )
 
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	switch {
-	case err == nil:
-		return true, nil
-	case os.IsNotExist(err):
-		return false, nil
-	}
-	return false, err
-}
-
-// TODO: review this
 func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
-	doChownDestination, err := chownDestinationRoot(destination)
+	skipChownRoot, err := isExistingDirectory(destination)
 	if err != nil {
 		return err
 	}
@@ -30,7 +20,7 @@ func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
 	return filepath.Walk(source, func(fullpath string, info os.FileInfo, err error) error {
 		// Do not alter the walk root iff. it existed before, as it doesn't fall under
 		// the domain of "things we should chown".
-		if !doChownDestination && (source == fullpath) {
+		if skipChownRoot && source == fullpath {
 			return nil
 		}
 
@@ -44,21 +34,3 @@ func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
 		return os.Lchown(fullpath, rootIDs.UID, rootIDs.GID)
 	})
 }
-
-// If the destination didn't already exist, or the destination isn't a
-// directory, then we should Lchown the destination. Otherwise, we shouldn't
-// Lchown the destination.
-func chownDestinationRoot(destination string) (bool, error) {
-	destExists, err := pathExists(destination)
-	if err != nil {
-		return false, err
-	}
-	destStat, err := os.Stat(destination)
-	if err != nil {
-		// This should *never* be reached, because the destination must've already
-		// been created while untar-ing the context.
-		return false, err
-	}
-
-	return !destExists || !destStat.IsDir(), nil
-}

builder/dockerfile/imagecontext.go

@@ -8,7 +8,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/remotecontext"
-	"github.com/docker/docker/layer"
+	dockerimage "github.com/docker/docker/image"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
@@ -92,6 +92,7 @@ type getAndMountFunc func(string) (builder.Image, builder.ReleaseableLayer, erro
 // all images so they can be unmounted at the end of the build.
 type imageSources struct {
 	byImageID map[string]*imageMount
+	withoutID []*imageMount
 	getImage  getAndMountFunc
 	cache     pathCache // TODO: remove
 }
@@ -121,7 +122,7 @@ func (m *imageSources) Get(idOrRef string) (*imageMount, error) {
 		return nil, err
 	}
 	im := newImageMount(image, layer)
-	m.byImageID[image.ImageID()] = im
+	m.Add(im)
 	return im, nil
 }
 
@@ -132,9 +133,25 @@ func (m *imageSources) Unmount() (retErr error) {
 			retErr = err
 		}
 	}
+	for _, im := range m.withoutID {
+		if err := im.unmount(); err != nil {
+			logrus.Error(err)
+			retErr = err
+		}
+	}
 	return
 }
 
+func (m *imageSources) Add(im *imageMount) {
+	switch im.image {
+	case nil:
+		im.image = &dockerimage.Image{}
+		m.withoutID = append(m.withoutID, im)
+	default:
+		m.byImageID[im.image.ImageID()] = im
+	}
+}
+
 // imageMount is a reference to an image that can be used as a builder.Source
 type imageMount struct {
 	image  builder.Image
@@ -172,6 +189,7 @@ func (im *imageMount) unmount() error {
 	if err := im.layer.Release(); err != nil {
 		return errors.Wrapf(err, "failed to unmount previous build image %s", im.image.ImageID())
 	}
+	im.layer = nil
 	return nil
 }
 
@@ -179,10 +197,10 @@ func (im *imageMount) Image() builder.Image {
 	return im.image
 }
 
-func (im *imageMount) ImageID() string {
-	return im.image.ImageID()
+func (im *imageMount) Layer() builder.ReleaseableLayer {
+	return im.layer
 }
 
-func (im *imageMount) DiffID() layer.DiffID {
-	return im.layer.DiffID()
+func (im *imageMount) ImageID() string {
+	return im.image.ImageID()
 }

builder/dockerfile/internals.go

@@ -12,7 +12,6 @@ import (
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
-	"github.com/docker/docker/builder"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
@@ -65,14 +64,44 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta
 	return nil
 }
 
-func (b *Builder) exportImage(state *dispatchState, image builder.Image) error {
-	config, err := image.MarshalJSON()
+func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runConfig *container.Config) error {
+	newLayer, err := imageMount.Layer().Commit()
+	if err != nil {
+		return err
+	}
+
+	// add an image mount without an image so the layer is properly unmounted
+	// if there is an error before we can add the full mount with image
+	b.imageSources.Add(newImageMount(nil, newLayer))
+
+	parentImage, ok := imageMount.Image().(*image.Image)
+	if !ok {
+		return errors.Errorf("unexpected image type")
+	}
+
+	newImage := image.NewChildImage(parentImage, image.ChildConfig{
+		Author:          state.maintainer,
+		ContainerConfig: runConfig,
+		DiffID:          newLayer.DiffID(),
+		Config:          copyRunConfig(state.runConfig),
+	})
+
+	// TODO: it seems strange to marshal this here instead of just passing in the
+	// image struct
+	config, err := newImage.MarshalJSON()
 	if err != nil {
 		return errors.Wrap(err, "failed to encode image config")
 	}
 
-	state.imageID, err = b.docker.CreateImage(config, state.imageID)
-	return err
+	exportedImage, err := b.docker.CreateImage(config, state.imageID)
+	if err != nil {
+		return errors.Wrapf(err, "failed to export image")
+	}
+
+	state.imageID = exportedImage.ImageID()
+	b.imageSources.Add(newImageMount(exportedImage, newLayer))
+	b.buildStages.update(state.imageID)
+	return nil
 }
 
 func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error {
@@ -82,46 +111,46 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 	runConfigWithCommentCmd := copyRunConfig(
 		state.runConfig,
 		withCmdCommentString(fmt.Sprintf("%s %s in %s ", inst.cmdName, srcHash, inst.dest)))
-	containerID, err := b.probeAndCreate(state, runConfigWithCommentCmd)
-	if err != nil || containerID == "" {
-		return err
-	}
-
-	// Twiddle the destination when it's a relative path - meaning, make it
-	// relative to the WORKINGDIR
-	dest, err := normaliseDest(inst.cmdName, state.runConfig.WorkingDir, inst.dest)
-	if err != nil {
+	hit, err := b.probeCache(state, runConfigWithCommentCmd)
+	if err != nil || hit {
 		return err
 	}
 
 	imageMount, err := b.imageSources.Get(state.imageID)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to get destination image %q", state.imageID)
 	}
-	destSource, err := imageMount.Source()
+	destInfo, err := createDestInfo(state.runConfig.WorkingDir, inst, imageMount)
 	if err != nil {
-		return errors.Wrapf(err, "failed to mount copy source")
+		return err
 	}
 
-	destInfo := newCopyInfoFromSource(destSource, dest, "")
 	opts := copyFileOptions{
 		decompress: inst.allowLocalDecompression,
 		archiver:   b.archiver,
 	}
 	for _, info := range inst.infos {
-		if err := copyFile(destInfo, info, opts); err != nil {
-			return err
+		if err := performCopyForInfo(destInfo, info, opts); err != nil {
+			return errors.Wrapf(err, "failed to copy files")
 		}
 	}
+	return b.exportImage(state, imageMount, runConfigWithCommentCmd)
+}
 
-	newImage := imageMount.Image().NewChild(image.ChildConfig{
-		Author:          state.maintainer,
-		DiffID:          imageMount.DiffID(),
-		ContainerConfig: runConfigWithCommentCmd,
-		// TODO: ContainerID?
-		// TODO: Config?
-	})
-	return b.exportImage(state, newImage)
+func createDestInfo(workingDir string, inst copyInstruction, imageMount *imageMount) (copyInfo, error) {
+	// Twiddle the destination when it's a relative path - meaning, make it
+	// relative to the WORKINGDIR
+	dest, err := normaliseDest(workingDir, inst.dest)
+	if err != nil {
+		return copyInfo{}, errors.Wrapf(err, "invalid %s", inst.cmdName)
+	}
+
+	destMount, err := imageMount.Source()
+	if err != nil {
+		return copyInfo{}, errors.Wrapf(err, "failed to mount copy source")
+	}
+
+	return newCopyInfoFromSource(destMount, dest, ""), nil
 }
 
 // For backwards compat, if there's just one info then use it as the

builder/dockerfile/internals_unix.go

@@ -12,7 +12,7 @@ import (
 
 // normaliseDest normalises the destination of a COPY/ADD command in a
 // platform semantically consistent way.
-func normaliseDest(cmdName, workingDir, requested string) (string, error) {
+func normaliseDest(workingDir, requested string) (string, error) {
 	dest := filepath.FromSlash(requested)
 	endsInSlash := strings.HasSuffix(requested, string(os.PathSeparator))
 	if !system.IsAbs(requested) {

builder/dockerfile/internals_windows.go

@@ -12,7 +12,7 @@ import (
 
 // normaliseDest normalises the destination of a COPY/ADD command in a
 // platform semantically consistent way.
-func normaliseDest(cmdName, workingDir, requested string) (string, error) {
+func normaliseDest(workingDir, requested string) (string, error) {
 	dest := filepath.FromSlash(requested)
 	endsInSlash := strings.HasSuffix(dest, string(os.PathSeparator))
 
@@ -32,7 +32,7 @@ func normaliseDest(cmdName, workingDir, requested string) (string, error) {
 	// we only want to validate where the DriveColon part has been supplied.
 	if filepath.IsAbs(dest) {
 		if strings.ToUpper(string(dest[0])) != "C" {
-			return "", fmt.Errorf("Windows does not support %s with a destinations not on the system drive (C:)", cmdName)
+			return "", fmt.Errorf("Windows does not support destinations not on the system drive (C:)")
 		}
 		dest = dest[2:] // Strip the drive letter
 	}
@@ -44,7 +44,7 @@ func normaliseDest(cmdName, workingDir, requested string) (string, error) {
 		}
 		if !system.IsAbs(dest) {
 			if string(workingDir[0]) != "C" {
-				return "", fmt.Errorf("Windows does not support %s with relative paths when WORKDIR is not the system drive", cmdName)
+				return "", fmt.Errorf("Windows does not support relative paths when WORKDIR is not the system drive")
 			}
 			dest = filepath.Join(string(os.PathSeparator), workingDir[2:], dest)
 			// Make sure we preserve any trailing slash

builder/dockerfile/internals_windows_test.go

@@ -2,16 +2,22 @@
 
 package dockerfile
 
-import "testing"
+import (
+	"fmt"
+	"testing"
+
+	"github.com/docker/docker/pkg/testutil"
+	"github.com/stretchr/testify/assert"
+)
 
 func TestNormaliseDest(t *testing.T) {
 	tests := []struct{ current, requested, expected, etext string }{
-		{``, `D:\`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
-		{``, `e:/`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
+		{``, `D:\`, ``, `Windows does not support destinations not on the system drive (C:)`},
+		{``, `e:/`, ``, `Windows does not support destinations not on the system drive (C:)`},
 		{`invalid`, `./c1`, ``, `Current WorkingDir invalid is not platform consistent`},
 		{`C:`, ``, ``, `Current WorkingDir C: is not platform consistent`},
 		{`C`, ``, ``, `Current WorkingDir C is not platform consistent`},
-		{`D:\`, `.`, ``, "Windows does not support TEST with relative paths when WORKDIR is not the system drive"},
+		{`D:\`, `.`, ``, "Windows does not support relative paths when WORKDIR is not the system drive"},
 		{``, `D`, `D`, ``},
 		{``, `./a1`, `.\a1`, ``},
 		{``, `.\b1`, `.\b1`, ``},
@@ -32,20 +38,16 @@ func TestNormaliseDest(t *testing.T) {
 		{`C:\wdm`, `foo/bar/`, `\wdm\foo\bar\`, ``},
 		{`C:\wdn`, `foo\bar/`, `\wdn\foo\bar\`, ``},
 	}
-	for _, i := range tests {
-		got, err := normaliseDest("TEST", i.current, i.requested)
-		if err != nil && i.etext == "" {
-			t.Fatalf("TestNormaliseDest Got unexpected error %q for %s %s. ", err.Error(), i.current, i.requested)
-		}
-		if i.etext != "" && ((err == nil) || (err != nil && err.Error() != i.etext)) {
-			if err == nil {
-				t.Fatalf("TestNormaliseDest Expected an error for %s %s but didn't get one", i.current, i.requested)
-			} else {
-				t.Fatalf("TestNormaliseDest Wrong error text for %s %s - %s", i.current, i.requested, err.Error())
+	for _, testcase := range tests {
+		msg := fmt.Sprintf("Input: %s, %s", testcase.current, testcase.requested)
+		actual, err := normaliseDest(testcase.current, testcase.requested)
+		if testcase.etext == "" {
+			if !assert.NoError(t, err, msg) {
+				continue
 			}
-		}
-		if i.etext == "" && got != i.expected {
-			t.Fatalf("TestNormaliseDest Expected %q for %q and %q. Got %q", i.expected, i.current, i.requested, got)
+			assert.Equal(t, testcase.expected, actual, msg)
+		} else {
+			testutil.ErrorContains(t, err, testcase.etext)
 		}
 	}
 }

builder/dockerfile/mockbackend_test.go

@@ -9,9 +9,7 @@ import (
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
 	containerpkg "github.com/docker/docker/container"
-	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
-	"github.com/docker/docker/pkg/idtools"
 	"golang.org/x/net/context"
 )
 
@@ -80,12 +78,8 @@ func (m *MockBackend) MakeImageCache(cacheFrom []string) builder.ImageCache {
 	return nil
 }
 
-func (m *MockBackend) CreateImage(config []byte, parent string) (string, error) {
-	return "c411d1d", nil
-}
-
-func (m *MockBackend) IDMappings() *idtools.IDMappings {
-	return &idtools.IDMappings{}
+func (m *MockBackend) CreateImage(config []byte, parent string) (builder.Image, error) {
+	return nil, nil
 }
 
 type mockImage struct {
@@ -106,10 +100,6 @@ func (i *mockImage) MarshalJSON() ([]byte, error) {
 	return json.Marshal(rawImage(*i))
 }
 
-func (i *mockImage) NewChild(child image.ChildConfig) *image.Image {
-	return nil
-}
-
 type mockImageCache struct {
 	getCacheFunc func(parentID string, cfg *container.Config) (string, error)
 }
@@ -131,6 +121,10 @@ func (l *mockLayer) Mount() (string, error) {
 	return "mountPath", nil
 }
 
+func (l *mockLayer) Commit() (builder.ReleaseableLayer, error) {
+	return nil, nil
+}
+
 func (l *mockLayer) DiffID() layer.DiffID {
-	return layer.DiffID("abcdef12345")
+	return layer.DiffID("abcdef")
 }

cmd/dockerd/daemon.go

@@ -449,7 +449,7 @@ func initRouter(s *apiserver.Server, d *daemon.Daemon, c *cluster.Cluster) {
 		image.NewRouter(d, decoder),
 		systemrouter.NewRouter(d, c),
 		volume.NewRouter(d),
-		build.NewRouter(buildbackend.NewBackend(d, d), d),
+		build.NewRouter(buildbackend.NewBackend(d, d, d.IDMappings()), d),
 		swarmrouter.NewRouter(c),
 		pluginrouter.NewRouter(d.PluginManager()),
 		distributionrouter.NewRouter(d),

daemon/archive.go

@@ -359,4 +359,4 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 	})
 	daemon.LogContainerEvent(container, "copy")
 	return reader, nil
-}
+}

daemon/build.go

@@ -1,6 +1,8 @@
 package daemon
 
 import (
+	"io"
+
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
@@ -13,7 +15,6 @@ import (
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
-	"io"
 )
 
 type releaseableLayer struct {
@@ -23,12 +24,14 @@ type releaseableLayer struct {
 }
 
 func (rl *releaseableLayer) Mount() (string, error) {
-	if rl.roLayer == nil {
-		return "", errors.New("can not mount an image with no root FS")
-	}
 	var err error
+	var chainID layer.ChainID
+	if rl.roLayer != nil {
+		chainID = rl.roLayer.ChainID()
+	}
+
 	mountID := stringid.GenerateRandomID()
-	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, rl.roLayer.ChainID(), nil)
+	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, chainID, nil)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to create rwlayer")
 	}
@@ -36,15 +39,41 @@ func (rl *releaseableLayer) Mount() (string, error) {
 	return rl.rwLayer.Mount("")
 }
 
-func (rl *releaseableLayer) Release() error {
-	rl.releaseRWLayer()
-	return rl.releaseROLayer()
+func (rl *releaseableLayer) Commit() (builder.ReleaseableLayer, error) {
+	var chainID layer.ChainID
+	if rl.roLayer != nil {
+		chainID = rl.roLayer.ChainID()
+	}
+
+	stream, err := rl.rwLayer.TarStream()
+	if err != nil {
+		return nil, err
+	}
+
+	newLayer, err := rl.layerStore.Register(stream, chainID)
+	if err != nil {
+		return nil, err
+	}
+
+	if layer.IsEmpty(newLayer.DiffID()) {
+		_, err := rl.layerStore.Release(newLayer)
+		return &releaseableLayer{layerStore: rl.layerStore}, err
+	}
+	return &releaseableLayer{layerStore: rl.layerStore, roLayer: newLayer}, nil
 }
 
 func (rl *releaseableLayer) DiffID() layer.DiffID {
+	if rl.roLayer == nil {
+		return layer.DigestSHA256EmptyTar
+	}
 	return rl.roLayer.DiffID()
 }
 
+func (rl *releaseableLayer) Release() error {
+	rl.releaseRWLayer()
+	return rl.releaseROLayer()
+}
+
 func (rl *releaseableLayer) releaseRWLayer() error {
 	if rl.rwLayer == nil {
 		return nil
@@ -67,8 +96,8 @@ func (rl *releaseableLayer) releaseROLayer() error {
 }
 
 func newReleasableLayerForImage(img *image.Image, layerStore layer.Store) (builder.ReleaseableLayer, error) {
-	if img.RootFS.ChainID() == "" {
-		return nil, nil
+	if img == nil || img.RootFS.ChainID() == "" {
+		return &releaseableLayer{layerStore: layerStore}, nil
 	}
 	// Hold a reference to the image layer so that it can't be removed before
 	// it is released
@@ -109,6 +138,11 @@ func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfi
 // Every call to GetImageAndReleasableLayer MUST call releasableLayer.Release() to prevent
 // leaking of layers.
 func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+	if refOrID == "" {
+		layer, err := newReleasableLayerForImage(nil, daemon.layerStore)
+		return nil, layer, err
+	}
+
 	if !opts.ForcePull {
 		image, _ := daemon.GetImage(refOrID)
 		// TODO: shouldn't we error out if error is different from "not found" ?
@@ -129,19 +163,19 @@ func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID st
 // CreateImage creates a new image by adding a config and ID to the image store.
 // This is similar to LoadImage() except that it receives JSON encoded bytes of
 // an image instead of a tar archive.
-func (daemon *Daemon) CreateImage(config []byte, parent string) (string, error) {
+func (daemon *Daemon) CreateImage(config []byte, parent string) (builder.Image, error) {
 	id, err := daemon.imageStore.Create(config)
 	if err != nil {
-		return "", err
+		return nil, errors.Wrapf(err, "failed to create image")
 	}
 
 	if parent != "" {
 		if err := daemon.imageStore.SetParent(id, image.ID(parent)); err != nil {
-			return "", err
+			return nil, errors.Wrapf(err, "failed to set parent %s", parent)
 		}
 	}
-	// TODO: do we need any daemon.LogContainerEventWithAttributes?
-	return id.String(), nil
+
+	return daemon.imageStore.Get(id)
 }
 
 // IDMappings returns uid/gid mappings for the builder

daemon/commit.go

@@ -188,7 +188,7 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str
 		Config:          newConfig,
 		DiffID:          l.DiffID(),
 	}
-	config, err := json.Marshal(parent.NewChild(cc))
+	config, err := json.Marshal(image.NewChildImage(parent, cc))
 	if err != nil {
 		return "", err
 	}

image/image.go

@@ -4,14 +4,14 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"runtime"
+	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/dockerversion"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
-	"runtime"
-	"strings"
 )
 
 // ID is the content-addressable ID of an image.
@@ -125,10 +125,13 @@ type ChildConfig struct {
 	Config          *container.Config
 }
 
-// NewChild creates a new Image as a child of this image.
-func (img *Image) NewChild(child ChildConfig) *Image {
+// NewChildImage creates a new Image as a child of this image.
+func NewChildImage(img *Image, child ChildConfig) *Image {
 	isEmptyLayer := layer.IsEmpty(child.DiffID)
 	rootFS := img.RootFS
+	if rootFS == nil {
+		rootFS = NewRootFS()
+	}
 	if !isEmptyLayer {
 		rootFS.Append(child.DiffID)
 	}

image/store.go

@@ -2,7 +2,6 @@ package image
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -10,6 +9,7 @@ import (
 	"github.com/docker/distribution/digestset"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
+	"github.com/pkg/errors"
 )
 
 // Store is an interface for creating and accessing images
@@ -147,7 +147,7 @@ func (is *store) Create(config []byte) (ID, error) {
 	if layerID != "" {
 		l, err = is.ls.Get(layerID)
 		if err != nil {
-			return "", err
+			return "", errors.Wrapf(err, "failed to get layer %s", layerID)
 		}
 	}
 

integration-cli/docker_cli_build_test.go

@@ -5870,15 +5870,17 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 	dockerfile := `
 		FROM busybox AS first
 		COPY foo bar
+
 		FROM busybox
-    %s
-    COPY baz baz
-    RUN echo mno > baz/cc
+		%s
+		COPY baz baz
+		RUN echo mno > baz/cc
+
 		FROM busybox
-    COPY bar /
-    COPY --from=1 baz sub/
-    COPY --from=0 bar baz
-    COPY --from=first bar bay`
+		COPY bar /
+		COPY --from=1 baz sub/
+		COPY --from=0 bar baz
+		COPY --from=first bar bay`
 
 	ctx := fakecontext.New(c, "",
 		fakecontext.WithDockerfile(fmt.Sprintf(dockerfile, "")),
@@ -5892,32 +5894,25 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 
 	cli.BuildCmd(c, "build1", build.WithExternalBuildContext(ctx))
 
-	out := cli.DockerCmd(c, "run", "build1", "cat", "bar").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "def")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "sub/aa").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "ghi")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "sub/cc").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "mno")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "baz").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "abc")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "bay").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "abc")
+	cli.DockerCmd(c, "run", "build1", "cat", "bar").Assert(c, icmd.Expected{Out: "def"})
+	cli.DockerCmd(c, "run", "build1", "cat", "sub/aa").Assert(c, icmd.Expected{Out: "ghi"})
+	cli.DockerCmd(c, "run", "build1", "cat", "sub/cc").Assert(c, icmd.Expected{Out: "mno"})
+	cli.DockerCmd(c, "run", "build1", "cat", "baz").Assert(c, icmd.Expected{Out: "abc"})
+	cli.DockerCmd(c, "run", "build1", "cat", "bay").Assert(c, icmd.Expected{Out: "abc"})
 
 	result := cli.BuildCmd(c, "build2", build.WithExternalBuildContext(ctx))
 
 	// all commands should be cached
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 7)
+	c.Assert(getIDByName(c, "build1"), checker.Equals, getIDByName(c, "build2"))
 
 	err := ioutil.WriteFile(filepath.Join(ctx.Dir, "Dockerfile"), []byte(fmt.Sprintf(dockerfile, "COPY baz/aa foo")), 0644)
 	c.Assert(err, checker.IsNil)
 
 	// changing file in parent block should not affect last block
 	result = cli.BuildCmd(c, "build3", build.WithExternalBuildContext(ctx))
-
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 5)
 
-	c.Assert(getIDByName(c, "build1"), checker.Equals, getIDByName(c, "build2"))
-
 	err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo"), []byte("pqr"), 0644)
 	c.Assert(err, checker.IsNil)
 
@@ -5925,10 +5920,8 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 	result = cli.BuildCmd(c, "build4", build.WithExternalBuildContext(ctx))
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 5)
 
-	out = cli.DockerCmd(c, "run", "build4", "cat", "bay").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "pqr")
-	out = cli.DockerCmd(c, "run", "build4", "cat", "baz").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "pqr")
+	cli.DockerCmd(c, "run", "build4", "cat", "bay").Assert(c, icmd.Expected{Out: "pqr"})
+	cli.DockerCmd(c, "run", "build4", "cat", "baz").Assert(c, icmd.Expected{Out: "pqr"})
 }
 
 func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) {

pkg/testutil/tempfile/tempfile.go

@@ -34,3 +34,23 @@ func (f *TempFile) Name() string {
 func (f *TempFile) Remove() {
 	os.Remove(f.Name())
 }
+
+// TempDir is a temporary directory that can be used with unit tests. TempDir
+// reduces the boilerplate setup required in each test case by handling
+// setup errors.
+type TempDir struct {
+	Path string
+}
+
+// NewTempDir returns a new temp file with contents
+func NewTempDir(t require.TestingT, prefix string) *TempDir {
+	path, err := ioutil.TempDir("", prefix+"-")
+	require.NoError(t, err)
+
+	return &TempDir{Path: path}
+}
+
+// Remove removes the file
+func (f *TempDir) Remove() {
+	os.Remove(f.Path)
+}