Fix leftover cgroup directory issue #1196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyphar
reviewed
Nov 19, 2016
| @@ -148,6 +145,11 @@ func (m *Manager) Apply(pid int) (err error) { | |||
| return err | |||
| } | |||
| paths[sys.Name()] = p | |||
|
|
|||
| if err := sys.Apply(d); err != nil { | |||
| m.Paths = paths | |||
There was a problem hiding this comment.
If we want to do it this way, where we change m.Paths gradually, why not just change the above paths[sys.Name()] to m.Paths[sys.Name()]? Or has that map been used by callers so we have to replace the reference?
There was a problem hiding this comment.
Because m.Paths was initialized as nil when we create the container, but yes we can initialize m.Paths and change it directly, I've mended the commit.
hqhq
force-pushed
the
fix_cgroup_leftover
branch
from
492603c
to
1792693
Compare
yummypeng
reviewed
Nov 21, 2016
| return cgroups.EnterPid(m.Paths, pid) | ||
| } | ||
|
|
||
| paths := make(map[string]string) | ||
| m.Paths = make(map[string]string) |
There was a problem hiding this comment.
Why not just put this line of code before the above if so that the duplicated line inside the above if can be omitted?
In the cases that we got failure on a subsystem's Apply, we'll get some subsystems' cgroup directories leftover. On Docker's point of view, start a container failed, use `docker rm` to remove the container, but some cgroup files are leftover. Sometimes we don't want to clean everyting up when something went wrong, because we need these inter situation information to debug what's going on, but cgroup directories are not useful information we want to keep. Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
hqhq
force-pushed
the
fix_cgroup_leftover
branch
from
1792693
to
14d58e1
Compare
|
/me will test this right now. This actually will make some of the rootless cgroup manager changes in #774 easier to do. |
|
LGTM. |
|
LGTM |
hqhq
added a commit
to hqhq/runc
that referenced
this pull request
Jan 17, 2017
In the cases that we got failure on a subsystem's Apply, we'll get some subsystems' cgroup directories leftover. On Docker's point of view, start a container failed, use docker rm to remove the container, but some cgroup files are leftover. Sometimes we don't want to clean everyting up when something went wrong, because we need these inter situation information to debug what's going on, but cgroup directories are not useful information we want to keep. Fixes: http://code.huawei.com/docker/docker/issues/81 Cherry-picked form: opencontainers#1196 Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
hqhq
added a commit
to hqhq/runc
that referenced
this pull request
Jan 17, 2017
Fix leftover cgroup directory issue In the cases that we got failure on a subsystem's Apply, we'll get some subsystems' cgroup directories leftover. On Docker's point of view, start a container failed, use docker rm to remove the container, but some cgroup files are leftover. Sometimes we don't want to clean everyting up when something went wrong, because we need these inter situation information to debug what's going on, but cgroup directories are not useful information we want to keep. Fixes: http://code.huawei.com/docker/docker/issues/81 Cherry-picked form: opencontainers#1196 Signed-off-by: Qiang Huang <h.huangqiang@huawei.com> See merge request docker/runc!27
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the cases that we got failure on a subsystem's Apply,
we'll get some subsystems' cgroup directories leftover.
On Docker's point of view, start a container failed, use
docker rmto remove the container, but some cgroup filesare leftover.
Sometimes we don't want to clean everyting up when something
went wrong, because we need these inter situation
information to debug what's going on, but cgroup directories
are not useful information we want to keep.
Signed-off-by: Qiang Huang h.huangqiang@huawei.com