-
Notifications
You must be signed in to change notification settings - Fork 3.8k
metastore: Add WithTransaction convenience method #7781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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]>
|
Skipping CI for Draft Pull Request. |
kzys
left a comment
There was a problem hiding this 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.
|
Sure, I got half of LCOW ported over. I'll finish this out and include here |
|
Well, maybe overlay would be better. LCOW isn't tested in CI |
|
@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: Beforefunc (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
}Afterfunc (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 |
kzys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM.
|
rockylinux is having a bad day. |
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]>
7b68644 to
aa8a389
Compare
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 deferringt.Rollback()even forsnapshotter.Commit()which will causet.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.MetaStoreand 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: