Check for non-active/supported cgroups #77
Conversation
Signed-off-by: Michael Crosby <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
+ Coverage 14.79% 14.91% +0.11%
==========================================
Files 23 24 +1
Lines 4588 4619 +31
==========================================
+ Hits 679 689 +10
- Misses 3787 3807 +20
- Partials 122 123 +1
Continue to review full report at Codecov.
|
crosbymichael
force-pushed
the
non-active
branch
2 times, most recently
from
8f9b3d2 to
79e9b4e
Compare
|
I was able to verify this on a system that Tonis setup for me. I'm happy with these changes and this is good to review. > docker run alpine true
docker: Error response from daemon: controller is not supported: unknown.
ERRO[0000] error waiting for container: context canceled
> cp * /usr/bin/ #patched containerd
> docker run alpine true
> docker run alpine ls
bin
dev
etc
home
lib
media
mnt
opt
proc
root
run
sbin
srv
sys
tmp
usr
var |
| if os.IsNotExist(errors.Cause(err)) { | ||
| return nil, ErrCgroupDeleted | ||
| } | ||
| if err == ErrControllerNotActive { |
There was a problem hiding this comment.
Do we need to change the location for if os.IsNotExist(errors.Cause(err)) and if err == ErrControllerNotActive, like
if err == ErrControllerNotActive {
for _, o := range skip {
if skerr := o(s, path, err); skerr != nil {
if skerr != ErrSkipSubsystem {
return nil, skerr
}
}
}
continue
}
if os.IsNotExist(errors.Cause(err)) {
return nil, ErrCgroupDeleted
}
first check controller is support, then check the directory is exist ? if the controller is not support, we can ignore the controller directory is not exist
There was a problem hiding this comment.
I think this is fine to keep the existing logics for Load, the isnotexist is for when the cgroup directory structure has been removed, not an active controller. I think this is fine for the Load case to get the ErrCgroupDeleted error which is handled
|
Overall looks really good to me, from reading the patch, I think it can solve my problem (I mount some controller like pid, cpu, memory through cgroup2) |
| path := existingPath(paths, "") | ||
| _, err = path("net_prio") | ||
| if err == nil { | ||
| t.Fatal("error for net_prio shoulld not be nil") |
cgroup.go
Outdated
|
|
||
| // New returns a new control via the cgroup cgroups interface | ||
| func New(hierarchy Hierarchy, path Path, resources *specs.LinuxResources) (Cgroup, error) { | ||
| func New(hierarchy Hierarchy, path Path, resources *specs.LinuxResources, skip ...SkipOpts) (Cgroup, error) { |
There was a problem hiding this comment.
// ...checks whether device subsystems exist, provide SkipOpts func(s) to make various subsystems required or optional (if not provided defaults to requires devices subsystem and other subsystems are optional...)
| return &cgroup{ | ||
| path: path, | ||
| subsystems: subsystems, | ||
| subsystems: active, |
There was a problem hiding this comment.
how about adding a list for skipped .. or inactive?
There was a problem hiding this comment.
not sure we need it
cgroup.go
Outdated
|
|
||
| // Load will load an existing cgroup and allow it to be controlled | ||
| func Load(hierarchy Hierarchy, path Path) (Cgroup, error) { | ||
| func Load(hierarchy Hierarchy, path Path, skip ...SkipOpts) (Cgroup, error) { |
There was a problem hiding this comment.
ditto for update to function description
This adds a flexible way for subsystems to be skipped or required within the cgroups package. Signed-off-by: Michael Crosby <[email protected]>
crosbymichael
force-pushed
the
non-active
branch
from
79e9b4e to
4a9f0f7
Compare
7 participants
Fixes #76
I still need to get a system setup to test this fix.
Signed-off-by: Michael Crosby [email protected]