Skip to content

fix: miss remove temp file in createSnapshot #2900

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.

Sign up for GitHub

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

Merged
crosbymichael merged 1 commit into from
Jan 15, 2019
Merged

fix: miss remove temp file in createSnapshot #2900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions snapshots/native/native.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,8 @@ func (o *snapshotter) Walk(ctx context.Context, fn func(context.Context, snapsho
return storage.WalkInfo(ctx, fn)
}

func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) ([]mount.Mount, error) {
func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) (_ []mount.Mount, err error) {
var (
err error
path, td string
)

Expand Down
2 changes: 1 addition & 1 deletion snapshots/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ func (o *snapshotter) getCleanupDirectories(ctx context.Context, t storage.Trans
return cleanup, nil
}

func (o *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) ([]mount.Mount, error) {
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Jan 2, 2019

Choose a reason for hiding this comment

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

Can you point out the branches which are not getting cleaned up correctly because of this? The block from line 398 seems to have redeclared err.

Copy link
Copy Markdown
Contributor Author

@Ace-Tang Ace-Tang Jan 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the line 399(https://github.com/containerd/containerd/pull/2900/files#diff-adfa4892be9b7d1d353e8eba2cd335d6L399) and line 406 (https://github.com/containerd/containerd/pull/2900/files#diff-adfa4892be9b7d1d353e8eba2cd335d6L406) has new defined err, but this part code always good.
I found the problem when I implement a new snapshotter qcow2 for kata runtime, I re-use lots of overlay code , in debug process, found many uncleaned tmp snapshot directory like

$ sudo ls /var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots
1  11  14  7  new-888620822

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Jan 14, 2019

Choose a reason for hiding this comment

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

My only concern was previously it required a very explicit setting of err to hit the cleanup section, but I don't see a code path which doesn't expect this block to run. So looks good to have that as the default behavior

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
Expand Down