Fixes set memory to unlimited by boynux · Pull Request #1127 · opencontainers/runc

boynux · 2016-10-19T10:29:26Z

Should set Swap to '-1' (unlimited) if memory is set to unlimited. Otherwise kernel validation fails.
these changes required to fix this issue in Docker project: moby/moby#22578

coolljt0725 · 2016-10-20T11:58:04Z

libcontainer/cgroups/fs/memory.go

+	// Memeory set to '-1' which is unlimited. In this situation we need to update swap to -1
+	// to avoid kernel validation failures.
+	if cgroup.Resources.Memory == -1 {
+		if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(-1, 10)); err != nil {


Should we make sure memory.memsw.limit_in_bytes exists first or ignore the ErrNotExist error? I think if the system does not enable memory swap, this file does not exist

Good point, I just followed the convention (see the exiting code), I can add checks if you think it's required. However the getMemoryData is suspicious to fail too.

This is also not libcontainer's concern, a value is assigned, libcontainer set it, simple as that. Otherwise we'll end up with another huge validation done by Docker and other high level tools.

hqhq · 2016-10-20T12:00:13Z

libcontainer/cgroups/fs/memory.go

+	// Memeory set to '-1' which is unlimited. In this situation we need to update swap to -1
+	// to avoid kernel validation failures.
+	if cgroup.Resources.Memory == -1 {
+		if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(-1, 10)); err != nil {


libcontainer as a low level library usually don't do this kind of thing, it only does what it's told to do.

So if docker want to set memory as -1, it should pass memorySwap as -1 as well, libcontainer only adapt the writing sequence to make the logic right.

I agree that aggressive validation may add unnecessary overhead the the low level libraries like RunC.

It's not "unnecessary overhead" that @hqhq is referring to (the overhead of doing a single write / check is not important here). He's talking about libcontainer doing things it wasn't told to do explicitly. libcontainer is meant to be stupid (though clever enough to know when to be clever about it), specifically this means that we in general don't take initiative past what the user has told us to do -- it's not libcontainer's job to change the settings that a user asked for.

If you wanted to idiot-proof this then you could put it inside libcontainer/specconv so that runC makes this easier for users. But then again, I'm not even sure if we should be doing that to be honest.

boynux · 2016-10-20T15:38:28Z

@hqhq @cyphar @coolljt0725 Shall I update the PR or these changes are sufficient?

hqhq · 2016-10-21T00:59:52Z

@boynux You should update your PR.

I think a change like this would be sufficient,

diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go
index 2a3ccf0..d3ba108 100644
--- a/libcontainer/cgroups/fs/memory.go
+++ b/libcontainer/cgroups/fs/memory.go
@@ -98,7 +98,7 @@ func setKernelMemory(path string, kernelMemoryLimit int64) error {
 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 {
+       if cgroup.Resources.Memory != 0 && cgroup.Resources.MemorySwap != 0 {
                memoryUsage, err := getMemoryData(path, "")
                if err != nil {
                        return err
@@ -108,6 +108,7 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
                // 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) {
+                       // Here includes the case that `Limit == -1 && MemorySwap == -1`
                        if err := writeFile(path, "memory.memsw.limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
                                return err
                        }

boynux · 2016-10-21T13:15:27Z

@hqhq That's actually the first thing I've tried, RunC assumption is that SwapMemory == -1 means skip Swap update! And the proposed changes will break the existing tests. (Unless we want to change the behaviour with this PR) Since it's a known implementation right now I'm not sure about the impact on other depending projects.

The other thing is should I actually check for SwapMemory == -1 at all. Since if user requests for unlimited memory we can implicitly set SwapMemory to '-1' as it will be rejected by Kernel otherwise.

hqhq · 2016-10-22T01:23:16Z

@boynux There's no such assumption in RunC AFAIK and there shouldn't be. If only test case assumes that, you can change it. That might break Docker and that's something you should fix in moby/moby#22578.

Check for SwapMemory == -1 or not doesn't matter in this case, I'm ok with either. Usually RunC shouldn't do much validations if kernel can do the same thing, unless error message from kernel is vague and we want it to be clear. If SwapMemory is negative and it's not -1, I don't think there are much valuable information we can give than Invalid argument which is given by kernel.

boynux · 2016-10-23T08:06:06Z

@hqhq Now it should work as expected!

If memory and swap both set to '-1' will be set both to unlimited.
If wrong configuration provided (for example unlimited memory and limited swap) will fail with kernel error.
'0' means no update (was '-1' for swap before)

For Docker there should not be any issue, but I'm not sure about other clients that may rely on previous behavior!

LK4D4 · 2016-11-10T16:33:51Z

@hqhq is this okay for you?

hqhq · 2016-11-11T01:25:32Z

libcontainer/cgroups/fs/memory_test.go

+	const (
+		memoryBefore     = 314572800 // 300M
+		memoryswapBefore = 629145600 // 600M
+		memoryAfter      = 0         // Unlimited


Why 0 presents unlimited? Doesn't sound reasonable. Note that unit test didn't use real cgroup fs for test, so you better add an integration test to cover this case.

@hqhq Ok, I'll try to add integration test to cover this use case.

@hqhq Ok I managed to write the test there is only one issue and that is how to get host memory. I checked bats and could not find a clean way (except global vars) to get the hosts total memory. If global values are find I can push the changes, otherwise please give me a direction.

You can check cgroup values of your container like we did in: https://github.com/docker/runc/blob/master/tests/integration/cgroups.bats#L55

Thanks for your reply.
Correct, my problem was the host machine total amount of RAM :)
I found a clean solution. Will push my changes when I did some clean ups.

@hqhq I'm done with integration tests.

Seems memory swap is not enabled in build server kernel. I'm going to put a check there.

boynux · 2016-12-09T13:59:47Z

@hqhq Any updates here?

hqhq · 2016-12-10T01:01:04Z

Super busy these days, will take a look next week, sorry for the late.

hqhq · 2016-12-14T11:57:22Z

update.go

+				dest *uint64
+			}{
+				{"memory", r.Memory.Limit},
+				{"memory-swap", r.Memory.Swap},


The rule that '-1' means reset applies to all *.limit_in_bytes, so you can do this for all memory related limitations.

By that do you mean kernel-memory and kernel-memory-tcp and memory-reservation also?

hqhq · 2016-12-14T11:58:59Z

tests/integration/unlimited-memory.bats

+    sed -i "s/\(\"resources\": {\)/\1\n${DATA}/" ${BUSYBOX_BUNDLE}/config.json
+
+    # run a detached busybox to work with
+    runc run -d --console /dev/pts/ptmx test_cgroups_unlimited_mem


Needs update to use --console-socket.

hqhq · 2016-12-14T12:01:08Z

tests/integration/unlimited-memory.bats

@@ -0,0 +1,73 @@
+#!/usr/bin/env bats


You can add this case in update.bats, no need to create a new file.

hqhq · 2016-12-14T12:10:57Z

libcontainer/cgroups/fs/memory_test.go

 		memoryAfter      = 524288000 // 500M
 		memoryswapBefore = 629145600 // 600M
-		memoryswapAfter  = 629145600 // 600M
+		memoryswapAfter  = 0         // Unlimited


We took negative value means not change in unit test, and that's the point of this unit test, since that's not true after your PR, you can remove that unit test, and no need to add another.

@boynux Can you check my comment here and amend your unit test changes?

hqhq · 2017-02-09T02:38:53Z

tests/integration/update.bats

+    [ "$status" -eq 0 ]
+
+    # Get System memory limit
+    SYSTEM_MEMORY=$(cat "${CGROUP_SYSTEM_MEMORY}/memory.limit_in_bytes")


You can do the test at line 125 so you don't have to reset to initial test value again.

@hqhq I updated the tests please have a look if everything if fine I'll merge the commits and do a master rebase. Thank you

hqhq · 2017-02-10T01:24:11Z

libcontainer/cgroups/fs/memory.go

+	// -1 means unlimited memory
+	if cgroup.Resources.Memory == -1 {
+		// Only set swap if it's enbled in kernel
+		if _, err := readFile(path, "memory.memsw.limit_in_bytes"); err == nil {


You can use os.Stat, otherwise looks good to me.

hqhq · 2017-02-10T09:09:55Z

Please squash your commits.

boynux · 2017-02-10T09:14:04Z

Done!

boynux · 2017-02-10T09:52:28Z

@hqhq Should I rebase my changes with master?

Note: I just did it on my machine and managed to pass all the tests. If you want I can push it now.

hqhq · 2017-02-11T08:52:49Z

@boynux You don't have to as long as there's no conflict with master.

LGTM

Thanks for your patience.

boynux · 2017-02-13T20:15:04Z

How should we get the second approval?

dqminh · 2017-02-13T20:17:37Z

libcontainer/cgroups/fs/memory.go

-const cgroupKernelMemoryLimit = "memory.kmem.limit_in_bytes"
+const (
+	cgroupKernelMemoryLimit = "memory.kmem.limit_in_bytes"
+	cgroupMemorySwap        = "memory.memsw.limit_in_bytes"


this should also be cgroupMemorySwapLimit just for consistency ?

dqminh · 2017-02-13T20:37:05Z

libcontainer/cgroups/fs/memory.go

@@ -108,28 +121,28 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
 		// 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) {


when reset memory swap to -1 while the container still have memory.limit and memory.memsw.limit, this will fail and we will set memory limit to -1 while memsw is still in place. I think that will cause an error.
We should check if we want to reset memory / memoryswap here and change the order accordingly.

If we set memsw to -1 without changing memlimit I don't think it fails, since memory will be '0' (means no change) and it just sets memsw to -1 (which means unlimited)

Did I miss something here?

runc update test_update --memory-swap -1 [ "$status" -eq 0 ]

This test should cover what you just said, doesn't it? I just added it the test suite and it passed!

@dqminh Changed const name to be more "consistent" and also added that test just in case ;)

ah i see what's happening here. currentmemLimit < uint64(memSwap) with memSwap = -1 is practically always true because uint64(-1) is basically MaxUint64.

That being said, do you think its clearer to just add if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) so we dont depend on this uint64(-1) casting. I dont really like it.

boynux · 2017-02-14T21:00:27Z

@dqminh ping!

dqminh · 2017-02-15T00:04:40Z

libcontainer/cgroups/fs/memory.go

@@ -108,28 +121,28 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
 		// 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) {


ah i see what's happening here. currentmemLimit < uint64(memSwap) with memSwap = -1 is practically always true because uint64(-1) is basically MaxUint64.

That being said, do you think its clearer to just add if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) so we dont depend on this uint64(-1) casting. I dont really like it.

dqminh · 2017-02-15T00:05:06Z

tests/integration/update.bats

        check_cgroup_value $CGROUP_MEMORY "memory.memsw.limit_in_bytes" 96468992
    fi

+    if [ -f "$CGROUP_MEMORY/memory.memsw.limit_in_bytes" ]; then


this test can be merge into the conditional above right, make it clearer to see that we have an existing memory-swap limit too.

I'll merge them.

For casting -1 to uint64 I think that's a defined behavior and pretty explanatory, but since you missed it on the first try it says that can be more intuitive. being said that I'll also add a check explicitly for -1.

Kernel validation fails if memory set to -1 which is unlimited but swap is not set so. Signed-off-by: Mohammad Arab <[email protected]>

boynux · 2017-02-15T07:24:27Z

Ping @hqhq @dqminh

Changes are done, please check again.

dqminh · 2017-02-15T13:48:07Z

LGTM

boynux · 2017-02-15T18:10:22Z

@hqhq sorry for the trouble but we need your approval again.

hqhq · 2017-02-16T01:51:12Z

LGTM

In cgroupv1, command `runc update $ID --memory -1` sets both memory and memory+swap to -1 (aka unlimited). This was introduced by commit 18ebc51 (Reset Swap when memory is set to unlimited, Oct 19 2016). This is not the case for cgroupv2. Fix it. References: - opencontainers#1127 - moby/moby#22578 Signed-off-by: Kir Kolyshkin <[email protected]>

GordonTheTurtle added status/0-triage dco/no labels Oct 19, 2016

boynux force-pushed the fix-set-mem-to-unlimited branch from 6254b18 to 9a25275 Compare October 19, 2016 10:29

GordonTheTurtle added the dco/no label Oct 19, 2016

boynux force-pushed the fix-set-mem-to-unlimited branch from 9a25275 to e5dd699 Compare October 19, 2016 10:51

GordonTheTurtle removed the dco/no label Oct 19, 2016

coolljt0725 reviewed Oct 20, 2016

View reviewed changes

hqhq reviewed Oct 20, 2016

View reviewed changes

boynux force-pushed the fix-set-mem-to-unlimited branch 2 times, most recently from 1e7b56a to f4d035a Compare October 23, 2016 08:02

hqhq reviewed Nov 11, 2016

View reviewed changes

boynux force-pushed the fix-set-mem-to-unlimited branch 7 times, most recently from 69b4300 to bca1765 Compare December 1, 2016 14:22

hqhq reviewed Dec 14, 2016

View reviewed changes

hqhq reviewed Feb 9, 2017

View reviewed changes

boynux force-pushed the fix-set-mem-to-unlimited branch 3 times, most recently from cbec8e1 to d2999da Compare February 9, 2017 17:23

hqhq reviewed Feb 10, 2017

View reviewed changes

boynux force-pushed the fix-set-mem-to-unlimited branch from 6b6e69c to b218edc Compare February 10, 2017 09:11

dqminh requested changes Feb 13, 2017

View reviewed changes

boynux force-pushed the fix-set-mem-to-unlimited branch 2 times, most recently from 9a30595 to 61c5f4e Compare February 13, 2017 21:09

dqminh reviewed Feb 15, 2017

View reviewed changes

Reset Swap when memory is set to unlimited (-1)

18ebc51

Kernel validation fails if memory set to -1 which is unlimited but swap is not set so. Signed-off-by: Mohammad Arab <[email protected]>

boynux force-pushed the fix-set-mem-to-unlimited branch from 61c5f4e to 18ebc51 Compare February 15, 2017 07:12

dqminh approved these changes Feb 15, 2017

View reviewed changes

hqhq merged commit 6b1d0e7 into opencontainers:master Feb 16, 2017

williammartin mentioned this pull request May 2, 2017

runc:[2:INIT] processes not progressing when calling runc exec #1432

Closed

kolyshkin mentioned this pull request Apr 2, 2020

cgroupv2: set mem+swap to max if memory is set to max #2285

Closed

This was referenced May 20, 2020

fix two swap limit regressions in cgroup v2 #2405

Closed

Fix some cases of swap setting #2426

Merged

Conversation

boynux commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boynux Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyphar Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boynux commented Oct 20, 2016

Uh oh!

hqhq commented Oct 21, 2016

Uh oh!

boynux commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hqhq commented Oct 22, 2016

Uh oh!

boynux commented Oct 23, 2016

Uh oh!

LK4D4 commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boynux commented Dec 9, 2016

Uh oh!

hqhq commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Feb 10, 2017

Uh oh!

boynux commented Feb 10, 2017

Uh oh!

boynux commented Feb 10, 2017

Uh oh!

hqhq commented Feb 11, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boynux commented Oct 19, 2016 •

edited

Loading

boynux Oct 20, 2016 •

edited

Loading

cyphar Oct 20, 2016 •

edited

Loading

boynux commented Oct 21, 2016 •

edited

Loading

hqhq commented Feb 11, 2017 •

edited by caniszczyk

Loading

boynux Feb 13, 2017 •

edited

Loading

boynux commented Feb 15, 2017 •

edited

Loading

dqminh commented Feb 15, 2017 •

edited by caniszczyk

Loading