From f36ed4b174bb7e8951db6b61e2202a45a7251e30 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 20 Dec 2015 22:30:35 +1100 Subject: [PATCH] libcontainer: cgroups: don't Set in Apply Apply and Set are two separate operations, and it doesn't make sense to group the two together (especially considering that the bootstrap process is added to the cgroup as well). The only exception to this is the memory cgroup, which requires the configuration to be set before processes can join. One of the weird cases to deal with is systemd. Systemd sets some of the cgroup configuration options, but not all of them. Because memory is a special case, we need to explicitly set memory in the systemd Apply(). Otherwise, the rest can be safely re-applied in .Set() as usual. Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs/apply_raw.go | 20 ++- libcontainer/cgroups/fs/blkio.go | 7 +- libcontainer/cgroups/fs/cpu.go | 7 +- libcontainer/cgroups/fs/cpuset.go | 5 - libcontainer/cgroups/fs/devices.go | 7 +- libcontainer/cgroups/fs/freezer.go | 7 +- libcontainer/cgroups/fs/hugetlb.go | 7 +- libcontainer/cgroups/fs/memory.go | 1 - libcontainer/cgroups/fs/net_cls.go | 7 +- libcontainer/cgroups/fs/net_prio.go | 7 +- libcontainer/cgroups/fs/pids.go | 7 +- libcontainer/cgroups/systemd/apply_systemd.go | 164 +++++------------- libcontainer/process_linux.go | 3 + 13 files changed, 66 insertions(+), 183 deletions(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index 60646cd40..c67c55acd 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -106,8 +106,6 @@ func (m *Manager) Apply(pid int) (err error) { return nil } - var c = m.Cgroups - d, err := getCgroupData(m.Cgroups, pid) if err != nil { return err @@ -136,13 +134,6 @@ func (m *Manager) Apply(pid int) (err error) { paths[sys.Name()] = p } m.Paths = paths - - if paths["cpu"] != "" { - if err := CheckCpushares(paths["cpu"], c.Resources.CpuShares); err != nil { - return err - } - } - return nil } @@ -181,6 +172,11 @@ func (m *Manager) GetStats() (*cgroups.Stats, error) { func (m *Manager) Set(container *configs.Config) error { for name, path := range m.Paths { + // We can't set this here, because after being applied, memcg doesn't + // allow a non-empty cgroup from having its limits changed. + if name == "memory" { + continue + } sys, err := subsystems.Get(name) if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) { continue @@ -189,6 +185,12 @@ func (m *Manager) Set(container *configs.Config) error { return err } } + + if m.Paths["cpu"] != "" { + if err := CheckCpushares(m.Paths["cpu"], container.Cgroups.Resources.CpuShares); err != nil { + return err + } + } return nil } diff --git a/libcontainer/cgroups/fs/blkio.go b/libcontainer/cgroups/fs/blkio.go index 518cb63f6..a142cb991 100644 --- a/libcontainer/cgroups/fs/blkio.go +++ b/libcontainer/cgroups/fs/blkio.go @@ -22,15 +22,10 @@ func (s *BlkioGroup) Name() string { } func (s *BlkioGroup) Apply(d *cgroupData) error { - dir, err := d.join("blkio") + _, err := d.join("blkio") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go index ad5f427ec..a4ef28a60 100644 --- a/libcontainer/cgroups/fs/cpu.go +++ b/libcontainer/cgroups/fs/cpu.go @@ -22,15 +22,10 @@ func (s *CpuGroup) Name() string { func (s *CpuGroup) Apply(d *cgroupData) error { // We always want to join the cpu group, to allow fair cpu scheduling // on a container basis - dir, err := d.join("cpu") + _, err := d.join("cpu") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index b7632108f..a9e3ab72a 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -63,11 +63,6 @@ func (s *CpusetGroup) ApplyDir(dir string, cgroup *configs.Cgroup, pid int) erro if err := s.ensureParent(dir, root); err != nil { return err } - // the default values inherit from parent cgroup are already set in - // s.ensureParent, cover these if we have our own - if err := s.Set(dir, cgroup); err != nil { - return err - } // because we are not using d.join we need to place the pid into the procs file // unlike the other subsystems if err := writeFile(dir, "cgroup.procs", strconv.Itoa(pid)); err != nil { diff --git a/libcontainer/cgroups/fs/devices.go b/libcontainer/cgroups/fs/devices.go index a9883eb4b..a41ce801f 100644 --- a/libcontainer/cgroups/fs/devices.go +++ b/libcontainer/cgroups/fs/devices.go @@ -15,17 +15,12 @@ func (s *DevicesGroup) Name() string { } func (s *DevicesGroup) Apply(d *cgroupData) error { - dir, err := d.join("devices") + _, err := d.join("devices") if err != nil { // We will return error even it's `not found` error, devices // cgroup is hard requirement for container's security. return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/freezer.go b/libcontainer/cgroups/fs/freezer.go index 6aaad4e00..e70dfe3b9 100644 --- a/libcontainer/cgroups/fs/freezer.go +++ b/libcontainer/cgroups/fs/freezer.go @@ -19,15 +19,10 @@ func (s *FreezerGroup) Name() string { } func (s *FreezerGroup) Apply(d *cgroupData) error { - dir, err := d.join("freezer") + _, err := d.join("freezer") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/hugetlb.go b/libcontainer/cgroups/fs/hugetlb.go index ca106da44..2f9727719 100644 --- a/libcontainer/cgroups/fs/hugetlb.go +++ b/libcontainer/cgroups/fs/hugetlb.go @@ -19,15 +19,10 @@ func (s *HugetlbGroup) Name() string { } func (s *HugetlbGroup) Apply(d *cgroupData) error { - dir, err := d.join("hugetlb") + _, err := d.join("hugetlb") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 6b9687ca2..0329692ae 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -32,7 +32,6 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) { return err } } - if err := s.Set(path, d.config); err != nil { return err } diff --git a/libcontainer/cgroups/fs/net_cls.go b/libcontainer/cgroups/fs/net_cls.go index 638237312..8a4054ba8 100644 --- a/libcontainer/cgroups/fs/net_cls.go +++ b/libcontainer/cgroups/fs/net_cls.go @@ -15,15 +15,10 @@ func (s *NetClsGroup) Name() string { } func (s *NetClsGroup) Apply(d *cgroupData) error { - dir, err := d.join("net_cls") + _, err := d.join("net_cls") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/net_prio.go b/libcontainer/cgroups/fs/net_prio.go index 0dabaae72..d0ab2af89 100644 --- a/libcontainer/cgroups/fs/net_prio.go +++ b/libcontainer/cgroups/fs/net_prio.go @@ -15,15 +15,10 @@ func (s *NetPrioGroup) Name() string { } func (s *NetPrioGroup) Apply(d *cgroupData) error { - dir, err := d.join("net_prio") + _, err := d.join("net_prio") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/fs/pids.go b/libcontainer/cgroups/fs/pids.go index 23893db5d..96cbb896c 100644 --- a/libcontainer/cgroups/fs/pids.go +++ b/libcontainer/cgroups/fs/pids.go @@ -18,15 +18,10 @@ func (s *PidsGroup) Name() string { } func (s *PidsGroup) Apply(d *cgroupData) error { - dir, err := d.join("pids") + _, err := d.join("pids") if err != nil && !cgroups.IsNotFound(err) { return err } - - if err := s.Set(dir, d.config); err != nil { - return err - } - return nil } diff --git a/libcontainer/cgroups/systemd/apply_systemd.go b/libcontainer/cgroups/systemd/apply_systemd.go index b8f92dbc7..952820e62 100644 --- a/libcontainer/cgroups/systemd/apply_systemd.go +++ b/libcontainer/cgroups/systemd/apply_systemd.go @@ -282,13 +282,6 @@ func (m *Manager) Apply(pid int) error { paths[s.Name()] = subsystemPath } m.Paths = paths - - if paths["cpu"] != "" { - if err := fs.CheckCpushares(paths["cpu"], c.Resources.CpuShares); err != nil { - return err - } - } - return nil } @@ -335,80 +328,43 @@ func join(c *configs.Cgroup, subsystem string, pid int) (string, error) { } func joinCpu(c *configs.Cgroup, pid int) error { - path, err := getSubsystemPath(c, "cpu") + _, err := join(c, "cpu", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - if c.Resources.CpuQuota != 0 { - if err = writeFile(path, "cpu.cfs_quota_us", strconv.FormatInt(c.Resources.CpuQuota, 10)); err != nil { - return err - } - } - if c.Resources.CpuPeriod != 0 { - if err = writeFile(path, "cpu.cfs_period_us", strconv.FormatInt(c.Resources.CpuPeriod, 10)); err != nil { - return err - } - } - if c.Resources.CpuRtPeriod != 0 { - if err = writeFile(path, "cpu.rt_period_us", strconv.FormatInt(c.Resources.CpuRtPeriod, 10)); err != nil { - return err - } - } - if c.Resources.CpuRtRuntime != 0 { - if err = writeFile(path, "cpu.rt_runtime_us", strconv.FormatInt(c.Resources.CpuRtRuntime, 10)); err != nil { - return err - } - } - return nil } func joinFreezer(c *configs.Cgroup, pid int) error { - path, err := join(c, "freezer", pid) + _, err := join(c, "freezer", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - freezer, err := subsystems.Get("freezer") - if err != nil { - return err - } - return freezer.Set(path, c) + return nil } func joinNetPrio(c *configs.Cgroup, pid int) error { - path, err := join(c, "net_prio", pid) + _, err := join(c, "net_prio", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - netPrio, err := subsystems.Get("net_prio") - if err != nil { - return err - } - return netPrio.Set(path, c) + return nil } func joinNetCls(c *configs.Cgroup, pid int) error { - path, err := join(c, "net_cls", pid) + _, err := join(c, "net_cls", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - netcls, err := subsystems.Get("net_cls") - if err != nil { - return err - } - return netcls.Set(path, c) + return nil } func joinPids(c *configs.Cgroup, pid int) error { - path, err := join(c, "pids", pid) + _, err := join(c, "pids", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - pids, err := subsystems.Get("pids") - if err != nil { - return err - } - return pids.Set(path, c) + return nil } func getSubsystemPath(c *configs.Cgroup, subsystem string) (string, error) { @@ -476,6 +432,11 @@ func (m *Manager) GetStats() (*cgroups.Stats, error) { func (m *Manager) Set(container *configs.Config) error { for name, path := range m.Paths { + // We can't set this here, because after being applied, memcg doesn't + // allow a non-empty cgroup from having its limits changed. + if name == "memory" { + continue + } sys, err := subsystems.Get(name) if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) { continue @@ -485,6 +446,11 @@ func (m *Manager) Set(container *configs.Config) error { } } + if m.Paths["cpu"] != "" { + if err := fs.CheckCpushares(m.Paths["cpu"], container.Cgroups.Resources.CpuShares); err != nil { + return err + } + } return nil } @@ -504,17 +470,13 @@ func getUnitName(c *configs.Cgroup) string { // because systemd will re-write the device settings if it needs to re-apply the cgroup context. // This happens at least for v208 when any sibling unit is started. func joinDevices(c *configs.Cgroup, pid int) error { - path, err := join(c, "devices", pid) + _, err := join(c, "devices", pid) // Even if it's `not found` error, we'll return err because devices cgroup // is hard requirement for container security. if err != nil { return err } - devices, err := subsystems.Get("devices") - if err != nil { - return err - } - return devices.Set(path, c) + return nil } func setKernelMemory(c *configs.Cgroup) error { @@ -527,9 +489,10 @@ func setKernelMemory(c *configs.Cgroup) error { return err } + // Shamelessly copied from fs.(*MemoryGroup).Set(). This doesn't get called + // by manager.Set, so we need to do it here. if c.Resources.KernelMemory > 0 { - err = writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(c.Resources.KernelMemory, 10)) - if err != nil { + if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(c.Resources.KernelMemory, 10)); err != nil { return err } } @@ -538,21 +501,27 @@ func setKernelMemory(c *configs.Cgroup) error { } func joinMemory(c *configs.Cgroup, pid int) error { - path, err := getSubsystemPath(c, "memory") + path, err := join(c, "memory", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - // -1 disables memoryswap - if c.Resources.MemorySwap > 0 { - err = writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(c.Resources.MemorySwap, 10)) - if err != nil { + // Shamelessly copied from fs.(*MemoryGroup).Set(). This doesn't include the + // kernel memory limit (which is done in setKernelMemory). All of these are + // not set by manager.Set, we have do do it here. + + if c.Resources.Memory != 0 { + if err := writeFile(path, "memory.limit_in_bytes", strconv.FormatInt(c.Resources.Memory, 10)); err != nil { return err } } - if c.Resources.MemoryReservation > 0 { - err = writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatInt(c.Resources.MemoryReservation, 10)) - if err != nil { + if c.Resources.MemoryReservation != 0 { + if err := writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatInt(c.Resources.MemoryReservation, 10)); err != nil { + return err + } + } + if c.Resources.MemorySwap > 0 { + if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(c.Resources.MemorySwap, 10)); err != nil { return err } } @@ -561,10 +530,8 @@ func joinMemory(c *configs.Cgroup, pid int) error { return err } } - if c.Resources.MemorySwappiness >= 0 && c.Resources.MemorySwappiness <= 100 { - err = writeFile(path, "memory.swappiness", strconv.FormatInt(c.Resources.MemorySwappiness, 10)) - if err != nil { + if err := writeFile(path, "memory.swappiness", strconv.FormatInt(c.Resources.MemorySwappiness, 10)); err != nil { return err } } else if c.Resources.MemorySwappiness == -1 { @@ -594,68 +561,25 @@ func joinCpuset(c *configs.Cgroup, pid int) error { // expects device path instead of major minor numbers, which is also confusing // for users. So we use fs work around for now. func joinBlkio(c *configs.Cgroup, pid int) error { - path, err := getSubsystemPath(c, "blkio") + _, err := join(c, "blkio", pid) if err != nil { return err } - // systemd doesn't directly support this in the dbus properties - if c.Resources.BlkioLeafWeight != 0 { - if err := writeFile(path, "blkio.leaf_weight", strconv.FormatUint(uint64(c.Resources.BlkioLeafWeight), 10)); err != nil { - return err - } - } - for _, wd := range c.Resources.BlkioWeightDevice { - if err := writeFile(path, "blkio.weight_device", wd.WeightString()); err != nil { - return err - } - if err := writeFile(path, "blkio.leaf_weight_device", wd.LeafWeightString()); err != nil { - return err - } - } - for _, td := range c.Resources.BlkioThrottleReadBpsDevice { - if err := writeFile(path, "blkio.throttle.read_bps_device", td.String()); err != nil { - return err - } - } - for _, td := range c.Resources.BlkioThrottleWriteBpsDevice { - if err := writeFile(path, "blkio.throttle.write_bps_device", td.String()); err != nil { - return err - } - } - for _, td := range c.Resources.BlkioThrottleReadIOPSDevice { - if err := writeFile(path, "blkio.throttle.read_iops_device", td.String()); err != nil { - return err - } - } - for _, td := range c.Resources.BlkioThrottleWriteIOPSDevice { - if err := writeFile(path, "blkio.throttle.write_iops_device", td.String()); err != nil { - return err - } - } - return nil } func joinHugetlb(c *configs.Cgroup, pid int) error { - path, err := join(c, "hugetlb", pid) + _, err := join(c, "hugetlb", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - hugetlb, err := subsystems.Get("hugetlb") - if err != nil { - return err - } - return hugetlb.Set(path, c) + return nil } func joinPerfEvent(c *configs.Cgroup, pid int) error { - path, err := join(c, "perf_event", pid) + _, err := join(c, "perf_event", pid) if err != nil && !cgroups.IsNotFound(err) { return err } - perfEvent, err := subsystems.Get("perf_event") - if err != nil { - return err - } - return perfEvent.Set(path, c) + return nil } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 114c71b3b..5b32eb108 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -204,6 +204,9 @@ func (p *initProcess) start() (err error) { if err := p.manager.Apply(p.pid()); err != nil { return newSystemError(err) } + if err := p.manager.Set(p.config.Config); err != nil { + return newSystemError(err) + } defer func() { if err != nil { // TODO: should not be the responsibility to call here