Enter cgroups as part of NsEnter #143
Conversation
|
Ping @crosbymichael @vmarmol |
| "github.com/docker/libcontainer/label" | ||
| "github.com/docker/libcontainer/system" | ||
| ) | ||
|
|
||
| // ExecIn uses an existing pid and joins the pid's namespaces with the new command. | ||
| func ExecIn(container *libcontainer.Config, state *libcontainer.State, args []string) error { | ||
| // Enter cgroups. |
There was a problem hiding this comment.
Im not sure this is correct. Shouldn't our parent place the child's PID into the cgroups?
There was a problem hiding this comment.
Since we're exec()ing I think its fine. We'd be having the waitpid() parent waiting for the child in the namespace also in the cgroups which I think is fine, the overhead is to that container anyways.
There was a problem hiding this comment.
good point @crosbymichael. In the case of ExecIn() this is fine. But in the case of 'docker exec', this is not desirable since we will end up moving docker into user container.
May be I could made docker fork+exec dockerinit with a 'cgenter option', which will then enter the cgroups and then exec itself as 'nsenter'.
|
PTAL @vmarmol @crosbymichael |
| @@ -86,7 +86,7 @@ void nsenter() | |||
| } | |||
| int found_nsenter = 0; | |||
| for (c = 0; c < argc; ++c) { | |||
| if (strcmp(argv[c], kNsEnter) == 0) { | |||
| if (strncmp(argv[c], kNsEnter, strlen(kNsEnter)) == 0) { | |||
There was a problem hiding this comment.
I think these are equivalent since strcmp will do the equivalent of strlen() with kNsEnter
|
Just two nits, else looks good :) |
|
LGTM thanks for the fix @vishh! |
|
Ping @crosbymichael |
|
This does not work on systemd The paths are incorrect |
|
Maybe for systemd and the fs implementation we need to store the paths in the container's state so that is is trivial to add more processes to the existing groups and ensure that the paths are properly cleaned up after, even if the container's parent process is SIGKILLed we can look at the state on disk and cleanup the cgroups. |
|
@crosbymichael I updated the patch as per your suggestion. PTAL |
| @@ -18,6 +18,9 @@ type State struct { | |||
|
|
|||
| // Network runtime state. | |||
| NetworkState network.NetworkState `json:"network_state,omitempty"` | |||
|
|
|||
| // Path to all the cgroup dirs. | |||
| CgroupPaths []string `json:"cgroup_paths,omitempty"` | |||
There was a problem hiding this comment.
What do you think about making this a map with the subsystem and then the path. So it would look like:
{
"cgroup_paths": {
"devices": "/sys/fs/cgroup/devices/dockcer...."
}
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
…ering cgroups and will be useful for cleanups too in the future. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
… path as value. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
|
Looking good so far: {
"cgroup_paths": {
"blkio": "/sys/fs/cgroup/blkio/system.slice/docker-docker-koye.scope",
"cpu": "/sys/fs/cgroup/cpu,cpuacct/system.slice/docker-docker-koye.scope",
"cpuacct": "/sys/fs/cgroup/cpu,cpuacct/system.slice/docker-docker-koye.scope",
"cpuset": "/sys/fs/cgroup/cpuset/system.slice/docker-docker-koye.scope",
"devices": "/sys/fs/cgroup/devices/system.slice/docker-docker-koye.scope",
"freezer": "/sys/fs/cgroup/freezer/system.slice/docker-docker-koye.scope",
"memory": "/sys/fs/cgroup/memory/system.slice/docker-docker-koye.scope",
"perf_event": "/sys/fs/cgroup/perf_event/system.slice/docker-docker-koye.scope"
},
"init_pid": 28718,
"init_start_time": "646736",
"network_state": {}
} |
The current paths for the different systemd cgroup subsystems that systemd manages and that we have to manage are very inconsistent. This patch cleans up those differences and allows consistent paths to be used. Signed-off-by: Michael Crosby <michael@docker.com>
Cleanup systemd cgroup code
|
LGTM, merging. |
Enter cgroups as part of NsEnter
Fixes #145