From a67dab0ac2dd3829858e35ba643613a1e358ba87 Mon Sep 17 00:00:00 2001 From: lifubang Date: Sun, 10 May 2020 07:14:05 +0800 Subject: [PATCH 1/2] Revert "CreateCgroupPath: only enable needed controllers" 1. Partially revert "CreateCgroupPath: only enable needed controllers" If we update a resource which did not limited in the beginning, it will have no effective. 2. Returns err if we use an non enabled controller, or else the user may feel success, but actually there are no effective. Signed-off-by: lifubang --- libcontainer/cgroups/fs2/create.go | 78 +++++++++++++++--------------- libcontainer/cgroups/fs2/fs2.go | 47 ++++++------------ 2 files changed, 55 insertions(+), 70 deletions(-) diff --git a/libcontainer/cgroups/fs2/create.go b/libcontainer/cgroups/fs2/create.go index ba50c8d47..7be9ece0b 100644 --- a/libcontainer/cgroups/fs2/create.go +++ b/libcontainer/cgroups/fs2/create.go @@ -1,6 +1,7 @@ package fs2 import ( + "bytes" "fmt" "io/ioutil" "os" @@ -10,57 +11,56 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -// neededControllers returns the string to write to cgroup.subtree_control, -// containing the list of controllers to enable (for example, "+cpu +pids"), -// based on (1) controllers available and (2) resources that are being set. -// -// The resulting string does not include "pseudo" controllers such as -// "freezer" and "devices". -func neededControllers(cgroup *configs.Cgroup) ([]string, error) { - var list []string +func supportedControllers(cgroup *configs.Cgroup) ([]byte, error) { + const file = UnifiedMountpoint + "/cgroup.controllers" + return ioutil.ReadFile(file) +} +// needAnyControllers returns whether we enable some supported controllers or not, +// based on (1) controllers available and (2) resources that are being set. +// We don't check "pseudo" controllers such as +// "freezer" and "devices". +func needAnyControllers(cgroup *configs.Cgroup) (bool, error) { if cgroup == nil { - return list, nil + return false, nil } // list of all available controllers - const file = UnifiedMountpoint + "/cgroup.controllers" - content, err := ioutil.ReadFile(file) + content, err := supportedControllers(cgroup) if err != nil { - return list, err + return false, err } avail := make(map[string]struct{}) for _, ctr := range strings.Fields(string(content)) { avail[ctr] = struct{}{} } - // add the controller if available - add := func(controller string) { - if _, ok := avail[controller]; ok { - list = append(list, "+"+controller) - } + // check whether the controller if available or not + have := func(controller string) bool { + _, ok := avail[controller] + return ok } - if isPidsSet(cgroup) { - add("pids") + if isPidsSet(cgroup) && have("pids") { + return true, nil } - if isMemorySet(cgroup) { - add("memory") + if isMemorySet(cgroup) && have("memory") { + return true, nil } - if isIoSet(cgroup) { - add("io") + if isIoSet(cgroup) && have("io") { + return true, nil } - if isCpuSet(cgroup) { - add("cpu") + if isCpuSet(cgroup) && have("cpu") { + return true, nil } - if isCpusetSet(cgroup) { - add("cpuset") + if isCpusetSet(cgroup) && have("cpuset") { + return true, nil } - if isHugeTlbSet(cgroup) { - add("hugetlb") + if isHugeTlbSet(cgroup) && have("hugetlb") { + return true, nil } - return list, nil + return false, nil } // containsDomainController returns whether the current config contains domain controller or not. @@ -70,18 +70,19 @@ func containsDomainController(cg *configs.Cgroup) bool { return isMemorySet(cg) || isIoSet(cg) || isCpuSet(cg) || isHugeTlbSet(cg) } -// CreateCgroupPath creates cgroupv2 path, enabling all the -// needed controllers in the process. +// CreateCgroupPath creates cgroupv2 path, enabling all the supported controllers. func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) { if !strings.HasPrefix(path, UnifiedMountpoint) { return fmt.Errorf("invalid cgroup path %s", path) } - ctrs, err := neededControllers(c) + content, err := supportedControllers(c) if err != nil { return err } - allCtrs := strings.Join(ctrs, " ") + + ctrs := bytes.Fields(content) + res := append([]byte("+"), bytes.Join(ctrs, []byte(" +"))...) elements := strings.Split(path, "/") elements = elements[3:] @@ -131,13 +132,14 @@ func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) { } } } - // enable needed controllers + // enable all supported controllers if i < len(elements)-1 { file := filepath.Join(current, "cgroup.subtree_control") - if err := ioutil.WriteFile(file, []byte(allCtrs), 0644); err != nil { + if err := ioutil.WriteFile(file, res, 0644); err != nil { // try write one by one - for _, ctr := range ctrs { - _ = ioutil.WriteFile(file, []byte(ctr), 0644) + allCtrs := bytes.Split(res, []byte(" ")) + for _, ctr := range allCtrs { + _ = ioutil.WriteFile(file, ctr, 0644) } } // Some controllers might not be enabled when rootless or containerized, diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 4da759576..b7fe44277 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -76,8 +76,7 @@ func (m *manager) Apply(pid int) error { // - "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" if m.rootless { if m.config.Path == "" { - cl, clErr := neededControllers(m.config) - if clErr == nil && len(cl) == 0 { + if blNeed, nErr := needAnyControllers(m.config); nErr == nil && !blNeed { return nil } return errors.Wrap(err, "rootless needs no limits + no cgrouppath when no permission is granted for cgroups") @@ -175,30 +174,21 @@ func (m *manager) Set(container *configs.Config) error { if err := m.getControllers(); err != nil { return err } - var errs []error // pids (since kernel 4.5) - if _, ok := m.controllers["pids"]; ok { - if err := setPids(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setPids(m.dirPath, container.Cgroups); err != nil { + return err } // memory (since kernel 4.5) - if _, ok := m.controllers["memory"]; ok { - if err := setMemory(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setMemory(m.dirPath, container.Cgroups); err != nil { + return err } // io (since kernel 4.5) - if _, ok := m.controllers["io"]; ok { - if err := setIo(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setIo(m.dirPath, container.Cgroups); err != nil { + return err } // cpu (since kernel 4.15) - if _, ok := m.controllers["cpu"]; ok { - if err := setCpu(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setCpu(m.dirPath, container.Cgroups); err != nil { + return err } // devices (since kernel 4.15, pseudo-controller) // @@ -206,26 +196,19 @@ func (m *manager) Set(container *configs.Config) error { // However, errors from other subsystems are not ignored. // see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" if err := setDevices(m.dirPath, container.Cgroups); err != nil && !m.rootless { - errs = append(errs, err) + return err } // cpuset (since kernel 5.0) - if _, ok := m.controllers["cpuset"]; ok { - if err := setCpuset(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setCpuset(m.dirPath, container.Cgroups); err != nil { + return err } // hugetlb (since kernel 5.6) - if _, ok := m.controllers["hugetlb"]; ok { - if err := setHugeTlb(m.dirPath, container.Cgroups); err != nil { - errs = append(errs, err) - } + if err := setHugeTlb(m.dirPath, container.Cgroups); err != nil { + return err } // freezer (since kernel 5.2, pseudo-controller) if err := setFreezer(m.dirPath, container.Cgroups.Freezer); err != nil { - errs = append(errs, err) - } - if len(errs) > 0 { - return errors.Errorf("error while setting cgroup v2: %+v", errs) + return err } m.config = container.Cgroups return nil From 275157193cf9e739a53e1be9973f9bb85d1e4129 Mon Sep 17 00:00:00 2001 From: lifubang Date: Thu, 21 May 2020 00:17:51 +0800 Subject: [PATCH 2/2] add testcase for enable all supported controllers in cgroupv2 Signed-off-by: lifubang --- tests/integration/cgroups.bats | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats index 84737eaaf..d5df2858b 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -110,6 +110,17 @@ EOF runc run -d --console-socket $CONSOLE_SOCKET test_cgroups_permissions [ "$status" -eq 0 ] + if [ "$CGROUP_UNIFIED" != "no" ]; then + if [ -n "${RUNC_USE_SYSTEMD}" ] ; then + if [ $(id -u) = "0" ]; then + check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/machine.slice/cgroup.controllers)" + else + check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers)" + fi + else + check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/cgroup.controllers)" + fi + fi } @test "runc exec (limits + cgrouppath + permission on the cgroup dir) succeeds" {