Respect container's cgroup path #1872
Conversation
|
LGTM |
|
Bump, need another LGTM or some feedback here! |
|
@cyphar WDYT? |
|
|
||
| scanner := bufio.NewScanner(f) |
There was a problem hiding this comment.
My main concern with this change is that it switches away from bufio to storing the entire contents of /proc/self/mountinfo in memory. From memory, we switched away from doing this a long time ago in #496 because of performance problems -- I think effectively reverting this change without a solid justification is probably less-than-ideal.
gcapizzi
force-pushed
the
better-find-cgroup-mountpoint
branch
from
f817b96 to
a60eea8
Compare
Respect the container's cgroup path when finding the container's cgroup mount point, which is useful in multi-tenant environments, where containers have their own unique cgroup mounts Signed-off-by: Danail Branekov <danailster@gmail.com> Signed-off-by: Oliver Stenbom <ostenbom@pivotal.io> Signed-off-by: Giuseppe Capizzi <gcapizzi@pivotal.io>
gcapizzi
force-pushed
the
better-find-cgroup-mountpoint
branch
from
a60eea8 to
a1d5398
Compare
|
@cyphar we've switched back to a |
Cherrypick PR opencontainers/runc#1872 to prove that it fixes containerd hanging. We suspect that runc does not read the mount table properly hence trying to kill the wrong process [#159192734] Submodule src/github.com/opencontainers/runc 9d48320..d01993b: > Respect container's cgroup path
|
LGTM. Last call for 1.0 merges! /cc @opencontainers/runc-maintainers |
|
Dammit, PullApprove is broken again -- I guess they want to stop us from releasing. 😉 /cc @caniszczyk |
|
LGTM |
1 similar comment
|
LGTM |
| for _, opt := range strings.Split(fields[len(fields)-1], ",") { | ||
| if opt == subsystem { | ||
| return fields[4], fields[3], nil | ||
| fields := strings.Fields(txt) |
There was a problem hiding this comment.
Replacing strings.Split with strings.Fields is a noticeable performance hit. This is why we need benchmarks.
(this is no longer a problem, just looking through older changes)
Hi there!
We have a testing environment where we have multiple instances of garden/runc running, where each instance creates and destroys containers for its tests. Each of these testing nodes are run in isolation on the same host, so each have their own cgroup directories to minimise interference.
We have noticed that occasionally, when runc is trying to write properties to a cgroup, it chooses a neighbours cgroup by accident.
We think this is to do with the implementation of
FindCgroupMountpointAndRoot, which finds the first occurrence of a subsystem in the mount table. When we run multiple instances in parallel, this is not always the right one.We'd like to improve this function to respect the containers cgroup path, which we can find from the containers spec.
Looking forward to your feedback,
Cheers, the garden team.
Signed-off-by: Danail Branekov danailster@gmail.com
Signed-off-by: Oliver Stenbom ostenbom@pivotal.io