Get PIDs from cgroups recursively #330
Conversation
libcontainer/cgroups/fs/apply_raw.go
Outdated
There was a problem hiding this comment.
u are not getting the error from Walk here?
There was a problem hiding this comment.
Nope, I'm not returning it as well. I think I need to return at leas in case of "cgroup.procs"
There was a problem hiding this comment.
Idea was to get as much pids as possible :)
|
@crosbymichael I changed to return any error on procs file processing. |
libcontainer/cgroups/fs/apply_raw.go
Outdated
There was a problem hiding this comment.
why is this check here? shouldn't it be at the top?
There was a problem hiding this comment.
I don't want to stop process on errors for other files. In user-namespace this can be an issue.
There was a problem hiding this comment.
humm, interesting...
But there error that you get in iErr happens from the previous iteration right?
There was a problem hiding this comment.
@crosbymichael if you're talking about the walk iteration, iErr is the error for the current path or a file directly under the current path
There was a problem hiding this comment.
yes, I think iError is error from Stat on p.
|
Don't we need to change |
|
@LK4D4 thanks, we will test out this patch. |
|
@hqhq You're right, I'll fix tomorrow. It's weird that implementations are different. |
|
@hqhq Updated for systemd. |
There was a problem hiding this comment.
Can we move the code into a common functions in utils to avoid duplication?
There was a problem hiding this comment.
Let's do this, what do you think about same cgroup for both implementations? Like "devices".
Also lookup cgroup for systemd is changed to "device" to be consistent with fs implementation. Signed-off-by: Alexander Morozov <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
|
IANTM but |
|
What's up with janky? |
|
Closes #331 |
|
@mrunalp the "Closes #XXX" has to be part of the description of the PR. (you can edit it) Janky is a little sick, you need to test manually. |
Get PIDs from cgroups recursively
|
We are having some issues with this patch, still investigating. |
|
@LK4D4 ^^ |
|
@ibuildthecloud there's a panic being fixed, see moby/moby#16989 |
|
Anybody want to try my original patch? |
|
@ibuildthecloud feel free to review #332 |
config: Fix indents for process.apparmorProfile and .selinuxLabel
8 participants
Closes #331
Closes #329
@ibuildthecloud I carried and slightly changed code. Now you can just review and not spend time on my comments in your branch.