snapshots/overlay: cleanup rollback code for overlay.prepare#1854
snapshots/overlay: cleanup rollback code for overlay.prepare#1854dmcgowan merged 1 commit intocontainerd:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1854 +/- ##
==========================================
+ Coverage 49.12% 49.18% +0.06%
==========================================
Files 86 86
Lines 8246 8244 -2
==========================================
+ Hits 4051 4055 +4
+ Misses 3525 3519 -6
Partials 670 670
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
nit: just call it rollback
There was a problem hiding this comment.
I don't know why not call rollback when commit is fail before.
Do you know the reason?
if err = t.Commit(); err != nil {
return nil, errors.Wrap(err, "commit failed")
}
There was a problem hiding this comment.
Should we call rollback when the Commit success?
There was a problem hiding this comment.
I just meant renaming isRollback to rollback 😅
I don't know why the commit failure wouldn't cause a rollback previously (cc @dmcgowan)
There was a problem hiding this comment.
Commit already does a rollback when there is a failure on commit, calling Rollback would be redundant. The functions are designed that either Commit or Rollback need to be called, never both.
There was a problem hiding this comment.
@mlaventure Sorry for my English. shame. The pr has been merged. I will modify it in the other pr later.
@dmcgowan Thanks
There was a problem hiding this comment.
@dmcgowan Seem like there are many place calling both commit and rollback.
for example: naive.go
func (o *snapshotter) Remove(ctx context.Context, key string) (err error) {
ctx, t, err := o.ms.TransactionContext(ctx, true)
if err != nil {
return err
}
defer func() {
if err != nil && t != nil {
if rerr := t.Rollback(); rerr != nil {
log.G(ctx).WithError(rerr).Warn("failed to rollback transaction")
}
}
}()
id, _, err := storage.Remove(ctx, key)
if err != nil {
return errors.Wrap(err, "failed to remove")
}
path := o.getSnapshotDir(id)
renamed := filepath.Join(o.root, "snapshots", "rm-"+id)
if err := os.Rename(path, renamed); err != nil {
if !os.IsNotExist(err) {
return errors.Wrap(err, "failed to rename")
}
renamed = ""
}
err = t.Commit()
t = nil
if err != nil {
if renamed != "" {
if err1 := os.Rename(renamed, path); err1 != nil {
// May cause inconsistent data on disk
log.G(ctx).WithError(err1).WithField("path", renamed).Errorf("Failed to rename after failed commit")
}
}
return errors.Wrap(err, "failed to commit")
}
if renamed != "" {
if err := os.RemoveAll(renamed); err != nil {
// Must be cleaned up, any "rm-*" could be removed if no active transactions
log.G(ctx).WithError(err).WithField("path", renamed).Warnf("Failed to remove root filesystem")
}
}
return nil
}
There was a problem hiding this comment.
@dmcgowan @stevvooe @crosbymichael
I think we can refactor it.
e.g.
type Handle func(ctx context.Context) bool
func (ms *MetaStore) HandleTransaction(ctx context.Context, writable bool, f Handle) error {
ctx, t, err := ms.TransactionContext(ctx, writable)
if err != nil {
return err
}
if f(ctx) {
if writable {
err = t.Commit()
}
}else{
err = t.Rollback()
}
return err
}
func userFunc(){
ms.HandleTransaction(ctx, writable, func(ctx context.Context) bool {
user xxxHandle
})
}
improve err message Signed-off-by: yason <[email protected]>
73ebab9 to
bb02302
Compare
|
LGTM |
| defer func() { | ||
| if isRollback { | ||
| if rerr := t.Rollback(); rerr != nil { | ||
| log.G(ctx).WithError(rerr).Warn("Failure rolling back transaction") |
There was a problem hiding this comment.
Can you update this error message to failed to rollback transaction
There was a problem hiding this comment.
The pr has been merged. I will modify it in the other pr later.
|
LGTM |
Signed-off-by: yason [email protected]
use defer