From 859a780d6f326dd3751893620b3198dd55af4ac1 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 11 May 2020 15:19:30 +1000 Subject: [PATCH] cgroups: add GetFreezerState() helper to Manager This is effectively a nicer implementation of the container.isPaused() helper, but to be used within the cgroup code for handling some fun issues we have to fix with the systemd cgroup driver. Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/cgroups.go | 3 ++ libcontainer/cgroups/fs/apply_raw.go | 40 +++++++++++------- libcontainer/cgroups/fs/freezer.go | 34 ++++++++++++++- libcontainer/cgroups/fs2/freezer.go | 63 ++++++++++++++++++---------- libcontainer/cgroups/fs2/fs2.go | 4 ++ libcontainer/cgroups/systemd/v1.go | 12 ++++++ libcontainer/cgroups/systemd/v2.go | 8 ++++ libcontainer/container_linux.go | 25 ++--------- libcontainer/container_linux_test.go | 5 +++ 9 files changed, 135 insertions(+), 59 deletions(-) diff --git a/libcontainer/cgroups/cgroups.go b/libcontainer/cgroups/cgroups.go index cbc430bb..2bb2d1e8 100644 --- a/libcontainer/cgroups/cgroups.go +++ b/libcontainer/cgroups/cgroups.go @@ -44,6 +44,9 @@ type Manager interface { // GetCgroups returns the cgroup data as configured. GetCgroups() (*configs.Cgroup, error) + + // GetFreezerState retrieves the current FreezerState of the cgroup. + GetFreezerState() (configs.FreezerState, error) } type NotFoundError struct { diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index c3060145..c3d2c9ef 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -166,9 +166,6 @@ func (m *manager) Apply(pid int) (err error) { } for _, sys := range m.getSubsystems() { - // TODO: Apply should, ideally, be reentrant or be broken up into a separate - // create and join phase so that the cgroup hierarchy for a container can be - // created then join consists of writing the process pids to cgroup.procs p, err := d.path(sys.Name()) if err != nil { // The non-presence of the devices subsystem is @@ -181,10 +178,10 @@ func (m *manager) Apply(pid int) (err error) { m.paths[sys.Name()] = p if err := sys.Apply(d); err != nil { - // In the case of rootless (including euid=0 in userns), where an explicit cgroup path hasn't - // been set, we don't bail on error in case of permission problems. - // Cases where limits have been set (and we couldn't create our own - // cgroup) are handled by Set. + // In the case of rootless (including euid=0 in userns), where an + // explicit cgroup path hasn't been set, we don't bail on error in + // case of permission problems. Cases where limits have been set + // (and we couldn't create our own cgroup) are handled by Set. if isIgnorableError(m.rootless, err) && m.cgroups.Path == "" { delete(m.paths, sys.Name()) continue @@ -272,22 +269,25 @@ func (m *manager) Set(container *configs.Config) error { // Freeze toggles the container's freezer cgroup depending on the state // provided -func (m *manager) Freeze(state configs.FreezerState) error { - if m.cgroups == nil { +func (m *manager) Freeze(state configs.FreezerState) (Err error) { + path := m.GetPaths()["freezer"] + if m.cgroups == nil || path == "" { return errors.New("cannot toggle freezer: cgroups not configured for container") } - paths := m.GetPaths() - dir := paths["freezer"] prevState := m.cgroups.Resources.Freezer m.cgroups.Resources.Freezer = state + defer func() { + if Err != nil { + m.cgroups.Resources.Freezer = prevState + } + }() + freezer, err := m.getSubsystems().Get("freezer") if err != nil { return err } - err = freezer.Set(dir, m.cgroups) - if err != nil { - m.cgroups.Resources.Freezer = prevState + if err := freezer.Set(path, m.cgroups); err != nil { return err } return nil @@ -413,3 +413,15 @@ func (m *manager) GetPaths() map[string]string { func (m *manager) GetCgroups() (*configs.Cgroup, error) { return m.cgroups, nil } + +func (m *manager) GetFreezerState() (configs.FreezerState, error) { + paths := m.GetPaths() + dir := paths["freezer"] + freezer, err := m.getSubsystems().Get("freezer") + + // If the container doesn't have the freezer cgroup, say it's undefined. + if err != nil || dir == "" { + return configs.Undefined, nil + } + return freezer.(*FreezerGroup).GetState(dir) +} diff --git a/libcontainer/cgroups/fs/freezer.go b/libcontainer/cgroups/fs/freezer.go index 9dc81bdb..b5b77da9 100644 --- a/libcontainer/cgroups/fs/freezer.go +++ b/libcontainer/cgroups/fs/freezer.go @@ -3,13 +3,16 @@ package fs import ( + "errors" "fmt" + "os" "strings" "time" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" + "golang.org/x/sys/unix" ) type FreezerGroup struct { @@ -39,11 +42,11 @@ func (s *FreezerGroup) Set(path string, cgroup *configs.Cgroup) error { return err } - state, err := fscommon.ReadFile(path, "freezer.state") + state, err := s.GetState(path) if err != nil { return err } - if strings.TrimSpace(state) == string(cgroup.Resources.Freezer) { + if state == cgroup.Resources.Freezer { break } @@ -65,3 +68,30 @@ func (s *FreezerGroup) Remove(d *cgroupData) error { func (s *FreezerGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } + +func (s *FreezerGroup) GetState(path string) (configs.FreezerState, error) { + for { + state, err := fscommon.ReadFile(path, "freezer.state") + if err != nil { + // If the kernel is too old, then we just treat the freezer as + // being in an "undefined" state. + if os.IsNotExist(err) || errors.Is(err, unix.ENODEV) { + err = nil + } + return configs.Undefined, err + } + switch strings.TrimSpace(state) { + case "THAWED": + return configs.Thawed, nil + case "FROZEN": + return configs.Frozen, nil + case "FREEZING": + // Make sure we get a stable freezer state, so retry if the cgroup + // is still undergoing freezing. This should be a temporary delay. + time.Sleep(1 * time.Millisecond) + continue + default: + return configs.Undefined, fmt.Errorf("unknown freezer.state %q", state) + } + } +} diff --git a/libcontainer/cgroups/fs2/freezer.go b/libcontainer/cgroups/fs2/freezer.go index 130c63f3..213ff65a 100644 --- a/libcontainer/cgroups/fs2/freezer.go +++ b/libcontainer/cgroups/fs2/freezer.go @@ -3,32 +3,49 @@ package fs2 import ( - "strconv" + stdErrors "errors" + "os" "strings" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" "github.com/pkg/errors" + "golang.org/x/sys/unix" ) func setFreezer(dirPath string, state configs.FreezerState) error { - var desired int + if err := supportsFreezer(dirPath); err != nil { + // We can ignore this request as long as the user didn't ask us to + // freeze the container (since without the freezer cgroup, that's a + // no-op). + if state == configs.Undefined || state == configs.Thawed { + err = nil + } + return errors.Wrap(err, "freezer not supported") + } + + var stateStr string switch state { case configs.Undefined: return nil case configs.Frozen: - desired = 1 + stateStr = "1" case configs.Thawed: - desired = 0 + stateStr = "0" default: - return errors.Errorf("unknown freezer state %+v", state) + return errors.Errorf("invalid freezer state %q requested", state) } - supportedErr := supportsFreezer(dirPath) - if supportedErr != nil && desired != 0 { - // can ignore error if desired == 1 - return errors.Wrap(supportedErr, "freezer not supported") + + if err := fscommon.WriteFile(dirPath, "cgroup.freeze", stateStr); err != nil { + return err } - return freezeWithInt(dirPath, desired) + // Confirm that the cgroup did actually change states. + if actualState, err := getFreezer(dirPath); err != nil { + return err + } else if actualState != state { + return errors.Errorf(`expected "cgroup.freeze" to be in state %q but was in %q`, state, actualState) + } + return nil } func supportsFreezer(dirPath string) error { @@ -36,18 +53,22 @@ func supportsFreezer(dirPath string) error { return err } -// freeze writes desired int to "cgroup.freeze". -func freezeWithInt(dirPath string, desired int) error { - desiredS := strconv.Itoa(desired) - if err := fscommon.WriteFile(dirPath, "cgroup.freeze", desiredS); err != nil { - return err - } - got, err := fscommon.ReadFile(dirPath, "cgroup.freeze") +func getFreezer(dirPath string) (configs.FreezerState, error) { + state, err := fscommon.ReadFile(dirPath, "cgroup.freeze") if err != nil { - return err + // If the kernel is too old, then we just treat the freezer as being in + // an "undefined" state. + if os.IsNotExist(err) || stdErrors.Is(err, unix.ENODEV) { + err = nil + } + return configs.Undefined, err } - if gotS := strings.TrimSpace(string(got)); gotS != desiredS { - return errors.Errorf("expected \"cgroup.freeze\" in %q to be %q, got %q", dirPath, desiredS, gotS) + switch strings.TrimSpace(state) { + case "0": + return configs.Thawed, nil + case "1": + return configs.Frozen, nil + default: + return configs.Undefined, errors.Errorf(`unknown "cgroup.freeze" state: %q`, state) } - return nil } diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index a0881dac..4da75957 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -240,3 +240,7 @@ func (m *manager) GetPaths() map[string]string { func (m *manager) GetCgroups() (*configs.Cgroup, error) { return m.config, nil } + +func (m *manager) GetFreezerState() (configs.FreezerState, error) { + return getFreezer(m.dirPath) +} diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 70440ea6..933d46e0 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -439,3 +439,15 @@ func (m *legacyManager) GetPaths() map[string]string { func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) { return m.cgroups, nil } + +func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) { + path, err := getSubsystemPath(m.cgroups, "freezer") + if err != nil && !cgroups.IsNotFound(err) { + return configs.Undefined, err + } + freezer, err := legacySubsystems.Get("freezer") + if err != nil { + return configs.Undefined, err + } + return freezer.(*fs.FreezerGroup).GetState(path) +} diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index d43c855b..a23b40e1 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -319,3 +319,11 @@ func (m *unifiedManager) GetPaths() map[string]string { func (m *unifiedManager) GetCgroups() (*configs.Cgroup, error) { return m.cgroups, nil } + +func (m *unifiedManager) GetFreezerState() (configs.FreezerState, error) { + fsMgr, err := m.fsManager() + if err != nil { + return configs.Undefined, err + } + return fsMgr.GetFreezerState() +} diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 5cea3d6c..e7220843 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1847,30 +1847,11 @@ func (c *linuxContainer) runType() Status { } func (c *linuxContainer) isPaused() (bool, error) { - var filename, pausedState string - - fcg := c.cgroupManager.Path("freezer") - if !cgroups.IsCgroup2UnifiedMode() { - if fcg == "" { - // container doesn't have a freezer cgroup - return false, nil - } - filename = "freezer.state" - pausedState = "FROZEN" - } else { - filename = "cgroup.freeze" - pausedState = "1" - } - - data, err := ioutil.ReadFile(filepath.Join(fcg, filename)) + state, err := c.cgroupManager.GetFreezerState() if err != nil { - // If freezer cgroup is not mounted, the container would just be not paused. - if os.IsNotExist(err) || errors.Is(err, unix.ENODEV) { - return false, nil - } - return false, newSystemErrorWithCause(err, "checking if container is paused") + return false, err } - return bytes.Equal(bytes.TrimSpace(data), []byte(pausedState)), nil + return state == configs.Frozen, nil } func (c *linuxContainer) currentState() (*State, error) { diff --git a/libcontainer/container_linux_test.go b/libcontainer/container_linux_test.go index 356e8f1a..72f0835c 100644 --- a/libcontainer/container_linux_test.go +++ b/libcontainer/container_linux_test.go @@ -61,10 +61,15 @@ func (m *mockCgroupManager) Path(subsys string) string { func (m *mockCgroupManager) Freeze(state configs.FreezerState) error { return nil } + func (m *mockCgroupManager) GetCgroups() (*configs.Cgroup, error) { return nil, nil } +func (m *mockCgroupManager) GetFreezerState() (configs.FreezerState, error) { + return configs.Thawed, nil +} + func (m *mockIntelRdtManager) Apply(pid int) error { return nil }