Merge pull request #592 from hqhq/hq_fix_update_memory

Fix problem when update memory and swap memory

libcontainer/cgroups/fs/memory.go

@@ -65,22 +65,59 @@ func (s *MemoryGroup) SetKernelMemory(path string, cgroup *configs.Cgroup) error
 	return nil
 }
 
-func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
-	if cgroup.Resources.Memory != 0 {
-		if err := writeFile(path, "memory.limit_in_bytes", strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
+func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
+	// When memory and swap memory are both set, we need to handle the cases
+	// for updating container.
+	if cgroup.Resources.Memory != 0 && cgroup.Resources.MemorySwap != 0 {
+		memoryUsage, err := getMemoryData(path, "")
+		if err != nil {
 			return err
 		}
+
+		// When update memory limit, we should adapt the write sequence
+		// for memory and swap memory, so it won't fail because the new
+		// value and the old value don't fit kernel's validation.
+		if memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) {
+			if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
+				return err
+			}
+			if err := writeFile(path, "memory.limit_in_bytes", strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
+				return err
+			}
+		} else {
+			if err := writeFile(path, "memory.limit_in_bytes", strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
+				return err
+			}
+			if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
+				return err
+			}
+		}
+	} else {
+		if cgroup.Resources.Memory != 0 {
+			if err := writeFile(path, "memory.limit_in_bytes", strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
+				return err
+			}
+		}
+		if cgroup.Resources.MemorySwap > 0 {
+			if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
+				return err
+			}
+		}
 	}
+
+	return nil
+}
+
+func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
+	if err := setMemoryAndSwap(path, cgroup); err != nil {
+		return err
+	}
+
 	if cgroup.Resources.MemoryReservation != 0 {
 		if err := writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemoryReservation, 10)); err != nil {
 			return err
 		}
 	}
-	if cgroup.Resources.MemorySwap > 0 {
-		if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
-			return err
-		}
-	}
 	if cgroup.Resources.KernelMemoryTCP != 0 {
 		if err := writeFile(path, "memory.kmem.tcp.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemoryTCP, 10)); err != nil {
 			return err

libcontainer/cgroups/fs/memory_test.go

@@ -86,6 +86,94 @@ func TestMemorySetMemoryswap(t *testing.T) {
 	}
 }
 
+func TestMemorySetMemoryLargerThanSwap(t *testing.T) {
+	helper := NewCgroupTestUtil("memory", t)
+	defer helper.cleanup()
+
+	const (
+		memoryBefore     = 314572800 // 300M
+		memoryswapBefore = 524288000 // 500M
+		memoryAfter      = 629145600 // 600M
+		memoryswapAfter  = 838860800 // 800M
+	)
+
+	helper.writeFileContents(map[string]string{
+		"memory.limit_in_bytes":       strconv.Itoa(memoryBefore),
+		"memory.memsw.limit_in_bytes": strconv.Itoa(memoryswapBefore),
+		// Set will call getMemoryData when memory and swap memory are
+		// both set, fake these fields so we don't get error.
+		"memory.usage_in_bytes":     "0",
+		"memory.max_usage_in_bytes": "0",
+		"memory.failcnt":            "0",
+	})
+
+	helper.CgroupData.config.Resources.Memory = memoryAfter
+	helper.CgroupData.config.Resources.MemorySwap = memoryswapAfter
+	memory := &MemoryGroup{}
+	if err := memory.Set(helper.CgroupPath, helper.CgroupData.config); err != nil {
+		t.Fatal(err)
+	}
+
+	value, err := getCgroupParamUint(helper.CgroupPath, "memory.limit_in_bytes")
+	if err != nil {
+		t.Fatalf("Failed to parse memory.limit_in_bytes - %s", err)
+	}
+	if value != memoryAfter {
+		t.Fatal("Got the wrong value, set memory.limit_in_bytes failed.")
+	}
+	value, err = getCgroupParamUint(helper.CgroupPath, "memory.memsw.limit_in_bytes")
+	if err != nil {
+		t.Fatalf("Failed to parse memory.memsw.limit_in_bytes - %s", err)
+	}
+	if value != memoryswapAfter {
+		t.Fatal("Got the wrong value, set memory.memsw.limit_in_bytes failed.")
+	}
+}
+
+func TestMemorySetSwapSmallerThanMemory(t *testing.T) {
+	helper := NewCgroupTestUtil("memory", t)
+	defer helper.cleanup()
+
+	const (
+		memoryBefore     = 629145600 // 600M
+		memoryswapBefore = 838860800 // 800M
+		memoryAfter      = 314572800 // 300M
+		memoryswapAfter  = 524288000 // 500M
+	)
+
+	helper.writeFileContents(map[string]string{
+		"memory.limit_in_bytes":       strconv.Itoa(memoryBefore),
+		"memory.memsw.limit_in_bytes": strconv.Itoa(memoryswapBefore),
+		// Set will call getMemoryData when memory and swap memory are
+		// both set, fake these fields so we don't get error.
+		"memory.usage_in_bytes":     "0",
+		"memory.max_usage_in_bytes": "0",
+		"memory.failcnt":            "0",
+	})
+
+	helper.CgroupData.config.Resources.Memory = memoryAfter
+	helper.CgroupData.config.Resources.MemorySwap = memoryswapAfter
+	memory := &MemoryGroup{}
+	if err := memory.Set(helper.CgroupPath, helper.CgroupData.config); err != nil {
+		t.Fatal(err)
+	}
+
+	value, err := getCgroupParamUint(helper.CgroupPath, "memory.limit_in_bytes")
+	if err != nil {
+		t.Fatalf("Failed to parse memory.limit_in_bytes - %s", err)
+	}
+	if value != memoryAfter {
+		t.Fatal("Got the wrong value, set memory.limit_in_bytes failed.")
+	}
+	value, err = getCgroupParamUint(helper.CgroupPath, "memory.memsw.limit_in_bytes")
+	if err != nil {
+		t.Fatalf("Failed to parse memory.memsw.limit_in_bytes - %s", err)
+	}
+	if value != memoryswapAfter {
+		t.Fatal("Got the wrong value, set memory.memsw.limit_in_bytes failed.")
+	}
+}
+
 func TestMemorySetKernelMemory(t *testing.T) {
 	helper := NewCgroupTestUtil("memory", t)
 	defer helper.cleanup()