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

Do not remove containers from memory on error #31012

Merged
merged 1 commit into from May 10, 2017

Conversation

@cpuguy83
Contributor

cpuguy83 commented Feb 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor

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

Contributor

cpuguy83 commented Feb 17, 2017

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

return errors.Wrapf(e, "error while removing %s", dir)
}
}
}

This comment has been minimized.

@dmcgowan

dmcgowan Feb 17, 2017

Member

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

@dmcgowan

dmcgowan Feb 17, 2017

Member

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

if pe.Path == dir {
return nil
}
continue

This comment has been minimized.

@dmcgowan

dmcgowan Feb 17, 2017

Member

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

@dmcgowan

dmcgowan Feb 17, 2017

Member

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

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 18, 2017

Contributor

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

@cpuguy83

cpuguy83 Feb 18, 2017

Contributor

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

@@ -0,0 +1,75 @@
package system

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 21, 2017

Contributor

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

@cpuguy83

cpuguy83 Feb 21, 2017

Contributor

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 22, 2017

Contributor

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.

Contributor

cpuguy83 commented Feb 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 2, 2017

Contributor

@cpuguy83 any news here?

Contributor

LK4D4 commented Mar 2, 2017

@cpuguy83 any news here?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 4, 2017

Contributor

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.

Contributor

cpuguy83 commented Mar 4, 2017

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.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 20, 2017

Contributor

@cpuguy83 need rebase

Contributor

LK4D4 commented Mar 20, 2017

@cpuguy83 need rebase

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 22, 2017

Contributor

Rebased

Contributor

cpuguy83 commented Mar 22, 2017

Rebased

return errors.Wrapf(err, "unable to remove filesystem for %s", container.ID)
}
daemon.nameIndex.Delete(container.ID)

This comment has been minimized.

@crosbymichael

crosbymichael Apr 3, 2017

Contributor

ahahahaha

@crosbymichael

crosbymichael Apr 3, 2017

Contributor

ahahahaha

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 3, 2017

Contributor

?

@cpuguy83

cpuguy83 Apr 3, 2017

Contributor

?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 7, 2017

Member

For EnsureRemoveAll shouldn't we try to lazy-unmount the path(or mounted subpaths) first, before trying to delete anything. Can't think of a case where you want to delete files inside a mountpoint first and then unmount it. For example, if overlay/aufs is still mounted we don't want to delete files from the merge dir because they just start to create whiteouts. Lazy unmount everything before delete should fix everything but aufs diff folders problem.

I really don't like the repeated autoremove call and don't understand why it's needed. EnsureRemoveAll should take care of the shm issue with lazy unmount. If there are no mounts under container root then why should it fail to be deleted?

Member

tonistiigi commented Apr 7, 2017

For EnsureRemoveAll shouldn't we try to lazy-unmount the path(or mounted subpaths) first, before trying to delete anything. Can't think of a case where you want to delete files inside a mountpoint first and then unmount it. For example, if overlay/aufs is still mounted we don't want to delete files from the merge dir because they just start to create whiteouts. Lazy unmount everything before delete should fix everything but aufs diff folders problem.

I really don't like the repeated autoremove call and don't understand why it's needed. EnsureRemoveAll should take care of the shm issue with lazy unmount. If there are no mounts under container root then why should it fail to be deleted?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

For EnsureRemoveAll shouldn't we try to lazy-unmount the path(or mounted subpaths) first

By this point it should already be unmounted, but we end up with these ebusy errors. EnsureRemoveAll is replacing os.RemoveAll.

I really don't like the repeated autoremove call

Agree that it kind of sucks, but we're still getting errors. CI was failing when running lots of concurrent containers (TestEventsLimit, I think?). With the retries it at least gave time for everything to cool down.
I think for autoremove we should at least make a best effort to make sure it goes away.

Contributor

cpuguy83 commented Apr 7, 2017

For EnsureRemoveAll shouldn't we try to lazy-unmount the path(or mounted subpaths) first

By this point it should already be unmounted, but we end up with these ebusy errors. EnsureRemoveAll is replacing os.RemoveAll.

I really don't like the repeated autoremove call

Agree that it kind of sucks, but we're still getting errors. CI was failing when running lots of concurrent containers (TestEventsLimit, I think?). With the retries it at least gave time for everything to cool down.
I think for autoremove we should at least make a best effort to make sure it goes away.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

@tonistiigi Do you suggest we do a recursive unmount at the beginning of EnsureRemoveAll()?

Contributor

cpuguy83 commented Apr 7, 2017

@tonistiigi Do you suggest we do a recursive unmount at the beginning of EnsureRemoveAll()?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 7, 2017

Member

By this point it should already be unmounted, but we end up with these ebusy errors. EnsureRemoveAll is replacing os.RemoveAll.

So what causes them? Are we sure there are no bugs in unmounting? Or is this the aufs lower level dirs case? I don't think this helps for that, we would need some periodic deletion on the background.

Agree that it kind of sucks, but we're still getting errors. CI was failing when running lots of concurrent containers (TestEventsLimit, I think?). With the retries it at least gave time for everything to cool down.

Do we know what the actual problem was that caused this. Container process should be dead on exit event. I also do not understand why this would only apply to autoremove case and not regular removal.

Do you suggest we do a recursive unmount at the beginning of EnsureRemoveAll()?

Yes, but I think we also need to detect the case where the main dir itself is not a mountpoint but there are submounts inside it.

Member

tonistiigi commented Apr 7, 2017

By this point it should already be unmounted, but we end up with these ebusy errors. EnsureRemoveAll is replacing os.RemoveAll.

So what causes them? Are we sure there are no bugs in unmounting? Or is this the aufs lower level dirs case? I don't think this helps for that, we would need some periodic deletion on the background.

