Merge pull request #14366 from k8s-infra-cherrypick-robot/cherry-pick-14334-to-release/2.4

[release/2.4] erofs: sync a layer before committing its snapshot

docs/snapshotters/erofs.md

@@ -141,6 +141,13 @@ forget to restart containerd after changing the configuration.
       # Enable fsverity support for EROFS layers, default is false
       enable_fsverity = true
 
+      # fsync the snapshot directory, everything in it and its entry in
+      # snapshots/ before a snapshot is committed, so a power loss cannot
+      # leave a committed snapshot without its blob or directory. Blobs served
+      # from layer_content_caches are not synced. Costs a few fsyncs per layer,
+      # default is false
+      fsync_on_commit = true
+
       # Optional: Additional mount options for overlayfs
       ovl_mount_options = []
 

plugins/snapshots/erofs/erofs.go

@@ -51,6 +51,9 @@ type SnapshotterConfig struct {
 	enableFsverity bool
 	// setImmutable enables IMMUTABLE_FL file attribute for EROFS layers
 	setImmutable bool
+	// fsyncOnCommit makes Commit sync the layer blob and the snapshot
+	// directories before the snapshot is recorded as committed
+	fsyncOnCommit bool
 	// defaultSize creates a default size writable layer for active snapshots
 	defaultSize int64
 	remapIDs    bool
@@ -88,6 +91,14 @@ func WithImmutable() Opt {
 	}
 }
 
+// WithFsyncOnCommit makes Commit sync the layer blob and the snapshot
+// directories to disk before the snapshot is recorded as committed
+func WithFsyncOnCommit() Opt {
+	return func(config *SnapshotterConfig) {
+		config.fsyncOnCommit = true
+	}
+}
+
 // WithDmverityMode sets the dm-verity mode: "auto" (default), "on" (required), or "off" (disabled)
 func WithDmverityMode(mode string) Opt {
 	return func(config *SnapshotterConfig) {
@@ -131,6 +142,7 @@ type snapshotter struct {
 	ovlOptions         []string
 	enableFsverity     bool
 	setImmutable       bool
+	fsyncOnCommit      bool
 	defaultWritable    int64
 	blockMode          bool
 	remapIDs           bool
@@ -234,6 +246,7 @@ func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) {
 		ovlOptions:         config.ovlOptions,
 		enableFsverity:     config.enableFsverity,
 		setImmutable:       config.setImmutable,
+		fsyncOnCommit:      config.fsyncOnCommit,
 		defaultWritable:    config.defaultSize,
 		blockMode:          config.defaultSize > 0,
 		remapIDs:           config.remapIDs,
@@ -283,6 +296,33 @@ func (s *snapshotter) layerBlobPath(id string) string {
 	return filepath.Join(s.root, "snapshots", id, "layer.erofs")
 }
 
+// syncDir fsyncs the immediate entries of dir, then dir itself and its parent,
+// so that the files in dir, its entry list and its own entry in the parent are
+// on disk before the caller records them elsewhere. It does not recurse.
+func syncDir(dir string) error {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return fmt.Errorf("failed to read %s for sync: %w", dir, err)
+	}
+	paths := make([]string, 0, len(entries)+2)
+	for _, e := range entries {
+		paths = append(paths, filepath.Join(dir, e.Name()))
+	}
+	paths = append(paths, dir, filepath.Dir(dir))
+	for _, p := range paths {
+		f, err := os.Open(p)
+		if err != nil {
+			return fmt.Errorf("failed to open %s for sync: %w", p, err)
+		}
+		err = f.Sync()
+		f.Close()
+		if err != nil {
+			return fmt.Errorf("failed to sync %s: %w", p, err)
+		}
+	}
+	return nil
+}
+
 func (s *snapshotter) fsMetaPath(id string) string {
 	return filepath.Join(s.root, "snapshots", id, "fsmeta.erofs")
 }
@@ -963,6 +1003,22 @@ func (s *snapshotter) Commit(ctx context.Context, name, key string, opts ...snap
 
 	// Note: dm-verity formatting is handled by the EROFS differ, not here
 
+	// The metadata store syncs its own file when the snapshot is committed, so
+	// with fsync_on_commit enabled the layer is made durable first; otherwise
+	// a power loss shortly after a successful pull can leave a committed
+	// snapshot whose blob is truncated or whose directory does not exist, and
+	// nothing re-pulls a layer whose chain ID already exists. Everything in
+	// the snapshot directory is synced rather than a list of known files, then
+	// the directory and its entry in snapshots/, which was created by a rename
+	// in Prepare and may still be unwritten. A blob served from the layer
+	// content cache lives elsewhere and is not synced; whoever populates the
+	// cache must make it durable.
+	if s.fsyncOnCommit {
+		if err := syncDir(filepath.Join(s.root, "snapshots", id)); err != nil {
+			return err
+		}
+	}
+
 	return s.ms.WithTransaction(ctx, true, func(ctx context.Context) error {
 		if _, err := os.Stat(layerBlob); err != nil {
 			return fmt.Errorf("failed to get the converted erofs blob: %w", err)

plugins/snapshots/erofs/erofs_linux_test.go

@@ -143,6 +143,21 @@ func TestGetCleanupDirectoriesSkipsSnapshotTempDirs(t *testing.T) {
 	assert.Equal(t, []string{orphanDir}, cleanup)
 }
 
+// TestSyncDir checks that a snapshot directory holding files and
+// subdirectories can be synced, and that a directory that cannot be read
+// returns an error instead of being skipped.
+func TestSyncDir(t *testing.T) {
+	root := t.TempDir()
+	snapshotDir := filepath.Join(root, "snapshots", "1")
+	require.NoError(t, os.MkdirAll(filepath.Join(snapshotDir, "fs"), 0700))
+	require.NoError(t, os.WriteFile(filepath.Join(snapshotDir, "layer.erofs"), []byte("erofs"), 0644))
+
+	require.NoError(t, syncDir(snapshotDir))
+
+	err := syncDir(filepath.Join(root, "snapshots", "missing"))
+	require.ErrorIs(t, err, os.ErrNotExist)
+}
+
 // TestWritableSize exercises the LabelSnapshotMaxSize override that the
 // block-mode mkfs path passes to X-containerd.mkfs.size. Covers the
 // happy path (label overrides default), fallback cases (missing, empty,

plugins/snapshots/erofs/plugin/plugin.go

@@ -51,6 +51,13 @@ type Config struct {
 	// If `SetImmutable` is enabled, IMMUTABLE_FL will be set on layer blobs.
 	SetImmutable bool `toml:"set_immutable"`
 
+	// FsyncOnCommit makes Commit fsync the snapshot directory, everything in
+	// it and its entry in snapshots/ before the snapshot is recorded, so that
+	// a power loss cannot leave a committed snapshot without its blob or
+	// directory. Blobs served from a layer content cache are not synced. Off
+	// by default, like the diff service's sync_fs.
+	FsyncOnCommit bool `toml:"fsync_on_commit"`
+
 	// DefaultSize is the default size of a writable layer in string
 	DefaultSize string `toml:"default_size"`
 
@@ -114,6 +121,10 @@ func init() {
 				opts = append(opts, erofs.WithImmutable())
 			}
 
+			if config.FsyncOnCommit {
+				opts = append(opts, erofs.WithFsyncOnCommit())
+			}
+
 			if config.DefaultSize != "" {
 				size, err := units.RAMInBytes(config.DefaultSize)
 				if err != nil {