diff --git a/components/engine/api/server/backend/build/backend.go b/components/engine/api/server/backend/build/backend.go index bf171441ca..00aa7c3126 100644 --- a/components/engine/api/server/backend/build/backend.go +++ b/components/engine/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} } diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index 07a5500adb..5cd6c838f9 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/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 } diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go index c50570a6fd..d35ed9b91a 100644 --- a/components/engine/builder/dockerfile/builder.go +++ b/components/engine/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), diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index 268036e26c..0026bdc158 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/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)) } - // TODO: do I have to change destPath to the filename? - return fixPermissions(srcPath, destPath, rootIDs) + 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 + } + return destStat.IsDir(), nil } diff --git a/components/engine/builder/dockerfile/copy_test.go b/components/engine/builder/dockerfile/copy_test.go new file mode 100644 index 0000000000..aee225b5ff --- /dev/null +++ b/components/engine/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) + } +} diff --git a/components/engine/builder/dockerfile/copy_unix.go b/components/engine/builder/dockerfile/copy_unix.go index ecbbd33368..326d95bb39 100644 --- a/components/engine/builder/dockerfile/copy_unix.go +++ b/components/engine/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 -} diff --git a/components/engine/builder/dockerfile/imagecontext.go b/components/engine/builder/dockerfile/imagecontext.go index 4b47862ce6..2a5b6e2579 100644 --- a/components/engine/builder/dockerfile/imagecontext.go +++ b/components/engine/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) Layer() builder.ReleaseableLayer { + return im.layer +} + func (im *imageMount) ImageID() string { return im.image.ImageID() } - -func (im *imageMount) DiffID() layer.DiffID { - return im.layer.DiffID() -} diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 095b6069c4..213e64a52e 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/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 errors.Wrapf(err, "failed to get destination image %q", state.imageID) + } + destInfo, err := createDestInfo(state.runConfig.WorkingDir, inst, imageMount) if err != nil { return err } - destSource, err := imageMount.Source() - if err != nil { - return errors.Wrapf(err, "failed to mount copy source") - } - 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 diff --git a/components/engine/builder/dockerfile/internals_unix.go b/components/engine/builder/dockerfile/internals_unix.go index 17102cb443..f4784e1cca 100644 --- a/components/engine/builder/dockerfile/internals_unix.go +++ b/components/engine/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) { diff --git a/components/engine/builder/dockerfile/internals_windows.go b/components/engine/builder/dockerfile/internals_windows.go index 81a0386251..bb3285925f 100644 --- a/components/engine/builder/dockerfile/internals_windows.go +++ b/components/engine/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 diff --git a/components/engine/builder/dockerfile/internals_windows_test.go b/components/engine/builder/dockerfile/internals_windows_test.go index 868a6671a3..b4c8d4b3cb 100644 --- a/components/engine/builder/dockerfile/internals_windows_test.go +++ b/components/engine/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) } } } diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index cf4c83edd8..876bb91f1b 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/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") } diff --git a/components/engine/cmd/dockerd/daemon.go b/components/engine/cmd/dockerd/daemon.go index 428a877dc9..d5c411f06d 100644 --- a/components/engine/cmd/dockerd/daemon.go +++ b/components/engine/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), diff --git a/components/engine/daemon/archive.go b/components/engine/daemon/archive.go index 5db1ef9826..bd00daca53 100644 --- a/components/engine/daemon/archive.go +++ b/components/engine/daemon/archive.go @@ -359,4 +359,4 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str }) daemon.LogContainerEvent(container, "copy") return reader, nil -} \ No newline at end of file +} diff --git a/components/engine/daemon/build.go b/components/engine/daemon/build.go index 3005ede7d1..3faa2affd4 100644 --- a/components/engine/daemon/build.go +++ b/components/engine/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 diff --git a/components/engine/daemon/commit.go b/components/engine/daemon/commit.go index 8125d28f65..6011628092 100644 --- a/components/engine/daemon/commit.go +++ b/components/engine/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 } diff --git a/components/engine/image/image.go b/components/engine/image/image.go index 20650a8429..9f892d298a 100644 --- a/components/engine/image/image.go +++ b/components/engine/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) } diff --git a/components/engine/image/store.go b/components/engine/image/store.go index 26ae109a06..17a3b7880a 100644 --- a/components/engine/image/store.go +++ b/components/engine/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) } } diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 482de38b6c..b9a5dff789 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/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) { diff --git a/components/engine/pkg/testutil/tempfile/tempfile.go b/components/engine/pkg/testutil/tempfile/tempfile.go index b375cb9561..01474babff 100644 --- a/components/engine/pkg/testutil/tempfile/tempfile.go +++ b/components/engine/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) +}