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

Fix error removing diff path #34587

Merged
merged 1 commit into from Aug 22, 2017

Conversation

Projects
None yet
7 participants
@cpuguy83
Contributor

cpuguy83 commented Aug 21, 2017

In d42dbdd the code was re-arranged to
better report errors, and ignore non-errors.
In doing so we removed a deferred remove of the AUFS diff path, but did
not replace it with a non-deferred one.

This fixes the issue and makes the code a bit more readable.

// Atomically remove each directory in turn by first moving it out of the
// way (so that docker doesn't find it anymore) before doing removal of
// the whole tree.
if err := atomicRemove(mountpoint); err != nil {

This comment has been minimized.

@tonistiigi

tonistiigi Aug 21, 2017

Member

Why is the order of deletion changed in here? I think it makes more sense to remove mount before diffs because they are more likely to get EBUSY(and in some versions, you can't remove diff when it is mounted). And layers last as it should never fail.

@tonistiigi

tonistiigi Aug 21, 2017

Member

Why is the order of deletion changed in here? I think it makes more sense to remove mount before diffs because they are more likely to get EBUSY(and in some versions, you can't remove diff when it is mounted). And layers last as it should never fail.

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 21, 2017

Contributor

I changed this to match the original defer ordering.
I'll double check this later. Fine either way.

@cpuguy83

cpuguy83 Aug 21, 2017

Contributor

I changed this to match the original defer ordering.
I'll double check this later. Fine either way.

This comment has been minimized.

@tonistiigi

tonistiigi Aug 21, 2017

Member

I see. This indeed matches the order before #33960

@tonistiigi

tonistiigi Aug 21, 2017

Member

I see. This indeed matches the order before #33960

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Aug 21, 2017

Collaborator

@cpuguy83 to make this easier for cherrypicking in a patch release, can we make this commit as minimal as possible, addressing the removed defer only?

I suggest you make it "more readable" in another PR, it's important for the diff to be small.

Collaborator

tiborvass commented Aug 21, 2017

@cpuguy83 to make this easier for cherrypicking in a patch release, can we make this commit as minimal as possible, addressing the removed defer only?

I suggest you make it "more readable" in another PR, it's important for the diff to be small.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Aug 21, 2017

Member

Should we add a cleanup of diff/*-removing to aufs.Init() ?

Member

tonistiigi commented Aug 21, 2017

Should we add a cleanup of diff/*-removing to aufs.Init() ?

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 21, 2017

Collaborator

@tonistiigi I was about to suggest the same, I think we should for ppl who installed 17.06.1

Collaborator

vieux commented Aug 21, 2017

@tonistiigi I was about to suggest the same, I think we should for ppl who installed 17.06.1

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Aug 21, 2017

Collaborator

@tonistiigi @vieux shouldn't that be only in the patch release, and not master?

Collaborator

tiborvass commented Aug 21, 2017

@tonistiigi @vieux shouldn't that be only in the patch release, and not master?

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 22, 2017

Collaborator

@tiborvass

if ppl go from 17.06.1 -> 17.07.0 they won't get the fix if we put it in the patch release only

Collaborator

vieux commented Aug 22, 2017

@tiborvass

if ppl go from 17.06.1 -> 17.07.0 they won't get the fix if we put it in the patch release only

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 22, 2017

Contributor

to make this easier for cherrypicking in a patch release, can we make this commit as minimal as possible, addressing the removed defer only?

The primary reason I moved it out rather than keeping it all in one function is I needed to add additional error handling for the IsExist(Err) case where a remove fails, and now we have a dir that has been renamed but not removed, which just added to the complexity.

Should we add a cleanup of diff/*-removing to aufs.Init() ?
SGTM

Contributor

cpuguy83 commented Aug 22, 2017

to make this easier for cherrypicking in a patch release, can we make this commit as minimal as possible, addressing the removed defer only?

The primary reason I moved it out rather than keeping it all in one function is I needed to add additional error handling for the IsExist(Err) case where a remove fails, and now we have a dir that has been renamed but not removed, which just added to the complexity.

Should we add a cleanup of diff/*-removing to aufs.Init() ?
SGTM

@andrewhsu

This comment has been minimized.

Show comment
Hide comment
@andrewhsu

andrewhsu Aug 22, 2017

Contributor

Would it be suitable to have a unit test in the corresponding aufs_test.go?

Contributor

andrewhsu commented Aug 22, 2017

Would it be suitable to have a unit test in the corresponding aufs_test.go?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 22, 2017

Contributor

Added tests to make sure that there's no -removing dir hanging around after Remove() and also that Init() cleans up dirs with -removing

Contributor

cpuguy83 commented Aug 22, 2017

Added tests to make sure that there's no -removing dir hanging around after Remove() and also that Init() cleans up dirs with -removing

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 22, 2017

Collaborator

@tonistiigi @tiborvass LGTY now? Let's get this one in asap

Collaborator

vieux commented Aug 22, 2017

@tonistiigi @tiborvass LGTY now? Let's get this one in asap

@tonistiigi

LGTM

Show outdated Hide outdated daemon/graphdriver/aufs/aufs_test.go
Fix error removing diff path
In d42dbdd the code was re-arranged to
better report errors, and ignore non-errors.
In doing so we removed a deferred remove of the AUFS diff path, but did
not replace it with a non-deferred one.

This fixes the issue and makes the code a bit more readable.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 22, 2017

Collaborator

LGTM

Collaborator

vieux commented Aug 22, 2017

LGTM

p := filepath.Join(root, path)
dirs, err := ioutil.ReadDir(p)
if err != nil {
logrus.WithError(err).WithField("dir", p).Error("error reading dir entries")

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Would be great to mention aufs in the error message.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Would be great to mention aufs in the error message.

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Unless some higher caller already inserts that info

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Unless some higher caller already inserts that info

@@ -142,6 +142,23 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
}
}
for _, path := range []string{"mnt", "diff"} {
p := filepath.Join(root, path)
dirs, err := ioutil.ReadDir(p)

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

It is a bit confusing to call the variable dirs because they may not all be dirs. Entries?

@tiborvass

tiborvass Aug 22, 2017

Collaborator

It is a bit confusing to call the variable dirs because they may not all be dirs. Entries?

}
for _, dir := range dirs {
if strings.HasSuffix(dir.Name(), "-removing") {
logrus.WithField("dir", dir.Name()).Debug("Cleaning up stale layer dir")

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Mention aufs

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Mention aufs

if strings.HasSuffix(dir.Name(), "-removing") {
logrus.WithField("dir", dir.Name()).Debug("Cleaning up stale layer dir")
if err := system.EnsureRemoveAll(filepath.Join(p, dir.Name())); err != nil {
logrus.WithField("dir", dir.Name()).WithError(err).Error("Error removing stale layer dir")

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Aufs

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Aufs

@@ -317,31 +334,25 @@ func (a *Driver) Remove(id string) error {
retries++
logger.Warnf("unmount failed due to EBUSY: retry count: %d", retries)
time.Sleep(100 * time.Millisecond)
continue

This comment has been minimized.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Sorry I can't see on the phone the whole function. Just want to make sure the return statement at the end the block is wanted and shouldn't instead be a logging statement followed by this removed continue.

@tiborvass

tiborvass Aug 22, 2017

Collaborator

Sorry I can't see on the phone the whole function. Just want to make sure the return statement at the end the block is wanted and shouldn't instead be a logging statement followed by this removed continue.

This comment has been minimized.

@vieux

vieux Aug 22, 2017

Collaborator

it's a continue at the end of a for loop, it was useless

@vieux

vieux Aug 22, 2017

Collaborator

it's a continue at the end of a for loop, it was useless

@andrewhsu

This comment has been minimized.

Show comment
Hide comment
@andrewhsu

andrewhsu Aug 22, 2017

Contributor

Test LGTM: I've verified that without this fix, the tests added to aufs_test.go fail on the master branch:

$ make test-unit
...
--- FAIL: TestRemoveImage (0.00s)
	aufs_test.go:155: Error should not be nil because dirs with id 1-removing should be delted: diff
--- FAIL: TestInitStaleCleanup (0.02s)
	aufs_test.go:824: cleanup failed
FAIL
...

Nit-pick (not required for merge): in the future, it may be more helpful to have changes to tests as a separate git commit to the PR. Also, spelling: delted.

Contributor

andrewhsu commented Aug 22, 2017

Test LGTM: I've verified that without this fix, the tests added to aufs_test.go fail on the master branch:

$ make test-unit
...
--- FAIL: TestRemoveImage (0.00s)
	aufs_test.go:155: Error should not be nil because dirs with id 1-removing should be delted: diff
--- FAIL: TestInitStaleCleanup (0.02s)
	aufs_test.go:824: cleanup failed
FAIL
...

Nit-pick (not required for merge): in the future, it may be more helpful to have changes to tests as a separate git commit to the PR. Also, spelling: delted.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Aug 22, 2017

Collaborator

@andrewhsu the spelling mistake was fixed already.

LGTM despite minor nitpicks

Collaborator

tiborvass commented Aug 22, 2017

@andrewhsu the spelling mistake was fixed already.

LGTM despite minor nitpicks

@vieux vieux merged commit bbcdc7d into moby:master Aug 22, 2017

2 of 6 checks passed

experimental Jenkins build Docker-PRs-experimental 36427 is running
Details
janky Jenkins build Docker-PRs 45052 is running
Details
powerpc Jenkins build Docker-PRs-powerpc 5444 is running
Details
z Jenkins build Docker-PRs-s390x 5220 is running
Details
dco-signed All commits are signed
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16500 has succeeded
Details
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 22, 2017

Collaborator

let's do a follow up PR

Collaborator

vieux commented Aug 22, 2017

let's do a follow up PR

@justlaputa

This comment has been minimized.

Show comment
Hide comment
@justlaputa

justlaputa Sep 6, 2017

@cpuguy83 hi, do you have plan to backport this fix to older version of docker, like 1.10 or 1.12?

justlaputa commented Sep 6, 2017

@cpuguy83 hi, do you have plan to backport this fix to older version of docker, like 1.10 or 1.12?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 6, 2017

Contributor

@judtlaputa this is a specific fix for a regression in 17.06.1.

Contributor

cpuguy83 commented Sep 6, 2017

@judtlaputa this is a specific fix for a regression in 17.06.1.

@justlaputa

This comment has been minimized.

Show comment
Hide comment
@justlaputa

justlaputa Sep 6, 2017

@cpuguy83 oh, sorry. I should comment on the original PR #33960

justlaputa commented Sep 6, 2017

@cpuguy83 oh, sorry. I should comment on the original PR #33960

pld-gitsync pushed a commit to pld-linux/docker-ce that referenced this pull request Sep 9, 2017

up to 17.06.2
resolves annoying disk space eater "-removing" regression with aufs/diff

moby/moby#34587

moby/moby#21925 (comment)
https://stackoverflow.com/a/45798794
moby/moby#22207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment