Merge pull request #7989 from jlhawn/graphdriver_differ_refactor

Refactor use of graphdriver.Differ

archive/changes.go

@@ -313,7 +313,8 @@ func collectFileInfo(sourceDir string) (*FileInfo, error) {
 	return root, nil
 }
 
-// Compare two directories and generate an array of Change objects describing the changes
+// ChangesDirs compares two directories and generates an array of Change objects describing the changes.
+// If oldDir is "", then all files in newDir will be Add-Changes.
 func ChangesDirs(newDir, oldDir string) ([]Change, error) {
 	var (
 		oldRoot, newRoot *FileInfo
@@ -321,7 +322,9 @@ func ChangesDirs(newDir, oldDir string) ([]Change, error) {
 		errs             = make(chan error, 2)
 	)
 	go func() {
-		oldRoot, err1 = collectFileInfo(oldDir)
+		if oldDir != "" {
+			oldRoot, err1 = collectFileInfo(oldDir)
+		}
 		errs <- err1
 	}()
 	go func() {

daemon/container.go

@@ -18,7 +18,6 @@ import (
 
 	"github.com/docker/docker/archive"
 	"github.com/docker/docker/daemon/execdriver"
-	"github.com/docker/docker/daemon/graphdriver"
 	"github.com/docker/docker/engine"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/links"
@@ -755,21 +754,13 @@ func (container *Container) GetSize() (int64, int64) {
 	}
 	defer container.Unmount()
 
-	if differ, ok := driver.(graphdriver.Differ); ok {
-		sizeRw, err = differ.DiffSize(container.ID)
-		if err != nil {
-			log.Errorf("Warning: driver %s couldn't return diff size of container %s: %s", driver, container.ID, err)
-			// FIXME: GetSize should return an error. Not changing it now in case
-			// there is a side-effect.
-			sizeRw = -1
-		}
-	} else {
-		changes, _ := container.changes()
-		if changes != nil {
-			sizeRw = archive.ChangesSize(container.basefs, changes)
-		} else {
-			sizeRw = -1
-		}
+	initID := fmt.Sprintf("%s-init", container.ID)
+	sizeRw, err = driver.DiffSize(container.ID, initID)
+	if err != nil {
+		log.Errorf("Warning: driver %s couldn't return diff size of container %s: %s", driver, container.ID, err)
+		// FIXME: GetSize should return an error. Not changing it now in case
+		// there is a side-effect.
+		sizeRw = -1
 	}
 
 	if _, err = os.Stat(container.basefs); err != nil {

daemon/daemon.go

@@ -935,46 +935,13 @@ func (daemon *Daemon) Unmount(container *Container) error {
 }
 
 func (daemon *Daemon) Changes(container *Container) ([]archive.Change, error) {
-	if differ, ok := daemon.driver.(graphdriver.Differ); ok {
-		return differ.Changes(container.ID)
-	}
-	cDir, err := daemon.driver.Get(container.ID, "")
-	if err != nil {
-		return nil, fmt.Errorf("Error getting container rootfs %s from driver %s: %s", container.ID, container.daemon.driver, err)
-	}
-	defer daemon.driver.Put(container.ID)
-	initDir, err := daemon.driver.Get(container.ID+"-init", "")
-	if err != nil {
-		return nil, fmt.Errorf("Error getting container init rootfs %s from driver %s: %s", container.ID, container.daemon.driver, err)
-	}
-	defer daemon.driver.Put(container.ID + "-init")
-	return archive.ChangesDirs(cDir, initDir)
+	initID := fmt.Sprintf("%s-init", container.ID)
+	return daemon.driver.Changes(container.ID, initID)
 }
 
 func (daemon *Daemon) Diff(container *Container) (archive.Archive, error) {
-	if differ, ok := daemon.driver.(graphdriver.Differ); ok {
-		return differ.Diff(container.ID)
-	}
-
-	changes, err := daemon.Changes(container)
-	if err != nil {
-		return nil, err
-	}
-
-	cDir, err := daemon.driver.Get(container.ID, "")
-	if err != nil {
-		return nil, fmt.Errorf("Error getting container rootfs %s from driver %s: %s", container.ID, container.daemon.driver, err)
-	}
-
-	archive, err := archive.ExportChanges(cDir, changes)
-	if err != nil {
-		return nil, err
-	}
-	return ioutils.NewReadCloserWrapper(archive, func() error {
-		err := archive.Close()
-		daemon.driver.Put(container.ID)
-		return err
-	}), nil
+	initID := fmt.Sprintf("%s-init", container.ID)
+	return daemon.driver.Diff(container.ID, initID)
 }
 
 func (daemon *Daemon) Run(c *Container, pipes *execdriver.Pipes, startCallback execdriver.StartCallback) (int, error) {

daemon/graphdriver/aufs/aufs.go

@@ -294,23 +294,44 @@ func (a *Driver) Put(id string) {
 	}
 }
 
-// Returns an archive of the contents for the id
-func (a *Driver) Diff(id string) (archive.Archive, error) {
+// Diff produces an archive of the changes between the specified
+// layer and its parent layer which may be "".
+func (a *Driver) Diff(id, parent string) (archive.Archive, error) {
+	// AUFS doesn't need the parent layer to produce a diff.
 	return archive.TarWithOptions(path.Join(a.rootPath(), "diff", id), &archive.TarOptions{
 		Compression: archive.Uncompressed,
 	})
 }
 
-func (a *Driver) ApplyDiff(id string, diff archive.ArchiveReader) error {
+func (a *Driver) applyDiff(id string, diff archive.ArchiveReader) error {
 	return archive.Untar(diff, path.Join(a.rootPath(), "diff", id), nil)
 }
 
-// Returns the size of the contents for the id
-func (a *Driver) DiffSize(id string) (int64, error) {
+// DiffSize calculates the changes between the specified id
+// and its parent and returns the size in bytes of the changes
+// relative to its base filesystem directory.
+func (a *Driver) DiffSize(id, parent string) (bytes int64, err error) {
+	// AUFS doesn't need the parent layer to calculate the diff size.
 	return utils.TreeSize(path.Join(a.rootPath(), "diff", id))
 }
 
-func (a *Driver) Changes(id string) ([]archive.Change, error) {
+// ApplyDiff extracts the changeset from the given diff into the
+// layer with the specified id and parent, returning the size of the
+// new layer in bytes.
+func (a *Driver) ApplyDiff(id, parent string, diff archive.ArchiveReader) (bytes int64, err error) {
+	// AUFS doesn't need the parent id to apply the diff.
+	if err = a.applyDiff(id, diff); err != nil {
+		return
+	}
+
+	return a.DiffSize(id, parent)
+}
+
+// Changes produces a list of changes between the specified layer
+// and its parent layer. If parent is "", then all changes will be ADD changes.
+func (a *Driver) Changes(id, parent string) ([]archive.Change, error) {
+	// AUFS doesn't have snapshots, so we need to get changes from all parent
+	// layers.
 	layers, err := a.getParentLayerPaths(id)
 	if err != nil {
 		return nil, err
@@ -323,9 +344,6 @@ func (a *Driver) getParentLayerPaths(id string) ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	if len(parentIds) == 0 {
-		return nil, fmt.Errorf("Dir %s does not have any parent layers", id)
-	}
 	layers := make([]string, len(parentIds))
 
 	// Get the diff paths for all the parent ids

daemon/graphdriver/aufs/aufs_test.go

@@ -330,7 +330,7 @@ func TestGetDiff(t *testing.T) {
 	}
 	f.Close()
 
-	a, err := d.Diff("1")
+	a, err := d.Diff("1", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -374,7 +374,7 @@ func TestChanges(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	changes, err := d.Changes("2")
+	changes, err := d.Changes("2", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -413,7 +413,7 @@ func TestChanges(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	changes, err = d.Changes("3")
+	changes, err = d.Changes("3", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -465,7 +465,7 @@ func TestDiffSize(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	diffSize, err := d.DiffSize("1")
+	diffSize, err := d.DiffSize("1", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -507,7 +507,7 @@ func TestChildDiffSize(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	diffSize, err := d.DiffSize("1")
+	diffSize, err := d.DiffSize("1", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -519,7 +519,7 @@ func TestChildDiffSize(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	diffSize, err = d.DiffSize("2")
+	diffSize, err = d.DiffSize("2", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -602,7 +602,7 @@ func TestApplyDiff(t *testing.T) {
 	}
 	f.Close()
 
-	diff, err := d.Diff("1")
+	diff, err := d.Diff("1", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -614,7 +614,7 @@ func TestApplyDiff(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if err := d.ApplyDiff("3", diff); err != nil {
+	if err := d.applyDiff("3", diff); err != nil {
 		t.Fatal(err)
 	}
 

daemon/graphdriver/btrfs/btrfs.go

@@ -44,9 +44,11 @@ func Init(home string, options []string) (graphdriver.Driver, error) {
 		return nil, err
 	}
 
-	return &Driver{
+	driver := &Driver{
 		home: home,
-	}, nil
+	}
+
+	return graphdriver.NaiveDiffDriver(driver), nil
 }
 
 type Driver struct {

daemon/graphdriver/devmapper/driver.go

@@ -43,7 +43,7 @@ func Init(home string, options []string) (graphdriver.Driver, error) {
 		home:      home,
 	}
 
-	return d, nil
+	return graphdriver.NaiveDiffDriver(d), nil
 }
 
 func (d *Driver) String() string {

daemon/graphdriver/driver.go

@@ -19,26 +19,56 @@ const (
 
 type InitFunc func(root string, options []string) (Driver, error)
 
-type Driver interface {
+// ProtoDriver defines the basic capabilities of a driver.
+// This interface exists solely to be a minimum set of methods
+// for client code which choose not to implement the entire Driver
+// interface and use the NaiveDiffDriver wrapper constructor.
+//
+// Use of ProtoDriver directly by client code is not recommended.
+type ProtoDriver interface {
+	// String returns a string representation of this driver.
 	String() string
-
+	// Create creates a new, empty, filesystem layer with the
+	// specified id and parent. Parent may be "".
 	Create(id, parent string) error
+	// Remove attempts to remove the filesystem layer with this id.
 	Remove(id string) error
-
+	// Get returns the mountpoint for the layered filesystem referred
+	// to by this id. You can optionally specify a mountLabel or "".
+	// Returns the absolute path to the mounted layered filesystem.
 	Get(id, mountLabel string) (dir string, err error)
+	// Put releases the system resources for the specified id,
+	// e.g, unmounting layered filesystem.
 	Put(id string)
+	// Exists returns whether a filesystem layer with the specified
+	// ID exists on this driver.
 	Exists(id string) bool
-
+	// Status returns a set of key-value pairs which give low
+	// level diagnostic status about this driver.
 	Status() [][2]string
-
+	// Cleanup performs necessary tasks to release resources
+	// held by the driver, e.g., unmounting all layered filesystems
+	// known to this driver.
 	Cleanup() error
 }
 
-type Differ interface {
-	Diff(id string) (archive.Archive, error)
-	Changes(id string) ([]archive.Change, error)
-	ApplyDiff(id string, diff archive.ArchiveReader) error
-	DiffSize(id string) (bytes int64, err error)
+// Driver is the interface for layered/snapshot file system drivers.
+type Driver interface {
+	ProtoDriver
+	// Diff produces an archive of the changes between the specified
+	// layer and its parent layer which may be "".
+	Diff(id, parent string) (archive.Archive, error)
+	// Changes produces a list of changes between the specified layer
+	// and its parent layer. If parent is "", then all changes will be ADD changes.
+	Changes(id, parent string) ([]archive.Change, error)
+	// ApplyDiff extracts the changeset from the given diff into the
+	// layer with the specified id and parent, returning the size of the
+	// new layer in bytes.
+	ApplyDiff(id, parent string, diff archive.ArchiveReader) (bytes int64, err error)
+	// DiffSize calculates the changes between the specified id
+	// and its parent and returns the size in bytes of the changes
+	// relative to its base filesystem directory.
+	DiffSize(id, parent string) (bytes int64, err error)
 }
 
 var (

daemon/graphdriver/fsdiff.go

@@ -0,0 +1,165 @@
+package graphdriver
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/docker/docker/archive"
+	"github.com/docker/docker/pkg/ioutils"
+	"github.com/docker/docker/pkg/log"
+	"github.com/docker/docker/utils"
+)
+
+// naiveDiffDriver takes a ProtoDriver and adds the
+// capability of the Diffing methods which it may or may not
+// support on its own. See the comment on the exported
+// NaiveDiffDriver function below.
+// Notably, the AUFS driver doesn't need to be wrapped like this.
+type naiveDiffDriver struct {
+	ProtoDriver
+}
+
+// NaiveDiffDriver returns a fully functional driver that wraps the
+// given ProtoDriver and adds the capability of the following methods which
+// it may or may not support on its own:
+//     Diff(id, parent string) (archive.Archive, error)
+//     Changes(id, parent string) ([]archive.Change, error)
+//     ApplyDiff(id, parent string, diff archive.ArchiveReader) (bytes int64, err error)
+//     DiffSize(id, parent string) (bytes int64, err error)
+func NaiveDiffDriver(driver ProtoDriver) Driver {
+	return &naiveDiffDriver{ProtoDriver: driver}
+}
+
+// Diff produces an archive of the changes between the specified
+// layer and its parent layer which may be "".
+func (gdw *naiveDiffDriver) Diff(id, parent string) (arch archive.Archive, err error) {
+	driver := gdw.ProtoDriver
+
+	layerFs, err := driver.Get(id, "")
+	if err != nil {
+		return nil, err
+	}
+
+	defer func() {
+		if err != nil {
+			driver.Put(id)
+		}
+	}()
+
+	if parent == "" {
+		archive, err := archive.Tar(layerFs, archive.Uncompressed)
+		if err != nil {
+			return nil, err
+		}
+		return ioutils.NewReadCloserWrapper(archive, func() error {
+			err := archive.Close()
+			driver.Put(id)
+			return err
+		}), nil
+	}
+
+	parentFs, err := driver.Get(parent, "")
+	if err != nil {
+		return nil, err
+	}
+	defer driver.Put(parent)
+
+	changes, err := archive.ChangesDirs(layerFs, parentFs)
+	if err != nil {
+		return nil, err
+	}
+
+	archive, err := archive.ExportChanges(layerFs, changes)
+	if err != nil {
+		return nil, err
+	}
+
+	return ioutils.NewReadCloserWrapper(archive, func() error {
+		err := archive.Close()
+		driver.Put(id)
+		return err
+	}), nil
+}
+
+// Changes produces a list of changes between the specified layer
+// and its parent layer. If parent is "", then all changes will be ADD changes.
+func (gdw *naiveDiffDriver) Changes(id, parent string) ([]archive.Change, error) {
+	driver := gdw.ProtoDriver
+
+	layerFs, err := driver.Get(id, "")
+	if err != nil {
+		return nil, err
+	}
+	defer driver.Put(id)
+
+	parentFs := ""
+
+	if parent != "" {
+		parentFs, err = driver.Get(parent, "")
+		if err != nil {
+			return nil, err
+		}
+		defer driver.Put(parent)
+	}
+
+	return archive.ChangesDirs(layerFs, parentFs)
+}
+
+// ApplyDiff extracts the changeset from the given diff into the
+// layer with the specified id and parent, returning the size of the
+// new layer in bytes.
+func (gdw *naiveDiffDriver) ApplyDiff(id, parent string, diff archive.ArchiveReader) (bytes int64, err error) {
+	driver := gdw.ProtoDriver
+
+	// Mount the root filesystem so we can apply the diff/layer.
+	layerFs, err := driver.Get(id, "")
+	if err != nil {
+		return
+	}
+	defer driver.Put(id)
+
+	start := time.Now().UTC()
+	log.Debugf("Start untar layer")
+	if err = archive.ApplyLayer(layerFs, diff); err != nil {
+		return
+	}
+	log.Debugf("Untar time: %vs", time.Now().UTC().Sub(start).Seconds())
+
+	if parent == "" {
+		return utils.TreeSize(layerFs)
+	}
+
+	parentFs, err := driver.Get(parent, "")
+	if err != nil {
+		err = fmt.Errorf("Driver %s failed to get image parent %s: %s", driver, parent, err)
+		return
+	}
+	defer driver.Put(parent)
+
+	changes, err := archive.ChangesDirs(layerFs, parentFs)
+	if err != nil {
+		return
+	}
+
+	return archive.ChangesSize(layerFs, changes), nil
+}
+
+// DiffSize calculates the changes between the specified layer
+// and its parent and returns the size in bytes of the changes
+// relative to its base filesystem directory.
+func (gdw *naiveDiffDriver) DiffSize(id, parent string) (bytes int64, err error) {
+	driver := gdw.ProtoDriver
+
+	changes, err := gdw.Changes(id, parent)
+	if err != nil {
+		return
+	}
+
+	layerFs, err := driver.Get(id, "")
+	if err != nil {
+		return
+	}
+	defer driver.Put(id)
+
+	return archive.ChangesSize(layerFs, changes), nil
+}

daemon/graphdriver/vfs/driver.go

@@ -19,7 +19,7 @@ func Init(home string, options []string) (graphdriver.Driver, error) {
 	d := &Driver{
 		home: home,
 	}
-	return d, nil
+	return graphdriver.NaiveDiffDriver(d), nil
 }
 
 type Driver struct {

graph/graph.go

@@ -100,27 +100,9 @@ func (graph *Graph) Get(name string) (*image.Image, error) {
 	img.SetGraph(graph)
 
 	if img.Size < 0 {
-		rootfs, err := graph.driver.Get(img.ID, "")
+		size, err := graph.driver.DiffSize(img.ID, img.Parent)
 		if err != nil {
-			return nil, fmt.Errorf("Driver %s failed to get image rootfs %s: %s", graph.driver, img.ID, err)
-		}
-		defer graph.driver.Put(img.ID)
-
-		var size int64
-		if img.Parent == "" {
-			if size, err = utils.TreeSize(rootfs); err != nil {
-				return nil, err
-			}
-		} else {
-			parentFs, err := graph.driver.Get(img.Parent, "")
-			if err != nil {
-				return nil, err
-			}
-			changes, err := archive.ChangesDirs(rootfs, parentFs)
-			if err != nil {
-				return nil, err
-			}
-			size = archive.ChangesSize(rootfs, changes)
+			return nil, fmt.Errorf("unable to calculate size of image id %q: %s", img.ID, err)
 		}
 
 		img.Size = size
@@ -197,14 +179,9 @@ func (graph *Graph) Register(img *image.Image, jsonData []byte, layerData archiv
 	if err := graph.driver.Create(img.ID, img.Parent); err != nil {
 		return fmt.Errorf("Driver %s failed to create image rootfs %s: %s", graph.driver, img.ID, err)
 	}
-	// Mount the root filesystem so we can apply the diff/layer
-	rootfs, err := graph.driver.Get(img.ID, "")
-	if err != nil {
-		return fmt.Errorf("Driver %s failed to get image rootfs %s: %s", graph.driver, img.ID, err)
-	}
-	defer graph.driver.Put(img.ID)
+	// Apply the diff/layer
 	img.SetGraph(graph)
-	if err := image.StoreImage(img, jsonData, layerData, tmp, rootfs); err != nil {
+	if err := image.StoreImage(img, jsonData, layerData, tmp); err != nil {
 		return err
 	}
 	// Commit

image/image.go

@@ -10,8 +10,6 @@ import (
 	"time"
 
 	"github.com/docker/docker/archive"
-	"github.com/docker/docker/daemon/graphdriver"
-	"github.com/docker/docker/pkg/ioutils"
 	"github.com/docker/docker/pkg/log"
 	"github.com/docker/docker/runconfig"
 	"github.com/docker/docker/utils"
@@ -72,51 +70,18 @@ func LoadImage(root string) (*Image, error) {
 	return img, nil
 }
 
-func StoreImage(img *Image, jsonData []byte, layerData archive.ArchiveReader, root, layer string) error {
+func StoreImage(img *Image, jsonData []byte, layerData archive.ArchiveReader, root string) error {
 	// Store the layer
 	var (
 		size   int64
 		err    error
 		driver = img.graph.Driver()
 	)
-	if err := os.MkdirAll(layer, 0755); err != nil {
-		return err
-	}
 
 	// If layerData is not nil, unpack it into the new layer
 	if layerData != nil {
-		if differ, ok := driver.(graphdriver.Differ); ok {
-			if err := differ.ApplyDiff(img.ID, layerData); err != nil {
-				return err
-			}
-
-			if size, err = differ.DiffSize(img.ID); err != nil {
-				return err
-			}
-		} else {
-			start := time.Now().UTC()
-			log.Debugf("Start untar layer")
-			if err := archive.ApplyLayer(layer, layerData); err != nil {
-				return err
-			}
-			log.Debugf("Untar time: %vs", time.Now().UTC().Sub(start).Seconds())
-
-			if img.Parent == "" {
-				if size, err = utils.TreeSize(layer); err != nil {
-					return err
-				}
-			} else {
-				parent, err := driver.Get(img.Parent, "")
-				if err != nil {
-					return err
-				}
-				defer driver.Put(img.Parent)
-				changes, err := archive.ChangesDirs(layer, parent)
-				if err != nil {
-					return err
-				}
-				size = archive.ChangesSize(layer, changes)
-			}
+		if size, err = driver.ApplyDiff(img.ID, img.Parent, layerData); err != nil {
+			return err
 		}
 	}
 
@@ -178,52 +143,10 @@ func (img *Image) TarLayer() (arch archive.Archive, err error) {
 	if img.graph == nil {
 		return nil, fmt.Errorf("Can't load storage driver for unregistered image %s", img.ID)
 	}
-	driver := img.graph.Driver()
-	if differ, ok := driver.(graphdriver.Differ); ok {
-		return differ.Diff(img.ID)
-	}
-
-	imgFs, err := driver.Get(img.ID, "")
-	if err != nil {
-		return nil, err
-	}
-
-	defer func() {
-		if err != nil {
-			driver.Put(img.ID)
-		}
-	}()
 
-	if img.Parent == "" {
-		archive, err := archive.Tar(imgFs, archive.Uncompressed)
-		if err != nil {
-			return nil, err
-		}
-		return ioutils.NewReadCloserWrapper(archive, func() error {
-			err := archive.Close()
-			driver.Put(img.ID)
-			return err
-		}), nil
-	}
+	driver := img.graph.Driver()
 
-	parentFs, err := driver.Get(img.Parent, "")
-	if err != nil {
-		return nil, err
-	}
-	defer driver.Put(img.Parent)
-	changes, err := archive.ChangesDirs(imgFs, parentFs)
-	if err != nil {
-		return nil, err
-	}
-	archive, err := archive.ExportChanges(imgFs, changes)
-	if err != nil {
-		return nil, err
-	}
-	return ioutils.NewReadCloserWrapper(archive, func() error {
-		err := archive.Close()
-		driver.Put(img.ID)
-		return err
-	}), nil
+	return driver.Diff(img.ID, img.Parent)
 }
 
 // Image includes convenience proxy functions to its graph