From 4ec1fd107e0b5acadb10e90cdc560e976358987d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 6 Feb 2018 13:27:55 -0500 Subject: [PATCH] Refactor commit The goal of this refactor is to make it easier to integrate buildkit and containerd snapshotters. Commit is used from two places (api and build), each calls it with distinct arguments. Refactored to pull out the common commit logic and provide different interfaces for each consumer. Signed-off-by: Daniel Nephin Upstream-commit: daff039049aea6e19a4bda1df2834d14b4198bc0 Component: engine --- .../engine/api/server/router/image/backend.go | 2 +- .../api/server/router/image/image_routes.go | 24 ++-- .../engine/api/types/backend/backend.go | 30 ++-- components/engine/api/types/configs.go | 13 -- components/engine/builder/builder.go | 6 +- .../builder/dockerfile/dispatchers_test.go | 3 +- .../engine/builder/dockerfile/internals.go | 23 ++- .../builder/dockerfile/mockbackend_test.go | 7 +- components/engine/daemon/commit.go | 134 +++++++++++------- .../integration-cli/docker_cli_events_test.go | 2 +- 10 files changed, 136 insertions(+), 108 deletions(-) diff --git a/components/engine/api/server/router/image/backend.go b/components/engine/api/server/router/image/backend.go index 9a588a71a9..831213cdc3 100644 --- a/components/engine/api/server/router/image/backend.go +++ b/components/engine/api/server/router/image/backend.go @@ -21,7 +21,7 @@ type Backend interface { } type containerBackend interface { - Commit(name string, config *backend.ContainerCommitConfig) (imageID string, err error) + CreateImageFromContainer(name string, config *backend.CreateImageConfig) (imageID string, err error) } type imageBackend interface { diff --git a/components/engine/api/server/router/image/image_routes.go b/components/engine/api/server/router/image/image_routes.go index dc16a23af0..633ff02fb3 100644 --- a/components/engine/api/server/router/image/image_routes.go +++ b/components/engine/api/server/router/image/image_routes.go @@ -34,33 +34,29 @@ func (s *imageRouter) postCommit(ctx context.Context, w http.ResponseWriter, r * return err } - cname := r.Form.Get("container") - + // TODO: remove pause arg, and always pause in backend pause := httputils.BoolValue(r, "pause") version := httputils.VersionFromContext(ctx) if r.FormValue("pause") == "" && versions.GreaterThanOrEqualTo(version, "1.13") { pause = true } - c, _, _, err := s.decoder.DecodeConfig(r.Body) + config, _, _, err := s.decoder.DecodeConfig(r.Body) if err != nil && err != io.EOF { //Do not fail if body is empty. return err } - commitCfg := &backend.ContainerCommitConfig{ - ContainerCommitConfig: types.ContainerCommitConfig{ - Pause: pause, - Repo: r.Form.Get("repo"), - Tag: r.Form.Get("tag"), - Author: r.Form.Get("author"), - Comment: r.Form.Get("comment"), - Config: c, - MergeConfigs: true, - }, + commitCfg := &backend.CreateImageConfig{ + Pause: pause, + Repo: r.Form.Get("repo"), + Tag: r.Form.Get("tag"), + Author: r.Form.Get("author"), + Comment: r.Form.Get("comment"), + Config: config, Changes: r.Form["changes"], } - imgID, err := s.backend.Commit(cname, commitCfg) + imgID, err := s.backend.CreateImageFromContainer(r.Form.Get("container"), commitCfg) if err != nil { return err } diff --git a/components/engine/api/types/backend/backend.go b/components/engine/api/types/backend/backend.go index 74cea50035..ce1a4dcdbf 100644 --- a/components/engine/api/types/backend/backend.go +++ b/components/engine/api/types/backend/backend.go @@ -5,7 +5,6 @@ import ( "io" "time" - "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" ) @@ -94,13 +93,26 @@ type ExecProcessConfig struct { User string `json:"user,omitempty"` } -// ContainerCommitConfig is a wrapper around -// types.ContainerCommitConfig that also -// transports configuration changes for a container. -type ContainerCommitConfig struct { - types.ContainerCommitConfig +// CreateImageConfig is the configuration for creating an image from a +// container. +type CreateImageConfig struct { + Repo string + Tag string + Pause bool + Author string + Comment string + Config *container.Config Changes []string - // TODO: ContainerConfig is only used by the dockerfile Builder, so remove it - // once the Builder has been updated to use a different interface - ContainerConfig *container.Config +} + +// CommitConfig is the configuration for creating an image as part of a build. +type CommitConfig struct { + Author string + Comment string + Config *container.Config + ContainerConfig *container.Config + ContainerID string + ContainerMountLabel string + ContainerOS string + ParentImageID string } diff --git a/components/engine/api/types/configs.go b/components/engine/api/types/configs.go index 54d3e39fb9..fd604bce9c 100644 --- a/components/engine/api/types/configs.go +++ b/components/engine/api/types/configs.go @@ -25,19 +25,6 @@ type ContainerRmConfig struct { ForceRemove, RemoveVolume, RemoveLink bool } -// ContainerCommitConfig contains build configs for commit operation, -// and is used when making a commit with the current state of the container. -type ContainerCommitConfig struct { - Pause bool - Repo string - Tag string - Author string - Comment string - // merge container config into commit config before commit - MergeConfigs bool - Config *container.Config -} - // ExecConfig is a small subset of the Config struct that holds the configuration // for the exec feature of docker. type ExecConfig struct { diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index b7d5edea0a..66d4a6dd09 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/builder/builder.go @@ -11,6 +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/containerfs" "golang.org/x/net/context" @@ -39,8 +40,9 @@ type Backend interface { ImageBackend ExecBackend - // Commit creates a new Docker image from an existing Docker container. - Commit(string, *backend.ContainerCommitConfig) (string, error) + // CommitBuildStep creates a new Docker image from the config generated by + // a build step. + CommitBuildStep(backend.CommitConfig) (image.ID, error) // ContainerCreateWorkdir creates the workdir ContainerCreateWorkdir(containerID string) error diff --git a/components/engine/builder/dockerfile/dispatchers_test.go b/components/engine/builder/dockerfile/dispatchers_test.go index 0cdf2ad437..3b5b37b7d6 100644 --- a/components/engine/builder/dockerfile/dispatchers_test.go +++ b/components/engine/builder/dockerfile/dispatchers_test.go @@ -13,6 +13,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/builder/dockerfile/instructions" "github.com/docker/docker/builder/dockerfile/shell" + "github.com/docker/docker/image" "github.com/docker/docker/pkg/system" "github.com/docker/go-connections/nat" "github.com/stretchr/testify/assert" @@ -445,7 +446,7 @@ func TestRunWithBuildArgs(t *testing.T) { assert.Equal(t, strslice.StrSlice{""}, config.Config.Entrypoint) return container.ContainerCreateCreatedBody{ID: "12345"}, nil } - mockBackend.commitFunc = func(cID string, cfg *backend.ContainerCommitConfig) (string, error) { + mockBackend.commitFunc = func(cfg backend.CommitConfig) (image.ID, error) { // Check the runConfig.Cmd sent to commit() assert.Equal(t, origCmd, cfg.Config.Cmd) assert.Equal(t, cachedCmd, cfg.ContainerConfig.Cmd) diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 5ce5f89744..2af4449ab5 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -101,24 +101,17 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta return nil } - commitCfg := &backend.ContainerCommitConfig{ - ContainerCommitConfig: types.ContainerCommitConfig{ - Author: dispatchState.maintainer, - Pause: true, - // TODO: this should be done by Commit() - Config: copyRunConfig(dispatchState.runConfig), - }, + commitCfg := backend.CommitConfig{ + Author: dispatchState.maintainer, + // TODO: this copy should be done by Commit() + Config: copyRunConfig(dispatchState.runConfig), ContainerConfig: containerConfig, + ContainerID: id, } - // Commit the container - imageID, err := b.docker.Commit(id, commitCfg) - if err != nil { - return err - } - - dispatchState.imageID = imageID - return nil + imageID, err := b.docker.CommitBuildStep(commitCfg) + dispatchState.imageID = string(imageID) + return err } func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runConfig *container.Config) error { diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index 6c19f17ebc..6395de190b 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/builder/dockerfile/mockbackend_test.go @@ -10,6 +10,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/containerfs" "golang.org/x/net/context" @@ -18,7 +19,7 @@ import ( // MockBackend implements the builder.Backend interface for unit testing type MockBackend struct { containerCreateFunc func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) - commitFunc func(string, *backend.ContainerCommitConfig) (string, error) + commitFunc func(backend.CommitConfig) (image.ID, error) getImageFunc func(string) (builder.Image, builder.ReleaseableLayer, error) makeImageCacheFunc func(cacheFrom []string) builder.ImageCache } @@ -38,9 +39,9 @@ func (m *MockBackend) ContainerRm(name string, config *types.ContainerRmConfig) return nil } -func (m *MockBackend) Commit(cID string, cfg *backend.ContainerCommitConfig) (string, error) { +func (m *MockBackend) CommitBuildStep(c backend.CommitConfig) (image.ID, error) { if m.commitFunc != nil { - return m.commitFunc(cID, cfg) + return m.commitFunc(c) } return "", nil } diff --git a/components/engine/daemon/commit.go b/components/engine/daemon/commit.go index de621168a9..d70f198ed7 100644 --- a/components/engine/daemon/commit.go +++ b/components/engine/daemon/commit.go @@ -12,7 +12,6 @@ import ( "github.com/docker/docker/api/types/backend" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder/dockerfile" - "github.com/docker/docker/container" "github.com/docker/docker/errdefs" "github.com/docker/docker/image" "github.com/docker/docker/layer" @@ -122,9 +121,10 @@ func merge(userConf, imageConf *containertypes.Config) error { return nil } -// Commit creates a new filesystem image from the current state of a container. -// The image can optionally be tagged into a repository. -func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (string, error) { +// CreateImageFromContainer creates a new image from a container. The container +// config will be updated by applying the change set to the custom config, then +// applying that config over the existing container config. +func (daemon *Daemon) CreateImageFromContainer(name string, c *backend.CreateImageConfig) (string, error) { start := time.Now() container, err := daemon.GetContainer(name) if err != nil { @@ -150,26 +150,51 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str daemon.containerPause(container) defer daemon.containerUnpause(container) } - if !system.IsOSSupported(container.OS) { - return "", system.ErrNotSupportedOperatingSystem - } - if c.MergeConfigs && c.Config == nil { + if c.Config == nil { c.Config = container.Config } - newConfig, err := dockerfile.BuildFromConfig(c.Config, c.Changes, container.OS) if err != nil { return "", err } - - if c.MergeConfigs { - if err := merge(newConfig, container.Config); err != nil { - return "", err - } + if err := merge(newConfig, container.Config); err != nil { + return "", err } - rwTar, err := daemon.exportContainerRw(container) + id, err := daemon.commitImage(backend.CommitConfig{ + Author: c.Author, + Comment: c.Comment, + Config: newConfig, + ContainerConfig: container.Config, + ContainerID: container.ID, + ContainerMountLabel: container.MountLabel, + ContainerOS: container.OS, + ParentImageID: string(container.ImageID), + }) + if err != nil { + return "", err + } + + imageRef, err := daemon.tagCommit(c.Repo, c.Tag, id) + if err != nil { + return "", err + } + daemon.LogContainerEventWithAttributes(container, "commit", map[string]string{ + "comment": c.Comment, + "imageID": id.String(), + "imageRef": imageRef, + }) + containerActions.WithValues("commit").UpdateSince(start) + return id.String(), nil +} + +func (daemon *Daemon) commitImage(c backend.CommitConfig) (image.ID, error) { + layerStore, ok := daemon.layerStores[c.ContainerOS] + if !ok { + return "", system.ErrNotSupportedOperatingSystem + } + rwTar, err := exportContainerRw(layerStore, c.ContainerID, c.ContainerMountLabel) if err != nil { return "", err } @@ -180,35 +205,31 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str }() var parent *image.Image - if container.ImageID == "" { + if c.ParentImageID == "" { parent = new(image.Image) parent.RootFS = image.NewRootFS() } else { - parent, err = daemon.imageStore.Get(container.ImageID) + parent, err = daemon.imageStore.Get(image.ID(c.ParentImageID)) if err != nil { return "", err } } - l, err := daemon.layerStores[container.OS].Register(rwTar, parent.RootFS.ChainID()) + l, err := layerStore.Register(rwTar, parent.RootFS.ChainID()) if err != nil { return "", err } - defer layer.ReleaseAndLog(daemon.layerStores[container.OS], l) + defer layer.ReleaseAndLog(layerStore, l) - containerConfig := c.ContainerConfig - if containerConfig == nil { - containerConfig = container.Config - } cc := image.ChildConfig{ - ContainerID: container.ID, + ContainerID: c.ContainerID, Author: c.Author, Comment: c.Comment, - ContainerConfig: containerConfig, - Config: newConfig, + ContainerConfig: c.ContainerConfig, + Config: c.Config, DiffID: l.DiffID(), } - config, err := json.Marshal(image.NewChildImage(parent, cc, container.OS)) + config, err := json.Marshal(image.NewChildImage(parent, cc, c.ContainerOS)) if err != nil { return "", err } @@ -218,23 +239,27 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str return "", err } - if container.ImageID != "" { - if err := daemon.imageStore.SetParent(id, container.ImageID); err != nil { + if c.ParentImageID != "" { + if err := daemon.imageStore.SetParent(id, image.ID(c.ParentImageID)); err != nil { return "", err } } + return id, nil +} +// TODO: remove from Daemon, move to api backend +func (daemon *Daemon) tagCommit(repo string, tag string, id image.ID) (string, error) { imageRef := "" - if c.Repo != "" { - newTag, err := reference.ParseNormalizedNamed(c.Repo) // todo: should move this to API layer + if repo != "" { + newTag, err := reference.ParseNormalizedNamed(repo) // todo: should move this to API layer if err != nil { return "", err } if !reference.IsNameOnly(newTag) { - return "", errors.Errorf("unexpected repository name: %s", c.Repo) + return "", errors.Errorf("unexpected repository name: %s", repo) } - if c.Tag != "" { - if newTag, err = reference.WithTag(newTag, c.Tag); err != nil { + if tag != "" { + if newTag, err = reference.WithTag(newTag, tag); err != nil { return "", err } } @@ -243,26 +268,17 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str } imageRef = reference.FamiliarString(newTag) } - - attributes := map[string]string{ - "comment": c.Comment, - "imageID": id.String(), - "imageRef": imageRef, - } - daemon.LogContainerEventWithAttributes(container, "commit", attributes) - containerActions.WithValues("commit").UpdateSince(start) - return id.String(), nil + return imageRef, nil } -func (daemon *Daemon) exportContainerRw(container *container.Container) (arch io.ReadCloser, err error) { - // Note: Indexing by OS is safe as only called from `Commit` which has already performed validation - rwlayer, err := daemon.layerStores[container.OS].GetRWLayer(container.ID) +func exportContainerRw(layerStore layer.Store, id, mountLabel string) (arch io.ReadCloser, err error) { + rwlayer, err := layerStore.GetRWLayer(id) if err != nil { return nil, err } defer func() { if err != nil { - daemon.layerStores[container.OS].ReleaseRWLayer(rwlayer) + layerStore.ReleaseRWLayer(rwlayer) } }() @@ -270,7 +286,7 @@ func (daemon *Daemon) exportContainerRw(container *container.Container) (arch io // mount the layer if needed. But the Diff() function for windows requests that // the layer should be mounted when calling it. So we reserve this mount call // until windows driver can implement Diff() interface correctly. - _, err = rwlayer.Mount(container.GetMountLabel()) + _, err = rwlayer.Mount(mountLabel) if err != nil { return nil, err } @@ -283,8 +299,28 @@ func (daemon *Daemon) exportContainerRw(container *container.Container) (arch io return ioutils.NewReadCloserWrapper(archive, func() error { archive.Close() err = rwlayer.Unmount() - daemon.layerStores[container.OS].ReleaseRWLayer(rwlayer) + layerStore.ReleaseRWLayer(rwlayer) return err }), nil } + +// CommitBuildStep is used by the builder to create an image for each step in +// the build. +// +// This method is different from CreateImageFromContainer: +// * it doesn't attempt to validate container state +// * it doesn't send a commit action to metrics +// * it doesn't log a container commit event +// +// This is a temporary shim. Should be removed when builder stops using commit. +func (daemon *Daemon) CommitBuildStep(c backend.CommitConfig) (image.ID, error) { + container, err := daemon.GetContainer(c.ContainerID) + if err != nil { + return "", err + } + c.ContainerMountLabel = container.MountLabel + c.ContainerOS = container.OS + c.ParentImageID = string(container.ImageID) + return daemon.commitImage(c) +} diff --git a/components/engine/integration-cli/docker_cli_events_test.go b/components/engine/integration-cli/docker_cli_events_test.go index be91087b66..bb2978b1b6 100644 --- a/components/engine/integration-cli/docker_cli_events_test.go +++ b/components/engine/integration-cli/docker_cli_events_test.go @@ -601,7 +601,7 @@ func (s *DockerSuite) TestEventsFilterType(c *check.C) { events = strings.Split(strings.TrimSpace(out), "\n") // Events generated by the container that builds the image - c.Assert(events, checker.HasLen, 3, check.Commentf("Events == %s", events)) + c.Assert(events, checker.HasLen, 2, check.Commentf("Events == %s", events)) out, _ = dockerCmd( c,