From d57f5bb28623f71202bd9acdcd0eb2f471bbee6e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 20 May 2020 11:49:51 -0700 Subject: [PATCH 1/2] cgroupv1: don't ignore MemorySwap if Memory==-1 Commit 18ebc51b3cc3 "Reset Swap when memory is set to unlimited (-1)" added handling of the case when a user updates the container limits to set memory to unlimited (-1) but do not set any other limits. Apparently, in this case, if swap limit was previously set, kernel fails to set memory.limit_in_bytes to -1 if memory.memsw.limit_in_bytes is not set to -1. What the above commit fails to handle correctly is the request when Memory is set to -1 and MemorySwap is set to some specific limit N (where N > 0). In this case, the value of N is silently discarded and MemorySwap is set to -1 instead. This is wrong thing to do, as the limit set, even if incorrectly, should not be ignored. Fix this by only assigning MemorySwap == -1 in case it was not explicitly set. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/memory.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 39809ef0..95087690 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -74,9 +74,9 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) { } func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error { - // If the memory update is set to -1 we should also - // set swap to -1, it means unlimited memory. - if cgroup.Resources.Memory == -1 { + // If the memory update is set to -1 and the swap is not explicitly + // set, we should also set swap to -1, it means unlimited memory. + if cgroup.Resources.Memory == -1 && cgroup.Resources.MemorySwap == 0 { // Only set swap if it's enabled in kernel if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) { cgroup.Resources.MemorySwap = -1 From 3c6e8ac4d21452c658a55d1f6ebfc8e1290956f7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 20 May 2020 12:12:44 -0700 Subject: [PATCH 2/2] cgroupv2: set mem+swap to max if mem set to max ... and mem+swap is not explicitly set otherwise. This ensures compatibility with cgroupv1 controller which interprets things this way. With this fixed, we can finally enable swap tests for cgroupv2. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/v2.go | 12 ++++++------ libcontainer/cgroups/utils.go | 6 ++++++ tests/integration/update.bats | 20 ++++++++++++++------ 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 2fcf312a..e78de667 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -57,12 +57,12 @@ func genV2ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) properties = append(properties, newProp("MemoryLow", uint64(c.Resources.MemoryReservation))) } - // swap is set - if c.Resources.MemorySwap != 0 { - swap, err := cgroups.ConvertMemorySwapToCgroupV2Value(c.Resources.MemorySwap, c.Resources.Memory) - if err != nil { - return nil, err - } + + swap, err := cgroups.ConvertMemorySwapToCgroupV2Value(c.Resources.MemorySwap, c.Resources.Memory) + if err != nil { + return nil, err + } + if swap != 0 { properties = append(properties, newProp("MemorySwapMax", uint64(swap))) } diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 3b6ef4af..01321692 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -612,6 +612,12 @@ func ConvertCPUQuotaCPUPeriodToCgroupV2Value(quota int64, period uint64) string // for use by cgroup v2 drivers. A conversion is needed since Resources.MemorySwap // is defined as memory+swap combined, while in cgroup v2 swap is a separate value. func ConvertMemorySwapToCgroupV2Value(memorySwap, memory int64) (int64, error) { + // for compatibility with cgroup1 controller, set swap to unlimited in + // case the memory is set to unlimited, and swap is not explicitly set, + // treating the request as "set both memory and swap to unlimited". + if memory == -1 && memorySwap == 0 { + return -1, nil + } if memorySwap == -1 || memorySwap == 0 { // -1 is "max", 0 is "unset", so treat as is return memorySwap, nil diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 76d931f5..72ee6109 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -80,8 +80,7 @@ EOF SD_MEM_SWAP="MemorySwapMax" SYSTEM_MEM="max" SYSTEM_MEM_SWAP="max" - # checking swap is currently disabled for v2 - #CGROUP_MEMORY=$CGROUP_PATH + CGROUP_MEMORY=$CGROUP_PATH ;; esac SD_UNLIMITED="infinity" @@ -135,10 +134,19 @@ EOF check_systemd_value "$SD_MEM_SWAP" $SD_UNLIMITED # update memory swap - runc update test_update --memory-swap 96468992 - [ "$status" -eq 0 ] - check_cgroup_value "$MEM_SWAP" 96468992 - check_systemd_value "$SD_MEM_SWAP" 96468992 + if [ "$CGROUP_UNIFIED" = "yes" ]; then + # for cgroupv2, memory and swap can only be set together + runc update test_update --memory 52428800 --memory-swap 96468992 + [ "$status" -eq 0 ] + # for cgroupv2, swap is a separate limit (it does not include mem) + check_cgroup_value "$MEM_SWAP" $((96468992-52428800)) + check_systemd_value "$SD_MEM_SWAP" $((96468992-52428800)) + else + runc update test_update --memory-swap 96468992 + [ "$status" -eq 0 ] + check_cgroup_value "$MEM_SWAP" 96468992 + check_systemd_value "$SD_MEM_SWAP" 96468992 + fi fi # try to remove memory limit