Skip to content

Conversation

@dcantah
Copy link
Member

@dcantah dcantah commented Dec 8, 2022

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. Any failures are rolled back, and if it's a writable transaction the method 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 and removes a ton of boilerplate with getting a new snapshotter off the ground

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 and overlay snapshotters to use this. If this seems sound, I'll move over the other snapshotters in a follow-up.

Another API choice that doesn't have this as a method could be just passing in a pointer to the store:

// WithTransaction will 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 WithTransaction(ctx context.Context, ms *Metastore, writable bool, fn TransactionCallback) error {
     // foobarbaz
}

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 <[email protected]>
@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dcantah dcantah marked this pull request as ready for review December 8, 2022 08:20
@dcantah dcantah added this to the 1.7 milestone Dec 8, 2022
Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about having a draft PR to actually use WithTransaction from one of the existing snapshotters? I'm fine to have the commit in this PR if you prefer.

I'd like to confirm WithTransaction is useful for others before exposing.

@dcantah
Copy link
Member Author

dcantah commented Dec 13, 2022

Sure, I got half of LCOW ported over. I'll finish this out and include here

@dcantah
Copy link
Member Author

dcantah commented Dec 13, 2022

Well, maybe overlay would be better. LCOW isn't tested in CI

@dcantah
Copy link
Member Author

dcantah commented Dec 14, 2022

@kzys I moved over the overlay snapshotter here as well. There's a couple standouts in terms of simplification, Update is a big one as well as any writable transaction:

Before

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)
 		if err != nil {
 			return snapshots.Info{}, err
 		}
 		if info.Labels == nil {
 			info.Labels = make(map[string]string)
 		}
 		info.Labels[upperdirKey] = o.upperPath(id)
 	}

 	rollback = false
 	if err := t.Commit(); err != nil {
 		return snapshots.Info{}, err
 	}

 	return info, nil
 }

After

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 err
		}

		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)
		}
		return nil
	})
	return newInfo, err
}

The manual rollback checks can be elided, as well as needing to worry about committing ourselves as it all happens internally based on what you passed to the writable argument.

Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM.

@dmcgowan dmcgowan self-assigned this Dec 15, 2022
@dcantah
Copy link
Member Author

dcantah commented Dec 15, 2022

rockylinux is having a bad day. /usr/lib/ruby/vendor_ruby/fog/core/wait_for.rb:9:in 'block in wait_for': The specified wait_for timeout (2 seconds) was exceeded (Fog::Errors::TimeoutError)

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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants