Merge pull request #36224 from dnephin/refactor-commit

Refactor Daemon.Commit()

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 {

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{
+	commitCfg := &backend.CreateImageConfig{
-		ContainerCommitConfig: types.ContainerCommitConfig{
+		Pause:   pause,
-			Pause:        pause,
+		Repo:    r.Form.Get("repo"),
-			Repo:         r.Form.Get("repo"),
+		Tag:     r.Form.Get("tag"),
-			Tag:          r.Form.Get("tag"),
+		Author:  r.Form.Get("author"),
-			Author:       r.Form.Get("author"),
+		Comment: r.Form.Get("comment"),
-			Comment:      r.Form.Get("comment"),
+		Config:  config,
-			Config:       c,
-			MergeConfigs: true,
-		},
 		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
 	}

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
+// CreateImageConfig is the configuration for creating an image from a
-// types.ContainerCommitConfig that also
+// container.
-// transports configuration changes for a container.
+type CreateImageConfig struct {
-type ContainerCommitConfig struct {
+	Repo    string
-	types.ContainerCommitConfig
+	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
 }

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 {

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.
+	// CommitBuildStep creates a new Docker image from the config generated by
-	Commit(string, *backend.ContainerCommitConfig) (string, error)
+	// a build step.
+	CommitBuildStep(backend.CommitConfig) (image.ID, error)
 	// ContainerCreateWorkdir creates the workdir
 	ContainerCreateWorkdir(containerID string) error
 

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)

builder/dockerfile/internals.go

@@ -101,24 +101,17 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta
 		return nil
 	}
 
-	commitCfg := &backend.ContainerCommitConfig{
+	commitCfg := backend.CommitConfig{
-		ContainerCommitConfig: types.ContainerCommitConfig{
+		Author: dispatchState.maintainer,
-			Author: dispatchState.maintainer,
+		// TODO: this copy should be done by Commit()
-			Pause:  true,
+		Config:          copyRunConfig(dispatchState.runConfig),
-			// TODO: this should be done by Commit()
-			Config: copyRunConfig(dispatchState.runConfig),
-		},
 		ContainerConfig: containerConfig,
+		ContainerID:     id,
 	}
 
-	// Commit the container
+	imageID, err := b.docker.CommitBuildStep(commitCfg)
-	imageID, err := b.docker.Commit(id, commitCfg)
+	dispatchState.imageID = string(imageID)
-	if err != nil {
+	return err
-		return err
-	}
-
-	dispatchState.imageID = imageID
-	return nil
 }
 
 func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runConfig *container.Config) error {

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
 }

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.
+// CreateImageFromContainer creates a new image from a container. The container
-// The image can optionally be tagged into a repository.
+// config will be updated by applying the change set to the custom config, then
-func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (string, error) {
+// 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 err := merge(newConfig, container.Config); err != nil {
-	if c.MergeConfigs {
+		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,
+		ContainerConfig: c.ContainerConfig,
-		Config:          newConfig,
+		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 c.ParentImageID != "" {
-		if err := daemon.imageStore.SetParent(id, container.ImageID); err != nil {
+		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 != "" {
+	if repo != "" {
-		newTag, err := reference.ParseNormalizedNamed(c.Repo) // todo: should move this to API layer
+		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 tag != "" {
-			if newTag, err = reference.WithTag(newTag, c.Tag); err != nil {
+			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)
 	}
-
+	return imageRef, nil
-	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
 }
 
-func (daemon *Daemon) exportContainerRw(container *container.Container) (arch io.ReadCloser, err error) {
+func exportContainerRw(layerStore layer.Store, id, mountLabel string) (arch io.ReadCloser, err error) {
-	// Note: Indexing by OS is safe as only called from `Commit` which has already performed validation
+	rwlayer, err := layerStore.GetRWLayer(id)
-	rwlayer, err := daemon.layerStores[container.OS].GetRWLayer(container.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)
+}

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,