From 7de95cbc4cb35d372442cd4ed1ce6c837568d22a Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Thu, 15 Jun 2023 14:28:57 +0000 Subject: [PATCH] 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 --- snapshots/blockfile/blockfile.go | 71 ++++++++++++++-- .../blockfile/blockfile_loopsetup_test.go | 81 ++++++++++++++----- 2 files changed, 125 insertions(+), 27 deletions(-) diff --git a/snapshots/blockfile/blockfile.go b/snapshots/blockfile/blockfile.go index dd6257936..16666f199 100644 --- 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 index 06b4dd306..bda55f58b 100644 --- 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 { + if err := scratchDevFile.Truncate(loopbackSize); err != nil { + scratchDevFile.Close() + return nil, fmt.Errorf("failed to resize %s file: %w", scratch, err) + } + + if err := scratchDevFile.Sync(); err != nil { + scratchDevFile.Close() + return nil, fmt.Errorf("failed to sync %s file: %w", scratch, err) + } + scratchDevFile.Close() + + if out, err := exec.Command(mkfs, scratch).CombinedOutput(); err != nil { return nil, fmt.Errorf("failed to make ext4 filesystem (out: %q): %w", out, 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 - } + defaultOpts := []string{"loop", "direct-io", "sync"} - scratch := filepath.Join(t.TempDir(), "scratch") - err = fs.CopyFile(scratch, loop.File) - if err != nil { + 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 }