diff --git a/api/server/router/image/backend.go b/api/server/router/image/backend.go index 140a8d59fd..dcf554cef3 100644 --- a/api/server/router/image/backend.go +++ b/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/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 249683bab8..618ccdf0d8 100644 --- a/api/server/router/image/image_routes.go +++ b/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/api/types/backend/backend.go b/api/types/backend/backend.go index 3dee4a4e3c..ffd556e944 100644 --- a/api/types/backend/backend.go +++ b/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/api/types/configs.go b/api/types/configs.go index 9faf5748a3..f6537a27f2 100644 --- a/api/types/configs.go +++ b/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/builder/builder.go b/builder/builder.go index 12f3daa1a5..95320c9a6c 100644 --- a/builder/builder.go +++ b/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/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 6b8232e108..a96d579905 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/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/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 3c636fed77..4b4f638b1f 100644 --- a/builder/dockerfile/internals.go +++ b/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/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index bf76c0e2e0..06adb928fd 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/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/daemon/commit.go b/daemon/commit.go index 7b78cecc12..5f2e3c689d 100644 --- a/daemon/commit.go +++ b/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/integration-cli/docker_cli_events_test.go b/integration-cli/docker_cli_events_test.go index be91087b66..bb2978b1b6 100644 --- a/integration-cli/docker_cli_events_test.go +++ b/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,