Skip to content
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

Jump to bottom

Fix error handling with not-exist errors on remove #33960

Merged
merged 1 commit into from Jul 21, 2017
Merged

Fix error handling with not-exist errors on remove #33960

merged 1 commit into from Jul 21, 2017

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jul 5, 2017 •
edited

Specifically, none of the graphdrivers are supposed to return a
not-exist type of error on remove (or at least that's how they are
currently handled).

Found that AUFS still had one case where a not-exist error could escape,
when checking if the directory is mounted we call a Statfs on the
path.

This fixes AUFS to not return an error in this case, but also
double-checks at the daemon level on layer remove that the error is not
a not-exist type of error.

Addresses a user issue:
#21704 (comment)

boaz0
boaz0 approved these changes Jul 6, 2017
View reviewed changes
Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

thaJeztah
thaJeztah approved these changes Jul 8, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

ping @tiborvass @tonistiigi PTAL

@thaJeztah
Copy link
Member

ugh, this one is really flaky https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/15435/console

14:55:07 ----------------------------------------------------------------------
14:55:07 FAIL: check_test.go:97: DockerSuite.TearDownTest
14:55:07 
14:55:07 check_test.go:98:
14:55:07     testEnv.Clean(c, dockerBinary)
14:55:07 environment/clean.go:67:
14:55:07     t.Fatalf("error removing containers %v : %v (%s)", containers, result.Error, result.Combined())
14:55:07 ... Error: error removing containers [5d6286b535ed] : exit status 1 (Error response from daemon: Could not kill running container 5d6286b535ed3f876bd07e11b3a9b734a14c6591f1b8f429fb4a08cd26f68e0c, cannot remove - Cannot kill container 5d6286b535ed3f876bd07e11b3a9b734a14c6591f1b8f429fb4a08cd26f68e0c: invalid container: 5d6286b535ed3f876bd07e11b3a9b734a14c6591f1b8f429fb4a08cd26f68e0c
14:55:07 )
14:55:07 
14:55:07 
14:55:07 ----------------------------------------------------------------------
14:55:07 PANIC: docker_api_stats_test.go:62: DockerSuite.TestAPIStatsStoppedContainerInGoroutines
14:55:07 
14:55:07 ... Panic: Fixture has panicked (see related PANIC)
14:55:07 
14:55:07 ----------------------------------------------------------------------

@thaJeztah thaJeztah mentioned this pull request Jul 8, 2017
Closed
@cpuguy83
Copy link
Member Author

Ping

tonistiigi
tonistiigi reviewed Jul 17, 2017
View reviewed changes
daemon/graphdriver/aufs/aufs.go Outdated
@@ -286,6 +286,9 @@ func (a *Driver) Remove(id string) error {
for {
mounted, err := a.mounted(mountpoint)
if err != nil {
if os.IsNotExist(err) {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't we just get another not exist error again later?

Copy link
Member

Choose a reason for hiding this comment

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

The rename after this seems to already handle it. For the other paths after that, I don't know how we could be sure that they do not exist without checking.

@thaJeztah thaJeztah mentioned this pull request Jul 18, 2017
Closed
@thaJeztah
Copy link
Member

ping @tonistiigi PTAL

tonistiigi
tonistiigi reviewed Jul 20, 2017
View reviewed changes
daemon/graphdriver/aufs/aufs.go Outdated
}
return err
if !os.IsNotExist(err) {
Copy link
Member

Choose a reason for hiding this comment

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

this is already checked in line 327

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@cpuguy83
Fix error handling with not-exist errors on remove
d42dbdd 
Specifically, none of the graphdrivers are supposed to return a
not-exist type of error on remove (or at least that's how they are
currently handled).

Found that AUFS still had one case where a not-exist error could escape,
when checking if the directory is mounted we call a `Statfs` on the
path.

This fixes AUFS to not return an error in this case, but also
double-checks at the daemon level on layer remove that the error is not
a `not-exist` type of error.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@tonistiigi
Copy link
Member

LGTM

@thaJeztah thaJeztah merged commit 67eeb04 into moby:master Jul 21, 2017
@cpuguy83 cpuguy83 deleted the ignore_not_exist_err branch July 21, 2017 11:53
This was referenced Jul 26, 2017
Merged
Merged
@tonistiigi tonistiigi mentioned this pull request Aug 21, 2017
Merged
@justlaputa
Copy link

hi, will this PR be backported to even older versions like 1.10 or 1.12?

@cpuguy83
Copy link
Member Author

cpuguy83 commented Sep 6, 2017

@justlaputa this issue is a fair bit more complicated and this also isn't really fixing any long standing issues, just a sticky situation that can occur in 17.06.0.

Also the only maintained versions are currently 17.06 and 17.07.
Docker Inc. does maintain a 17.03 branch for EE customers, however this issue doesn't really apply to 17.03.

@justlaputa
Copy link

@cpuguy83 : thanks for your detail explanation, I got the situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

@thaJeztah thaJeztah thaJeztah approved these changes

@boaz0 boaz0 boaz0 approved these changes

Assignees

@vieux vieux

Labels
status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@cpuguy83 @thaJeztah @tonistiigi @justlaputa @boaz0 @vieux @GordonTheTurtle