Cgroup manager: Add a MakeThreaded() method

[iholder101]

Currently, there's no way to make a cgroup threaded via cgroup manager. Therefore, in Kubevirt, I had to implement it myself [1][2].

As I think this will be helpful to many, not just Kubevirt, this PR adds a MakeThreaded() method to cgroups manager. Obviously it is relevant to v2 only, for v1 it acts as a no-op.

Fixes #3690

[1] PR that implements this in Kubevirt: https://github.com/kubevirt/kubevirt/pull/8869 [2] Implemented in commit: https://github.com/kubevirt/kubevirt/pull/8869/commits/868ca2138355206bc0836dba11104dcadfe4b507

[iholder101]

/cc @kolyshkin

Would you mind taking a look? Thanks!

[dcantah]

@iholder101 Not a maintainer here so I'll leave sign off to others. This was a funny coincidence, I was looking to add the same to https://github.com/containerd/cgroups and then went to see if runc supported this yet, and seems it may soon 🤣

[iholder101]

@iholder101 Not a maintainer here so I'll leave sign off to others. This was a funny coincidence, I was looking to add the same to https://github.com/containerd/cgroups and then went to see if runc supported this yet, and seems it may soon rofl

:smiley: The beauty of open source.. :)

Thanks for your review, appreciate it!

[iholder101]

/retest

[kolyshkin]

Implementation LGTM overall. I am just not sure if we want this specific "MakeThreaded" method, or some more generic approach like, say, GetType (returning string, error) and SetType (with string as an argument).

Also note that fs2's CreateCgroupPath reads and writes cgroup.type.

[kolyshkin]

@AkihiroSuda PTAL

[iholder101]

Implementation LGTM overall. I am just not sure if we want this specific "MakeThreaded" method, or some more generic approach like, say, GetType (returning string, error) and SetType (with string as an argument).

Also note that fs2's CreateCgroupPath reads and writes cgroup.type.

Makes sense :+1: I guess we'll wait for @AkihiroSuda to see there are no objections. If there's a consensus, I'll gladly change the implementation

[mrunalp]

Needs rebase.

[iholder101]

Needs rebase.

Done :+1: Sorry for the delay. Can you PTAL?

[iholder101]

@mrunalp ping

[iholder101]

another ping @mrunalp @kolyshkin

[kolyshkin]

@iholder101 sorry for not seeing it earlier. I took another look, and I'm not sure we need this.

I mean, what this PR does can probably be implemented via something like

err := cgroups.WriteFile(m.Path("")+"/cgroup.type", "threaded")

Surely, this code has some assumptions, notably:

1. Writing threaded is more efficient than reading, comparing, and conditionally writing (this needs to be checked in the kernel sources);
2. If it can't be made threaded, write() will return an error.

So, considering that this functionality

• can be implemented in one line of code using existing runc/libcontainer/cgroups;
• is not used by runc itself;
• takes 54 lines in 7 files to implement,

does it really make sense to add it?

[kolyshkin]

1. Writing threaded is more efficient than reading, comparing, and conditionally writing (this needs to be checked in the kernel sources);

Apparently it's not true, the kernel does some locking and draining, so it might be faster to read and write. Still, a few lines of code.

[iholder101]

@iholder101 sorry for not seeing it earlier. I took another look, and I'm not sure we need this.

I mean, what this PR does can probably be implemented via something like

err := cgroups.WriteFile(m.Path("")+"/cgroup.type", "threaded")

Surely, this code has some assumptions, notably:

1. Writing threaded is more efficient than reading, comparing, and conditionally writing (this needs to be checked in the kernel sources);
2. If it can't be made threaded, write() will return an error.

So, considering that this functionality

• can be implemented in one line of code using existing runc/libcontainer/cgroups;
• is not used by runc itself;
• takes 54 lines in 7 files to implement,

does it really make sense to add it?

You're right, but this argument can be given for any interface change whatsoever.

For example, GetPids() simply reads cgroup.procs which can be read manually from cgroups.ReadFile. Same thing goes for Set() or any other method. After all, anything (except maybe device permissions which are implemented via eBPF on v2) can be performed manually by reading and writing to files.

I wonder: what is the purpose of this interface? In my mind, the purpose is to simplify and provide a declarative interface for the operations that can be performed on a cgroup. To an advanced user, it might be obvious that "threaded" needs to be written to "cgroup.type", but I think this kind of misses the point of having such an interface in the first place.

Another example which proves this point is Apply(). The documentation says Apply creates a cgroup, if not yet created, and adds a process with the specified pid into that cgroup, but what about threads? On v2, to create a new threaded sub-cgroup there's a need to:

1. Create a folder inside the root cgroup
2. Make it threaded manually
3. Write the TIDs into cgroup.threads

I don't really understand how it makes sense that this operation requires one method call when processes are involved, but requires many manual operations for threads.

I would like to see an interface with many methods that covers, more or less, everything that can be done with cgroups. That would help the user to know which operations stand at their hands without having to dive deep into the documentation and become an advanced user. Do you agree with this? If not, I would be interested to know what's your perspective regarding the purpose of this interface in general.

[iholder101]

@kolyshkin ping.

[iholder101]

@iholder101 sorry for not seeing it earlier. I took another look, and I'm not sure we need this. I mean, what this PR does can probably be implemented via something like

err := cgroups.WriteFile(m.Path("")+"/cgroup.type", "threaded")

Surely, this code has some assumptions, notably:

1. Writing threaded is more efficient than reading, comparing, and conditionally writing (this needs to be checked in the kernel sources);
2. If it can't be made threaded, write() will return an error.

So, considering that this functionality

• can be implemented in one line of code using existing runc/libcontainer/cgroups;
• is not used by runc itself;
• takes 54 lines in 7 files to implement,

does it really make sense to add it?

You're right, but this argument can be given for any interface change whatsoever.

For example, GetPids() simply reads cgroup.procs which can be read manually from cgroups.ReadFile. Same thing goes for Set() or any other method. After all, anything (except maybe device permissions which are implemented via eBPF on v2) can be performed manually by reading and writing to files.

I wonder: what is the purpose of this interface? In my mind, the purpose is to simplify and provide a declarative interface for the operations that can be performed on a cgroup. To an advanced user, it might be obvious that "threaded" needs to be written to "cgroup.type", but I think this kind of misses the point of having such an interface in the first place.

Another example which proves this point is Apply(). The documentation says Apply creates a cgroup, if not yet created, and adds a process with the specified pid into that cgroup, but what about threads? On v2, to create a new threaded sub-cgroup there's a need to:

1. Create a folder inside the root cgroup
2. Make it threaded manually
3. Write the TIDs into cgroup.threads

I don't really understand how it makes sense that this operation requires one method call when processes are involved, but requires many manual operations for threads.

I would like to see an interface with many methods that covers, more or less, everything that can be done with cgroups. That would help the user to know which operations stand at their hands without having to dive deep into the documentation and become an advanced user. Do you agree with this? If not, I would be interested to know what's your perspective regarding the purpose of this interface in general.

@kolyshkin Sadly, a year have passed since my last ping (although we've already witnessed another need for it) and the discussion was cut off.

If you're still willing to discuss this, I think we should make the effort of expanding the cgroup manager interface, since it currently lacks a lot of basic functionality which is being re-implemented by users continuously. I'm willing to consider helping with both design and implementation.