From dee6b481fe0da1d845261ffff2e610fb05898d3c Mon Sep 17 00:00:00 2001 From: Josh Hawn Date: Wed, 10 Sep 2014 20:30:52 -0700 Subject: [PATCH] Refactor use of graphdriver.Differ Some graphdrivers are Differs and type assertions are made in various places throughout the project. Differ offers some convenience in generating/applying diffs of filesystem layers but for most graphdrivers another code path is taken. This patch brings all of the logic related to filesystem diffs in one place, and simplifies the implementation of some common types like Image, Daemon, and Container. Signed-off-by: Josh Hawn --- archive/changes.go | 7 +- daemon/container.go | 23 ++-- daemon/daemon.go | 41 +------ daemon/graphdriver/aufs/aufs.go | 36 ++++-- daemon/graphdriver/aufs/aufs_test.go | 16 +-- daemon/graphdriver/btrfs/btrfs.go | 6 +- daemon/graphdriver/devmapper/driver.go | 2 +- daemon/graphdriver/driver.go | 41 ++++++- daemon/graphdriver/fsdiff.go | 162 +++++++++++++++++++++++++ daemon/graphdriver/vfs/driver.go | 2 +- graph/graph.go | 31 +---- image/image.go | 87 +------------ 12 files changed, 263 insertions(+), 191 deletions(-) create mode 100644 daemon/graphdriver/fsdiff.go diff --git a/archive/changes.go b/archive/changes.go index 231f3064c..f80f7219b 100644 --- a/archive/changes.go +++ b/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() { diff --git a/daemon/container.go b/daemon/container.go index 014899fc3..963af1b91 100644 --- a/daemon/container.go +++ b/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 { diff --git a/daemon/daemon.go b/daemon/daemon.go index 7ee7948a2..db37d24bf 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -937,46 +937,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) { diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index ebd492938..8dc0f0de1 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/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 diff --git a/daemon/graphdriver/aufs/aufs_test.go b/daemon/graphdriver/aufs/aufs_test.go index 081fb8898..a5ea8579e 100644 --- a/daemon/graphdriver/aufs/aufs_test.go +++ b/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) } diff --git a/daemon/graphdriver/btrfs/btrfs.go b/daemon/graphdriver/btrfs/btrfs.go index c491fd790..1064a2663 100644 --- a/daemon/graphdriver/btrfs/btrfs.go +++ b/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.NewGenericDriverWrapper(driver), nil } type Driver struct { diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go index 5940d5879..215af4015 100644 --- a/daemon/graphdriver/devmapper/driver.go +++ b/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.NewGenericDriverWrapper(d), nil } func (d *Driver) String() string { diff --git a/daemon/graphdriver/driver.go b/daemon/graphdriver/driver.go index 90ed1d816..c389705df 100644 --- a/daemon/graphdriver/driver.go +++ b/daemon/graphdriver/driver.go @@ -19,26 +19,55 @@ const ( type InitFunc func(root string, options []string) (Driver, error) -type Driver interface { +// GenericDriver defines the basic capabilities of a driver. +type GenericDriver 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 attepmts 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) +type Driver interface { + GenericDriver + + // 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 ( diff --git a/daemon/graphdriver/fsdiff.go b/daemon/graphdriver/fsdiff.go new file mode 100644 index 000000000..03d109e93 --- /dev/null +++ b/daemon/graphdriver/fsdiff.go @@ -0,0 +1,162 @@ +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" +) + +// GenericDriverWrapper takes a generic Driver and adds the +// capability of the following methods which it doesn't +// 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) +// Notably, the AUFS driver doesn't need to be wrapped like this. +type GenericDriverWrapper struct { + GenericDriver +} + +// NewGenericDriverWrapper returns a fully functional driver that wraps the given GenericDriver. +func NewGenericDriverWrapper(driver GenericDriver) Driver { + return &GenericDriverWrapper{GenericDriver: driver} +} + +// Diff produces an archive of the changes between the specified +// layer and its parent layer which may be "". +func (gdw *GenericDriverWrapper) Diff(id, parent string) (arch archive.Archive, err error) { + driver := gdw.GenericDriver + + 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 *GenericDriverWrapper) Changes(id, parent string) ([]archive.Change, error) { + driver := gdw.GenericDriver + + 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 *GenericDriverWrapper) ApplyDiff(id, parent string, diff archive.ArchiveReader) (bytes int64, err error) { + driver := gdw.GenericDriver + + // 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 *GenericDriverWrapper) DiffSize(id, parent string) (bytes int64, err error) { + driver := gdw.GenericDriver + + 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 +} diff --git a/daemon/graphdriver/vfs/driver.go b/daemon/graphdriver/vfs/driver.go index 4075892c4..0bbf9d421 100644 --- a/daemon/graphdriver/vfs/driver.go +++ b/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.NewGenericDriverWrapper(d), nil } type Driver struct { diff --git a/graph/graph.go b/graph/graph.go index e6c9cb43d..84a372978 100644 --- a/graph/graph.go +++ b/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 diff --git a/image/image.go b/image/image.go index 468d01293..9e58cfe72 100644 --- a/image/image.go +++ b/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 - } - - 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