From 18ebc51b3cc773ac1be840eb517c636377208c3f Mon Sep 17 00:00:00 2001 From: Mohammad Arab Date: Wed, 19 Oct 2016 12:15:55 +0200 Subject: [PATCH] Reset Swap when memory is set to unlimited (-1) Kernel validation fails if memory set to -1 which is unlimited but swap is not set so. Signed-off-by: Mohammad Arab --- libcontainer/cgroups/fs/memory.go | 33 ++++++++++++++------- libcontainer/cgroups/fs/memory_test.go | 41 -------------------------- tests/integration/update.bats | 29 ++++++++++++++++-- update.go | 16 ++++++---- 4 files changed, 60 insertions(+), 59 deletions(-) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 2a3ccf0085a..8d2868326a1 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -16,7 +16,11 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -const cgroupKernelMemoryLimit = "memory.kmem.limit_in_bytes" +const ( + cgroupKernelMemoryLimit = "memory.kmem.limit_in_bytes" + cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" + cgroupMemoryLimit = "memory.limit_in_bytes" +) type MemoryGroup struct { } @@ -96,9 +100,18 @@ func setKernelMemory(path string, kernelMemoryLimit int64) error { } func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error { + // If the memory update is set to -1 we should also set swap to -1 + // -1 means unlimited memory + if cgroup.Resources.Memory == -1 { + // Only set swap if it's enbled in kernel + if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) { + cgroup.Resources.MemorySwap = -1 + } + } + // 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 { + if cgroup.Resources.Memory != 0 && cgroup.Resources.MemorySwap != 0 { memoryUsage, err := getMemoryData(path, "") if err != nil { return err @@ -107,29 +120,29 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error { // 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 { + if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) { + if err := writeFile(path, cgroupMemorySwapLimit, 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 { + if err := writeFile(path, cgroupMemoryLimit, 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 { + if err := writeFile(path, cgroupMemoryLimit, 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 { + if err := writeFile(path, cgroupMemorySwapLimit, 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 { + if err := writeFile(path, cgroupMemoryLimit, 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 { + if cgroup.Resources.MemorySwap != 0 { + if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil { return err } } diff --git a/libcontainer/cgroups/fs/memory_test.go b/libcontainer/cgroups/fs/memory_test.go index 5e86e1b4ed1..c13c4340caa 100644 --- a/libcontainer/cgroups/fs/memory_test.go +++ b/libcontainer/cgroups/fs/memory_test.go @@ -86,47 +86,6 @@ func TestMemorySetMemoryswap(t *testing.T) { } } -func TestMemorySetNegativeMemoryswap(t *testing.T) { - helper := NewCgroupTestUtil("memory", t) - defer helper.cleanup() - - const ( - memoryBefore = 314572800 // 300M - memoryAfter = 524288000 // 500M - memoryswapBefore = 629145600 // 600M - memoryswapAfter = 629145600 // 600M - ) - - helper.writeFileContents(map[string]string{ - "memory.limit_in_bytes": strconv.Itoa(memoryBefore), - "memory.memsw.limit_in_bytes": strconv.Itoa(memoryswapBefore), - }) - - helper.CgroupData.config.Resources.Memory = memoryAfter - // Negative value means not change - helper.CgroupData.config.Resources.MemorySwap = -1 - 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 TestMemorySetMemoryLargerThanSwap(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 9e4c1102e7f..9aaf1b9c35f 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -45,7 +45,7 @@ function check_cgroup_value() { expected=$3 current=$(cat $cgroup/$source) - [ "$current" -eq "$expected" ] + [ "$current" == "$expected" ] } # TODO: test rt cgroup updating @@ -62,6 +62,8 @@ function check_cgroup_value() { eval CGROUP_${g}="${base_path}/runc-update-integration-test" done + CGROUP_SYSTEM_MEMORY=$(grep "cgroup" /proc/self/mountinfo | gawk 'toupper($NF) ~ /\<'MEMORY'\>/ { print $5; exit }') + # check that initial values were properly set check_cgroup_value $CGROUP_BLKIO "blkio.weight" 1000 check_cgroup_value $CGROUP_CPU "cpu.cfs_period_us" 1000000 @@ -110,17 +112,38 @@ function check_cgroup_value() { [ "$status" -eq 0 ] check_cgroup_value $CGROUP_MEMORY "memory.limit_in_bytes" 52428800 - # update memory soft limit runc update test_update --memory-reservation 33554432 [ "$status" -eq 0 ] check_cgroup_value $CGROUP_MEMORY "memory.soft_limit_in_bytes" 33554432 - # update memory swap (if available) + # Run swap memory tests if swap is avaialble if [ -f "$CGROUP_MEMORY/memory.memsw.limit_in_bytes" ]; then + # try to remove memory swap limit + runc update test_update --memory-swap -1 + [ "$status" -eq 0 ] + # Get System memory swap limit + SYSTEM_MEMORY_SW=$(cat "${CGROUP_SYSTEM_MEMORY}/memory.memsw.limit_in_bytes") + check_cgroup_value $CGROUP_MEMORY "memory.memsw.limit_in_bytes" ${SYSTEM_MEMORY_SW} + + # update memory swap runc update test_update --memory-swap 96468992 [ "$status" -eq 0 ] check_cgroup_value $CGROUP_MEMORY "memory.memsw.limit_in_bytes" 96468992 + fi; + + # try to remove memory limit + runc update test_update --memory -1 + [ "$status" -eq 0 ] + + # Get System memory limit + SYSTEM_MEMORY=$(cat "${CGROUP_SYSTEM_MEMORY}/memory.limit_in_bytes") + # check memory limited is gone + check_cgroup_value $CGROUP_MEMORY "memory.limit_in_bytes" ${SYSTEM_MEMORY} + + # check swap memory limited is gone + if [ -f "$CGROUP_MEMORY/memory.memsw.limit_in_bytes" ]; then + check_cgroup_value $CGROUP_MEMORY "memory.memsw.limit_in_bytes" ${SYSTEM_MEMORY} fi # update kernel memory limit diff --git a/update.go b/update.go index 1c1264409d7..5193c921e1f 100644 --- a/update.go +++ b/update.go @@ -193,16 +193,22 @@ other options are ignored. opt string dest *uint64 }{ + {"memory", r.Memory.Limit}, + {"memory-swap", r.Memory.Swap}, {"kernel-memory", r.Memory.Kernel}, {"kernel-memory-tcp", r.Memory.KernelTCP}, - {"memory", r.Memory.Limit}, {"memory-reservation", r.Memory.Reservation}, - {"memory-swap", r.Memory.Swap}, } { if val := context.String(pair.opt); val != "" { - v, err := units.RAMInBytes(val) - if err != nil { - return fmt.Errorf("invalid value for %s: %s", pair.opt, err) + var v int64 + + if val != "-1" { + v, err = units.RAMInBytes(val) + if err != nil { + return fmt.Errorf("invalid value for %s: %s", pair.opt, err) + } + } else { + v = -1 } *pair.dest = uint64(v) }