Move containerd process into docker cgroup for versions >= v1.11 #26391
Conversation
| type process struct{ name, file string } | ||
| dockerProcs := []process{{dockerProcessName, dockerPidFile}} | ||
| if dockerVersion.GTE(containerdVersion) { | ||
| dockerProcs = append(dockerProcs, process{containerdProcessName, ""}) |
There was a problem hiding this comment.
Is there a pid-file for containerd?
There was a problem hiding this comment.
I decided to try one more search, and found it! /run/docker/libcontainerd/docker-containerd.pid I can't find any documentation of that though...
There was a problem hiding this comment.
I'm not surprised about the lack of documentation. Let's add it.
There was a problem hiding this comment.
Oops, sorry! Forgot to update this to the new constant.
|
LGTM. Thanks for the cleanup @timstclair ! |
k8s-github-robot
added
size/M
timstclair
added
release-note-none
|
Fixed. Thanks! |
| dockerProcessName = "docker" | ||
| dockerPidFile = "/var/run/docker.pid" | ||
| containerdProcessName = "docker-containerd" | ||
| containerdPidFile = "/run/docker/libcontainerd/docker-containerd.pid" |
There was a problem hiding this comment.
Was this /var/run or /run? Just checking..
There was a problem hiding this comment.
/run is right (it has more docker information than /var/run)
There was a problem hiding this comment.
Is using this pid file documented and stable? We should probably ask the Docker team to do that if not.
There was a problem hiding this comment.
I can't find any documentation around this path, but we do have a fallback (pidof) if the file isn't found. I'll follow up with a docker issue to check the stability.
vishh
added
the
lgtm
|
@timstclair If there is a PR that is switching docker versions to v1.11, then I'd suggest marking this PR as a pre-req for that PR. |
|
Thanks, done. |
k8s-github-robot
removed
the
lgtm
|
Reapplying LGTM after trivial fix (addressing @vishh's comment) |
timstclair
added
the
lgtm
dchen1107
added
the
priority/critical-urgent
|
According to the perfdash, cpu usage of docker drops 50% after introducing docker 1.11.1, but that is not the truth since this pr is not merged, and part of resource overhead is not accounting to docker cgroup. :-( Bumping the priority to p0 so that we can have a better resource overhead monitoring for 1.3. |
|
GCE e2e build/test passed for commit e4d8dea. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit e4d8dea. |
|
Automatic merge from submit-queue |
Addresses #23397 (comment)
/cc @vishh @kubernetes/sig-node