From aa8a389c51088acefc8b60611b1c61857a41f2a6 Mon Sep 17 00:00:00 2001 From: Danny Canter Date: Wed, 14 Dec 2022 02:49:56 -0800 Subject: [PATCH] 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