Merge pull request #495 from cyphar/fix-memcg-set

cgroups: set memory cgroups in Set

libcontainer/cgroups/fs/apply_raw.go

@@ -193,12 +193,6 @@ func (m *Manager) GetStats() (*cgroups.Stats, error) {
 
 func (m *Manager) Set(container *configs.Config) error {
 	for _, sys := range subsystems {
-		// We can't set this here, because after being applied, memcg doesn't
-		// allow a non-empty cgroup from having its limits changed.
-		if sys.Name() == "memory" {
-			continue
-		}
-
 		// Generate fake cgroup data.
 		d, err := getCgroupData(container.Cgroups, -1)
 		if err != nil {

libcontainer/cgroups/fs/memory.go

@@ -32,7 +32,9 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) {
 				return err
 			}
 		}
-		if err := s.Set(path, d.config); err != nil {
+		// We have to set kernel memory here, as we can't change it once
+		// processes have been attached.
+		if err := s.SetKernelMemory(path, d.config); err != nil {
 			return err
 		}
 	}
@@ -49,7 +51,17 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) {
 	if err != nil && !cgroups.IsNotFound(err) {
 		return err
 	}
+	return nil
+}
 
+func (s *MemoryGroup) SetKernelMemory(path string, cgroup *configs.Cgroup) error {
+	// This has to be done separately because it has special constraints (it
+	// can't be done after there are processes attached to the cgroup).
+	if cgroup.Resources.KernelMemory > 0 {
+		if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemory, 10)); err != nil {
+			return err
+		}
+	}
 	return nil
 }
 
@@ -69,12 +81,6 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
 			return err
 		}
 	}
-	if cgroup.Resources.KernelMemory > 0 {
-		if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemory, 10)); err != nil {
-			return err
-		}
-	}
-
 	if cgroup.Resources.OomKillDisable {
 		if err := writeFile(path, "memory.oom_control", "1"); err != nil {
 			return err

libcontainer/cgroups/fs/memory_test.go

@@ -100,7 +100,7 @@ func TestMemorySetKernelMemory(t *testing.T) {
 
 	helper.CgroupData.config.Resources.KernelMemory = kernelMemoryAfter
 	memory := &MemoryGroup{}
-	if err := memory.Set(helper.CgroupPath, helper.CgroupData.config); err != nil {
+	if err := memory.SetKernelMemory(helper.CgroupPath, helper.CgroupData.config); err != nil {
 		t.Fatal(err)
 	}
 

libcontainer/cgroups/systemd/apply_systemd.go

@@ -460,12 +460,6 @@ func (m *Manager) GetStats() (*cgroups.Stats, error) {
 
 func (m *Manager) Set(container *configs.Config) error {
 	for _, sys := range subsystems {
-		// We can't set this here, because after being applied, memcg doesn't
-		// allow a non-empty cgroup from having its limits changed.
-		if sys.Name() == "memory" {
-			continue
-		}
-
 		// Get the subsystem path, but don't error out for not found cgroups.
 		path, err := getSubsystemPath(container.Cgroups, sys.Name())
 		if err != nil && !cgroups.IsNotFound(err) {
@@ -520,57 +514,16 @@ func setKernelMemory(c *configs.Cgroup) error {
 		return err
 	}
 
-	// Shamelessly copied from fs.(*MemoryGroup).Set(). This doesn't get called
+	// This doesn't get called by manager.Set, so we need to do it here.
-	// by manager.Set, so we need to do it here.
+	s := &fs.MemoryGroup{}
-	if c.Resources.KernelMemory > 0 {
+	return s.SetKernelMemory(path, c)
-		if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(c.Resources.KernelMemory, 10)); err != nil {
-			return err
-		}
-	}
-
-	return nil
 }
 
 func joinMemory(c *configs.Cgroup, pid int) error {
-	path, err := join(c, "memory", pid)
+	_, err := join(c, "memory", pid)
 	if err != nil && !cgroups.IsNotFound(err) {
 		return err
 	}
-
-	// 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 {
-		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
-		}
-	}
-	if c.Resources.OomKillDisable {
-		if err := writeFile(path, "memory.oom_control", "1"); err != nil {
-			return err
-		}
-	}
-	if c.Resources.MemorySwappiness >= 0 && c.Resources.MemorySwappiness <= 100 {
-		if err := writeFile(path, "memory.swappiness", strconv.FormatInt(c.Resources.MemorySwappiness, 10)); err != nil {
-			return err
-		}
-	} else if c.Resources.MemorySwappiness == -1 {
-		return nil
-	} else {
-		return fmt.Errorf("invalid value:%d. valid memory swappiness range is 0-100", c.Resources.MemorySwappiness)
-	}
-
 	return nil
 }