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 <dnephin@docker.com>

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{
-		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
 	}

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
 }

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.
-	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
 

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{
-		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 {

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.
-// 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 err := merge(newConfig, container.Config); err != nil {
+		return "", err
+	}
 
-	if c.MergeConfigs {
-		if err := merge(newConfig, container.Config); err != nil {
-			return "", err
-		}
+	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
 	}
 
-	rwTar, err := daemon.exportContainerRw(container)
+	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)
+}

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,