From 3b71cfd407bdf0b0e5a8c576dff02cd3d33c89ab Mon Sep 17 00:00:00 2001 From: Danny Canter Date: Wed, 7 Dec 2022 23:37:44 -0800 Subject: [PATCH 1/2] metastore: Add WithTransaction convenience method Most snapshotters end up manually handling the rollback logic, either by calling `t.Rollback()` in every failure path, setting up a custom defer func to log on certain errors, or just deferring `t.Rollback()` even for `snapshotter.Commit()` which *will* cause `t.Rollback()` to return an error afaict, but it's just never checked and luckily bolt handles this alright... The devmapper snapshotter has a solution to this which is to have a method that starts either a read-only or writable transaction inside the method, and you pass in a callback to do your bidding and any failures are rolled back, and if it's writable will handle the commit for you. This seems like the right model to me, it removes the burden from the snapshot author to remember to either defer/call rollback in every method for every failure case. This change exposes the convenience method from devmapper to the snapshots/storage package as a method off of `storage.MetaStore` and moves over the devmapper snapshotter to use this. Signed-off-by: Danny Canter --- snapshots/devmapper/snapshotter.go | 73 +++++++----------------------- snapshots/storage/metastore.go | 47 +++++++++++++++++++ 2 files changed, 64 insertions(+), 56 deletions(-) diff --git a/snapshots/devmapper/snapshotter.go b/snapshots/devmapper/snapshotter.go index 601b95212..7494b3817 100644 --- a/snapshots/devmapper/snapshotter.go +++ b/snapshots/devmapper/snapshotter.go @@ -42,11 +42,14 @@ import ( type fsType string const ( - metadataFileName = "metadata.db" - fsTypeExt4 fsType = "ext4" - fsTypeExt2 fsType = "ext2" - fsTypeXFS fsType = "xfs" - devmapperSnapshotFsType = "containerd.io/snapshot/devmapper/fstype" + fsTypeExt4 fsType = "ext4" + fsTypeExt2 fsType = "ext2" + fsTypeXFS fsType = "xfs" +) + +const ( + metadataFileName = "metadata.db" + devmapperSnapshotFsType = "containerd.io/snapshot/devmapper/fstype" ) type closeFunc func() error @@ -111,7 +114,7 @@ func (s *Snapshotter) Stat(ctx context.Context, key string) (snapshots.Info, err err error ) - err = s.withTransaction(ctx, false, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, false, func(ctx context.Context) error { _, info, _, err = storage.GetInfo(ctx, key) return err }) @@ -124,7 +127,7 @@ func (s *Snapshotter) Update(ctx context.Context, info snapshots.Info, fieldpath log.G(ctx).Debugf("update: %s", strings.Join(fieldpaths, ", ")) var err error - err = s.withTransaction(ctx, true, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, true, func(ctx context.Context) error { info, err = storage.UpdateInfo(ctx, info, fieldpaths...) return err }) @@ -143,7 +146,7 @@ func (s *Snapshotter) Usage(ctx context.Context, key string) (snapshots.Usage, e usage snapshots.Usage ) - err = s.withTransaction(ctx, false, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, false, func(ctx context.Context) error { id, info, usage, err = storage.GetInfo(ctx, key) if err != nil { return err @@ -183,7 +186,7 @@ func (s *Snapshotter) Mounts(ctx context.Context, key string) ([]mount.Mount, er err error ) - err = s.withTransaction(ctx, false, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, false, func(ctx context.Context) error { snap, err = storage.GetSnapshot(ctx, key) return err }) @@ -206,7 +209,7 @@ func (s *Snapshotter) Prepare(ctx context.Context, key, parent string, opts ...s err error ) - err = s.withTransaction(ctx, true, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, true, func(ctx context.Context) error { mounts, err = s.createSnapshot(ctx, snapshots.KindActive, key, parent, opts...) return err }) @@ -223,7 +226,7 @@ func (s *Snapshotter) View(ctx context.Context, key, parent string, opts ...snap err error ) - err = s.withTransaction(ctx, true, func(ctx context.Context) error { + err = s.store.WithTransaction(ctx, true, func(ctx context.Context) error { mounts, err = s.createSnapshot(ctx, snapshots.KindView, key, parent, opts...) return err }) @@ -237,7 +240,7 @@ func (s *Snapshotter) View(ctx context.Context, key, parent string, opts ...snap func (s *Snapshotter) Commit(ctx context.Context, name, key string, opts ...snapshots.Opt) error { log.G(ctx).WithFields(logrus.Fields{"name": name, "key": key}).Debug("commit") - return s.withTransaction(ctx, true, func(ctx context.Context) error { + return s.store.WithTransaction(ctx, true, func(ctx context.Context) error { id, snapInfo, _, err := storage.GetInfo(ctx, key) if err != nil { return err @@ -294,7 +297,7 @@ func (s *Snapshotter) Commit(ctx context.Context, name, key string, opts ...snap func (s *Snapshotter) Remove(ctx context.Context, key string) error { log.G(ctx).WithField("key", key).Debug("remove") - return s.withTransaction(ctx, true, func(ctx context.Context) error { + return s.store.WithTransaction(ctx, true, func(ctx context.Context) error { return s.removeDevice(ctx, key) }) } @@ -330,7 +333,7 @@ func (s *Snapshotter) removeDevice(ctx context.Context, key string) error { // Walk iterates through all metadata Info for the stored snapshots and calls the provided function for each. func (s *Snapshotter) Walk(ctx context.Context, fn snapshots.WalkFunc, fs ...string) error { log.G(ctx).Debug("walk") - return s.withTransaction(ctx, false, func(ctx context.Context) error { + return s.store.WithTransaction(ctx, false, func(ctx context.Context) error { return storage.WalkInfo(ctx, fn, fs...) }) } @@ -530,48 +533,6 @@ func (s *Snapshotter) buildMounts(ctx context.Context, snap storage.Snapshot, fi return mounts } -// withTransaction wraps fn callback with containerd's meta store transaction. -// If callback returns an error or transaction is not writable, database transaction will be discarded. -func (s *Snapshotter) withTransaction(ctx context.Context, writable bool, fn func(ctx context.Context) error) error { - ctx, trans, err := s.store.TransactionContext(ctx, writable) - if err != nil { - return err - } - - var result *multierror.Error - - err = fn(ctx) - if err != nil { - result = multierror.Append(result, err) - } - - // Always rollback if transaction is not writable - if err != nil || !writable { - if terr := trans.Rollback(); terr != nil { - log.G(ctx).WithError(terr).Error("failed to rollback transaction") - result = multierror.Append(result, fmt.Errorf("rollback failed: %w", terr)) - } - } else { - if terr := trans.Commit(); terr != nil { - log.G(ctx).WithError(terr).Error("failed to commit transaction") - result = multierror.Append(result, fmt.Errorf("commit failed: %w", terr)) - } - } - - if err := result.ErrorOrNil(); err != nil { - log.G(ctx).WithError(err).Debug("snapshotter error") - - // Unwrap if just one error - if len(result.Errors) == 1 { - return result.Errors[0] - } - - return err - } - - return nil -} - // Cleanup cleans up all removed and unused resources func (s *Snapshotter) Cleanup(ctx context.Context) error { log.G(ctx).Debug("cleanup") diff --git a/snapshots/storage/metastore.go b/snapshots/storage/metastore.go index 6ba2f15bf..fe39fd2ed 100644 --- a/snapshots/storage/metastore.go +++ b/snapshots/storage/metastore.go @@ -26,7 +26,9 @@ import ( "fmt" "sync" + "github.com/containerd/containerd/log" "github.com/containerd/containerd/snapshots" + "github.com/hashicorp/go-multierror" bolt "go.etcd.io/bbolt" ) @@ -104,6 +106,51 @@ func (ms *MetaStore) TransactionContext(ctx context.Context, writable bool) (con return ctx, tx, nil } +// TransactionCallback represents a callback to be invoked while under a metastore transaction. +type TransactionCallback func(ctx context.Context) error + +// WithTransaction is a convenience method to run a function `fn` while holding a meta store transaction. +// If the callback `fn` returns an error or the transaction is not writable, the database transaction will be discarded. +func (ms *MetaStore) WithTransaction(ctx context.Context, writable bool, fn TransactionCallback) error { + ctx, trans, err := ms.TransactionContext(ctx, writable) + if err != nil { + return err + } + + var result *multierror.Error + err = fn(ctx) + if err != nil { + result = multierror.Append(result, err) + } + + // Always rollback if transaction is not writable + if err != nil || !writable { + if terr := trans.Rollback(); terr != nil { + log.G(ctx).WithError(terr).Error("failed to rollback transaction") + + result = multierror.Append(result, fmt.Errorf("rollback failed: %w", terr)) + } + } else { + if terr := trans.Commit(); terr != nil { + log.G(ctx).WithError(terr).Error("failed to commit transaction") + + result = multierror.Append(result, fmt.Errorf("commit failed: %w", terr)) + } + } + + if err := result.ErrorOrNil(); err != nil { + log.G(ctx).WithError(err).Debug("snapshotter error") + + // Unwrap if just one error + if len(result.Errors) == 1 { + return result.Errors[0] + } + return err + } + + return nil +} + // Close closes the metastore and any underlying database connections func (ms *MetaStore) Close() error { ms.dbL.Lock() From aa8a389c51088acefc8b60611b1c61857a41f2a6 Mon Sep 17 00:00:00 2001 From: Danny Canter Date: Wed, 14 Dec 2022 02:49:56 -0800 Subject: [PATCH 2/2] overlay snapshotter: Make use of WithTransaction Move the overlay snapshotter over to using the WithTransaction convenience method. This simplifies needing to check if we need to rollback a transaction and saves us from needing to manually Commit ourselves. Signed-off-by: Danny Canter --- snapshots/overlay/overlay.go | 318 ++++++++++++++--------------------- 1 file changed, 130 insertions(+), 188 deletions(-) diff --git a/snapshots/overlay/overlay.go b/snapshots/overlay/overlay.go index 67c1d71f3..dba85ca69 100644 --- a/snapshots/overlay/overlay.go +++ b/snapshots/overlay/overlay.go @@ -127,15 +127,13 @@ func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) { // // Should be used for parent resolution, existence checks and to discern // the kind of snapshot. -func (o *snapshotter) Stat(ctx context.Context, key string) (snapshots.Info, error) { - ctx, t, err := o.ms.TransactionContext(ctx, false) - if err != nil { - return snapshots.Info{}, err - } - defer t.Rollback() - id, info, _, err := storage.GetInfo(ctx, key) - if err != nil { - return snapshots.Info{}, err +func (o *snapshotter) Stat(ctx context.Context, key string) (info snapshots.Info, err error) { + var id string + if err := o.ms.WithTransaction(ctx, false, func(ctx context.Context) error { + id, info, _, err = storage.GetInfo(ctx, key) + return err + }); err != nil { + return info, err } if o.upperdirLabel { @@ -144,47 +142,29 @@ func (o *snapshotter) Stat(ctx context.Context, key string) (snapshots.Info, err } info.Labels[upperdirKey] = o.upperPath(id) } - return info, nil } -func (o *snapshotter) Update(ctx context.Context, info snapshots.Info, fieldpaths ...string) (snapshots.Info, error) { - ctx, t, err := o.ms.TransactionContext(ctx, true) - if err != nil { - return snapshots.Info{}, err - } - - rollback := true - defer func() { - if rollback { - if rerr := t.Rollback(); rerr != nil { - log.G(ctx).WithError(rerr).Warn("failed to rollback transaction") - } - } - }() - - info, err = storage.UpdateInfo(ctx, info, fieldpaths...) - if err != nil { - return snapshots.Info{}, err - } - - if o.upperdirLabel { - id, _, _, err := storage.GetInfo(ctx, info.Name) +func (o *snapshotter) Update(ctx context.Context, info snapshots.Info, fieldpaths ...string) (newInfo snapshots.Info, err error) { + err = o.ms.WithTransaction(ctx, true, func(ctx context.Context) error { + newInfo, err = storage.UpdateInfo(ctx, info, fieldpaths...) if err != nil { - return snapshots.Info{}, err + return err } - if info.Labels == nil { - info.Labels = make(map[string]string) + + if o.upperdirLabel { + id, _, _, err := storage.GetInfo(ctx, newInfo.Name) + if err != nil { + return err + } + if newInfo.Labels == nil { + newInfo.Labels = make(map[string]string) + } + newInfo.Labels[upperdirKey] = o.upperPath(id) } - info.Labels[upperdirKey] = o.upperPath(id) - } - - rollback = false - if err := t.Commit(); err != nil { - return snapshots.Info{}, err - } - - return info, nil + return nil + }) + return newInfo, err } // Usage returns the resources taken by the snapshot identified by key. @@ -193,16 +173,17 @@ func (o *snapshotter) Update(ctx context.Context, info snapshots.Info, fieldpath // "upper") directory and may take some time. // // For committed snapshots, the value is returned from the metadata database. -func (o *snapshotter) Usage(ctx context.Context, key string) (snapshots.Usage, error) { - ctx, t, err := o.ms.TransactionContext(ctx, false) - if err != nil { - return snapshots.Usage{}, err - } - id, info, usage, err := storage.GetInfo(ctx, key) - t.Rollback() // transaction no longer needed at this point. - - if err != nil { - return snapshots.Usage{}, err +func (o *snapshotter) Usage(ctx context.Context, key string) (_ snapshots.Usage, err error) { + var ( + usage snapshots.Usage + info snapshots.Info + id string + ) + if err := o.ms.WithTransaction(ctx, false, func(ctx context.Context) error { + id, info, usage, err = storage.GetInfo(ctx, key) + return err + }); err != nil { + return usage, err } if info.Kind == snapshots.KindActive { @@ -212,10 +193,8 @@ func (o *snapshotter) Usage(ctx context.Context, key string) (snapshots.Usage, e // TODO(stevvooe): Consider not reporting an error in this case. return snapshots.Usage{}, err } - usage = snapshots.Usage(du) } - return usage, nil } @@ -231,117 +210,91 @@ func (o *snapshotter) View(ctx context.Context, key, parent string, opts ...snap // called on an read-write or readonly transaction. // // This can be used to recover mounts after calling View or Prepare. -func (o *snapshotter) Mounts(ctx context.Context, key string) ([]mount.Mount, error) { - ctx, t, err := o.ms.TransactionContext(ctx, false) - if err != nil { +func (o *snapshotter) Mounts(ctx context.Context, key string) (_ []mount.Mount, err error) { + var s storage.Snapshot + if err := o.ms.WithTransaction(ctx, false, func(ctx context.Context) error { + s, err = storage.GetSnapshot(ctx, key) + if err != nil { + return fmt.Errorf("failed to get active mount: %w", err) + } + return nil + }); err != nil { return nil, err } - s, err := storage.GetSnapshot(ctx, key) - t.Rollback() - if err != nil { - return nil, fmt.Errorf("failed to get active mount: %w", err) - } return o.mounts(s), nil } func (o *snapshotter) Commit(ctx context.Context, name, key string, opts ...snapshots.Opt) error { - ctx, t, err := o.ms.TransactionContext(ctx, true) - if err != nil { - return err - } - - defer func() { + return o.ms.WithTransaction(ctx, true, func(ctx context.Context) error { + // grab the existing id + id, _, _, err := storage.GetInfo(ctx, key) if err != nil { - if rerr := t.Rollback(); rerr != nil { - log.G(ctx).WithError(rerr).Warn("failed to rollback transaction") - } + return err } - }() - // grab the existing id - id, _, _, err := storage.GetInfo(ctx, key) - if err != nil { - return err - } + usage, err := fs.DiskUsage(ctx, o.upperPath(id)) + if err != nil { + return err + } - usage, err := fs.DiskUsage(ctx, o.upperPath(id)) - if err != nil { - return err - } - - if _, err = storage.CommitActive(ctx, key, name, snapshots.Usage(usage), opts...); err != nil { - return fmt.Errorf("failed to commit snapshot: %w", err) - } - return t.Commit() + if _, err = storage.CommitActive(ctx, key, name, snapshots.Usage(usage), opts...); err != nil { + return fmt.Errorf("failed to commit snapshot %s: %w", key, err) + } + return nil + }) } // Remove abandons the snapshot identified by key. The snapshot will // immediately become unavailable and unrecoverable. Disk space will // be freed up on the next call to `Cleanup`. func (o *snapshotter) Remove(ctx context.Context, key string) (err error) { - ctx, t, err := o.ms.TransactionContext(ctx, true) - if err != nil { - return err - } + var removals []string + // Remove directories after the transaction is closed, failures must not + // return error since the transaction is committed with the removal + // key no longer available. defer func() { - if err != nil { - if rerr := t.Rollback(); rerr != nil { - log.G(ctx).WithError(rerr).Warn("failed to rollback transaction") + if err == nil { + for _, dir := range removals { + if err := os.RemoveAll(dir); err != nil { + log.G(ctx).WithError(err).WithField("path", dir).Warn("failed to remove directory") + } } } }() - - _, _, err = storage.Remove(ctx, key) - if err != nil { - return fmt.Errorf("failed to remove: %w", err) - } - - if !o.asyncRemove { - var removals []string - removals, err = o.getCleanupDirectories(ctx) + return o.ms.WithTransaction(ctx, true, func(ctx context.Context) error { + _, _, err = storage.Remove(ctx, key) if err != nil { - return fmt.Errorf("unable to get directories for removal: %w", err) + return fmt.Errorf("failed to remove snapshot %s: %w", key, err) } - // Remove directories after the transaction is closed, failures must not - // return error since the transaction is committed with the removal - // key no longer available. - defer func() { - if err == nil { - for _, dir := range removals { - if err := os.RemoveAll(dir); err != nil { - log.G(ctx).WithError(err).WithField("path", dir).Warn("failed to remove directory") - } - } + if !o.asyncRemove { + removals, err = o.getCleanupDirectories(ctx) + if err != nil { + return fmt.Errorf("unable to get directories for removal: %w", err) } - }() - - } - - return t.Commit() + } + return nil + }) } // Walk the snapshots. func (o *snapshotter) Walk(ctx context.Context, fn snapshots.WalkFunc, fs ...string) error { - ctx, t, err := o.ms.TransactionContext(ctx, false) - if err != nil { - return err - } - defer t.Rollback() - if o.upperdirLabel { - return storage.WalkInfo(ctx, func(ctx context.Context, info snapshots.Info) error { - id, _, _, err := storage.GetInfo(ctx, info.Name) - if err != nil { - return err - } - if info.Labels == nil { - info.Labels = make(map[string]string) - } - info.Labels[upperdirKey] = o.upperPath(id) - return fn(ctx, info) - }, fs...) - } - return storage.WalkInfo(ctx, fn, fs...) + return o.ms.WithTransaction(ctx, false, func(ctx context.Context) error { + if o.upperdirLabel { + return storage.WalkInfo(ctx, func(ctx context.Context, info snapshots.Info) error { + id, _, _, err := storage.GetInfo(ctx, info.Name) + if err != nil { + return err + } + if info.Labels == nil { + info.Labels = make(map[string]string) + } + info.Labels[upperdirKey] = o.upperPath(id) + return fn(ctx, info) + }, fs...) + } + return storage.WalkInfo(ctx, fn, fs...) + }) } // Cleanup cleans up disk resources from removed or abandoned snapshots @@ -360,16 +313,17 @@ func (o *snapshotter) Cleanup(ctx context.Context) error { return nil } -func (o *snapshotter) cleanupDirectories(ctx context.Context) ([]string, error) { +func (o *snapshotter) cleanupDirectories(ctx context.Context) (_ []string, err error) { + var cleanupDirs []string // Get a write transaction to ensure no other write transaction can be entered // while the cleanup is scanning. - ctx, t, err := o.ms.TransactionContext(ctx, true) - if err != nil { + if err := o.ms.WithTransaction(ctx, true, func(ctx context.Context) error { + cleanupDirs, err = o.getCleanupDirectories(ctx) + return err + }); err != nil { return nil, err } - - defer t.Rollback() - return o.getCleanupDirectories(ctx) + return cleanupDirs, nil } func (o *snapshotter) getCleanupDirectories(ctx context.Context) ([]string, error) { @@ -402,12 +356,11 @@ func (o *snapshotter) getCleanupDirectories(ctx context.Context) ([]string, erro } func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) (_ []mount.Mount, err error) { - ctx, t, err := o.ms.TransactionContext(ctx, true) - if err != nil { - return nil, err - } + var ( + s storage.Snapshot + td, path string + ) - var td, path string defer func() { if err != nil { if td != "" { @@ -424,50 +377,39 @@ func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k } }() - snapshotDir := filepath.Join(o.root, "snapshots") - td, err = o.prepareDirectory(ctx, snapshotDir, kind) - if err != nil { - if rerr := t.Rollback(); rerr != nil { - log.G(ctx).WithError(rerr).Warn("failed to rollback transaction") + if err := o.ms.WithTransaction(ctx, true, func(ctx context.Context) (err error) { + snapshotDir := filepath.Join(o.root, "snapshots") + td, err = o.prepareDirectory(ctx, snapshotDir, kind) + if err != nil { + return fmt.Errorf("failed to create prepare snapshot dir: %w", err) } - return nil, fmt.Errorf("failed to create prepare snapshot dir: %w", err) - } - rollback := true - defer func() { - if rollback { - if rerr := t.Rollback(); rerr != nil { - log.G(ctx).WithError(rerr).Warn("failed to rollback transaction") + + s, err = storage.CreateSnapshot(ctx, kind, key, parent, opts...) + if err != nil { + return fmt.Errorf("failed to create snapshot: %w", err) + } + + if len(s.ParentIDs) > 0 { + st, err := os.Stat(o.upperPath(s.ParentIDs[0])) + if err != nil { + return fmt.Errorf("failed to stat parent: %w", err) + } + + stat := st.Sys().(*syscall.Stat_t) + if err := os.Lchown(filepath.Join(td, "fs"), int(stat.Uid), int(stat.Gid)); err != nil { + return fmt.Errorf("failed to chown: %w", err) } } - }() - s, err := storage.CreateSnapshot(ctx, kind, key, parent, opts...) - if err != nil { - return nil, fmt.Errorf("failed to create snapshot: %w", err) - } - - if len(s.ParentIDs) > 0 { - st, err := os.Stat(o.upperPath(s.ParentIDs[0])) - if err != nil { - return nil, fmt.Errorf("failed to stat parent: %w", err) + path = filepath.Join(snapshotDir, s.ID) + if err = os.Rename(td, path); err != nil { + return fmt.Errorf("failed to rename: %w", err) } + td = "" - stat := st.Sys().(*syscall.Stat_t) - - if err := os.Lchown(filepath.Join(td, "fs"), int(stat.Uid), int(stat.Gid)); err != nil { - return nil, fmt.Errorf("failed to chown: %w", err) - } - } - - path = filepath.Join(snapshotDir, s.ID) - if err = os.Rename(td, path); err != nil { - return nil, fmt.Errorf("failed to rename: %w", err) - } - td = "" - - rollback = false - if err = t.Commit(); err != nil { - return nil, fmt.Errorf("commit failed: %w", err) + return nil + }); err != nil { + return nil, err } return o.mounts(s), nil