Move the cgroups v2 hybrid check from init() #3432
Conversation
Other code that imports the libcontainer cgroups library may be vulnerable to panic in the function due to insufficient permissions. Signed-off-by: Mrunal Patel <[email protected]>
|
We could also save the error from the function and propagate it up. |
|
is a panic on a permissions error on that path really the correct response for all callers? returning a bool, error and letting the caller decide if it should be a fatal error would have been what I expected. |
Yes, that's what I posed in my comment above to the other maintainers. This particular function is a relative easy fix with 2 callers. We also have the existing Our options are:
|
| // If using cgroups-hybrid mode then add a "" controller indicating | ||
| // it should join the cgroups v2. | ||
| if cgroups.IsCgroup2HybridMode() { | ||
| subsystems = append(subsystems, &NameGroup{GroupName: "", Join: true}) |
There was a problem hiding this comment.
This is mutating a package var in a constructor. Can that race with other readers of subsystems? If the constructor is called multiple times, this will append multiple times, right?
There was a problem hiding this comment.
Ugh, yeah, from a quick look, we could move the subsystems under the manager instance.
There was a problem hiding this comment.
Pushed a commit to test that out (will check later).
Signed-off-by: Mrunal Patel <[email protected]>
| mu sync.Mutex | ||
| cgroups *configs.Cgroup | ||
| paths map[string]string | ||
| subsystems []subsystem |
There was a problem hiding this comment.
It does not make sense to have a per-manager subsystem list.
Instead, I think, we should keep subsystems global, and wrap their initialization into once.Do.
|
I took a look at it seems it's better to just avoid panicking in |
|
Closing in favor of #3433 |
|
Yeah that’s what I was debating, too. I think your getting rid of the panic is probably best for now.
… On Mar 26, 2022, at 5:35 PM, Kir Kolyshkin ***@***.***> wrote:
@kolyshkin commented on this pull request.
In libcontainer/cgroups/fs/fs.go:
> @@ -46,9 +29,10 @@ type subsystem interface {
}
type manager struct {
- mu sync.Mutex
- cgroups *configs.Cgroup
- paths map[string]string
+ mu sync.Mutex
+ cgroups *configs.Cgroup
+ paths map[string]string
+ subsystems []subsystem
It does not make sense to have a per-manager subsystem list.
Instead, I think, we should keep subsystems global, and wrap their initialization into once.Do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
Other code that imports the libcontainer cgroups library
may be vulnerable to panic in the function due to insufficient
permissions.
Signed-off-by: Mrunal Patel [email protected]