Skip to content

snapshots/overlay: cleanup rollback code for overlay.prepare#1854

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
yanxuean:overlay-prepare-rollback
Dec 1, 2017
Merged

snapshots/overlay: cleanup rollback code for overlay.prepare#1854
dmcgowan merged 1 commit intocontainerd:masterfrom
yanxuean:overlay-prepare-rollback

Conversation

@yanxuean
Copy link
Copy Markdown
Member

@yanxuean yanxuean commented Dec 1, 2017

Signed-off-by: yason [email protected]

use defer

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1854 into master will increase coverage by 0.06%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#linux 52.6% <66.66%> (+0.07%) ⬆️
#windows 44.25% <ø> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/overlay/overlay.go 56.13% <66.66%> (+2.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38fdf9c...73ebab9. Read the comment docs.

Comment thread snapshots/overlay/overlay.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: just call it rollback

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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")
	}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should we call rollback when the Commit success?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just meant renaming isRollback to rollback 😅

I don't know why the commit failure wouldn't cause a rollback previously (cc @dmcgowan)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mlaventure Sorry for my English. shame. The pr has been merged. I will modify it in the other pr later.
@dmcgowan Thanks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@yanxuean yanxuean Dec 2, 2017

Choose a reason for hiding this comment

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

@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
	})
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

improve err message
Signed-off-by: yason <[email protected]>
@yanxuean yanxuean force-pushed the overlay-prepare-rollback branch from 73ebab9 to bb02302 Compare December 1, 2017 16:10
@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

LGTM

@stevvooe stevvooe changed the title improve rollback for overlay.prepare snapshots/overlay: cleanup rollback code for overlay.prepare Dec 1, 2017
defer func() {
if isRollback {
if rerr := t.Rollback(); rerr != nil {
log.G(ctx).WithError(rerr).Warn("Failure rolling back transaction")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you update this error message to failed to rollback transaction

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The pr has been merged. I will modify it in the other pr later.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Dec 1, 2017

LGTM

@dmcgowan dmcgowan merged commit f6df9f6 into containerd:master Dec 1, 2017
@yanxuean yanxuean deleted the overlay-prepare-rollback branch December 2, 2017 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants