Skip to content

Check for non-active/supported cgroups #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions cgroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,49 @@ import (
)

// 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, opts ...InitOpts) (Cgroup, error) {
config := newInitConfig()
for _, o := range opts {
if err := o(config); err != nil {
return nil, err
}
}
subsystems, err := hierarchy()
if err != nil {
return nil, err
}
var active []Subsystem
for _, s := range subsystems {
// check if subsystem exists
if err := initializeSubsystem(s, path, resources); err != nil {
if err == ErrControllerNotActive {
if config.InitCheck != nil {
if skerr := config.InitCheck(s, path, err); skerr != nil {
if skerr != ErrIgnoreSubsystem {
return nil, skerr
}
}
}
continue
}
return nil, err
}
active = append(active, s)
}
return &cgroup{
path: path,
subsystems: subsystems,
subsystems: active,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding a list for skipped .. or inactive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need it

}, nil
}

// 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, opts ...InitOpts) (Cgroup, error) {
config := newInitConfig()
for _, o := range opts {
if err := o(config); err != nil {
return nil, err
}
}
var activeSubsystems []Subsystem
subsystems, err := hierarchy()
if err != nil {
Expand All @@ -60,6 +85,16 @@ func Load(hierarchy Hierarchy, path Path) (Cgroup, error) {
if os.IsNotExist(errors.Cause(err)) {
return nil, ErrCgroupDeleted
}
if err == ErrControllerNotActive {
Copy link

@Ace-Tang Ace-Tang Feb 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if config.InitCheck != nil {
if skerr := config.InitCheck(s, path, err); skerr != nil {
if skerr != ErrIgnoreSubsystem {
return nil, skerr
}
}
}
continue
}
return nil, err
}
if _, err := os.Lstat(s.Path(p)); err != nil {
Expand Down
61 changes: 61 additions & 0 deletions opts.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
Copyright The containerd Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cgroups

import (
"github.com/pkg/errors"
)

var (
// ErrIgnoreSubsystem allows the specific subsystem to be skipped
ErrIgnoreSubsystem = errors.New("skip subsystem")
// ErrDevicesRequired is returned when the devices subsystem is required but
// does not exist or is not active
ErrDevicesRequired = errors.New("devices subsystem is required")
)

// InitOpts allows configuration for the creation or loading of a cgroup
type InitOpts func(*InitConfig) error

// InitConfig provides configuration options for the creation
// or loading of a cgroup and its subsystems
type InitConfig struct {
// InitCheck can be used to check initialization errors from the subsystem
InitCheck InitCheck
}

func newInitConfig() *InitConfig {
return &InitConfig{
InitCheck: RequireDevices,
}
}

// InitCheck allows subsystems errors to be checked when initialized or loaded
type InitCheck func(Subsystem, Path, error) error

// AllowAny allows any subsystem errors to be skipped
func AllowAny(s Subsystem, p Path, err error) error {
return ErrIgnoreSubsystem
}

// RequireDevices requires the device subsystem but no others
func RequireDevices(s Subsystem, p Path, err error) error {
if s.Name() == Devices {
return ErrDevicesRequired
}
return ErrIgnoreSubsystem
}
5 changes: 4 additions & 1 deletion paths.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ func PidPath(pid int) Path {
return existingPath(paths, "")
}

// ErrControllerNotActive is returned when a controller is not supported or enabled
var ErrControllerNotActive = errors.New("controller is not supported")

func existingPath(paths map[string]string, suffix string) Path {
// localize the paths based on the root mount dest for nested cgroups
for n, p := range paths {
Expand All @@ -77,7 +80,7 @@ func existingPath(paths map[string]string, suffix string) Path {
root, ok := paths[string(name)]
if !ok {
if root, ok = paths[fmt.Sprintf("name=%s", name)]; !ok {
return "", fmt.Errorf("unable to find %q in controller set", name)
return "", ErrControllerNotActive
}
}
if suffix != "" {
Expand Down
26 changes: 26 additions & 0 deletions paths_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,29 @@ func TestEmptySubsystem(t *testing.T) {
}
}
}

func TestSystemd240(t *testing.T) {
const data = `8:net_cls:/
7:memory:/system.slice/docker.service
6:freezer:/
5:blkio:/system.slice/docker.service
4:devices:/system.slice/docker.service
3:cpuset:/
2:cpu,cpuacct:/system.slice/docker.service
1:name=systemd:/system.slice/docker.service
0::/system.slice/docker.service`
r := strings.NewReader(data)
paths, err := parseCgroupFromReader(r)
if err != nil {
t.Fatal(err)
}

path := existingPath(paths, "")
_, err = path("net_prio")
if err == nil {
t.Fatal("error for net_prio shoulld not be nil")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/s/shoulld/should/

}
if err != ErrControllerNotActive {
t.Fatalf("expected error %q but received %q", ErrControllerNotActive, err)
}
}