From 4b4bc995ad4ee4679ffbe38f1672be01c24256fc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 8 Apr 2020 03:37:47 -0700 Subject: [PATCH] CreateCgroupPath: only enable needed controllers 1. Instead of enabling all available controllers, figure out which ones are required, and only enable those. 2. Amend all setFoo() functions to call isFooSet(). While this might seem unnecessary, it might actually help to uncover a bug. Imagine someone: - adds a cgroup.Resources.CpuFoo setting; - modifies setCpu() to apply the new setting; - but forgets to amend isCpuSet() accordingly <-- BUG In this case, a test case modifying CpuFoo will help to uncover the BUG. This is the reason why it's added. This patch *could be* amended by enabling controllers on a best-effort basis, i.e. : - do not return an error early if we can't enable some controllers; - if we fail to enable all controllers at once (usually because one of them can't be enabled), try enabling them one by one. Currently this is not implemented, and it's not clear whether this would be a good way to go or not. [v2: add/use is${Controller}Set() functions] [v3: document neededControllers()] [v4: drop "best-effort" part] Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/cpu.go | 8 ++++ libcontainer/cgroups/fs2/cpuset.go | 8 ++++ libcontainer/cgroups/fs2/create.go | 74 +++++++++++++++++++++++++---- libcontainer/cgroups/fs2/fs2.go | 2 +- libcontainer/cgroups/fs2/hugetlb.go | 7 +++ libcontainer/cgroups/fs2/io.go | 12 +++++ libcontainer/cgroups/fs2/memory.go | 8 ++++ libcontainer/cgroups/fs2/pids.go | 7 +++ libcontainer/cgroups/systemd/v2.go | 2 +- 9 files changed, 117 insertions(+), 11 deletions(-) diff --git a/libcontainer/cgroups/fs2/cpu.go b/libcontainer/cgroups/fs2/cpu.go index 8ec85571..7129cf79 100644 --- a/libcontainer/cgroups/fs2/cpu.go +++ b/libcontainer/cgroups/fs2/cpu.go @@ -13,7 +13,15 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) +func isCpuSet(cgroup *configs.Cgroup) bool { + return cgroup.Resources.CpuWeight != 0 || cgroup.Resources.CpuMax != "" +} + func setCpu(dirPath string, cgroup *configs.Cgroup) error { + if !isCpuSet(cgroup) { + return nil + } + // NOTE: .CpuShares is not used here. Conversion is the caller's responsibility. if cgroup.Resources.CpuWeight != 0 { if err := fscommon.WriteFile(dirPath, "cpu.weight", strconv.FormatUint(cgroup.Resources.CpuWeight, 10)); err != nil { diff --git a/libcontainer/cgroups/fs2/cpuset.go b/libcontainer/cgroups/fs2/cpuset.go index 6492ac93..fb4456b4 100644 --- a/libcontainer/cgroups/fs2/cpuset.go +++ b/libcontainer/cgroups/fs2/cpuset.go @@ -7,7 +7,15 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) +func isCpusetSet(cgroup *configs.Cgroup) bool { + return cgroup.Resources.CpusetCpus != "" || cgroup.Resources.CpusetMems != "" +} + func setCpuset(dirPath string, cgroup *configs.Cgroup) error { + if !isCpusetSet(cgroup) { + return nil + } + if cgroup.Resources.CpusetCpus != "" { if err := fscommon.WriteFile(dirPath, "cpuset.cpus", cgroup.Resources.CpusetCpus); err != nil { return err diff --git a/libcontainer/cgroups/fs2/create.go b/libcontainer/cgroups/fs2/create.go index e4be1e3a..0cc2c1f6 100644 --- a/libcontainer/cgroups/fs2/create.go +++ b/libcontainer/cgroups/fs2/create.go @@ -1,29 +1,80 @@ package fs2 import ( - "bytes" "fmt" "io/ioutil" "os" "path/filepath" "strings" + + "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 + + if cgroup == nil { + return list, nil + } + + // list of all available controllers + const file = UnifiedMountpoint + "/cgroup.controllers" + content, err := ioutil.ReadFile(file) + if err != nil { + return list, 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) + } + } + + if isPidsSet(cgroup) { + add("pids") + } + if isMemorySet(cgroup) { + add("memory") + } + if isIoSet(cgroup) { + add("io") + } + if isCpuSet(cgroup) { + add("cpu") + } + if isCpusetSet(cgroup) { + add("cpuset") + } + if isHugeTlbSet(cgroup) { + add("hugetlb") + } + + return list, nil +} + // CreateCgroupPath creates cgroupv2 path, enabling all the -// available controllers in the process. -func CreateCgroupPath(path string) (Err error) { +// needed controllers in the process. +func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) { if !strings.HasPrefix(path, UnifiedMountpoint) { return fmt.Errorf("invalid cgroup path %s", path) } - const file = UnifiedMountpoint + "/cgroup.controllers" - content, err := ioutil.ReadFile(file) + ctrs, err := neededControllers(c) if err != nil { return err } - - ctrs := bytes.Fields(content) - res := append([]byte("+"), bytes.Join(ctrs, []byte(" +"))...) + allCtrs := strings.Join(ctrs, " ") elements := strings.Split(path, "/") elements = elements[3:] @@ -45,11 +96,16 @@ func CreateCgroupPath(path string) (Err error) { }() } } + // enable needed controllers if i < len(elements)-1 { - if err := ioutil.WriteFile(filepath.Join(current, "cgroup.subtree_control"), res, 0755); err != nil { + file := filepath.Join(current, "cgroup.subtree_control") + if err := ioutil.WriteFile(file, []byte(allCtrs), 0755); err != nil { + // XXX: we can enable _some_ controllers doing it one-by one + // instead of erroring out -- does it makes sense to do so? return err } } } + return nil } diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index f32f647d..d845a1b5 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -66,7 +66,7 @@ func (m *manager) getControllers() error { } func (m *manager) Apply(pid int) error { - if err := CreateCgroupPath(m.dirPath); err != nil { + if err := CreateCgroupPath(m.dirPath, m.config); err != nil { return err } if err := cgroups.WriteCgroupProc(m.dirPath, pid); err != nil && !m.rootless { diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go index c7610c0a..4a399aae 100644 --- a/libcontainer/cgroups/fs2/hugetlb.go +++ b/libcontainer/cgroups/fs2/hugetlb.go @@ -15,7 +15,14 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) +func isHugeTlbSet(cgroup *configs.Cgroup) bool { + return len(cgroup.Resources.HugetlbLimit) > 0 +} + func setHugeTlb(dirPath string, cgroup *configs.Cgroup) error { + if !isHugeTlbSet(cgroup) { + return nil + } for _, hugetlb := range cgroup.Resources.HugetlbLimit { if err := fscommon.WriteFile(dirPath, strings.Join([]string{"hugetlb", hugetlb.Pagesize, "max"}, "."), strconv.FormatUint(hugetlb.Limit, 10)); err != nil { return err diff --git a/libcontainer/cgroups/fs2/io.go b/libcontainer/cgroups/fs2/io.go index 677c8d28..bbe3ac06 100644 --- a/libcontainer/cgroups/fs2/io.go +++ b/libcontainer/cgroups/fs2/io.go @@ -14,7 +14,19 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) +func isIoSet(cgroup *configs.Cgroup) bool { + return cgroup.Resources.BlkioWeight != 0 || + len(cgroup.Resources.BlkioThrottleReadBpsDevice) > 0 || + len(cgroup.Resources.BlkioThrottleWriteBpsDevice) > 0 || + len(cgroup.Resources.BlkioThrottleReadIOPSDevice) > 0 || + len(cgroup.Resources.BlkioThrottleWriteIOPSDevice) > 0 +} + func setIo(dirPath string, cgroup *configs.Cgroup) error { + if !isIoSet(cgroup) { + return nil + } + if cgroup.Resources.BlkioWeight != 0 { filename := "io.bfq.weight" if err := fscommon.WriteFile(dirPath, filename, diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go index 09a92d46..681bedd1 100644 --- a/libcontainer/cgroups/fs2/memory.go +++ b/libcontainer/cgroups/fs2/memory.go @@ -32,7 +32,15 @@ func numToStr(value int64) (ret string) { return ret } +func isMemorySet(cgroup *configs.Cgroup) bool { + return cgroup.Resources.MemoryReservation != 0 || + cgroup.Resources.Memory != 0 || cgroup.Resources.MemorySwap != 0 +} + func setMemory(dirPath string, cgroup *configs.Cgroup) error { + if !isMemorySet(cgroup) { + return nil + } swap, err := cgroups.ConvertMemorySwapToCgroupV2Value(cgroup.Resources.MemorySwap, cgroup.Resources.Memory) if err != nil { return err diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index 0cf860f9..b8153d28 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -14,7 +14,14 @@ import ( "golang.org/x/sys/unix" ) +func isPidsSet(cgroup *configs.Cgroup) bool { + return cgroup.Resources.PidsLimit != 0 +} + func setPids(dirPath string, cgroup *configs.Cgroup) error { + if !isPidsSet(cgroup) { + return nil + } if val := numToStr(cgroup.Resources.PidsLimit); val != "" { if err := fscommon.WriteFile(dirPath, "pids.max", val); err != nil { return err diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 18280371..130fefb6 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -151,7 +151,7 @@ func (m *unifiedManager) Apply(pid int) error { if err != nil { return err } - if err := fs2.CreateCgroupPath(m.path); err != nil { + if err := fs2.CreateCgroupPath(m.path, m.cgroups); err != nil { return err } return nil