Do not remove containers from memory on error by cpuguy83 · Pull Request #31012 · moby/moby

cpuguy83 · 2017-02-14T18:55:54Z

Before this, if forceRemove is set the container data will be removed
no matter what, including if there are issues with removing containeron-disk state (rw layer, container root).

In practice this causes a lot of issues with leaked data sitting on
disk that users are not able to clean up themselves.
This is particularly a problem while the EBUSY errors on remove are so
prevalent. So for now let's not keep this behavior.

cpuguy83 · 2017-02-17T16:53:06Z

Pretty interesting that tests are getting stuck waiting for a delete event, seems it must be hitting an error on remove...

dmcgowan · 2017-02-17T22:59:00Z

else return err? Why try again if no action was taken

dmcgowan · 2017-02-17T23:01:59Z

Are we confident this is always caused by a race? If it isn't, this could loop forever...

Could track paths in the error like I'm doing for ebusy.

cpuguy83 · 2017-02-21T21:38:58Z

Also figured I would mention I added this only after CI was failing (waiting for a destroy event that never comes)

cpuguy83 · 2017-02-22T21:10:49Z

Ugh, I know what the problem here is...
Container removal is failing in one or more of the containers created for the events test (due to EBUSY on the container's shm mount), as such the destroy event is never sent to the client which just sits waiting for an event that will never come.

LK4D4 · 2017-03-02T15:42:33Z

@cpuguy83 any news here?

cpuguy83 · 2017-03-04T20:19:41Z

Ok, I have this "working".
In order for tests to pass I had to beef up autoremove to retry a few times + a backoff delay. This is because mounts seem to be leaking in the test system. I've implemented this retry with a
The situation as it is today we just leak containers if the autoremove fails.

Now this is a perfectly valid issue to have since older kernels (< 3.15) can run into this as well, and cause the client to hang if it's not able to resolve within the max retry attempts.
The reason it works for tests is because the tests are specifically spinning up a bunch of short-lived containers in parallel causing the leakage and the retry just gives enough time for things to get cleaned up after the containers are all stopped.

One potential thing I've thought about for dealing with the client hangs (for legitimate issues) is to have a timeout period waiting for the destroy event to occur after the die event comes. Open to other suggestions.

No matter what we do to fix the client issue, 1.13/17.03 clients will have a problem unless we emit a destroy event. We could fix this situation with a minor patch to fix this bug in cases where a destroy event is not emitted.

tonistiigi · 2017-05-03T23:59:02Z

LGTM

cpuguy83 · 2017-05-04T00:00:40Z

Squashed commits.

Before this, if `forceRemove` is set the container data will be removed no matter what, including if there are issues with removing container on-disk state (rw layer, container root). In practice this causes a lot of issues with leaked data sitting on disk that users are not able to clean up themselves. This is particularly a problem while the `EBUSY` errors on remove are so prevalent. So for now let's not keep this behavior. Signed-off-by: Brian Goff <[email protected]>

dmcgowan · 2017-05-09T17:06:46Z

LGTM

vdemeester

LGTM 🦁

Before this, if `forceRemove` is set the container data will be removed no matter what, including if there are issues with removing container on-disk state (rw layer, container root). In practice this causes a lot of issues with leaked data sitting on disk that users are not able to clean up themselves. This is particularly a problem while the `EBUSY` errors on remove are so prevalent. So for now let's not keep this behavior. Cherry-pick from moby#31012 Signed-off-by: Brian Goff <[email protected]> Signed-off-by: Wentao Zhang <[email protected]> (cherry picked from commit 54dcbab) Conflicts: daemon/delete.go daemon/graphdriver/aufs/aufs.go daemon/graphdriver/btrfs/btrfs.go daemon/graphdriver/devmapper/driver.go daemon/graphdriver/overlay/overlay.go pkg/mount/mount.go

Do not remove containers from memory on error Before this, if `forceRemove` is set the container data will be removed no matter what, including if there are issues with removing container on-disk state (rw layer, container root). In practice this causes a lot of issues with leaked data sitting on disk that users are not able to clean up themselves. This is particularly a problem while the `EBUSY` errors on remove are so prevalent. So for now let's not keep this behavior. Cherry-pick from <a href="moby#31012" rel="nofollow noreferrer" target="_blank">https://github.com/moby/moby/pull/31012</a> Signed-off-by: Brian Goff <a href="mailto:[email protected]">[email protected]</a> Signed-off-by: Wentao Zhang <a href="mailto:[email protected]">[email protected]</a> (cherry picked from commit <a href="/docker/docker/commit/54dcbab25ea4771da303fa95e0c26f2d39487b49" data-original="54dcbab2" data-link="false" data-project="7713" data-commit="54dcbab25ea4771da303fa95e0c26f2d39487b49" data-reference-type="commit" title="" class="gfm gfm-commit has-tooltip" data-original-title="Do not remove containers from memory on error">54dcbab2</a>) Conflicts: daemon/delete.go daemon/graphdriver/aufs/aufs.go daemon/graphdriver/btrfs/btrfs.go daemon/graphdriver/devmapper/driver.go daemon/graphdriver/overlay/overlay.go daemon/monitor.go pkg/mount/mount.go See merge request docker/docker!519

cpuguy83 mentioned this pull request Feb 15, 2017

orphaned diffs #22207

Closed

cpuguy83 added the status/2-code-review label Feb 15, 2017

cpuguy83 force-pushed the do_not_remove_containers_on_error branch 3 times, most recently from 938f073 to 55294dd Compare February 17, 2017 22:04

dmcgowan reviewed Feb 17, 2017

View reviewed changes

Comment thread pkg/system/rm.go Outdated

Copy link
Copy Markdown

Member

dmcgowan Feb 17, 2017

Choose a reason for hiding this comment

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

else return err? Why try again if no action was taken

dmcgowan reviewed Feb 17, 2017

View reviewed changes

cpuguy83 force-pushed the do_not_remove_containers_on_error branch from 55294dd to 5bbd0ee Compare February 21, 2017 17:49

cpuguy83 mentioned this pull request Feb 21, 2017

Docker gradually exhausts disk space on BTRFS #27653

Closed

cpuguy83 commented Feb 21, 2017

View reviewed changes

cpuguy83 force-pushed the do_not_remove_containers_on_error branch 2 times, most recently from f47d9e1 to 6197d92 Compare February 21, 2017 22:34

cpuguy83 force-pushed the do_not_remove_containers_on_error branch 4 times, most recently from 6f5feae to b647f29 Compare February 23, 2017 18:34

GordonTheTurtle assigned LK4D4 Mar 1, 2017

cpuguy83 force-pushed the do_not_remove_containers_on_error branch 9 times, most recently from 8c6f990 to d6ffbc6 Compare March 4, 2017 20:07

cpuguy83 force-pushed the do_not_remove_containers_on_error branch from 098f284 to 7266dd4 Compare May 4, 2017 00:00

thaJeztah added this to the 17.06.0 milestone May 4, 2017

cpuguy83 requested a review from justincormack May 5, 2017 11:37

cpuguy83 force-pushed the do_not_remove_containers_on_error branch from 7266dd4 to 54dcbab Compare May 5, 2017 21:02

thaJeztah added the status/needs-attention Calls for a collective discussion during a review session label May 9, 2017

vdemeester added the rebuild/powerpc label May 10, 2017

GordonTheTurtle removed the rebuild/powerpc label May 10, 2017

vdemeester approved these changes May 10, 2017

View reviewed changes

vdemeester added status/4-merge and removed status/2-code-review status/needs-attention Calls for a collective discussion during a review session labels May 10, 2017

cpuguy83 merged commit 815e8bb into moby:master May 10, 2017

cpuguy83 deleted the do_not_remove_containers_on_error branch May 10, 2017 10:28

thaJeztah added the impact/changelog label May 10, 2017

thaJeztah mentioned this pull request Jun 8, 2017

windows: docker system prune not reclaiming expected space #31253

Closed

cpuguy83 mentioned this pull request Jun 13, 2017

Unremovable dead container #32787

Closed

thaJeztah mentioned this pull request Jun 13, 2017

Docker does not free up disk space after container, volume and image removal #32420

Closed

thaJeztah removed this from the 17.06.0 milestone Jul 4, 2017

trapier mentioned this pull request Jul 7, 2017

[RFC] Make MountFlags=slave for RPM based distros #31490

Closed

kolyshkin mentioned this pull request Jun 30, 2020

is RecursiveUnmount needed? moby/sys#21

Closed

github-actions Bot mentioned this pull request Mar 17, 2021

Pod terminating qiwihui/pocket_readings#1049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remove containers from memory on error#31012

Do not remove containers from memory on error#31012
cpuguy83 merged 1 commit intomoby:masterfrom
cpuguy83:do_not_remove_containers_on_error

cpuguy83 commented Feb 14, 2017

Uh oh!

cpuguy83 commented Feb 17, 2017

Uh oh!

dmcgowan Feb 17, 2017

Uh oh!

dmcgowan Feb 17, 2017

Uh oh!

cpuguy83 Feb 18, 2017

Uh oh!

cpuguy83 Feb 21, 2017

Uh oh!

cpuguy83 commented Feb 22, 2017

Uh oh!

LK4D4 commented Mar 2, 2017

Uh oh!

cpuguy83 commented Mar 4, 2017 •

edited

Loading

Uh oh!

tonistiigi commented May 3, 2017

Uh oh!

cpuguy83 commented May 4, 2017

Uh oh!

dmcgowan commented May 9, 2017

Uh oh!

vdemeester left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

cpuguy83 commented Feb 14, 2017

Uh oh!

cpuguy83 commented Feb 17, 2017

Uh oh!

dmcgowan Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

dmcgowan Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 18, 2017

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 21, 2017

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Feb 22, 2017

Uh oh!

LK4D4 commented Mar 2, 2017

Uh oh!

cpuguy83 commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented May 3, 2017

Uh oh!

cpuguy83 commented May 4, 2017

Uh oh!

dmcgowan commented May 9, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cpuguy83 commented Mar 4, 2017 •

edited

Loading