From 932c771fa135ac8b0ba5d612fd14d2a1e1189c81 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 14 May 2017 14:18:48 -0400 Subject: [PATCH 1/3] Remove CopyOnBuild from the daemon. Add CreateImage() to the daemon Refactor daemon.Comit() and expose a Image.NewChild() Update copy to use IDMappings. Signed-off-by: Daniel Nephin Upstream-commit: bd5f92d2631df7c932b93e72e45b39cba19f2f3b Component: engine --- components/engine/builder/builder.go | 14 ++- .../engine/builder/dockerfile/builder.go | 4 + components/engine/builder/dockerfile/copy.go | 57 ++++++++++ .../engine/builder/dockerfile/copy_unix.go | 64 +++++++++++ .../engine/builder/dockerfile/copy_windows.go | 8 ++ .../engine/builder/dockerfile/dispatchers.go | 11 +- .../engine/builder/dockerfile/imagecontext.go | 32 +++--- .../engine/builder/dockerfile/internals.go | 43 +++++++- .../builder/dockerfile/mockbackend_test.go | 25 +++++ components/engine/daemon/archive.go | 104 +----------------- components/engine/daemon/archive_unix.go | 35 ------ components/engine/daemon/archive_windows.go | 5 - components/engine/daemon/build.go | 28 +++++ components/engine/daemon/commit.go | 66 +++-------- components/engine/image/image.go | 58 ++++++++++ components/engine/layer/empty.go | 5 + 16 files changed, 334 insertions(+), 225 deletions(-) create mode 100644 components/engine/builder/dockerfile/copy_unix.go create mode 100644 components/engine/builder/dockerfile/copy_windows.go diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index 1aea99ea90..07a5500adb 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/builder/builder.go @@ -11,6 +11,9 @@ 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" ) @@ -42,11 +45,9 @@ type Backend interface { // ContainerCreateWorkdir creates the workdir ContainerCreateWorkdir(containerID string) error - // ContainerCopy copies/extracts a source FileInfo to a destination path inside a container - // specified by a container object. - // TODO: extract in the builder instead of passing `decompress` - // TODO: use containerd/fs.changestream instead as a source - CopyOnBuild(containerID string, destPath string, srcRoot string, srcPath string, decompress bool) error + CreateImage(config []byte, parent string) (string, error) + + IDMappings() *idtools.IDMappings ImageCacheBuilder } @@ -96,10 +97,13 @@ type ImageCache interface { 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) + DiffID() layer.DiffID } diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go index 52b540ab5d..c50570a6fd 100644 --- a/components/engine/builder/dockerfile/builder.go +++ b/components/engine/builder/dockerfile/builder.go @@ -15,6 +15,8 @@ import ( "github.com/docker/docker/builder/dockerfile/command" "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/builder/remotecontext" + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/streamformatter" "github.com/docker/docker/pkg/stringid" "github.com/pkg/errors" @@ -98,6 +100,7 @@ type Builder struct { docker builder.Backend clientCtx context.Context + archiver *archive.Archiver buildStages *buildStages disableCommit bool buildArgs *buildArgs @@ -121,6 +124,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()), 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 db98eb92e1..ace6b9878a 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -13,9 +13,12 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/builder/remotecontext" + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" + "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" "github.com/docker/docker/pkg/urlutil" "github.com/pkg/errors" @@ -34,6 +37,10 @@ type copyInfo struct { hash string } +func (c copyInfo) fullPath() (string, error) { + return symlink.FollowSymlinkInScope(filepath.Join(c.root, c.path), c.root) +} + func newCopyInfoFromSource(source builder.Source, path string, hash string) copyInfo { return copyInfo{root: source.Root(), path: path, hash: hash} } @@ -355,3 +362,53 @@ func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote b lc, err := remotecontext.NewLazyContext(tmpDir) return lc, filename, err } + +type copyFileOptions struct { + decompress bool + archiver *archive.Archiver +} + +func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error { + srcPath, err := source.fullPath() + if err != nil { + return err + } + destPath, err := dest.fullPath() + if err != nil { + return err + } + + archiver := options.archiver + rootIDs := archiver.IDMappings.RootPair() + + src, err := os.Stat(srcPath) + if err != nil { + return err // TODO: errors.Wrapf + } + if src.IsDir() { + if err := archiver.CopyWithTar(srcPath, destPath); err != nil { + return err + } + return fixPermissions(srcPath, destPath, rootIDs) + } + + 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) + } + + if err := idtools.MkdirAllAndChownNew(filepath.Dir(destPath), 0755, rootIDs); err != nil { + return err + } + if err := archiver.CopyFileWithTar(srcPath, destPath); err != nil { + return err + } + // TODO: do I have to change destPath to the filename? + return fixPermissions(srcPath, destPath, rootIDs) +} diff --git a/components/engine/builder/dockerfile/copy_unix.go b/components/engine/builder/dockerfile/copy_unix.go new file mode 100644 index 0000000000..ecbbd33368 --- /dev/null +++ b/components/engine/builder/dockerfile/copy_unix.go @@ -0,0 +1,64 @@ +package dockerfile + +import ( + "os" + "path/filepath" + + "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) + if err != nil { + return err + } + + // We Walk on the source rather than on the destination because we don't + // want to change permissions on things we haven't created or modified. + 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) { + return nil + } + + // Path is prefixed by source: substitute with destination instead. + cleaned, err := filepath.Rel(source, fullpath) + if err != nil { + return err + } + + fullpath = filepath.Join(destination, cleaned) + 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/copy_windows.go b/components/engine/builder/dockerfile/copy_windows.go new file mode 100644 index 0000000000..78f5b09457 --- /dev/null +++ b/components/engine/builder/dockerfile/copy_windows.go @@ -0,0 +1,8 @@ +package dockerfile + +import "github.com/docker/docker/pkg/idtools" + +func fixPermissions(source, destination string, rootIDs idtools.IDPair) error { + // chown is not supported on Windows + return nil +} diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index ca67eb7f30..1935ac85c5 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -23,6 +23,7 @@ import ( "github.com/docker/docker/api/types/strslice" "github.com/docker/docker/builder" "github.com/docker/docker/builder/dockerfile/parser" + "github.com/docker/docker/image" "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" @@ -251,10 +252,8 @@ func parseBuildStageName(args []string) (string, error) { return stageName, nil } -// scratchImage is used as a token for the empty base image. It uses buildStage -// as a convenient implementation of builder.Image, but is not actually a -// buildStage. -var scratchImage builder.Image = &buildStage{} +// scratchImage is used as a token for the empty base image. +var scratchImage builder.Image = &image.Image{} func (b *Builder) getFromImage(shlex *ShellLex, name string) (builder.Image, error) { substitutionArgs := []string{} @@ -267,8 +266,8 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (builder.Image, err return nil, err } - if im, ok := b.buildStages.getByName(name); ok { - return im, nil + if stage, ok := b.buildStages.getByName(name); ok { + name = stage.ImageID() } // Windows cannot support a container with no base image. diff --git a/components/engine/builder/dockerfile/imagecontext.go b/components/engine/builder/dockerfile/imagecontext.go index f89cc8d9ce..5aba2ff936 100644 --- a/components/engine/builder/dockerfile/imagecontext.go +++ b/components/engine/builder/dockerfile/imagecontext.go @@ -6,37 +6,29 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/docker/api/types/backend" - "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" "github.com/docker/docker/builder/remotecontext" + "github.com/docker/docker/layer" "github.com/pkg/errors" "golang.org/x/net/context" ) type buildStage struct { - id string - config *container.Config + id string } -func newBuildStageFromImage(image builder.Image) *buildStage { - return &buildStage{id: image.ImageID(), config: image.RunConfig()} +func newBuildStage(imageID string) *buildStage { + return &buildStage{id: imageID} } func (b *buildStage) ImageID() string { return b.id } -func (b *buildStage) RunConfig() *container.Config { - return b.config -} - -func (b *buildStage) update(imageID string, runConfig *container.Config) { +func (b *buildStage) update(imageID string) { b.id = imageID - b.config = runConfig } -var _ builder.Image = &buildStage{} - // buildStages tracks each stage of a build so they can be retrieved by index // or by name. type buildStages struct { @@ -48,12 +40,12 @@ func newBuildStages() *buildStages { return &buildStages{byName: make(map[string]*buildStage)} } -func (s *buildStages) getByName(name string) (builder.Image, bool) { +func (s *buildStages) getByName(name string) (*buildStage, bool) { stage, ok := s.byName[strings.ToLower(name)] return stage, ok } -func (s *buildStages) get(indexOrName string) (builder.Image, error) { +func (s *buildStages) get(indexOrName string) (*buildStage, error) { index, err := strconv.Atoi(indexOrName) if err == nil { if err := s.validateIndex(index); err != nil { @@ -78,7 +70,7 @@ func (s *buildStages) validateIndex(i int) error { } func (s *buildStages) add(name string, image builder.Image) error { - stage := newBuildStageFromImage(image) + stage := newBuildStage(image.ImageID()) name = strings.ToLower(name) if len(name) > 0 { if _, ok := s.byName[name]; ok { @@ -90,8 +82,8 @@ func (s *buildStages) add(name string, image builder.Image) error { return nil } -func (s *buildStages) update(imageID string, runConfig *container.Config) { - s.sequence[len(s.sequence)-1].update(imageID, runConfig) +func (s *buildStages) update(imageID string) { + s.sequence[len(s.sequence)-1].update(imageID) } type getAndMountFunc func(string) (builder.Image, builder.ReleaseableLayer, error) @@ -190,3 +182,7 @@ func (im *imageMount) Image() builder.Image { 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 2d02e57a69..ef64455ffb 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -12,6 +12,8 @@ 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" ) @@ -37,7 +39,6 @@ func (b *Builder) commit(dispatchState *dispatchState, comment string) error { return b.commitContainer(dispatchState, id, runConfigWithCommentCmd) } -// TODO: see if any args can be dropped func (b *Builder) commitContainer(dispatchState *dispatchState, id string, containerConfig *container.Config) error { if b.disableCommit { return nil @@ -60,10 +61,20 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta } dispatchState.imageID = imageID - b.buildStages.update(imageID, dispatchState.runConfig) + b.buildStages.update(imageID) return nil } +func (b *Builder) exportImage(state *dispatchState, image builder.Image) error { + config, err := image.MarshalJSON() + if err != nil { + return errors.Wrap(err, "failed to encode image config") + } + + state.imageID, err = b.docker.CreateImage(config, state.imageID) + return err +} + func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error { srcHash := getSourceHashFromInfos(inst.infos) @@ -83,12 +94,34 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error return err } + imageMount, err := b.imageSources.Get(state.imageID) + if err != nil { + return err + } + destSource, err := imageMount.Source() + if err != nil { + return err + } + + destInfo := newCopyInfoFromSource(destSource, dest, "") + opts := copyFileOptions{ + decompress: inst.allowLocalDecompression, + archiver: b.archiver, + } for _, info := range inst.infos { - if err := b.docker.CopyOnBuild(containerID, dest, info.root, info.path, inst.allowLocalDecompression); err != nil { + if err := copyFile(destInfo, info, opts); err != nil { return err } } - return b.commitContainer(state, containerID, runConfigWithCommentCmd) + + newImage := imageMount.Image().NewChild(image.ChildConfig{ + Author: state.maintainer, + DiffID: imageMount.DiffID(), + ContainerConfig: runConfigWithCommentCmd, + // TODO: ContainerID? + // TODO: Config? + }) + return b.exportImage(state, newImage) } // For backwards compat, if there's just one info then use it as the @@ -182,7 +215,7 @@ func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container. fmt.Fprint(b.Stdout, " ---> Using cache\n") dispatchState.imageID = string(cachedID) - b.buildStages.update(dispatchState.imageID, runConfig) + b.buildStages.update(dispatchState.imageID) return true, nil } diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index 08ce18c2e8..cf4c83edd8 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/builder/dockerfile/mockbackend_test.go @@ -1,6 +1,7 @@ package dockerfile import ( + "encoding/json" "io" "github.com/docker/docker/api/types" @@ -8,6 +9,9 @@ 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" ) @@ -76,6 +80,14 @@ 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{} +} + type mockImage struct { id string config *container.Config @@ -89,6 +101,15 @@ func (i *mockImage) RunConfig() *container.Config { return i.config } +func (i *mockImage) MarshalJSON() ([]byte, error) { + type rawImage mockImage + 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) } @@ -109,3 +130,7 @@ func (l *mockLayer) Release() error { func (l *mockLayer) Mount() (string, error) { return "mountPath", nil } + +func (l *mockLayer) DiffID() layer.DiffID { + return layer.DiffID("abcdef12345") +} diff --git a/components/engine/daemon/archive.go b/components/engine/daemon/archive.go index 3fb75bbb22..5db1ef9826 100644 --- a/components/engine/daemon/archive.go +++ b/components/engine/daemon/archive.go @@ -10,9 +10,7 @@ import ( "github.com/docker/docker/container" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/chrootarchive" - "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" "github.com/pkg/errors" ) @@ -361,104 +359,4 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str }) daemon.LogContainerEvent(container, "copy") return reader, nil -} - -// CopyOnBuild copies/extracts a source FileInfo to a destination path inside a container -// specified by a container object. -// TODO: make sure callers don't unnecessarily convert destPath with filepath.FromSlash (Copy does it already). -// CopyOnBuild should take in abstract paths (with slashes) and the implementation should convert it to OS-specific paths. -func (daemon *Daemon) CopyOnBuild(cID, destPath, srcRoot, srcPath string, decompress bool) error { - fullSrcPath, err := symlink.FollowSymlinkInScope(filepath.Join(srcRoot, srcPath), srcRoot) - if err != nil { - return err - } - - destExists := true - destDir := false - rootIDs := daemon.idMappings.RootPair() - - // Work in daemon-local OS specific file paths - destPath = filepath.FromSlash(destPath) - - c, err := daemon.GetContainer(cID) - if err != nil { - return err - } - err = daemon.Mount(c) - if err != nil { - return err - } - defer daemon.Unmount(c) - - dest, err := c.GetResourcePath(destPath) - if err != nil { - return err - } - - // Preserve the trailing slash - // TODO: why are we appending another path separator if there was already one? - if strings.HasSuffix(destPath, string(os.PathSeparator)) || destPath == "." { - destDir = true - dest += string(os.PathSeparator) - } - - destPath = dest - - destStat, err := os.Stat(destPath) - if err != nil { - if !os.IsNotExist(err) { - //logrus.Errorf("Error performing os.Stat on %s. %s", destPath, err) - return err - } - destExists = false - } - - archiver := chrootarchive.NewArchiver(daemon.idMappings) - src, err := os.Stat(fullSrcPath) - if err != nil { - return err - } - - if src.IsDir() { - // copy as directory - if err := archiver.CopyWithTar(fullSrcPath, destPath); err != nil { - return err - } - return fixPermissions(fullSrcPath, destPath, rootIDs.UID, rootIDs.GID, destExists) - } - if decompress && archive.IsArchivePath(fullSrcPath) { - // Only try to untar if it is a file and that we've been told to decompress (when ADD-ing a remote file) - - // First try to unpack the source as an archive - // to support the untar feature we need to clean up the path a little bit - // because tar is very forgiving. First we need to strip off the archive's - // filename from the path but this is only added if it does not end in slash - tarDest := destPath - if strings.HasSuffix(tarDest, string(os.PathSeparator)) { - tarDest = filepath.Dir(destPath) - } - - // try to successfully untar the orig - err := archiver.UntarPath(fullSrcPath, tarDest) - /* - if err != nil { - logrus.Errorf("Couldn't untar to %s: %v", tarDest, err) - } - */ - return err - } - - // only needed for fixPermissions, but might as well put it before CopyFileWithTar - if destDir || (destExists && destStat.IsDir()) { - destPath = filepath.Join(destPath, filepath.Base(srcPath)) - } - - if err := idtools.MkdirAllAndChownNew(filepath.Dir(destPath), 0755, rootIDs); err != nil { - return err - } - if err := archiver.CopyFileWithTar(fullSrcPath, destPath); err != nil { - return err - } - - return fixPermissions(fullSrcPath, destPath, rootIDs.UID, rootIDs.GID, destExists) -} +} \ No newline at end of file diff --git a/components/engine/daemon/archive_unix.go b/components/engine/daemon/archive_unix.go index 8806e2e198..d5dfad78cb 100644 --- a/components/engine/daemon/archive_unix.go +++ b/components/engine/daemon/archive_unix.go @@ -3,9 +3,6 @@ package daemon import ( - "os" - "path/filepath" - "github.com/docker/docker/container" ) @@ -25,38 +22,6 @@ func checkIfPathIsInAVolume(container *container.Container, absPath string) (boo return toVolume, nil } -func fixPermissions(source, destination string, uid, gid int, destExisted bool) error { - // 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. - 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 err - } - doChownDestination := !destExisted || !destStat.IsDir() - - // We Walk on the source rather than on the destination because we don't - // want to change permissions on things we haven't created or modified. - 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) { - return nil - } - - // Path is prefixed by source: substitute with destination instead. - cleaned, err := filepath.Rel(source, fullpath) - if err != nil { - return err - } - - fullpath = filepath.Join(destination, cleaned) - return os.Lchown(fullpath, uid, gid) - }) -} - // isOnlineFSOperationPermitted returns an error if an online filesystem operation // is not permitted. func (daemon *Daemon) isOnlineFSOperationPermitted(container *container.Container) error { diff --git a/components/engine/daemon/archive_windows.go b/components/engine/daemon/archive_windows.go index 1c5a894f7c..ab105607d5 100644 --- a/components/engine/daemon/archive_windows.go +++ b/components/engine/daemon/archive_windows.go @@ -17,11 +17,6 @@ func checkIfPathIsInAVolume(container *container.Container, absPath string) (boo return false, nil } -func fixPermissions(source, destination string, uid, gid int, destExisted bool) error { - // chown is not supported on Windows - return nil -} - // isOnlineFSOperationPermitted returns an error if an online filesystem operation // is not permitted (such as stat or for copying). Running Hyper-V containers // cannot have their file-system interrogated from the host as the filter is diff --git a/components/engine/daemon/build.go b/components/engine/daemon/build.go index 1bf05ded2c..3005ede7d1 100644 --- a/components/engine/daemon/build.go +++ b/components/engine/daemon/build.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/image" "github.com/docker/docker/layer" + "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/registry" "github.com/pkg/errors" @@ -40,6 +41,10 @@ func (rl *releaseableLayer) Release() error { return rl.releaseROLayer() } +func (rl *releaseableLayer) DiffID() layer.DiffID { + return rl.roLayer.DiffID() +} + func (rl *releaseableLayer) releaseRWLayer() error { if rl.rwLayer == nil { return nil @@ -120,3 +125,26 @@ func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID st layer, err := newReleasableLayerForImage(image, daemon.layerStore) return image, layer, err } + +// 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) { + id, err := daemon.imageStore.Create(config) + if err != nil { + return "", err + } + + if parent != "" { + if err := daemon.imageStore.SetParent(id, image.ID(parent)); err != nil { + return "", err + } + } + // TODO: do we need any daemon.LogContainerEventWithAttributes? + return id.String(), nil +} + +// IDMappings returns uid/gid mappings for the builder +func (daemon *Daemon) IDMappings() *idtools.IDMappings { + return daemon.idMappings +} diff --git a/components/engine/daemon/commit.go b/components/engine/daemon/commit.go index e64c7d3333..8125d28f65 100644 --- a/components/engine/daemon/commit.go +++ b/components/engine/daemon/commit.go @@ -12,7 +12,6 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder/dockerfile" "github.com/docker/docker/container" - "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/docker/docker/pkg/ioutils" @@ -129,11 +128,6 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str return "", err } - containerConfig := c.ContainerConfig - if containerConfig == nil { - containerConfig = container.Config - } - // It is not possible to commit a running container on Windows and on Solaris. if (runtime.GOOS == "windows" || runtime.GOOS == "solaris") && container.IsRunning() { return "", errors.Errorf("%+v does not support commit of a running container", runtime.GOOS) @@ -165,60 +159,36 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str } }() - var history []image.History - rootFS := image.NewRootFS() - osVersion := "" - var osFeatures []string - - if container.ImageID != "" { - img, err := daemon.imageStore.Get(container.ImageID) + var parent *image.Image + if container.ImageID == "" { + parent = new(image.Image) + parent.RootFS = image.NewRootFS() + } else { + parent, err = daemon.imageStore.Get(container.ImageID) if err != nil { return "", err } - history = img.History - rootFS = img.RootFS - osVersion = img.OSVersion - osFeatures = img.OSFeatures } - l, err := daemon.layerStore.Register(rwTar, rootFS.ChainID()) + l, err := daemon.layerStore.Register(rwTar, parent.RootFS.ChainID()) if err != nil { return "", err } defer layer.ReleaseAndLog(daemon.layerStore, l) - h := image.History{ - Author: c.Author, - Created: time.Now().UTC(), - CreatedBy: strings.Join(containerConfig.Cmd, " "), - Comment: c.Comment, - EmptyLayer: true, + containerConfig := c.ContainerConfig + if containerConfig == nil { + containerConfig = container.Config } - - if diffID := l.DiffID(); layer.DigestSHA256EmptyTar != diffID { - h.EmptyLayer = false - rootFS.Append(diffID) + cc := image.ChildConfig{ + ContainerID: container.ID, + Author: c.Author, + Comment: c.Comment, + ContainerConfig: containerConfig, + Config: newConfig, + DiffID: l.DiffID(), } - - history = append(history, h) - - config, err := json.Marshal(&image.Image{ - V1Image: image.V1Image{ - DockerVersion: dockerversion.Version, - Config: newConfig, - Architecture: runtime.GOARCH, - OS: runtime.GOOS, - Container: container.ID, - ContainerConfig: *containerConfig, - Author: c.Author, - Created: h.Created, - }, - RootFS: rootFS, - History: history, - OSFeatures: osFeatures, - OSVersion: osVersion, - }) - + config, err := json.Marshal(parent.NewChild(cc)) if err != nil { return "", err } diff --git a/components/engine/image/image.go b/components/engine/image/image.go index 17935ac234..20650a8429 100644 --- a/components/engine/image/image.go +++ b/components/engine/image/image.go @@ -7,7 +7,11 @@ import ( "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. @@ -110,6 +114,48 @@ func (img *Image) MarshalJSON() ([]byte, error) { return json.Marshal(c) } +// ChildConfig is the configuration to apply to an Image to create a new +// Child image. Other properties of the image are copied from the parent. +type ChildConfig struct { + ContainerID string + Author string + Comment string + DiffID layer.DiffID + ContainerConfig *container.Config + Config *container.Config +} + +// NewChild creates a new Image as a child of this image. +func (img *Image) NewChild(child ChildConfig) *Image { + isEmptyLayer := layer.IsEmpty(child.DiffID) + rootFS := img.RootFS + if !isEmptyLayer { + rootFS.Append(child.DiffID) + } + imgHistory := NewHistory( + child.Author, + child.Comment, + strings.Join(child.ContainerConfig.Cmd, " "), + isEmptyLayer) + + return &Image{ + V1Image: V1Image{ + DockerVersion: dockerversion.Version, + Config: child.Config, + Architecture: runtime.GOARCH, + OS: runtime.GOOS, + Container: child.ContainerID, + ContainerConfig: *child.ContainerConfig, + Author: child.Author, + Created: imgHistory.Created, + }, + RootFS: rootFS, + History: append(img.History, imgHistory), + OSFeatures: img.OSFeatures, + OSVersion: img.OSVersion, + } +} + // History stores build commands that were used to create an image type History struct { // Created is the timestamp at which the image was created @@ -126,6 +172,18 @@ type History struct { EmptyLayer bool `json:"empty_layer,omitempty"` } +// NewHistory creates a new history struct from arguments, and sets the created +// time to the current time in UTC +func NewHistory(author, comment, createdBy string, isEmptyLayer bool) History { + return History{ + Author: author, + Created: time.Now().UTC(), + CreatedBy: createdBy, + Comment: comment, + EmptyLayer: isEmptyLayer, + } +} + // Exporter provides interface for loading and saving images type Exporter interface { Load(io.ReadCloser, io.Writer, bool) error diff --git a/components/engine/layer/empty.go b/components/engine/layer/empty.go index 3b6ffc82f7..80f2c12439 100644 --- a/components/engine/layer/empty.go +++ b/components/engine/layer/empty.go @@ -54,3 +54,8 @@ func (el *emptyLayer) DiffSize() (size int64, err error) { func (el *emptyLayer) Metadata() (map[string]string, error) { return make(map[string]string), nil } + +// IsEmpty returns true if the layer is an EmptyLayer +func IsEmpty(diffID DiffID) bool { + return diffID == DigestSHA256EmptyTar +} From 7be28755e37c371353fe1928e21b08544d357e0a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 May 2017 16:02:29 -0400 Subject: [PATCH 2/3] Rename LazyContext to LazySource. Signed-off-by: Daniel Nephin Upstream-commit: ecd44d23cc3c863c20467377cf568b920e2dd89c Component: engine --- components/engine/builder/dockerfile/copy.go | 6 +++--- .../engine/builder/dockerfile/imagecontext.go | 2 +- .../engine/builder/dockerfile/internals.go | 2 +- .../engine/builder/remotecontext/detect.go | 2 +- .../engine/builder/remotecontext/lazycontext.go | 16 ++++++++-------- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index ace6b9878a..268036e26c 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -155,7 +155,7 @@ func (o *copier) calcCopyInfo(origPath string, allowWildcards bool) ([]copyInfo, var err error o.source, err = imageSource.Source() if err != nil { - return nil, errors.Wrapf(err, "failed to copy") + return nil, errors.Wrapf(err, "failed to copy from %s", imageSource.ImageID()) } } @@ -359,7 +359,7 @@ func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote b return } - lc, err := remotecontext.NewLazyContext(tmpDir) + lc, err := remotecontext.NewLazySource(tmpDir) return lc, filename, err } @@ -383,7 +383,7 @@ func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error { src, err := os.Stat(srcPath) if err != nil { - return err // TODO: errors.Wrapf + return errors.Wrapf(err, "source path not found") } if src.IsDir() { if err := archiver.CopyWithTar(srcPath, destPath); err != nil { diff --git a/components/engine/builder/dockerfile/imagecontext.go b/components/engine/builder/dockerfile/imagecontext.go index 5aba2ff936..4b47862ce6 100644 --- a/components/engine/builder/dockerfile/imagecontext.go +++ b/components/engine/builder/dockerfile/imagecontext.go @@ -156,7 +156,7 @@ func (im *imageMount) Source() (builder.Source, error) { if err != nil { return nil, errors.Wrapf(err, "failed to mount %s", im.image.ImageID()) } - source, err := remotecontext.NewLazyContext(mountPath) + source, err := remotecontext.NewLazySource(mountPath) if err != nil { return nil, errors.Wrapf(err, "failed to create lazycontext for %s", mountPath) } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index ef64455ffb..095b6069c4 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -100,7 +100,7 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error } destSource, err := imageMount.Source() if err != nil { - return err + return errors.Wrapf(err, "failed to mount copy source") } destInfo := newCopyInfoFromSource(destSource, dest, "") diff --git a/components/engine/builder/remotecontext/detect.go b/components/engine/builder/remotecontext/detect.go index efac27f7ff..99385698cf 100644 --- a/components/engine/builder/remotecontext/detect.go +++ b/components/engine/builder/remotecontext/detect.go @@ -81,7 +81,7 @@ func withDockerfileFromContext(c modifiableContext, dockerfilePath string) (buil } func newGitRemote(gitURL string, dockerfilePath string) (builder.Source, *parser.Result, error) { - c, err := MakeGitContext(gitURL) // TODO: change this to NewLazyContext + c, err := MakeGitContext(gitURL) // TODO: change this to NewLazySource if err != nil { return nil, nil, err } diff --git a/components/engine/builder/remotecontext/lazycontext.go b/components/engine/builder/remotecontext/lazycontext.go index 283d82a7b1..41351a3583 100644 --- a/components/engine/builder/remotecontext/lazycontext.go +++ b/components/engine/builder/remotecontext/lazycontext.go @@ -12,30 +12,30 @@ import ( "github.com/pkg/errors" ) -// NewLazyContext creates a new LazyContext. LazyContext defines a hashed build +// NewLazySource creates a new LazyContext. LazyContext defines a hashed build // context based on a root directory. Individual files are hashed first time // they are asked. It is not safe to call methods of LazyContext concurrently. -func NewLazyContext(root string) (builder.Source, error) { - return &lazyContext{ +func NewLazySource(root string) (builder.Source, error) { + return &lazySource{ root: root, sums: make(map[string]string), }, nil } -type lazyContext struct { +type lazySource struct { root string sums map[string]string } -func (c *lazyContext) Root() string { +func (c *lazySource) Root() string { return c.root } -func (c *lazyContext) Close() error { +func (c *lazySource) Close() error { return nil } -func (c *lazyContext) Hash(path string) (string, error) { +func (c *lazySource) Hash(path string) (string, error) { cleanPath, fullPath, err := normalize(path, c.root) if err != nil { return "", err @@ -62,7 +62,7 @@ func (c *lazyContext) Hash(path string) (string, error) { return sum, nil } -func (c *lazyContext) prepareHash(relPath string, fi os.FileInfo) (string, error) { +func (c *lazySource) prepareHash(relPath string, fi os.FileInfo) (string, error) { p := filepath.Join(c.root, relPath) h, err := NewFileHash(p, relPath, fi) if err != nil { From 13d2adae3f0f304448a1e46a11088a21202e475b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 May 2017 17:03:29 -0400 Subject: [PATCH 3/3] 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 Upstream-commit: 51360965206b0db49cc0365dabb590063a17a9df Component: engine --- .../api/server/backend/build/backend.go | 5 +- components/engine/builder/builder.go | 8 +- .../engine/builder/dockerfile/builder.go | 9 +- components/engine/builder/dockerfile/copy.go | 67 +++++++++----- .../engine/builder/dockerfile/copy_test.go | 45 ++++++++++ .../engine/builder/dockerfile/copy_unix.go | 36 +------- .../engine/builder/dockerfile/imagecontext.go | 30 +++++-- .../engine/builder/dockerfile/internals.go | 87 ++++++++++++------- .../builder/dockerfile/internals_unix.go | 2 +- .../builder/dockerfile/internals_windows.go | 6 +- .../dockerfile/internals_windows_test.go | 36 ++++---- .../builder/dockerfile/mockbackend_test.go | 20 ++--- components/engine/cmd/dockerd/daemon.go | 2 +- components/engine/daemon/archive.go | 2 +- components/engine/daemon/build.go | 64 ++++++++++---- components/engine/daemon/commit.go | 2 +- components/engine/image/image.go | 11 ++- components/engine/image/store.go | 4 +- .../integration-cli/docker_cli_build_test.go | 41 ++++----- .../engine/pkg/testutil/tempfile/tempfile.go | 20 +++++ 20 files changed, 318 insertions(+), 179 deletions(-) create mode 100644 components/engine/builder/dockerfile/copy_test.go 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) +}