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.

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 runc ps issue #1013

Merged
merged 2 commits into from
Sep 12, 2016
Merged

Fix runc ps issue #1013

merged 2 commits into from
Sep 12, 2016

Conversation

hqhq
Copy link
Contributor

@hqhq hqhq commented Sep 1, 2016

After #1009, we don't always set cgroup.Paths, so
getCgroupPath() will return wrong cgroup path because
it'll take current process's cgroup as the parent, which
would be wrong when we try to find the cgroup path in
runc ps.

Fix it by using m.GetPath() to get the true cgroup
paths.

Also Fixes: #1034
Fixes: #1031

Reported-by: Yang Shukui [email protected]
Signed-off-by: Qiang Huang [email protected]

@crosbymichael
Copy link
Member

crosbymichael commented Sep 8, 2016

LGTM

Approved with PullApprove

@hqhq hqhq mentioned this pull request Sep 12, 2016
After opencontainers#1009, we don't always set `cgroup.Paths`, so
`getCgroupPath()` will return wrong cgroup path because
it'll take current process's cgroup as the parent, which
would be wrong when we try to find the cgroup path in
`runc ps` and `runc kill`.

Fix it by using `m.GetPath()` to get the true cgroup
paths.

Reported-by: Yang Shukui <[email protected]>
Signed-off-by: Qiang Huang <[email protected]>
@hqhq
Copy link
Contributor Author

hqhq commented Sep 12, 2016

I added a commit to fix update and pause issues, PTAL @crosbymichael @cyphar

BTW janky didn't cache this because we use runc run -d and it's the same process that execute all other runc commands, so it always get the right cgroup path. I didn't think of an easy way to enhance our test to cover this scenario. Thoughts?

@cyphar
Copy link
Member

cyphar commented Sep 12, 2016

LGTM. Though, I can't reproduce the original issue (presumably because my tmux setup has everything in the same systemd slice). Are there any other manager functions that we need to replace?

Approved with PullApprove

@hqhq
Copy link
Contributor Author

hqhq commented Sep 12, 2016

@cyphar Not as I can find, now getCgroupData is only called by Apply, and this doesn't impact systemd because it use different way to get cgroup path.

@crosbymichael
Copy link
Member

crosbymichael commented Sep 12, 2016

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 9a072b6 into opencontainers:master Sep 12, 2016
@hqhq hqhq deleted the fix_ps_issue branch September 13, 2016 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants