From b3bc7b28d09138a37ab5476eb46dfe74f8984f18 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 27 Mar 2017 18:36:28 -0700 Subject: [PATCH 1/3] Expose a smaller interface for the Builder retrieving images from daemon Removes 3 methods from the builder.Backend interface Remove the coupling between imageContexts, imageMounts and the builder. Signed-off-by: Daniel Nephin --- api/types/backend/build.go | 7 ++ builder/builder.go | 27 +++---- builder/dockerfile/builder.go | 7 +- builder/dockerfile/dispatchers.go | 83 +++++++++---------- builder/dockerfile/dispatchers_test.go | 25 +++--- builder/dockerfile/imagecontext.go | 75 ++++++++--------- builder/dockerfile/internals.go | 2 +- builder/dockerfile/mockbackend_test.go | 39 +++++---- daemon/archive.go | 33 -------- daemon/build.go | 108 +++++++++++++++++++++++++ daemon/image.go | 10 --- daemon/image_pull.go | 30 ------- 12 files changed, 238 insertions(+), 208 deletions(-) create mode 100644 daemon/build.go diff --git a/api/types/backend/build.go b/api/types/backend/build.go index 01fcbd1a73..ad19c9a1d0 100644 --- a/api/types/backend/build.go +++ b/api/types/backend/build.go @@ -22,3 +22,10 @@ type BuildConfig struct { ProgressWriter ProgressWriter Options *types.ImageBuildOptions } + +// GetImageAndLayerOptions are the options supported by GetImageAndLayer +type GetImageAndLayerOptions struct { + ForcePull bool + AuthConfig map[string]types.AuthConfig + Output io.Writer +} diff --git a/builder/builder.go b/builder/builder.go index ccc43f6cc2..359eb89b24 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -34,12 +34,8 @@ type Source interface { // Backend abstracts calls to a Docker Daemon. type Backend interface { - // TODO: use digest reference instead of name + GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ReleaseableLayer, error) - // GetImageOnBuild looks up a Docker image referenced by `name`. - GetImageOnBuild(name string) (Image, error) - // PullOnBuild tells Docker to pull image referenced by `name`. - PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (Image, error) // ContainerAttachRaw attaches to container. ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error // ContainerCreate creates a new Docker container and returns potential warnings @@ -62,15 +58,6 @@ type Backend interface { // 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 - - // MountImage returns mounted path with rootfs of an image. - MountImage(name string) (string, func() error, error) -} - -// Image represents a Docker image used by the builder. -type Image interface { - ImageID() string - RunConfig() *container.Config } // Result is the output produced by a Builder @@ -92,3 +79,15 @@ type ImageCache interface { // and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error. GetCache(parentID string, cfg *container.Config) (imageID string, err error) } + +// Image represents a Docker image used by the builder. +type Image interface { + ImageID() string + RunConfig() *container.Config +} + +// ReleaseableLayer is an image layer that can be mounted and released +type ReleaseableLayer interface { + Release() error + Mount() (string, error) +} diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index ac346d5cc8..f7df0b71b9 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -45,7 +45,10 @@ type BuildManager struct { // NewBuildManager creates a BuildManager func NewBuildManager(b builder.Backend) *BuildManager { - return &BuildManager{backend: b, pathCache: &syncmap.Map{}} + return &BuildManager{ + backend: b, + pathCache: &syncmap.Map{}, + } } // Build starts a new build from a BuildConfig @@ -122,8 +125,8 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder { docker: options.Backend, tmpContainers: map[string]struct{}{}, buildArgs: newBuildArgs(config.BuildArgs), + imageContexts: &imageContexts{}, } - b.imageContexts = &imageContexts{b: b, cache: options.PathCache} return b } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index af9f67f3c9..da8b181637 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -20,9 +20,9 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/docker/api" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" - "github.com/docker/docker/builder" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" "github.com/pkg/errors" @@ -160,23 +160,29 @@ func dispatchCopy(req dispatchRequest) error { } flFrom := req.flags.AddString("from", "") - if err := req.flags.Parse(); err != nil { return err } - var im *imageMount - if flFrom.IsUsed() { - var err error - im, err = req.builder.imageContexts.get(flFrom.Value) - if err != nil { - return err - } + im, err := req.builder.getImageMount(flFrom) + if err != nil { + return errors.Wrapf(err, "invalid from flag value %s", flFrom.Value) } - return req.builder.runContextCommand(req, false, false, "COPY", im) } +func (b *Builder) getImageMount(fromFlag *Flag) (*imageMount, error) { + if !fromFlag.IsUsed() { + // TODO: this could return the mount in the default case as well + return nil, nil + } + im, err := b.imageContexts.getMount(fromFlag.Value) + if err != nil || im != nil { + return im, err + } + return b.getImage(fromFlag.Value) +} + // FROM imagename[:tag | @digest] [AS build-stage-name] // func from(req dispatchRequest) error { @@ -192,20 +198,17 @@ func from(req dispatchRequest) error { req.builder.resetImageCache() req.state.noBaseImage = false req.state.stageName = stageName - if _, err := req.builder.imageContexts.add(stageName); err != nil { - return err - } - - image, err := req.builder.getFromImage(req.state, req.shlex, req.args[0]) + image, err := req.builder.getFromImage(req.shlex, req.args[0]) if err != nil { return err } - switch image { - case nil: + if image == nil { req.state.imageID = "" req.state.noBaseImage = true - default: - req.builder.imageContexts.update(image.ImageID(), image.RunConfig()) + image = newImageMount(nil, nil) + } + if err := req.builder.imageContexts.add(stageName, image); err != nil { + return err } req.state.baseImage = image @@ -228,7 +231,7 @@ func parseBuildStageName(args []string) (string, error) { return stageName, nil } -func (b *Builder) getFromImage(dispatchState *dispatchState, shlex *ShellLex, name string) (builder.Image, error) { +func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error) { substitutionArgs := []string{} for key, value := range b.buildArgs.GetAllMeta() { substitutionArgs = append(substitutionArgs, key+"="+value) @@ -254,7 +257,19 @@ func (b *Builder) getFromImage(dispatchState *dispatchState, shlex *ShellLex, na } return nil, nil } - return pullOrGetImage(b, name) + return b.getImage(name) +} + +func (b *Builder) getImage(name string) (*imageMount, error) { + image, layer, err := b.docker.GetImageAndLayer(b.clientCtx, name, backend.GetImageAndLayerOptions{ + ForcePull: b.options.PullParent, + AuthConfig: b.options.AuthConfigs, + Output: b.Output, + }) + if err != nil { + return nil, err + } + return newImageMount(image, layer), nil } // ONBUILD RUN echo yo @@ -801,29 +816,3 @@ func errBlankCommandNames(command string) error { func errTooManyArguments(command string) error { return fmt.Errorf("Bad input to %s, too many arguments", command) } - -// mountByRef creates an imageMount from a reference. pulling the image if needed. -func mountByRef(b *Builder, name string) (*imageMount, error) { - image, err := pullOrGetImage(b, name) - if err != nil { - return nil, err - } - im := b.imageContexts.newImageMount(image.ImageID()) - return im, nil -} - -func pullOrGetImage(b *Builder, name string) (builder.Image, error) { - var image builder.Image - if !b.options.PullParent { - image, _ = b.docker.GetImageOnBuild(name) - // TODO: shouldn't we error out if error is different from "not found" ? - } - if image == nil { - var err error - image, err = b.docker.PullOnBuild(b.clientCtx, name, b.options.AuthConfigs, b.Output) - if err != nil { - return nil, err - } - } - return image, nil -} diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 486b311ef8..5b8a23bb6c 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -47,16 +47,17 @@ func defaultDispatchReq(builder *Builder, args ...string) dispatchRequest { } func newBuilderWithMockBackend() *Builder { + mockBackend := &MockBackend{} b := &Builder{ options: &types.ImageBuildOptions{}, - docker: &MockBackend{}, + docker: mockBackend, buildArgs: newBuildArgs(make(map[string]*string)), tmpContainers: make(map[string]struct{}), Stdout: new(bytes.Buffer), clientCtx: context.Background(), disableCommit: true, + imageContexts: &imageContexts{}, } - b.imageContexts = &imageContexts{b: b} return b } @@ -198,12 +199,12 @@ func TestFromScratch(t *testing.T) { func TestFromWithArg(t *testing.T) { tag, expected := ":sometag", "expectedthisid" - getImage := func(name string) (builder.Image, error) { + getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) { assert.Equal(t, "alpine"+tag, name) - return &mockImage{id: "expectedthisid"}, nil + return &mockImage{id: "expectedthisid"}, nil, nil } b := newBuilderWithMockBackend() - b.docker.(*MockBackend).getImageOnBuildFunc = getImage + b.docker.(*MockBackend).getImageFunc = getImage require.NoError(t, arg(defaultDispatchReq(b, "THETAG="+tag))) req := defaultDispatchReq(b, "alpine${THETAG}") @@ -219,12 +220,12 @@ func TestFromWithArg(t *testing.T) { func TestFromWithUndefinedArg(t *testing.T) { tag, expected := "sometag", "expectedthisid" - getImage := func(name string) (builder.Image, error) { + getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) { assert.Equal(t, "alpine", name) - return &mockImage{id: "expectedthisid"}, nil + return &mockImage{id: "expectedthisid"}, nil, nil } b := newBuilderWithMockBackend() - b.docker.(*MockBackend).getImageOnBuildFunc = getImage + b.docker.(*MockBackend).getImageFunc = getImage b.options.BuildArgs = map[string]*string{"THETAG": &tag} req := defaultDispatchReq(b, "alpine${THETAG}") @@ -474,9 +475,11 @@ func TestRunWithBuildArgs(t *testing.T) { b.imageCache = imageCache mockBackend := b.docker.(*MockBackend) - mockBackend.getImageOnBuildImage = &mockImage{ - id: "abcdef", - config: &container.Config{Cmd: origCmd}, + mockBackend.getImageFunc = func(_ string) (builder.Image, builder.ReleaseableLayer, error) { + return &mockImage{ + id: "abcdef", + config: &container.Config{Cmd: origCmd}, + }, nil, nil } mockBackend.containerCreateFunc = func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) { // Check the runConfig.Cmd sent to create() diff --git a/builder/dockerfile/imagecontext.go b/builder/dockerfile/imagecontext.go index b9393b7bc3..571a20116e 100644 --- a/builder/dockerfile/imagecontext.go +++ b/builder/dockerfile/imagecontext.go @@ -19,49 +19,40 @@ type pathCache interface { // imageContexts is a helper for stacking up built image rootfs and reusing // them as contexts type imageContexts struct { - b *Builder - list []*imageMount // indexed list of stages - implicitMounts []*imageMount // implicitly mounted images - byName map[string]*imageMount - cache pathCache + list []*imageMount + byName map[string]*imageMount + cache pathCache } -func (ic *imageContexts) newImageMount(id string) *imageMount { - return &imageMount{ic: ic, id: id} -} - -func (ic *imageContexts) add(name string) (*imageMount, error) { - im := &imageMount{ic: ic} +func (ic *imageContexts) add(name string, im *imageMount) error { if len(name) > 0 { if ic.byName == nil { ic.byName = make(map[string]*imageMount) } if _, ok := ic.byName[name]; ok { - return nil, errors.Errorf("duplicate name %s", name) + return errors.Errorf("duplicate name %s", name) } ic.byName[name] = im } ic.list = append(ic.list, im) - return im, nil + return nil } func (ic *imageContexts) update(imageID string, runConfig *container.Config) { - ic.list[len(ic.list)-1].id = imageID - ic.list[len(ic.list)-1].runConfig = runConfig + ic.list[len(ic.list)-1].update(imageID, runConfig) } func (ic *imageContexts) validate(i int) error { if i < 0 || i >= len(ic.list)-1 { - var extraMsg string if i == len(ic.list)-1 { - extraMsg = " refers current build block" + return errors.Errorf("%d refers to current build stage", i) } - return errors.Errorf("invalid from flag value %d%s", i, extraMsg) + return errors.Errorf("index out of bounds") } return nil } -func (ic *imageContexts) get(indexOrName string) (*imageMount, error) { +func (ic *imageContexts) getMount(indexOrName string) (*imageMount, error) { index, err := strconv.Atoi(indexOrName) if err == nil { if err := ic.validate(index); err != nil { @@ -72,12 +63,7 @@ func (ic *imageContexts) get(indexOrName string) (*imageMount, error) { if im, ok := ic.byName[strings.ToLower(indexOrName)]; ok { return im, nil } - im, err := mountByRef(ic.b, indexOrName) - if err != nil { - return nil, errors.Wrapf(err, "invalid from flag value %s", indexOrName) - } - ic.implicitMounts = append(ic.implicitMounts, im) - return im, nil + return nil, nil } func (ic *imageContexts) unmount() (retErr error) { @@ -98,6 +84,7 @@ func (ic *imageContexts) unmount() (retErr error) { return } +// TODO: remove getCache/setCache from imageContexts func (ic *imageContexts) getCache(id, path string) (interface{}, bool) { if ic.cache != nil { if id == "" { @@ -114,48 +101,56 @@ func (ic *imageContexts) setCache(id, path string, v interface{}) { } } -// imageMount is a reference for getting access to a buildcontext that is backed -// by an existing image +// imageMount is a reference to an image that can be used as a builder.Source type imageMount struct { id string source builder.Source - release func() error - ic *imageContexts runConfig *container.Config + layer builder.ReleaseableLayer +} + +func newImageMount(image builder.Image, layer builder.ReleaseableLayer) *imageMount { + im := &imageMount{layer: layer} + if image != nil { + im.update(image.ImageID(), image.RunConfig()) + } + return im } func (im *imageMount) context() (builder.Source, error) { if im.source == nil { - if im.id == "" { - return nil, errors.Errorf("could not copy from empty context") + if im.id == "" || im.layer == nil { + return nil, errors.Errorf("empty context") } - p, release, err := im.ic.b.docker.MountImage(im.id) + mountPath, err := im.layer.Mount() if err != nil { return nil, errors.Wrapf(err, "failed to mount %s", im.id) } - source, err := remotecontext.NewLazyContext(p) + source, err := remotecontext.NewLazyContext(mountPath) if err != nil { - return nil, errors.Wrapf(err, "failed to create lazycontext for %s", p) + return nil, errors.Wrapf(err, "failed to create lazycontext for %s", mountPath) } - im.release = release im.source = source } return im.source, nil } func (im *imageMount) unmount() error { - if im.release != nil { - if err := im.release(); err != nil { - return errors.Wrapf(err, "failed to unmount previous build image %s", im.id) - } - im.release = nil + if err := im.layer.Release(); err != nil { + return errors.Wrapf(err, "failed to unmount previous build image %s", im.id) } return nil } +func (im *imageMount) update(imageID string, runConfig *container.Config) { + im.id = imageID + im.runConfig = runConfig +} + func (im *imageMount) ImageID() string { return im.id } + func (im *imageMount) RunConfig() *container.Config { return im.runConfig } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 963f0509f0..b70a61b8e6 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -372,7 +372,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression if imageSource != nil { source, err = imageSource.context() if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to copy") } } diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index bdd198ccfe..a874dfb550 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -15,30 +15,15 @@ import ( // MockBackend implements the builder.Backend interface for unit testing type MockBackend struct { - getImageOnBuildFunc func(string) (builder.Image, error) - getImageOnBuildImage *mockImage - containerCreateFunc func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) - commitFunc func(string, *backend.ContainerCommitConfig) (string, error) -} - -func (m *MockBackend) GetImageOnBuild(name string) (builder.Image, error) { - if m.getImageOnBuildFunc != nil { - return m.getImageOnBuildFunc(name) - } - if m.getImageOnBuildImage != nil { - return m.getImageOnBuildImage, nil - } - return &mockImage{id: "theid"}, nil + containerCreateFunc func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) + commitFunc func(string, *backend.ContainerCommitConfig) (string, error) + getImageFunc func(string) (builder.Image, builder.ReleaseableLayer, error) } func (m *MockBackend) TagImageWithReference(image.ID, reference.Named) error { return nil } -func (m *MockBackend) PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (builder.Image, error) { - return nil, nil -} - func (m *MockBackend) ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error { return nil } @@ -89,8 +74,12 @@ func (m *MockBackend) SquashImage(from string, to string) (string, error) { return "", nil } -func (m *MockBackend) MountImage(name string) (string, func() error, error) { - return "", func() error { return nil }, nil +func (m *MockBackend) GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) { + if m.getImageFunc != nil { + return m.getImageFunc(refOrID) + } + + return &mockImage{id: "theid"}, &mockLayer{}, nil } type mockImage struct { @@ -116,3 +105,13 @@ func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (str } return "", nil } + +type mockLayer struct{} + +func (l *mockLayer) Release() error { + return nil +} + +func (l *mockLayer) Mount() (string, error) { + return "mountPath", nil +} diff --git a/daemon/archive.go b/daemon/archive.go index 9b0a2f4a02..dbef0b4d48 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -8,12 +8,10 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/container" - "github.com/docker/docker/layer" "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/stringid" "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" "github.com/pkg/errors" @@ -470,34 +468,3 @@ func (daemon *Daemon) CopyOnBuild(cID, destPath, srcRoot, srcPath string, decomp return fixPermissions(fullSrcPath, destPath, rootUID, rootGID, destExists) } - -// MountImage returns mounted path with rootfs of an image. -func (daemon *Daemon) MountImage(name string) (string, func() error, error) { - img, err := daemon.GetImage(name) - if err != nil { - return "", nil, errors.Wrapf(err, "no such image: %s", name) - } - - mountID := stringid.GenerateRandomID() - rwLayer, err := daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) - if err != nil { - return "", nil, errors.Wrap(err, "failed to create rwlayer") - } - - mountPath, err := rwLayer.Mount("") - if err != nil { - metadata, releaseErr := daemon.layerStore.ReleaseRWLayer(rwLayer) - if releaseErr != nil { - err = errors.Wrapf(err, "failed to release rwlayer: %s", releaseErr.Error()) - } - layer.LogReleaseMetadata(metadata) - return "", nil, errors.Wrap(err, "failed to mount rwlayer") - } - - return mountPath, func() error { - rwLayer.Unmount() - metadata, err := daemon.layerStore.ReleaseRWLayer(rwLayer) - layer.LogReleaseMetadata(metadata) - return err - }, nil -} diff --git a/daemon/build.go b/daemon/build.go new file mode 100644 index 0000000000..44640efee5 --- /dev/null +++ b/daemon/build.go @@ -0,0 +1,108 @@ +package daemon + +import ( + "github.com/docker/distribution/reference" + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/backend" + "github.com/docker/docker/builder" + "github.com/docker/docker/image" + "github.com/docker/docker/layer" + "github.com/docker/docker/pkg/stringid" + "github.com/docker/docker/registry" + "github.com/pkg/errors" + "golang.org/x/net/context" + "io" +) + +type releaseableLayer struct { + rwLayer layer.RWLayer + release func(layer.RWLayer) error + mount func() (layer.RWLayer, error) +} + +func (rl *releaseableLayer) Release() error { + if rl.rwLayer == nil { + return nil + } + rl.rwLayer.Unmount() + return rl.release(rl.rwLayer) +} + +func (rl *releaseableLayer) Mount() (string, error) { + var err error + // daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) + rl.rwLayer, err = rl.mount() + if err != nil { + return "", errors.Wrap(err, "failed to create rwlayer") + } + + mountPath, err := rl.rwLayer.Mount("") + if err != nil { + releaseErr := rl.release(rl.rwLayer) + if releaseErr != nil { + err = errors.Wrapf(err, "failed to release rwlayer: %s", releaseErr.Error()) + } + return "", errors.Wrap(err, "failed to mount rwlayer") + } + return mountPath, err +} + +func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseableLayer, error) { + mountFunc := func() (layer.RWLayer, error) { + mountID := stringid.GenerateRandomID() + return daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) + } + + releaseFunc := func(rwLayer layer.RWLayer) error { + metadata, err := daemon.layerStore.ReleaseRWLayer(rwLayer) + layer.LogReleaseMetadata(metadata) + return err + } + + return &releaseableLayer{mount: mountFunc, release: releaseFunc}, nil +} + +// TODO: could this use the regular daemon PullImage ? +func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (*image.Image, error) { + ref, err := reference.ParseNormalizedNamed(name) + if err != nil { + return nil, err + } + ref = reference.TagNameOnly(ref) + + pullRegistryAuth := &types.AuthConfig{} + if len(authConfigs) > 0 { + // The request came with a full auth config file, we prefer to use that + repoInfo, err := daemon.RegistryService.ResolveRepository(ref) + if err != nil { + return nil, err + } + + resolvedConfig := registry.ResolveAuthConfig(authConfigs, repoInfo.Index) + pullRegistryAuth = &resolvedConfig + } + + if err := daemon.pullImageWithReference(ctx, ref, nil, pullRegistryAuth, output); err != nil { + return nil, err + } + return daemon.GetImage(name) +} + +// GetImageAndLayer returns an image and releaseable layer for a reference or ID +func (daemon *Daemon) GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) { + if !opts.ForcePull { + image, _ := daemon.GetImage(refOrID) + // TODO: shouldn't we error out if error is different from "not found" ? + if image != nil { + layer, err := daemon.getReleasableLayerForImage(image) + return image, layer, err + } + } + + image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output) + if err != nil { + return nil, nil, err + } + layer, err := daemon.getReleasableLayerForImage(image) + return image, layer, err +} diff --git a/daemon/image.go b/daemon/image.go index 43ee483ff0..d10457118f 100644 --- a/daemon/image.go +++ b/daemon/image.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/docker/distribution/reference" - "github.com/docker/docker/builder" "github.com/docker/docker/image" "github.com/docker/docker/pkg/stringid" ) @@ -75,12 +74,3 @@ func (daemon *Daemon) GetImage(refOrID string) (*image.Image, error) { } return daemon.imageStore.Get(imgID) } - -// GetImageOnBuild looks up a Docker image referenced by `name`. -func (daemon *Daemon) GetImageOnBuild(name string) (builder.Image, error) { - img, err := daemon.GetImage(name) - if err != nil { - return nil, err - } - return img, nil -} diff --git a/daemon/image_pull.go b/daemon/image_pull.go index e0ac92ce44..304fd9f024 100644 --- a/daemon/image_pull.go +++ b/daemon/image_pull.go @@ -7,7 +7,6 @@ import ( dist "github.com/docker/distribution" "github.com/docker/distribution/reference" "github.com/docker/docker/api/types" - "github.com/docker/docker/builder" "github.com/docker/docker/distribution" progressutils "github.com/docker/docker/distribution/utils" "github.com/docker/docker/pkg/progress" @@ -46,35 +45,6 @@ func (daemon *Daemon) PullImage(ctx context.Context, image, tag string, metaHead return daemon.pullImageWithReference(ctx, ref, metaHeaders, authConfig, outStream) } -// PullOnBuild tells Docker to pull image referenced by `name`. -func (daemon *Daemon) PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (builder.Image, error) { - ref, err := reference.ParseNormalizedNamed(name) - if err != nil { - return nil, err - } - ref = reference.TagNameOnly(ref) - - pullRegistryAuth := &types.AuthConfig{} - if len(authConfigs) > 0 { - // The request came with a full auth config file, we prefer to use that - repoInfo, err := daemon.RegistryService.ResolveRepository(ref) - if err != nil { - return nil, err - } - - resolvedConfig := registry.ResolveAuthConfig( - authConfigs, - repoInfo.Index, - ) - pullRegistryAuth = &resolvedConfig - } - - if err := daemon.pullImageWithReference(ctx, ref, nil, pullRegistryAuth, output); err != nil { - return nil, err - } - return daemon.GetImage(name) -} - func (daemon *Daemon) pullImageWithReference(ctx context.Context, ref reference.Named, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error { // Include a buffer so that slow client connections don't affect // transfer performance. From ab3a037a5b77220d0524ce2b17105e1daae39425 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 May 2017 13:05:25 -0400 Subject: [PATCH 2/3] Refactor interaction between dispatcher.from and dispatchState Signed-off-by: Daniel Nephin --- builder/builder.go | 2 +- builder/dockerfile/dispatchers.go | 73 ++++++++++++++++++------ builder/dockerfile/dispatchers_test.go | 4 +- builder/dockerfile/evaluator.go | 53 +++++++++++------ builder/dockerfile/imagecontext.go | 9 ++- builder/dockerfile/internals.go | 69 ---------------------- builder/dockerfile/mockbackend_test.go | 2 +- daemon/build.go | 26 ++++----- integration-cli/docker_cli_build_test.go | 2 +- 9 files changed, 117 insertions(+), 123 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 359eb89b24..f990635933 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -89,5 +89,5 @@ type Image interface { // ReleaseableLayer is an image layer that can be mounted and released type ReleaseableLayer interface { Release() error - Mount() (string, error) + Mount(string) (string, error) } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index da8b181637..83c2a6f247 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -23,6 +23,7 @@ import ( "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" + "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" "github.com/pkg/errors" @@ -196,24 +197,21 @@ func from(req dispatchRequest) error { } req.builder.resetImageCache() - req.state.noBaseImage = false - req.state.stageName = stageName image, err := req.builder.getFromImage(req.shlex, req.args[0]) if err != nil { return err } - if image == nil { - req.state.imageID = "" - req.state.noBaseImage = true - image = newImageMount(nil, nil) - } if err := req.builder.imageContexts.add(stageName, image); err != nil { return err } - req.state.baseImage = image - + req.state.beginStage(stageName, image) req.builder.buildArgs.ResetAllowed() - return req.builder.processImageFrom(req.state, image) + if image.ImageID() == "" { + // Typically this means they used "FROM scratch" + return nil + } + + return processOnBuild(req) } func parseBuildStageName(args []string) (string, error) { @@ -243,11 +241,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error } if im, ok := b.imageContexts.byName[name]; ok { - if len(im.ImageID()) > 0 { - return im, nil - } - // FROM scratch does not have an ImageID - return nil, nil + return im, nil } // Windows cannot support a container with no base image. @@ -255,7 +249,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error if runtime.GOOS == "windows" { return nil, errors.New("Windows does not support FROM scratch") } - return nil, nil + return newImageMount(nil, nil), nil } return b.getImage(name) } @@ -272,6 +266,53 @@ func (b *Builder) getImage(name string) (*imageMount, error) { return newImageMount(image, layer), nil } +func processOnBuild(req dispatchRequest) error { + dispatchState := req.state + // Process ONBUILD triggers if they exist + if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 { + word := "trigger" + if nTriggers > 1 { + word = "triggers" + } + fmt.Fprintf(req.builder.Stderr, "# Executing %d build %s...\n", nTriggers, word) + } + + // Copy the ONBUILD triggers, and remove them from the config, since the config will be committed. + onBuildTriggers := dispatchState.runConfig.OnBuild + dispatchState.runConfig.OnBuild = []string{} + + // Reset stdin settings as all build actions run without stdin + dispatchState.runConfig.OpenStdin = false + dispatchState.runConfig.StdinOnce = false + + // parse the ONBUILD triggers by invoking the parser + for _, step := range onBuildTriggers { + dockerfile, err := parser.Parse(strings.NewReader(step)) + if err != nil { + return err + } + + for _, n := range dockerfile.AST.Children { + if err := checkDispatch(n); err != nil { + return err + } + + upperCasedCmd := strings.ToUpper(n.Value) + switch upperCasedCmd { + case "ONBUILD": + return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") + case "MAINTAINER", "FROM": + return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd) + } + } + + if _, err := dispatchFromDockerfile(req.builder, dockerfile, dispatchState); err != nil { + return err + } + } + return nil +} + // ONBUILD RUN echo yo // // ONBUILD triggers run when the image is used in a FROM statement. diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 5b8a23bb6c..507f72bdcf 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -13,6 +13,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/pkg/system" "github.com/docker/docker/pkg/testutil" "github.com/docker/go-connections/nat" "github.com/stretchr/testify/assert" @@ -192,8 +193,9 @@ func TestFromScratch(t *testing.T) { } require.NoError(t, err) + assert.True(t, req.state.hasFromImage()) assert.Equal(t, "", req.state.imageID) - assert.Equal(t, true, req.state.noBaseImage) + assert.Equal(t, []string{"PATH=" + system.DefaultPathEnv}, req.state.runConfig.Env) } func TestFromWithArg(t *testing.T) { diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index 8402af9087..6fc2c6a6cd 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -28,6 +28,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/builder/dockerfile/command" "github.com/docker/docker/builder/dockerfile/parser" + "github.com/docker/docker/pkg/system" "github.com/docker/docker/runconfig/opts" "github.com/pkg/errors" ) @@ -184,37 +185,55 @@ type dispatchOptions struct { // dispatchState is a data object which is modified by dispatchers type dispatchState struct { - runConfig *container.Config - maintainer string - cmdSet bool - noBaseImage bool - imageID string - baseImage builder.Image - stageName string + runConfig *container.Config + maintainer string + cmdSet bool + imageID string + baseImage builder.Image + stageName string } func newDispatchState() *dispatchState { return &dispatchState{runConfig: &container.Config{}} } -func (r *dispatchState) updateRunConfig() { - r.runConfig.Image = r.imageID +func (s *dispatchState) updateRunConfig() { + s.runConfig.Image = s.imageID } // hasFromImage returns true if the builder has processed a `FROM ` line -func (r *dispatchState) hasFromImage() bool { - return r.imageID != "" || r.noBaseImage +func (s *dispatchState) hasFromImage() bool { + return s.imageID != "" || (s.baseImage != nil && s.baseImage.ImageID() == "") } -func (r *dispatchState) runConfigEnvMapping() map[string]string { - return opts.ConvertKVStringsToMap(r.runConfig.Env) -} - -func (r *dispatchState) isCurrentStage(target string) bool { +func (s *dispatchState) isCurrentStage(target string) bool { if target == "" { return false } - return strings.EqualFold(r.stageName, target) + return strings.EqualFold(s.stageName, target) +} + +func (s *dispatchState) beginStage(stageName string, image builder.Image) { + s.stageName = stageName + s.imageID = image.ImageID() + + if image.RunConfig() != nil { + s.runConfig = image.RunConfig() + } + s.baseImage = image + s.setDefaultPath() +} + +// Add the default PATH to runConfig.ENV if one exists for the platform and there +// is no PATH set. Note that windows won't have one as it's set by HCS +func (s *dispatchState) setDefaultPath() { + if system.DefaultPathEnv == "" { + return + } + envMap := opts.ConvertKVStringsToMap(s.runConfig.Env) + if _, ok := envMap["PATH"]; !ok { + s.runConfig.Env = append(s.runConfig.Env, "PATH="+system.DefaultPathEnv) + } } func handleOnBuildNode(ast *parser.Node, msg *bytes.Buffer) (*parser.Node, []string, error) { diff --git a/builder/dockerfile/imagecontext.go b/builder/dockerfile/imagecontext.go index 571a20116e..e0a25084ee 100644 --- a/builder/dockerfile/imagecontext.go +++ b/builder/dockerfile/imagecontext.go @@ -45,9 +45,9 @@ func (ic *imageContexts) update(imageID string, runConfig *container.Config) { func (ic *imageContexts) validate(i int) error { if i < 0 || i >= len(ic.list)-1 { if i == len(ic.list)-1 { - return errors.Errorf("%d refers to current build stage", i) + return errors.New("refers to current build stage") } - return errors.Errorf("index out of bounds") + return errors.New("index out of bounds") } return nil } @@ -122,7 +122,7 @@ func (im *imageMount) context() (builder.Source, error) { if im.id == "" || im.layer == nil { return nil, errors.Errorf("empty context") } - mountPath, err := im.layer.Mount() + mountPath, err := im.layer.Mount(im.id) if err != nil { return nil, errors.Wrapf(err, "failed to mount %s", im.id) } @@ -136,6 +136,9 @@ func (im *imageMount) context() (builder.Source, error) { } func (im *imageMount) unmount() error { + if im.layer == nil { + return nil + } if err := im.layer.Release(); err != nil { return errors.Wrapf(err, "failed to unmount previous build image %s", im.id) } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index b70a61b8e6..5affd1d5f9 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -22,7 +22,6 @@ import ( "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" - "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/builder/remotecontext" "github.com/docker/docker/pkg/httputils" "github.com/docker/docker/pkg/ioutils" @@ -480,74 +479,6 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression return copyInfos, nil } -func (b *Builder) processImageFrom(dispatchState *dispatchState, img builder.Image) error { - if img != nil { - dispatchState.imageID = img.ImageID() - - if img.RunConfig() != nil { - dispatchState.runConfig = img.RunConfig() - } - } - - // Check to see if we have a default PATH, note that windows won't - // have one as it's set by HCS - if system.DefaultPathEnv != "" { - if _, ok := dispatchState.runConfigEnvMapping()["PATH"]; !ok { - dispatchState.runConfig.Env = append(dispatchState.runConfig.Env, - "PATH="+system.DefaultPathEnv) - } - } - - if img == nil { - // Typically this means they used "FROM scratch" - return nil - } - - // Process ONBUILD triggers if they exist - if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 { - word := "trigger" - if nTriggers > 1 { - word = "triggers" - } - fmt.Fprintf(b.Stderr, "# Executing %d build %s...\n", nTriggers, word) - } - - // Copy the ONBUILD triggers, and remove them from the config, since the config will be committed. - onBuildTriggers := dispatchState.runConfig.OnBuild - dispatchState.runConfig.OnBuild = []string{} - - // Reset stdin settings as all build actions run without stdin - dispatchState.runConfig.OpenStdin = false - dispatchState.runConfig.StdinOnce = false - - // parse the ONBUILD triggers by invoking the parser - for _, step := range onBuildTriggers { - dockerfile, err := parser.Parse(strings.NewReader(step)) - if err != nil { - return err - } - - for _, n := range dockerfile.AST.Children { - if err := checkDispatch(n); err != nil { - return err - } - - upperCasedCmd := strings.ToUpper(n.Value) - switch upperCasedCmd { - case "ONBUILD": - return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") - case "MAINTAINER", "FROM": - return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd) - } - } - - if _, err := dispatchFromDockerfile(b, dockerfile, dispatchState); err != nil { - return err - } - } - return nil -} - // probeCache checks if cache match can be found for current build instruction. // If an image is found, probeCache returns `(true, nil)`. // If no image is found, it returns `(false, nil)`. diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index a874dfb550..4f4a59d545 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -112,6 +112,6 @@ func (l *mockLayer) Release() error { return nil } -func (l *mockLayer) Mount() (string, error) { +func (l *mockLayer) Mount(_ string) (string, error) { return "mountPath", nil } diff --git a/daemon/build.go b/daemon/build.go index 44640efee5..806f05c392 100644 --- a/daemon/build.go +++ b/daemon/build.go @@ -17,7 +17,7 @@ import ( type releaseableLayer struct { rwLayer layer.RWLayer release func(layer.RWLayer) error - mount func() (layer.RWLayer, error) + mount func(string) (layer.RWLayer, error) } func (rl *releaseableLayer) Release() error { @@ -28,10 +28,9 @@ func (rl *releaseableLayer) Release() error { return rl.release(rl.rwLayer) } -func (rl *releaseableLayer) Mount() (string, error) { +func (rl *releaseableLayer) Mount(imageID string) (string, error) { var err error - // daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) - rl.rwLayer, err = rl.mount() + rl.rwLayer, err = rl.mount(imageID) if err != nil { return "", errors.Wrap(err, "failed to create rwlayer") } @@ -47,8 +46,12 @@ func (rl *releaseableLayer) Mount() (string, error) { return mountPath, err } -func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseableLayer, error) { - mountFunc := func() (layer.RWLayer, error) { +func (daemon *Daemon) getReleasableLayerForImage() *releaseableLayer { + mountFunc := func(imageID string) (layer.RWLayer, error) { + img, err := daemon.GetImage(imageID) + if err != nil { + return nil, err + } mountID := stringid.GenerateRandomID() return daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) } @@ -59,7 +62,7 @@ func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseable return err } - return &releaseableLayer{mount: mountFunc, release: releaseFunc}, nil + return &releaseableLayer{mount: mountFunc, release: releaseFunc} } // TODO: could this use the regular daemon PullImage ? @@ -94,15 +97,10 @@ func (daemon *Daemon) GetImageAndLayer(ctx context.Context, refOrID string, opts image, _ := daemon.GetImage(refOrID) // TODO: shouldn't we error out if error is different from "not found" ? if image != nil { - layer, err := daemon.getReleasableLayerForImage(image) - return image, layer, err + return image, daemon.getReleasableLayerForImage(), nil } } image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output) - if err != nil { - return nil, nil, err - } - layer, err := daemon.getReleasableLayerForImage(image) - return image, layer, err + return image, daemon.getReleasableLayerForImage(), err } diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index fff95d66ef..99d6e785d7 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -5946,7 +5946,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) { dockerfile: ` FROM busybox COPY --from=0 foo bar`, - expectedError: "invalid from flag value 0 refers current build block", + expectedError: "invalid from flag value 0: refers to current build stage", }, { dockerfile: ` From 6c28e8edd5f047d5b1438f773d49882f28d7a006 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 May 2017 18:52:11 -0400 Subject: [PATCH 3/3] Refactor imageContexts into two different structs. buildStages now tracks the imageID and runConfig for a build stage imageMounter tracks image mounts so they can released when the build ends. Signed-off-by: Daniel Nephin --- api/types/backend/build.go | 2 +- builder/builder.go | 9 +- builder/dockerfile/builder.go | 8 +- builder/dockerfile/dispatchers.go | 42 +++--- builder/dockerfile/dispatchers_test.go | 9 +- builder/dockerfile/imagecontext.go | 197 ++++++++++++++++--------- builder/dockerfile/internals.go | 10 +- builder/dockerfile/mockbackend_test.go | 12 +- daemon/build.go | 96 +++++++----- 9 files changed, 229 insertions(+), 156 deletions(-) diff --git a/api/types/backend/build.go b/api/types/backend/build.go index ad19c9a1d0..5de35e6d88 100644 --- a/api/types/backend/build.go +++ b/api/types/backend/build.go @@ -23,7 +23,7 @@ type BuildConfig struct { Options *types.ImageBuildOptions } -// GetImageAndLayerOptions are the options supported by GetImageAndLayer +// GetImageAndLayerOptions are the options supported by GetImageAndReleasableLayer type GetImageAndLayerOptions struct { ForcePull bool AuthConfig map[string]types.AuthConfig diff --git a/builder/builder.go b/builder/builder.go index f990635933..3f9c7febaf 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -34,7 +34,7 @@ type Source interface { // Backend abstracts calls to a Docker Daemon. type Backend interface { - GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ReleaseableLayer, error) + ImageBackend // ContainerAttachRaw attaches to container. ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error @@ -60,6 +60,11 @@ type Backend interface { CopyOnBuild(containerID string, destPath string, srcRoot string, srcPath string, decompress bool) error } +// ImageBackend are the interface methods required from an image component +type ImageBackend interface { + GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ReleaseableLayer, error) +} + // Result is the output produced by a Builder type Result struct { ImageID string @@ -89,5 +94,5 @@ type Image interface { // ReleaseableLayer is an image layer that can be mounted and released type ReleaseableLayer interface { Release() error - Mount(string) (string, error) + Mount() (string, error) } diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index f7df0b71b9..f182b68cbf 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -102,11 +102,12 @@ type Builder struct { clientCtx context.Context tmpContainers map[string]struct{} - imageContexts *imageContexts // helper for storing contexts from builds + buildStages *buildStages disableCommit bool cacheBusted bool buildArgs *buildArgs imageCache builder.ImageCache + imageSources *imageSources } // newBuilder creates a new Dockerfile builder from an optional dockerfile and a Options. @@ -125,7 +126,8 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder { docker: options.Backend, tmpContainers: map[string]struct{}{}, buildArgs: newBuildArgs(config.BuildArgs), - imageContexts: &imageContexts{}, + buildStages: newBuildStages(), + imageSources: newImageSources(clientCtx, options), } return b } @@ -140,7 +142,7 @@ func (b *Builder) resetImageCache() { // Build runs the Dockerfile builder by parsing the Dockerfile and executing // the instructions from the file. func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*builder.Result, error) { - defer b.imageContexts.unmount() + defer b.imageSources.Unmount() // TODO: Remove source field from Builder b.source = source diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 83c2a6f247..03e91e011d 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -20,9 +20,9 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/docker/api" "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" + "github.com/docker/docker/builder" "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" @@ -174,14 +174,19 @@ func dispatchCopy(req dispatchRequest) error { func (b *Builder) getImageMount(fromFlag *Flag) (*imageMount, error) { if !fromFlag.IsUsed() { - // TODO: this could return the mount in the default case as well + // TODO: this could return the source in the default case as well? return nil, nil } - im, err := b.imageContexts.getMount(fromFlag.Value) - if err != nil || im != nil { - return im, err + + imageRefOrID := fromFlag.Value + stage, err := b.buildStages.get(fromFlag.Value) + if err != nil { + return nil, err } - return b.getImage(fromFlag.Value) + if stage != nil { + imageRefOrID = stage.ImageID() + } + return b.imageSources.Get(imageRefOrID) } // FROM imagename[:tag | @digest] [AS build-stage-name] @@ -201,7 +206,7 @@ func from(req dispatchRequest) error { if err != nil { return err } - if err := req.builder.imageContexts.add(stageName, image); err != nil { + if err := req.builder.buildStages.add(stageName, image); err != nil { return err } req.state.beginStage(stageName, image) @@ -229,7 +234,12 @@ func parseBuildStageName(args []string) (string, error) { return stageName, nil } -func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error) { +// 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{} + +func (b *Builder) getFromImage(shlex *ShellLex, name string) (builder.Image, error) { substitutionArgs := []string{} for key, value := range b.buildArgs.GetAllMeta() { substitutionArgs = append(substitutionArgs, key+"="+value) @@ -240,7 +250,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error return nil, err } - if im, ok := b.imageContexts.byName[name]; ok { + if im, ok := b.buildStages.getByName(name); ok { return im, nil } @@ -249,21 +259,13 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error if runtime.GOOS == "windows" { return nil, errors.New("Windows does not support FROM scratch") } - return newImageMount(nil, nil), nil + return scratchImage, nil } - return b.getImage(name) -} - -func (b *Builder) getImage(name string) (*imageMount, error) { - image, layer, err := b.docker.GetImageAndLayer(b.clientCtx, name, backend.GetImageAndLayerOptions{ - ForcePull: b.options.PullParent, - AuthConfig: b.options.AuthConfigs, - Output: b.Output, - }) + imageMount, err := b.imageSources.Get(name) if err != nil { return nil, err } - return newImageMount(image, layer), nil + return imageMount.Image(), nil } func processOnBuild(req dispatchRequest) error { diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 507f72bdcf..6134ce4a0f 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -49,15 +49,20 @@ func defaultDispatchReq(builder *Builder, args ...string) dispatchRequest { func newBuilderWithMockBackend() *Builder { mockBackend := &MockBackend{} + ctx := context.Background() b := &Builder{ options: &types.ImageBuildOptions{}, docker: mockBackend, buildArgs: newBuildArgs(make(map[string]*string)), tmpContainers: make(map[string]struct{}), Stdout: new(bytes.Buffer), - clientCtx: context.Background(), + clientCtx: ctx, disableCommit: true, - imageContexts: &imageContexts{}, + imageSources: newImageSources(ctx, builderOptions{ + Options: &types.ImageBuildOptions{}, + Backend: mockBackend, + }), + buildStages: newBuildStages(), } return b } diff --git a/builder/dockerfile/imagecontext.go b/builder/dockerfile/imagecontext.go index e0a25084ee..690d7436e2 100644 --- a/builder/dockerfile/imagecontext.go +++ b/builder/dockerfile/imagecontext.go @@ -5,10 +5,12 @@ import ( "strings" "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/pkg/errors" + "golang.org/x/net/context" ) type pathCache interface { @@ -16,35 +18,63 @@ type pathCache interface { Store(key, value interface{}) } -// imageContexts is a helper for stacking up built image rootfs and reusing -// them as contexts -type imageContexts struct { - list []*imageMount - byName map[string]*imageMount - cache pathCache +type buildStage struct { + id string + config *container.Config } -func (ic *imageContexts) add(name string, im *imageMount) error { - if len(name) > 0 { - if ic.byName == nil { - ic.byName = make(map[string]*imageMount) +func newBuildStageFromImage(image builder.Image) *buildStage { + return &buildStage{id: image.ImageID(), config: image.RunConfig()} +} + +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) { + 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 { + sequence []*buildStage + byName map[string]*buildStage +} + +func newBuildStages() *buildStages { + return &buildStages{byName: make(map[string]*buildStage)} +} + +func (s *buildStages) getByName(name string) (builder.Image, bool) { + stage, ok := s.byName[strings.ToLower(name)] + return stage, ok +} + +func (s *buildStages) get(indexOrName string) (builder.Image, error) { + index, err := strconv.Atoi(indexOrName) + if err == nil { + if err := s.validateIndex(index); err != nil { + return nil, err } - if _, ok := ic.byName[name]; ok { - return errors.Errorf("duplicate name %s", name) - } - ic.byName[name] = im + return s.sequence[index], nil } - ic.list = append(ic.list, im) - return nil + if im, ok := s.byName[strings.ToLower(indexOrName)]; ok { + return im, nil + } + return nil, nil } -func (ic *imageContexts) update(imageID string, runConfig *container.Config) { - ic.list[len(ic.list)-1].update(imageID, runConfig) -} - -func (ic *imageContexts) validate(i int) error { - if i < 0 || i >= len(ic.list)-1 { - if i == len(ic.list)-1 { +func (s *buildStages) validateIndex(i int) error { + if i < 0 || i >= len(s.sequence)-1 { + if i == len(s.sequence)-1 { return errors.New("refers to current build stage") } return errors.New("index out of bounds") @@ -52,30 +82,64 @@ func (ic *imageContexts) validate(i int) error { return nil } -func (ic *imageContexts) getMount(indexOrName string) (*imageMount, error) { - index, err := strconv.Atoi(indexOrName) - if err == nil { - if err := ic.validate(index); err != nil { - return nil, err +func (s *buildStages) add(name string, image builder.Image) error { + stage := newBuildStageFromImage(image) + name = strings.ToLower(name) + if len(name) > 0 { + if _, ok := s.byName[name]; ok { + return errors.Errorf("duplicate name %s", name) } - return ic.list[index], nil + s.byName[name] = stage } - if im, ok := ic.byName[strings.ToLower(indexOrName)]; ok { - return im, nil - } - return nil, nil + s.sequence = append(s.sequence, stage) + return nil } -func (ic *imageContexts) unmount() (retErr error) { - for _, iml := range append([][]*imageMount{}, ic.list, ic.implicitMounts) { - for _, im := range iml { - if err := im.unmount(); err != nil { - logrus.Error(err) - retErr = err - } - } +func (s *buildStages) update(imageID string, runConfig *container.Config) { + s.sequence[len(s.sequence)-1].update(imageID, runConfig) +} + +type getAndMountFunc func(string) (builder.Image, builder.ReleaseableLayer, error) + +// imageSources mounts images and provides a cache for mounted images. It tracks +// all images so they can be unmounted at the end of the build. +type imageSources struct { + byImageID map[string]*imageMount + getImage getAndMountFunc + cache pathCache // TODO: remove +} + +func newImageSources(ctx context.Context, options builderOptions) *imageSources { + getAndMount := func(idOrRef string) (builder.Image, builder.ReleaseableLayer, error) { + return options.Backend.GetImageAndReleasableLayer(ctx, idOrRef, backend.GetImageAndLayerOptions{ + ForcePull: options.Options.PullParent, + AuthConfig: options.Options.AuthConfigs, + Output: options.ProgressWriter.Output, + }) } - for _, im := range ic.byName { + + return &imageSources{ + byImageID: make(map[string]*imageMount), + getImage: getAndMount, + } +} + +func (m *imageSources) Get(idOrRef string) (*imageMount, error) { + if im, ok := m.byImageID[idOrRef]; ok { + return im, nil + } + + image, layer, err := m.getImage(idOrRef) + if err != nil { + return nil, err + } + im := newImageMount(image, layer) + m.byImageID[image.ImageID()] = im + return im, nil +} + +func (m *imageSources) Unmount() (retErr error) { + for _, im := range m.byImageID { if err := im.unmount(); err != nil { logrus.Error(err) retErr = err @@ -84,47 +148,43 @@ func (ic *imageContexts) unmount() (retErr error) { return } -// TODO: remove getCache/setCache from imageContexts -func (ic *imageContexts) getCache(id, path string) (interface{}, bool) { - if ic.cache != nil { +// TODO: remove getCache/setCache from imageSources +func (m *imageSources) getCache(id, path string) (interface{}, bool) { + if m.cache != nil { if id == "" { return nil, false } - return ic.cache.Load(id + path) + return m.cache.Load(id + path) } return nil, false } -func (ic *imageContexts) setCache(id, path string, v interface{}) { - if ic.cache != nil { - ic.cache.Store(id+path, v) +func (m *imageSources) setCache(id, path string, v interface{}) { + if m.cache != nil { + m.cache.Store(id+path, v) } } // imageMount is a reference to an image that can be used as a builder.Source type imageMount struct { - id string - source builder.Source - runConfig *container.Config - layer builder.ReleaseableLayer + image builder.Image + source builder.Source + layer builder.ReleaseableLayer } func newImageMount(image builder.Image, layer builder.ReleaseableLayer) *imageMount { - im := &imageMount{layer: layer} - if image != nil { - im.update(image.ImageID(), image.RunConfig()) - } + im := &imageMount{image: image, layer: layer} return im } -func (im *imageMount) context() (builder.Source, error) { +func (im *imageMount) Source() (builder.Source, error) { if im.source == nil { - if im.id == "" || im.layer == nil { + if im.layer == nil { return nil, errors.Errorf("empty context") } - mountPath, err := im.layer.Mount(im.id) + mountPath, err := im.layer.Mount() if err != nil { - return nil, errors.Wrapf(err, "failed to mount %s", im.id) + return nil, errors.Wrapf(err, "failed to mount %s", im.image.ImageID()) } source, err := remotecontext.NewLazyContext(mountPath) if err != nil { @@ -140,20 +200,11 @@ func (im *imageMount) unmount() error { return nil } if err := im.layer.Release(); err != nil { - return errors.Wrapf(err, "failed to unmount previous build image %s", im.id) + return errors.Wrapf(err, "failed to unmount previous build image %s", im.image.ImageID()) } return nil } -func (im *imageMount) update(imageID string, runConfig *container.Config) { - im.id = imageID - im.runConfig = runConfig -} - -func (im *imageMount) ImageID() string { - return im.id -} - -func (im *imageMount) RunConfig() *container.Config { - return im.runConfig +func (im *imageMount) Image() builder.Image { + return im.image } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 5affd1d5f9..456089d7fb 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -78,7 +78,7 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta } dispatchState.imageID = imageID - b.imageContexts.update(imageID, dispatchState.runConfig) + b.buildStages.update(imageID, dispatchState.runConfig) return nil } @@ -369,7 +369,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression source := b.source var err error if imageSource != nil { - source, err = imageSource.context() + source, err = imageSource.Source() if err != nil { return nil, errors.Wrapf(err, "failed to copy") } @@ -427,7 +427,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression if imageSource != nil { // fast-cache based on imageID - if h, ok := b.imageContexts.getCache(imageSource.id, origPath); ok { + if h, ok := b.imageSources.getCache(imageSource.Image().ImageID(), origPath); ok { copyInfos[0].hash = h.(string) return copyInfos, nil } @@ -473,7 +473,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression hasher.Write([]byte(strings.Join(subfiles, ","))) copyInfos[0].hash = "dir:" + hex.EncodeToString(hasher.Sum(nil)) if imageSource != nil { - b.imageContexts.setCache(imageSource.id, origPath, copyInfos[0].hash) + b.imageSources.setCache(imageSource.Image().ImageID(), origPath, copyInfos[0].hash) } return copyInfos, nil @@ -501,7 +501,7 @@ func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container. fmt.Fprint(b.Stdout, " ---> Using cache\n") logrus.Debugf("[BUILDER] Use cached version: %s", runConfig.Cmd) dispatchState.imageID = string(cache) - b.imageContexts.update(dispatchState.imageID, runConfig) + b.buildStages.update(dispatchState.imageID, runConfig) return true, nil } diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index 4f4a59d545..f61ccd5e65 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -66,15 +66,7 @@ func (m *MockBackend) CopyOnBuild(containerID string, destPath string, srcRoot s return nil } -func (m *MockBackend) HasExperimental() bool { - return false -} - -func (m *MockBackend) SquashImage(from string, to string) (string, error) { - return "", nil -} - -func (m *MockBackend) GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) { +func (m *MockBackend) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) { if m.getImageFunc != nil { return m.getImageFunc(refOrID) } @@ -112,6 +104,6 @@ func (l *mockLayer) Release() error { return nil } -func (l *mockLayer) Mount(_ string) (string, error) { +func (l *mockLayer) Mount() (string, error) { return "mountPath", nil } diff --git a/daemon/build.go b/daemon/build.go index 806f05c392..1bf05ded2c 100644 --- a/daemon/build.go +++ b/daemon/build.go @@ -1,6 +1,7 @@ package daemon import ( + "github.com/Sirupsen/logrus" "github.com/docker/distribution/reference" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" @@ -15,54 +16,62 @@ import ( ) type releaseableLayer struct { - rwLayer layer.RWLayer - release func(layer.RWLayer) error - mount func(string) (layer.RWLayer, error) + layerStore layer.Store + roLayer layer.Layer + rwLayer layer.RWLayer } -func (rl *releaseableLayer) Release() error { - if rl.rwLayer == nil { - return nil +func (rl *releaseableLayer) Mount() (string, error) { + if rl.roLayer == nil { + return "", errors.New("can not mount an image with no root FS") } - rl.rwLayer.Unmount() - return rl.release(rl.rwLayer) -} - -func (rl *releaseableLayer) Mount(imageID string) (string, error) { var err error - rl.rwLayer, err = rl.mount(imageID) + mountID := stringid.GenerateRandomID() + rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, rl.roLayer.ChainID(), nil) if err != nil { return "", errors.Wrap(err, "failed to create rwlayer") } - mountPath, err := rl.rwLayer.Mount("") - if err != nil { - releaseErr := rl.release(rl.rwLayer) - if releaseErr != nil { - err = errors.Wrapf(err, "failed to release rwlayer: %s", releaseErr.Error()) - } - return "", errors.Wrap(err, "failed to mount rwlayer") - } - return mountPath, err + return rl.rwLayer.Mount("") } -func (daemon *Daemon) getReleasableLayerForImage() *releaseableLayer { - mountFunc := func(imageID string) (layer.RWLayer, error) { - img, err := daemon.GetImage(imageID) - if err != nil { - return nil, err - } - mountID := stringid.GenerateRandomID() - return daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) - } +func (rl *releaseableLayer) Release() error { + rl.releaseRWLayer() + return rl.releaseROLayer() +} - releaseFunc := func(rwLayer layer.RWLayer) error { - metadata, err := daemon.layerStore.ReleaseRWLayer(rwLayer) - layer.LogReleaseMetadata(metadata) - return err +func (rl *releaseableLayer) releaseRWLayer() error { + if rl.rwLayer == nil { + return nil } + metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer) + layer.LogReleaseMetadata(metadata) + if err != nil { + logrus.Errorf("Failed to release RWLayer: %s", err) + } + return err +} - return &releaseableLayer{mount: mountFunc, release: releaseFunc} +func (rl *releaseableLayer) releaseROLayer() error { + if rl.roLayer == nil { + return nil + } + metadata, err := rl.layerStore.Release(rl.roLayer) + layer.LogReleaseMetadata(metadata) + return err +} + +func newReleasableLayerForImage(img *image.Image, layerStore layer.Store) (builder.ReleaseableLayer, error) { + if img.RootFS.ChainID() == "" { + return nil, nil + } + // Hold a reference to the image layer so that it can't be removed before + // it is released + roLayer, err := layerStore.Get(img.RootFS.ChainID()) + if err != nil { + return nil, errors.Wrapf(err, "failed to get layer for image %s", img.ImageID()) + } + return &releaseableLayer{layerStore: layerStore, roLayer: roLayer}, nil } // TODO: could this use the regular daemon PullImage ? @@ -75,7 +84,7 @@ func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfi pullRegistryAuth := &types.AuthConfig{} if len(authConfigs) > 0 { - // The request came with a full auth config file, we prefer to use that + // The request came with a full auth config, use it repoInfo, err := daemon.RegistryService.ResolveRepository(ref) if err != nil { return nil, err @@ -91,16 +100,23 @@ func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfi return daemon.GetImage(name) } -// GetImageAndLayer returns an image and releaseable layer for a reference or ID -func (daemon *Daemon) GetImageAndLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) { +// GetImageAndReleasableLayer returns an image and releaseable layer for a reference or ID. +// 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 !opts.ForcePull { image, _ := daemon.GetImage(refOrID) // TODO: shouldn't we error out if error is different from "not found" ? if image != nil { - return image, daemon.getReleasableLayerForImage(), nil + layer, err := newReleasableLayerForImage(image, daemon.layerStore) + return image, layer, err } } image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output) - return image, daemon.getReleasableLayerForImage(), err + if err != nil { + return nil, nil, err + } + layer, err := newReleasableLayerForImage(image, daemon.layerStore) + return image, layer, err }