snapshot: cleanup diffApply and prepare for DiffOp

This breaks the giant blob that was the diffApply function into two
separate parts, a differ and an applier, which results in more modular
code that should be easier to follow and easier to make any future
updates to. For example, if we want to optimize by allowing differ and
applier to run in parallel in the future, that's straightforward now.

There are also some fixes that weren't needed for MergeOp, but will be
for DiffOp, such as correctly handling the case where a deletion is
applied that is under parent directories which don't exist yet (the
correct behavior is, surprisingly, to create the parent directories as
that is what the image import/export code ends up doing).

Signed-off-by: Erik Sipsma <erik@sipsma.dev>

snapshot/diffapply_windows.go

@@ -11,12 +11,8 @@ import (
 	"github.com/pkg/errors"
 )
 
-func diffApply(ctx context.Context, lowerMountable, upperMountable, applyMountable Mountable, useHardlink bool, externalHardlinks map[uint64]struct{}, createWhiteoutDeletes bool) error {
+func (sn *mergeSnapshotter) diffApply(ctx context.Context, dest Mountable, diffs ...Diff) (_ snapshots.Usage, rerr error) {
-	return errors.New("diff apply not supported on windows")
+	return snapshots.Usage{}, errors.New("diffApply not yet supported on windows")
-}
-
-func diskUsage(ctx context.Context, mountable Mountable, externalHardlinks map[uint64]struct{}) (snapshots.Usage, error) {
-	return snapshots.Usage{}, errors.New("disk usage not supported on windows")
 }
 
 func needsUserXAttr(ctx context.Context, sn Snapshotter, lm leases.Manager) (bool, error) {

snapshot/localmounter.go

@@ -1,11 +1,9 @@
 package snapshot
 
 import (
-	"strings"
 	"sync"
 
 	"github.com/containerd/containerd/mount"
-	"github.com/pkg/errors"
 )
 
 type Mounter interface {
@@ -32,33 +30,3 @@ type localMounter struct {
 	target    string
 	release   func() error
 }
-
-// RWDirMount extracts out just a writable directory from the provided mounts and returns it.
-// It's intended to supply the directory to which changes being made to the mount can be
-// written directly. A writable directory includes an upperdir if provided an overlay or a rw
-// bind mount source. If the mount doesn't have a writable directory, an error is returned.
-func getRWDir(mounts []mount.Mount) (string, error) {
-	if len(mounts) != 1 {
-		return "", errors.New("cannot extract writable directory from zero or multiple mounts")
-	}
-	mnt := mounts[0]
-	switch mnt.Type {
-	case "overlay":
-		for _, opt := range mnt.Options {
-			if strings.HasPrefix(opt, "upperdir=") {
-				upperdir := strings.SplitN(opt, "=", 2)[1]
-				return upperdir, nil
-			}
-		}
-		return "", errors.New("cannot extract writable directory from overlay mount without upperdir")
-	case "bind", "rbind":
-		for _, opt := range mnt.Options {
-			if opt == "ro" {
-				return "", errors.New("cannot extract writable directory from read-only bind mount")
-			}
-		}
-		return mnt.Source, nil
-	default:
-		return "", errors.Errorf("cannot extract writable directory from unhandled mount type %q", mnt.Type)
-	}
-}

snapshot/merge.go

@@ -56,40 +56,36 @@ type mergeSnapshotter struct {
 	lm leases.Manager
 
 	// Whether we should try to implement merges by hardlinking between underlying directories
-	useHardlinks bool
+	tryCrossSnapshotLink bool
 
-	// Whether the snapshotter is overlay-based, which enables some required behavior like
+	// Whether the snapshotter is overlay-based, which enables some some optimizations like
-	// creation of whiteout devices to represent deletes in addition to some optimizations
+	// using the first merge input as the parent snapshot.
-	// like using the first merge input as the parent snapshot.
 	overlayBased bool
 }
 
 func NewMergeSnapshotter(ctx context.Context, sn Snapshotter, lm leases.Manager) MergeSnapshotter {
 	name := sn.Name()
-	_, useHardlinks := hardlinkMergeSnapshotters[name]
+	_, tryCrossSnapshotLink := hardlinkMergeSnapshotters[name]
 	_, overlayBased := overlayBasedSnapshotters[name]
 
 	if overlayBased && userns.RunningInUserNS() {
 		// When using an overlay-based snapshotter, if we are running rootless on a pre-5.11
 		// kernel, we will not have userxattr. This results in opaque xattrs not being visible
-		// to us and thus breaking the overlay-optimized differ. This also means that there are
+		// to us and thus breaking the overlay-optimized differ.
-		// cases where in order to apply a deletion, we'd need to create a whiteout device but
-		// may not have access to one to hardlink, so we just fall back to not using hardlinks
-		// at all in this case.
 		userxattr, err := needsUserXAttr(ctx, sn, lm)
 		if err != nil {
 			bklog.G(ctx).Debugf("failed to check user xattr: %v", err)
-			useHardlinks = false
+			tryCrossSnapshotLink = false
 		} else {
-			useHardlinks = userxattr
+			tryCrossSnapshotLink = userxattr
 		}
 	}
 
 	return &mergeSnapshotter{
-		Snapshotter:  sn,
+		Snapshotter:          sn,
-		lm:           lm,
+		lm:                   lm,
-		useHardlinks: useHardlinks,
+		tryCrossSnapshotLink: tryCrossSnapshotLink,
-		overlayBased: overlayBased,
+		overlayBased:         overlayBased,
 	}
 }
 
@@ -99,7 +95,8 @@ func (sn *mergeSnapshotter) Merge(ctx context.Context, key string, diffs []Diff,
 		// Overlay-based snapshotters can skip the base snapshot of the merge (if one exists) and just use it as the
 		// parent of the merge snapshot. Other snapshotters will start empty (with baseKey set to "").
 		// Find the baseKey by following the chain of diffs for as long as it follows the pattern of the current lower
-		// being the parent of the current upper and equal to the previous upper.
+		// being the parent of the current upper and equal to the previous upper, i.e.:
+		// Diff("", A) -> Diff(A, B) -> Diff(B, C), etc.
 		var baseIndex int
 		for i, diff := range diffs {
 			info, err := sn.Stat(ctx, diff.Upper)
@@ -134,37 +131,9 @@ func (sn *mergeSnapshotter) Merge(ctx context.Context, key string, diffs []Diff,
 		return errors.Wrapf(err, "failed to get mounts of %q", key)
 	}
 
-	// externalHardlinks keeps track of which inodes have been hard-linked between snapshots (which is
+	usage, err := sn.diffApply(ctx, applyMounts, diffs...)
-	// enabled when sn.useHardlinks is set to true)
-	externalHardlinks := make(map[uint64]struct{})
-
-	for _, diff := range diffs {
-		var lowerMounts Mountable
-		if diff.Lower != "" {
-			viewID := identity.NewID()
-			var err error
-			lowerMounts, err = sn.View(tempLeaseCtx, viewID, diff.Lower)
-			if err != nil {
-				return errors.Wrapf(err, "failed to get mounts of lower %q", diff.Lower)
-			}
-		}
-
-		viewID := identity.NewID()
-		upperMounts, err := sn.View(tempLeaseCtx, viewID, diff.Upper)
-		if err != nil {
-			return errors.Wrapf(err, "failed to get mounts of upper %q", diff.Upper)
-		}
-
-		err = diffApply(tempLeaseCtx, lowerMounts, upperMounts, applyMounts, sn.useHardlinks, externalHardlinks, sn.overlayBased)
-		if err != nil {
-			return err
-		}
-	}
-
-	// save the correctly calculated usage as a label on the committed key
-	usage, err := diskUsage(ctx, applyMounts, externalHardlinks)
 	if err != nil {
-		return errors.Wrap(err, "failed to get disk usage of diff apply merge")
+		return errors.Wrap(err, "failed to apply diffs")
 	}
 	if err := sn.Commit(ctx, key, prepareKey, withMergeUsage(usage)); err != nil {
 		return errors.Wrapf(err, "failed to commit %q", key)

snapshot/snapshotter_test.go

@@ -99,7 +99,7 @@ func newSnapshotter(ctx context.Context, snapshotterName string) (_ context.Cont
 	lm := leaseutil.WithNamespace(ctdmetadata.NewLeaseManager(mdb), ns)
 	snapshotter := NewMergeSnapshotter(ctx, FromContainerdSnapshotter(snapshotterName, mdb.Snapshotter(snapshotterName), nil), lm).(*mergeSnapshotter)
 	if noHardlink {
-		snapshotter.useHardlinks = false
+		snapshotter.tryCrossSnapshotLink = false
 	}
 
 	leaseID := identity.NewID()
@@ -198,6 +198,7 @@ func TestMerge(t *testing.T) {
 				requireMtime(t, filepath.Join(root, "c"), ts.Add(6*time.Second))
 				requireMtime(t, filepath.Join(root, "foo"), ts.Add(4*time.Second))
 				requireMtime(t, filepath.Join(root, "bar"), ts.Add(12*time.Second))
+				requireMtime(t, filepath.Join(root, "hardlink"), ts.Add(12*time.Second))
 				requireMtime(t, filepath.Join(root, "bar/A"), ts.Add(12*time.Second))
 				requireMtime(t, filepath.Join(root, "bar/B"), ts.Add(9*time.Second))
 				requireMtime(t, filepath.Join(root, "symlink"), ts.Add(3*time.Second))

util/overlay/overlay_linux.go

@@ -46,7 +46,7 @@ func GetUpperdir(lower, upper []mount.Mount) (string, error) {
 		case "overlay":
 			// lower snapshot is an overlay mount of multiple layers
 			var err error
-			lowerlayers, err = getOverlayLayers(lowerM)
+			lowerlayers, err = GetOverlayLayers(lowerM)
 			if err != nil {
 				return "", err
 			}
@@ -59,7 +59,7 @@ func GetUpperdir(lower, upper []mount.Mount) (string, error) {
 		if upperM.Type != "overlay" {
 			return "", errors.Errorf("upper snapshot isn't overlay mounted (type = %q)", upperM.Type)
 		}
-		upperlayers, err := getOverlayLayers(upperM)
+		upperlayers, err := GetOverlayLayers(upperM)
 		if err != nil {
 			return "", err
 		}
@@ -83,8 +83,8 @@ func GetUpperdir(lower, upper []mount.Mount) (string, error) {
 	return upperdir, nil
 }
 
-// getOverlayLayers returns all layer directories of an overlayfs mount.
+// GetOverlayLayers returns all layer directories of an overlayfs mount.
-func getOverlayLayers(m mount.Mount) ([]string, error) {
+func GetOverlayLayers(m mount.Mount) ([]string, error) {
 	var u string
 	var uFound bool
 	var l []string // l[0] = bottommost