Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions storage/drivers/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,15 @@ type DriverWithDiffer interface {
DifferTarget(id string) (string, error)
}

// ApplyDiffStaging is an interface for driver who can apply the diff without holding the main storage lock.
// This API is experimental and can be changed without bumping the major version number.
type ApplyDiffStaging interface {
// StartStagingDiffToApply applies the layer in a temporary directory. This can be done without holding the storage lock.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually please copy & paste the detailed description here, e.g. about the unusual CleanupTempDirFunc return value.

StartStagingDiffToApply(options ApplyDiffOpts) (tempdir.CleanupTempDirFunc, *tempdir.StageAddition, int64, error)
// CommitStagedLayer commits the staged layer from StartStagingDiffToApply(). This must be done while the storage lock.
CommitStagedLayer(id string, commit *tempdir.StageAddition) error
}

// Capabilities defines a list of capabilities a driver may implement.
// These capabilities are not required; however, they do determine how a
// graphdriver can be used.
Expand Down
86 changes: 78 additions & 8 deletions storage/drivers/overlay/overlay.go
Original file line number Diff line number Diff line change
Expand Up @@ -2369,21 +2369,91 @@ func (d *Driver) DifferTarget(id string) (string, error) {
return d.getDiffPath(id)
}

// ApplyDiff applies the new layer into a root
func (d *Driver) ApplyDiff(id, parent string, options graphdriver.ApplyDiffOpts) (size int64, err error) {
idMappings := options.Mappings
if idMappings == nil {
idMappings = &idtools.IDMappings{}
// StartStagingDiffToApply applies the new layer into a temporary directory.
// It returns a CleanupTempDirFunc which can nil or set regardless if the function return an error or not.
// CommitFunc is only set when there is no error returned and the int64 value returns the size of the layer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs updating

//
// This API is experimental and can be changed without bumping the major version number.
func (d *Driver) StartStagingDiffToApply(options graphdriver.ApplyDiffOpts) (tempdir.CleanupTempDirFunc, *tempdir.StageAddition, int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A warning that this can run concurrently with any other operations on the driver would be nice (both here and in the interface definition).

// FIXME: how to consolidate with d.getTempDirRoot(id) if we don't have the id?
tempDirRoot := filepath.Join(d.homeDirForImageStore(), tempDirName)
t, err := tempdir.NewTempDir(tempDirRoot)
if err != nil {
return nil, nil, -1, err
}

sa, err := t.StageAddition()
if err != nil {
return nil, nil, -1, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing t.Cleanup?

}

size, err := d.applyDiff(sa.Path, options)
if err != nil {
return t.Cleanup, nil, -1, err
}

return t.Cleanup, sa, size, nil
}

// CommitStagedLayer that was created with StartStagingDiffToApply().
//
// This API is experimental and can be changed without bumping the major version number.
func (d *Driver) CommitStagedLayer(id string, sa *tempdir.StageAddition) error {
applyDir, err := d.getDiffPath(id)
if err != nil {
return err
}

// FIXME: Is there a better way to do this?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I agree, this suggests a refactoring might be possible and beneficial.)

stat, err := system.Stat(applyDir)
if err != nil {
return err
}
if err := os.Chmod(sa.Path, os.FileMode(stat.Mode())); err != nil {
return err
}

if err := os.Chown(sa.Path, int(stat.UID()), int(stat.GID())); err != nil {
return err
}
if d.options.forceMask != nil {
st, err := idtools.GetContainersOverrideXattr(applyDir)
if err != nil {
return err
}
if err := idtools.SetContainersOverrideXattr(sa.Path, st); err != nil {
return err
}
}

// The os.Rename() function used by CommitFunc errors when the target directory already
// exists, as such delete the dir.
if err := os.Remove(applyDir); err != nil {
return err
}

return sa.Commit(applyDir)
}

// ApplyDiff applies the new layer into a root
func (d *Driver) ApplyDiff(id, parent string, options graphdriver.ApplyDiffOpts) (size int64, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Absolutely non-blocking: BTW it seems that nothing is using the parent field any more… we could drop it, OTOH the API stability promise of drivers.Driver is … undefined … or not strictly followed. Maybe worth at least adding a comment in the Driver interface.)

applyDir, err := d.getDiffPath(id)
if err != nil {
return 0, err
}
return d.applyDiff(applyDir, options)
}

// ApplyDiff applies the new layer into a root
func (d *Driver) applyDiff(target string, options graphdriver.ApplyDiffOpts) (size int64, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A warning that this can run concurrently with any other operations on the driver would be nice.

… and that might motivate auditing and documenting which fields of overlay.Driver are immutable after construction.

idMappings := options.Mappings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Non-blocking: So far, all callers were setting options.Mappings… I’d prefer to be clear and consistent about whether that field is mandatory or optional; in that sense, adapting for nil here, nested inside the driver’s call stack, is a bit unexpected.)

if idMappings == nil {
idMappings = &idtools.IDMappings{}
}

logrus.Debugf("Applying tar in %s", applyDir)
logrus.Debugf("Applying tar in %s", target)
// Overlay doesn't need the parent id to apply the diff
if err := untar(options.Diff, applyDir, &archive.TarOptions{
if err := untar(options.Diff, target, &archive.TarOptions{
UIDMaps: idMappings.UIDs(),
GIDMaps: idMappings.GIDs(),
IgnoreChownErrors: d.options.ignoreChownErrors,
Expand All @@ -2394,7 +2464,7 @@ func (d *Driver) ApplyDiff(id, parent string, options graphdriver.ApplyDiffOpts)
return 0, err
}

return directory.Size(applyDir)
return directory.Size(target)
}

func (d *Driver) getComposefsData(id string) string {
Expand Down
3 changes: 3 additions & 0 deletions storage/drivers/overlay/overlay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import (

const driverName = "overlay"

// check that Driver correctly implements the ApplyDiffTemporary interface
var _ graphdriver.ApplyDiffStaging = &Driver{}

func init() {
// Do not sure chroot to speed run time and allow archive
// errors or hangs to be debugged directly from the test process.
Expand Down