Skip to content

Add support for just joining in apply using cgroup paths#466

Merged
hqhq merged 1 commit into
opencontainers:masterfrom
mrunalp:cgroup_apply_paths
Jan 21, 2016
Merged

Add support for just joining in apply using cgroup paths#466
hqhq merged 1 commit into
opencontainers:masterfrom
mrunalp:cgroup_apply_paths

Conversation

@mrunalp

@mrunalp mrunalp commented Jan 11, 2016

Copy link
Copy Markdown
Contributor

This is for supporting joining cgroups by container id in docker.
See moby/moby#19244

Signed-off-by: Mrunal Patel mrunalp@gmail.com

@mrunalp

mrunalp commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

@LK4D4 @crosbymichael PTAL

@LK4D4

LK4D4 commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

LGTM

Comment thread libcontainer/cgroups/fs/apply_raw.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we validate these paths before assigning to m.Paths? I know they must be valid if they're from Docker, but we might need to support this for runc config file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

EnterPid enters the cgroups only if the path exists and that is current behavior for exec. I think it makes sense to have the same behavior here as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry didn't make it clearer, I meant maybe we should do something like (This is what we did in Apply()):

for name, path := range c.Paths {
        p, err := d.path(name)
        if err != nil {
                if cgroups.IsNotFound(err) {
                        continue
                }
                return err
        }
        paths[name] = path
}
m.Paths = paths

return cgroups.EnterPid(m.Paths, pid)

Doing this because m.Path will be used by GetPaths, GetStats, etc. It'll be easier for them if it holds only valid paths.

@mrunalp
mrunalp force-pushed the cgroup_apply_paths branch from 28958b0 to 809fe73 Compare January 20, 2016 00:37
@mrunalp

mrunalp commented Jan 20, 2016

Copy link
Copy Markdown
Contributor Author

@hqhq Updated.

Comment thread libcontainer/configs/cgroup_unix.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/represen/represents

@hqhq

hqhq commented Jan 20, 2016

Copy link
Copy Markdown
Contributor

A minor tip, otherwise LGTM, @LK4D4 needs a second look?

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
mrunalp force-pushed the cgroup_apply_paths branch from 809fe73 to 41d9d26 Compare January 20, 2016 19:23
@mrunalp

mrunalp commented Jan 20, 2016

Copy link
Copy Markdown
Contributor Author

@hqhq Updated.

@hqhq

hqhq commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

LGTM

hqhq added a commit that referenced this pull request Jan 21, 2016
Add support for just joining in apply using cgroup paths
@hqhq
hqhq merged commit 986637c into opencontainers:master Jan 21, 2016
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
In [1], I'd proposed replacing our old "user-specified process" with
"user-specified code" to help distinguish between 'create' (cloning
the container process) and 'start' (signaling the container process to
execve or similar the user-specified $STUFF_FROM_THE_process_CONFIG).
That PR was rejected, although the renaming proposed there had already
landed via dd0cd21 (Add a 'status' field to our state struct,
2016-05-26, opencontainers#462).

This PR attempts to find a common ground between "process" (preferred
by maintainers in opencontainers#466 [2,3,4], but which I consider incorrect [5])
and "code" (which maintainers found confusing [3,4,6]).  The Linux
execve(2) says "program" and unpacks that to "a binary executable, or
a script starting with a [shebang]" [7].  proc(5) documents
/proc/[pid]/exe by talking about "the executed command" [8].  The
POSIX exec docs call this the "process image" and talk about loading
it from the "new process image file" (although they also sprinkle in a
number of “program” references, apparently interchangeably with
“process image”) [9].

POSIX formally defines "command" [11], "executable file" [12], and
"program" [13].  The only reference to "process image" in the
definitions is in the "executable file" entry.  The "command"
definition is focused on the shell, the "executable file" definition
is focused on files, and the "program" definition talks about a
"prepared sequence of instructions to the system", so "program" seems
like the best fit.

[1]: opencontainers/runtime-spec#466
     Subject: runtime: Replace "user-specified process" with "user-specified code" in 'create'
[2]: opencontainers/runtime-spec#466 (comment)
[3]: opencontainers/runtime-spec#466 (comment)
[4]: opencontainers/runtime-spec#466 (comment)
[5]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_295
[6]: opencontainers/runtime-spec#466 (comment)
[7]: http://man7.org/linux/man-pages/man2/execve.2.html
[8]: http://man7.org/linux/man-pages/man5/proc.5.html
[9]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
[10]: https://git.kernel.org/cgit/docs/man-pages/man-pages.git/
[11]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_104
[12]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_154
[13]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_306

Signed-off-by: W. Trevor King <wking@tremily.us>
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