Agree that it kind of sucks, but we're still getting errors. CI was failing when running lots of concurrent containers (TestEventsLimit, I think?). With the retries it at least gave time for everything to cool down.

Do we know what the actual problem was that caused this. Container process should be dead on exit event. I also do not understand why this would only apply to autoremove case and not regular removal.

Do you suggest we do a recursive unmount at the beginning of EnsureRemoveAll()?

Yes, but I think we also need to detect the case where the main dir itself is not a mountpoint but there are submounts inside it.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

So what causes them? Are we sure there are no bugs in unmounting

Not definitely. Looks like mounts leaking and something holding it.

I also do not understand why this would only apply to autoremove case and not regular removal

Because the user has asked the daemon to remove it and won't get an error if it fails... though in the current case may hang due to lack of receiving an removal event.

Contributor

cpuguy83 commented Apr 7, 2017

So what causes them? Are we sure there are no bugs in unmounting

Not definitely. Looks like mounts leaking and something holding it.

I also do not understand why this would only apply to autoremove case and not regular removal

Because the user has asked the daemon to remove it and won't get an error if it fails... though in the current case may hang due to lack of receiving an removal event.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 28, 2017

Contributor

@tonistiigi

For the concurrency issue, here's the error:

time="2017-04-28T15:23:35.946846756Z" level=error msg="error removing container" container=23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f error="unable to remove filesystem for 23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f: remove /var/lib/docker/containers/23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f/shm: device or resource busy" 

Since this is a lazy unmount (at least twice here) this should never happen.
The CI is running on Ubuntu 14.04 with a very old kernel. At this point I'm going to assume this is a kernel bug.
If we don't retry the remove the CLI will hang since it never receives a remove event, and thus causes CI to panic due to a test timeout.

Contributor

cpuguy83 commented Apr 28, 2017

@tonistiigi

For the concurrency issue, here's the error:

time="2017-04-28T15:23:35.946846756Z" level=error msg="error removing container" container=23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f error="unable to remove filesystem for 23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f: remove /var/lib/docker/containers/23c9357dcd00a750d4aa158c1a3d9878c5ba360ded2c4434f8562573103b970f/shm: device or resource busy" 

Since this is a lazy unmount (at least twice here) this should never happen.
The CI is running on Ubuntu 14.04 with a very old kernel. At this point I'm going to assume this is a kernel bug.
If we don't retry the remove the CLI will hang since it never receives a remove event, and thus causes CI to panic due to a test timeout.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2017

Contributor

Also think this is preferable to the current situation where we are leaking container data on remove.

Contributor

cpuguy83 commented May 3, 2017

Also think this is preferable to the current situation where we are leaking container data on remove.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 3, 2017

Member

EnsureRemoveAll part LGTM, although not sure if we need to duplicate EBUSY check. For the autoremove part, just so I understand it correctly, you call unmount on shm path, get no error, then try to remove it(parent dir) and get an error? If this really is the case I still don't see why it should be in autoremove, then the retry should be in containerrm or in ensureremoveall.

Member

tonistiigi commented May 3, 2017

EnsureRemoveAll part LGTM, although not sure if we need to duplicate EBUSY check. For the autoremove part, just so I understand it correctly, you call unmount on shm path, get no error, then try to remove it(parent dir) and get an error? If this really is the case I still don't see why it should be in autoremove, then the retry should be in containerrm or in ensureremoveall.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2017

Contributor

@tonistiigi Updated to handle the delayed retry in EnsureRemoveAll and removed some of the special ebusy handling.

Contributor

cpuguy83 commented May 3, 2017

@tonistiigi Updated to handle the delayed retry in EnsureRemoveAll and removed some of the special ebusy handling.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 3, 2017

Member

LGTM

Member

tonistiigi commented May 3, 2017

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 4, 2017

Contributor

Squashed commits.

Contributor

cpuguy83 commented May 4, 2017

Squashed commits.

@thaJeztah thaJeztah added this to the 17.06.0 milestone May 4, 2017

@cpuguy83 cpuguy83 requested a review from justincormack May 5, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan May 9, 2017

Member

LGTM

Member

dmcgowan commented May 9, 2017

LGTM

@vdemeester

LGTM 🦁

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33736 has succeeded
Details
janky Jenkins build Docker-PRs 42340 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2867 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13560 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2430 has succeeded
Details

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

@thaJeztah thaJeztah removed this from the 17.06.0 milestone Jul 4, 2017

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

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 moby#31012

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Wentao Zhang <zhangwentao234@huawei.com>
(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

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'rm_container' into 'huawei-1.11.2'
Do not remove containers from memory on error

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

<br>

In practice this causes a lot of issues with leaked data sitting on<br>
disk that users are not able to clean up themselves.

<br>
This is particularly a problem while the `EBUSY` errors on remove are so<br>
prevalent. So for now let's not keep this behavior.     

<br>

Cherry-pick from <a href="moby#31012" rel="nofollow noreferrer" target="_blank">https://github.com/moby/moby/pull/31012</a>

<br>

Signed-off-by: Brian Goff <a href="mailto:cpuguy83@gmail.com">cpuguy83@gmail.com</a><br>
Signed-off-by: Wentao Zhang <a href="mailto:zhangwentao234@huawei.com">zhangwentao234@huawei.com</a><br>
(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>)     

<br>

Conflicts:<br>
    daemon/delete.go<br>
    daemon/graphdriver/aufs/aufs.go<br>
    daemon/graphdriver/btrfs/btrfs.go<br>
    daemon/graphdriver/devmapper/driver.go<br>
    daemon/graphdriver/overlay/overlay.go<br>
    daemon/monitor.go<br>
    pkg/mount/mount.go



See merge request docker/docker!519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment