snapshots/blockfile: deflaky the testsuite

* Use direct-io mode to reduce IO. * Add testViewHook helper to recovery the backing file since the ext4 might need writable permission to handle recovery. If the backing file needs recovery and it's for View snapshot, the readonly mount will cause error. * Use 8 MiB as capacity to reduce the IO. Signed-off-by: Wei Fu <[email protected]>
containerd · kzys · Jun 26, 2023 · Jun 15, 2023 · Jun 15, 2023 · Jun 15, 2023
commit 7de95cbc4cb35d372442cd4ed1ce6c837568d22a
diff --git a/snapshots/blockfile/blockfile.go b/snapshots/blockfile/blockfile.go
@@ -19,6 +19,7 @@ package blockfile
 import (
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 
@@ -27,10 +28,11 @@ import (
 	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/snapshots"
 	"github.com/containerd/containerd/snapshots/storage"
-
-	"github.com/containerd/continuity/fs"
 )
 
+// viewHookHelper is only used in test for recover the filesystem.
+type viewHookHelper func(backingFile string, fsType string, defaultOpts []string) error
+
 // SnapshotterConfig holds the configurable properties for the blockfile snapshotter
 type SnapshotterConfig struct {
 	// recreateScratch is whether scratch should be recreated even
@@ -44,6 +46,17 @@ type SnapshotterConfig struct {
 
 	// mountOptions are the base options added to the mount (defaults to ["loop"])
 	mountOptions []string
+
+	// testViewHookHelper is used to fsck or mount with rw to handle
+	// the recovery. If we mount ro for view snapshot, we might hit
+	// the issue like
+	//
+	//  (ext4) INFO: recovery required on readonly filesystem
+	//  (ext4) write access unavailable, cannot proceed (try mounting with noload)
+	//
+	// FIXME(fuweid): I don't hit the readonly issue in ssd storage. But it's
+	// easy to reproduce it in slow-storage.
+	testViewHookHelper viewHookHelper
 }
 
 // Opt is an option to configure the overlay snapshotter
@@ -55,7 +68,7 @@ func WithScratchFile(src string) Opt {
 	return func(root string, config *SnapshotterConfig) {
 		config.scratchGenerator = func(dst string) error {
 			// Copy src to dst
-			if err := fs.CopyFile(dst, src); err != nil {
+			if err := copyFileWithSync(dst, src); err != nil {
 				return fmt.Errorf("failed to copy scratch: %w", err)
 			}
 			return nil
@@ -78,12 +91,30 @@ func WithMountOptions(options []string) Opt {
 
 }
 
+// WithRecreateScratch is used to determine that scratch should be recreated
+// even if already exists.
+func WithRecreateScratch(recreate bool) Opt {
+	return func(root string, config *SnapshotterConfig) {
+		config.recreateScratch = recreate
+	}
+}
+
+// withViewHookHelper introduces hook for preparing snapshot for View. It
+// should be used in test only.
+func withViewHookHelper(fn viewHookHelper) Opt {
+	return func(_ string, config *SnapshotterConfig) {
+		config.testViewHookHelper = fn
+	}
+}
+
 type snapshotter struct {
 	root    string
 	scratch string
 	fsType  string
 	options []string
 	ms      *storage.MetaStore
+
+	testViewHookHelper viewHookHelper
 }
 
 // NewSnapshotter returns a Snapshotter which copies layers on the underlying
@@ -140,6 +171,8 @@ func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) {
 		fsType:  config.fsType,
 		options: config.mountOptions,
 		ms:      ms,
+
+		testViewHookHelper: config.testViewHookHelper,
 	}, nil
 }
 
@@ -343,18 +376,27 @@ func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k
 			return fmt.Errorf("failed to create snapshot: %w", err)
 		}
 
+		var path string
 		if len(s.ParentIDs) == 0 || s.Kind == snapshots.KindActive {
-			path := o.getBlockFile(s.ID)
+			path = o.getBlockFile(s.ID)
 
 			if len(s.ParentIDs) > 0 {
-				if err = fs.CopyFile(path, o.getBlockFile(s.ParentIDs[0])); err != nil {
+				if err = copyFileWithSync(path, o.getBlockFile(s.ParentIDs[0])); err != nil {
 					return fmt.Errorf("copying of parent failed: %w", err)
 				}
 			} else {
-				if err = fs.CopyFile(path, o.scratch); err != nil {
+				if err = copyFileWithSync(path, o.scratch); err != nil {
 					return fmt.Errorf("copying of scratch failed: %w", err)
 				}
 			}
+		} else {
+			path = o.getBlockFile(s.ParentIDs[0])
+		}
+
+		if o.testViewHookHelper != nil {
+			if err := o.testViewHookHelper(path, o.fsType, o.options); err != nil {
+				return fmt.Errorf("failed to handle the viewHookHelper: %w", err)
+			}
 		}
 
 		return nil
@@ -401,3 +443,20 @@ func (o *snapshotter) mounts(s storage.Snapshot) []mount.Mount {
 func (o *snapshotter) Close() error {
 	return o.ms.Close()
 }
+
+func copyFileWithSync(target, source string) error {
+	src, err := os.Open(source)
+	if err != nil {
+		return fmt.Errorf("failed to open source %s: %w", source, err)
+	}
+	defer src.Close()
+	tgt, err := os.Create(target)
+	if err != nil {
+		return fmt.Errorf("failed to open target %s: %w", target, err)
+	}
+	defer tgt.Close()
+	defer tgt.Sync()
+
+	_, err = io.Copy(tgt, src)
+	return err
+}
diff --git a/snapshots/blockfile/blockfile_loopsetup_test.go b/snapshots/blockfile/blockfile_loopsetup_test.go
@@ -26,9 +26,6 @@ import (
 	"testing"
 
 	"github.com/containerd/containerd/mount"
-	"github.com/containerd/continuity/fs"
-	"github.com/containerd/continuity/testutil/loopback"
-	"golang.org/x/sys/unix"
 )
 
 func setupSnapshotter(t *testing.T) ([]Opt, error) {
@@ -37,52 +34,94 @@ func setupSnapshotter(t *testing.T) ([]Opt, error) {
 		t.Skipf("Could not find mkfs.ext4: %v", err)
 	}
 
-	loopbackSize := int64(128 << 20) // 128 MB
+	loopbackSize := int64(8 << 20) // 8 MB
 	if os.Getpagesize() > 4096 {
 		loopbackSize = int64(650 << 20) // 650 MB
 	}
 
-	loop, err := loopback.New(loopbackSize)
+	scratch := filepath.Join(t.TempDir(), "scratch")
+	scratchDevFile, err := os.OpenFile(scratch, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create %s: %w", scratch, err)
 	}
-	defer loop.Close()
 
-	if out, err := exec.Command(mkfs, loop.Device).CombinedOutput(); err != nil {
-		return nil, fmt.Errorf("failed to make ext4 filesystem (out: %q): %w", out, err)
+	if err := scratchDevFile.Truncate(loopbackSize); err != nil {
+		scratchDevFile.Close()
+		return nil, fmt.Errorf("failed to resize %s file: %w", scratch, err)
 	}
-	// sync after a mkfs on the loopback before trying to mount the device
-	unix.Sync()
 
-	if err := testMount(t, loop.Device); err != nil {
-		return nil, err
+	if err := scratchDevFile.Sync(); err != nil {
+		scratchDevFile.Close()
+		return nil, fmt.Errorf("failed to sync %s file: %w", scratch, err)
 	}
+	scratchDevFile.Close()
 
-	scratch := filepath.Join(t.TempDir(), "scratch")
-	err = fs.CopyFile(scratch, loop.File)
-	if err != nil {
+	if out, err := exec.Command(mkfs, scratch).CombinedOutput(); err != nil {
+		return nil, fmt.Errorf("failed to make ext4 filesystem (out: %q): %w", out, err)
+	}
+
+	defaultOpts := []string{"loop", "direct-io", "sync"}
+
+	if err := testMount(t, scratch, defaultOpts); err != nil {
 		return nil, err
 	}
 
 	return []Opt{
 		WithScratchFile(scratch),
 		WithFSType("ext4"),
-		WithMountOptions([]string{"loop", "sync"}),
+		WithMountOptions(defaultOpts),
+		WithRecreateScratch(false), // reduce IO presure in CI
+		withViewHookHelper(testViewHook),
 	}, nil
 }
 
-func testMount(t *testing.T, device string) error {
+func testMount(t *testing.T, scratchFile string, opts []string) error {
 	root, err := os.MkdirTemp(t.TempDir(), "")
 	if err != nil {
 		return err
 	}
 	defer os.RemoveAll(root)
 
-	if out, err := exec.Command("mount", device, root).CombinedOutput(); err != nil {
-		return fmt.Errorf("failed to mount device %s (out: %q): %w", device, out, err)
+	m := []mount.Mount{
+		{
+			Type:    "ext4",
+			Source:  scratchFile,
+			Options: opts,
+		},
 	}
+
+	if err := mount.All(m, root); err != nil {
+		return fmt.Errorf("failed to mount device %s: %w", scratchFile, err)
+	}
+
 	if err := os.Remove(filepath.Join(root, "lost+found")); err != nil {
 		return err
 	}
-	return mount.UnmountAll(root, unix.MNT_DETACH)
+	return mount.UnmountAll(root, 0)
+}
+
+func testViewHook(backingFile string, fsType string, defaultOpts []string) error {
+	root, err := os.MkdirTemp("", "blockfile-testViewHook-XXXX")
+	if err != nil {
+		return err
+	}
+	defer os.RemoveAll(root)
+
+	// FIXME(fuweid): Mount with rw to force fs to handle recover
+	mountOpts := []mount.Mount{
+		{
+			Type:    fsType,
+			Source:  backingFile,
+			Options: defaultOpts,
+		},
+	}
+
+	if err := mount.All(mountOpts, root); err != nil {
+		return fmt.Errorf("failed to mount device %s: %w", backingFile, err)
+	}
+
+	if err := mount.UnmountAll(root, 0); err != nil {
+		return fmt.Errorf("failed to unmount device %s: %w", backingFile, err)
+	}
+	return nil
 }