fix two swap limit regressions in cgroup v2

[lifubang]

(1) never write empty string to memory.swap.max
Because the empty string means set swap to 0.

fixed by #2410

For example:
with memory limit:

"memory": {
        "limit": 33554432,
        "swap": 33558528
 }

The value of memory.swap.max is 4096.
After we run runc update --memory 33554432 test, the value of memory.swap.max becomes 0.

(2) If the memory update is set to -1 we should also set swap to -1, it means unlimited memory.

Signed-off-by: lifubang lifubang@acmcoder.com

[kolyshkin]

So, this basically means that if a user sets memory: -1, swap: N, the value of N for mem+swap is silently discarded, and the swap is instead set to unlimited. I don't think it's the right thing to do.

I also disagree that "unlimited memory" means "unlimited swap" as well. A configuration with no limit for RAM but some limit for swap is maybe unusual but not incorrect. Alas, we don't have a way to set a separate value for swap, when memory is set to -1, unless we set memory and swap to some values and then set memory to unlimited and don't touch swap.

I think the existing behavior (before this patch) is correct.

[lifubang]

I copied it from v1:

runc/libcontainer/cgroups/fs/memory.go

Lines 76 to 84 in 6bb6d0b

	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 {
	// Only set swap if it's enabled in kernel
	if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) {
	cgroup.Resources.MemorySwap = -1
	}
	}

I think v2 should keep compatible with v1.

So, this basically means that if a user sets memory: -1, swap: N, the value of N for mem+swap is silently discarded, and the swap is instead set to unlimited. I don't think it's the right thing to do.

Before this patch, it will print an error unable to set swap limit without memory limit.

[cyphar]

The issue is that the behaviour of the two cgroup versions is different -- but the meaning of MemorySwap in the runtime-spec is actually the original cgroupv1 meaning of memory.memsw.limit_in_bytes (which is the total memory + swap, as opposed to memory.swap.max in cgroupv2 which is just swap usage). If we'd done runtime-spec a little differently this wouldn't be such a headache. 😉

However, the problem is that requesting {Memory: -1, MemorySwap: N} is an illogical request -- by the definition of the runtime-spec you're asking for unlimited memory but limited memory+swap. In cgroupv1 there wasn't a way to just limit swap usage, so for some reason the runc implementation ignores that this request is invalid and sets both options to be unlimited. I personally think giving an error is much more reasonable (and that we should see if changing cgroupv1 to give an error will cause issues with Docker or podman).

I agree with @lifubang that we should have the same behaviour (as much as we can) when we are converting cgroupv1 options to cgroupv2 settings, because not doing that will cause a lot more headaches in the future.

[kolyshkin]

by the definition of the runtime-spec you're asking for unlimited memory but limited memory+swap.

So, there are actually three ways to handle such a request for a cgroupv2 controller:

1. Return an error (current behavior).
2. Set memory and swap both to unlimited (proposed behavior).
3. Set memory and swap both to N (which just occurred to me).

I think either 1 or 3 is more reasonable than option 2, despite cgroupv1 controller doing 2.

[lifubang]

I agree with @lifubang that we should have the same behaviour (as much as we can)

Yes, thanks.
I think at least the exit code of a command for cgroupv2 should be same with cgroupv1, but the value in mem.swap.max can be different.
So, for @kolyshkin 's last comment, I think we should not return an error for this case.
I have another thought, for the case memory: -1, swap: N(N!=-1), we only set mem.max to max, but don't touch mem.swap.max.

[kolyshkin]

I traced the cgroupv1 behavior to PR #1127 which have a pretty long discussion about this case.

It's funny but I was trying to fix the exact issue we're discussing here in #2285 but got sidetracked by the fact that cgroupv2 handles swap as a separate limit.

Anyway, I read it all and I think this is what needs to be done, PTAL #2426

[kolyshkin]

(2) If the memory update is set to -1 we should also set swap to -1, it means unlimited memory.

I don't think so (see above)

[lifubang]

How about split this PR into 2 parts? Because #2395 needs (1).

[kolyshkin]

How about split this PR into 2 parts? Because #2395 needs (1).

Sure, (1) can be merged right away

[kolyshkin]

I propose to close this one in favor of #2426