Fix memory update to -1 by boynux · Pull Request #22578 · moby/moby

boynux · 2016-05-07T20:31:22Z

- What I did
Close #22516

- How I did it
If memory parameter is not changed by update command set is as '-1'

- How to verify it
docker create --name test -m 50m busybox:latest
docker inspect -f '{{.HostConfig.Memory}}' test
#52428800
docker update -m -1 test
docker inspect -f '{{.HostConfig.Memory}}' test
# -1

docker run -it -d --name test2 -m 50m busybox:latest
docker stats test2
# Shows 50m memory limit
docker update -m -1 test2
docker stats test2
# Shows no memory limits (host memory)

- Description for the changelog
Remove memory constraints after creating/running container with update command

Possibility to set memory to '-1' which removes memory constraints

Signed-off-by: boynux [email protected]

runcom · 2016-05-08T10:49:27Z

ping @hqhq

boynux · 2016-05-08T12:05:39Z

There is a bug in my pull request I'm trying to fix that.

boynux · 2016-05-08T18:11:37Z

Pull request is ready for review, and I'm not sure why windowsTP5 is failing!
Ping @hqhq @runcom

hqhq · 2016-05-09T01:37:26Z

I think it makes more sense to use docker update --memory -1 <id> to unset memory limit to unlimited, because it's the way to reflect cgroup api.

boynux · 2016-05-09T07:14:26Z

@hqhq yes, seems more legit. I'll update my changes accordingly. I guess that would a bit easier as well.

boynux · 2016-05-11T07:27:51Z

ping @hqhq I'm done with your suggested changes, and I think gccgo failure is not related to my changes.

hqhq · 2016-05-11T09:30:47Z

@boynux Thanks, but docs need to be updated as well.

boynux · 2016-05-11T09:33:07Z

@hqhq sure, actually I wanted to ask this question. Where are the docs please?

Update:
I update Cli docs please let me know if other docs need update as well.

hqhq · 2016-05-16T08:12:17Z

@boynux It's not working in my box:

$ docker update -m -1 d3cc5496e730
Error response from daemon: Cannot update container d3cc5496e730be54b9c109a5216fa92aa5ec7ed3c2382266850e6eda0947b71b: rpc error: code = 2 desc = "failed to write 4294967295 to memory.limit_in_bytes: write /sys/fs/cgroup/memory/docker/d3cc5496e730be54b9c109a5216fa92aa5ec7ed3c2382266850e6eda0947b71b/memory.limit_in_bytes: invalid argument\n"

I guess your change is not complete.

hqhq · 2016-05-16T08:13:52Z

integration-cli/docker_cli_update_unix_test.go

It's not sufficient to update a created container, you should also update a started container.

correct, I'll add more tests.

boynux · 2016-05-17T15:32:53Z

@hqhq Regarding your test, that's interesting, the integration tests does exactly what you did and it passes the test (false-positive?). I also tried that several times with no issues. Would you please provide the create/run command for your container as well.

I'll check again to see if something is missing.

hqhq · 2016-05-18T01:03:57Z

@boynux The integration test updates a stopped container, and my test is update a running container, just use:

$ docker run -ti --rm -n test -m 300M ubuntu bash
[switch to another terminal]
$ docker update -m -1 test

LK4D4 · 2016-05-23T20:39:39Z

@boynux do you still have time to work on this?

boynux · 2016-05-23T20:46:50Z

Sorry I was very busy last week and this week too. I partially implemented
the run part but not ready to submit.

I don't promise this week but definitely next week I'm able to finish the
work. If there is schedule to release this this week then I'd suggest
someone picks up the last part otherwise I'll finish that by next week. (I
don't promise but if I find some time free this week I'll try to finish
that).

On Mon, May 23, 2016, 10:40 PM Alexander Morozov [email protected]
wrote:

@boynux https://github.com/boynux do you still have time to work on
this?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#22578 (comment)

coolljt0725 · 2016-09-21T09:24:50Z

@boynux set swap memory to -1 and then set memory to -1 still doesn't work ? what's the issue?

justincormack · 2016-09-24T21:32:54Z

@boynux can you rebase? Can take another look and see if we can resolve.

boynux · 2016-09-25T13:06:54Z

I've rebased the my fork against master. Unfortunately my tests are broken now (which used to work :( )

boynux · 2016-09-27T08:22:05Z

Wow! All test passed! What's going on here?
ping @coolljt0725, @justincormack

mlaventure · 2016-10-17T20:44:43Z

I couldn't test the swap option, the memory one works for me.

LGTM if someone can check the swap update.

re-ping @coolljt0725, @justincormack

coolljt0725 · 2016-10-18T09:08:06Z

sorry for late response, missing some note from github _:)

This doesn't work for me, I need to unset the memory.memsw.limit_in_bytes before docker update -m -1 xxx.

[root@fedora docker]# docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
64e9cbd26239        busybox             "sh"                31 seconds ago      Up 30 seconds                           agitated_boyd
[root@fedora docker]# cd 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b/
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# cat memory.limit_in_bytes
209715200
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# cat memory.memsw.limit_in_bytes
419430400
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# docker update -m -1 64e9cbd26239
Error response from daemon: Cannot update container 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b: rpc error: code = 2 desc = failed to write -1 to memory.limit_in_bytes: write /sys/fs/cgroup/memory/docker/64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b/memory.limit_in_bytes: invalid argument
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# echo -1 > memory.memsw.limit_in_bytes
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# docker update -m -1 64e9cbd26239
64e9cbd26239
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# docker version
Client:
 Version:      1.13.0-dev
 API version:  1.25
 Go version:   go1.6.3
 Git commit:   43922d0
 Built:        Tue Oct 18 08:46:30 2016
 OS/Arch:      linux/amd64
 Experimental: true

Server:
 Version:      1.13.0-dev
 API version:  1.25
 Go version:   go1.6.3
 Git commit:   43922d0
 Built:        Tue Oct 18 08:46:30 2016
 OS/Arch:      linux/amd64
 Experimental: true
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# cat memory.memsw.limit_in_bytes
9223372036854771712
[root@fedora 64e9cbd26239c17b669e3529f7f017370336d051b7cb6c2738cadc2cb724f64b]# cat memory.limit_in_bytes
9223372036854771712

Before unset the memory limit, we need to unset the --memory-swap first if the system has enable the memory swap limit because docker will set the memory swap limit to double the memory limit by default.

@mlaventure maybe your system doesn't enable memory swap limit or am I missing something?

boynux · 2016-10-18T12:08:45Z

I'll write a test for that. It would be easier to test it that way.

mlaventure · 2016-10-18T15:53:23Z

@coolljt0725 correct, memsw isn't enabled on my system :)

boynux · 2016-10-18T19:28:39Z

@coolljt0725 Your test suggests to also update containerd to reset swap? or it's sufficient to do it here? I'm not sure containerd does this in the correct order.

Update: I'm pretty unfamiliar with this code but I guess this is the issue: https://github.com/opencontainers/runc/blob/d1fc80226409b687245e2180d335827027bc3cca/libcontainer/cgroups/fs/memory.go#L98

From the tests I did it seems memory update happens before Swap limit, we can either update runC accordingly or split this into two requests. Or maybe I'm totally lost

boynux · 2016-10-18T23:05:59Z

@coolljt0725 @mlaventure I found the issue, this commit in RunC solves the problem (with some tweaks on Docker side)
So the issue is RunC simply ignores swap changes when it's -1 I just check to see if memory is set to -1 and then silently set swap mem to -1 too. So it passes Kernel validation.

Now what is the process to commit these changes into RunC? Should I do a PR and relate it to this ticket?

RunC Changes: boynux/runc@41afa94

mlaventure · 2016-10-18T23:55:21Z

@boynux yes, you will need to open a PR in the runc repo (https://github.com/opencontainers/runc). Once merge, we then need to update containerd to use the new version, then vendor containerd here 😅

boynux · 2016-10-19T04:58:45Z

Ok, cool. Then I will submit a PR for RunC, once merged I'll update here.
Thanks

boynux · 2016-10-20T11:34:17Z

@hqhq Can you review RunC pull request please.

hqhq · 2016-10-20T12:34:49Z

daemon/update_linux.go

This if condition is not needed.

So we're hacking like:
uint64(-1) = 18446744073709551615
int64(18446744073709551615) = -1

Is everyone comfortable with that? I prefer we change value type to explicitly say we allow negative value here.

I removed if statement as it's not needed, I think that was left over from master rebase.
The type change requires change to `containerd' types here: https://github.com/docker/containerd/blob/master/api/grpc/types/api.pb.go#L469

I can update that too, it makes sense to use int64 instead. Please give a thumbs up if my proposed change is correct.

Possibility to set memory to '-1' which indicates no memory imits. Signed-off-by: boynux <[email protected]>

thaJeztah · 2016-12-22T19:14:57Z

I'm going to close this for now, because we need the changes in RunC, but ping me when that is merged, then we can reopen

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 the status/0-triage label May 7, 2016

boynux force-pushed the fix-memory-update branch 2 times, most recently from 5b20ac4 to e98bea3 Compare May 8, 2016 09:51

runcom added status/2-code-review and removed status/0-triage labels May 8, 2016

boynux force-pushed the fix-memory-update branch from 527ed44 to d4b761f Compare May 8, 2016 15:28

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label May 9, 2016

boynux force-pushed the fix-memory-update branch 2 times, most recently from 68b107c to c59cec3 Compare May 10, 2016 20:43

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 11, 2016

boynux force-pushed the fix-memory-update branch from ceb8d3d to 3096cde Compare May 11, 2016 07:34

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 11, 2016

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 11, 2016

hqhq reviewed May 16, 2016
View reviewed changes

thaJeztah mentioned this pull request Sep 20, 2016

Setting OOM score for ready process caused "write /proc/5578/oom_score_adj: invalid argument" #25099

Closed

boynux force-pushed the fix-memory-update branch from 03f3c25 to 7f42efc Compare September 25, 2016 13:05

boynux mentioned this pull request Oct 19, 2016

Fixes set memory to unlimited opencontainers/runc#1127

Merged

vdemeester added the status/needs-vendoring label Oct 19, 2016

hqhq reviewed Oct 20, 2016

View reviewed changes

boynux force-pushed the fix-memory-update branch 2 times, most recently from e8dfe59 to e6edeeb Compare October 20, 2016 20:00

boynux mentioned this pull request Oct 20, 2016

MemoryLimit can be negative containerd/containerd#333

Closed

hqhq mentioned this pull request Oct 31, 2016

The default value of cfs_quota_us should be -1, which is negative opencontainers/runtime-spec#499

Closed

Fix update to remove memory limits

57e386c

Possibility to set memory to '-1' which indicates no memory imits. Signed-off-by: boynux <[email protected]>

boynux force-pushed the fix-memory-update branch from e6edeeb to 57e386c Compare December 1, 2016 08:55

thaJeztah closed this Dec 22, 2016

boynux mentioned this pull request Jan 4, 2017

MemoryLimit can be negative containerd/containerd#406

Closed

kolyshkin mentioned this pull request Apr 2, 2020

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

Closed

Conversation

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

Uh oh!

Uh oh!

runcom commented May 8, 2016

Uh oh!

boynux commented May 8, 2016

Uh oh!

boynux commented May 8, 2016

Uh oh!

hqhq commented May 9, 2016

Uh oh!

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

Uh oh!

Uh oh!

boynux commented May 11, 2016

Uh oh!

hqhq commented May 11, 2016

Uh oh!

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

Uh oh!

Uh oh!

hqhq commented May 16, 2016

Uh oh!

hqhq May 16, 2016

Choose a reason for hiding this comment

Uh oh!

boynux May 17, 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 May 17, 2016

Uh oh!

hqhq commented May 18, 2016

Uh oh!

LK4D4 commented May 23, 2016

Uh oh!

boynux commented May 23, 2016

Uh oh!

coolljt0725 commented Sep 21, 2016

Uh oh!

justincormack commented Sep 24, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mlaventure commented Oct 17, 2016

Uh oh!

coolljt0725 commented Oct 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

boynux commented Oct 18, 2016

Uh oh!

mlaventure commented Oct 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

boynux commented Oct 20, 2016

Uh oh!

hqhq Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

boynux Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 22, 2016

Uh oh!

Reviewers

boynux commented May 7, 2016 •

edited

Loading

boynux commented May 9, 2016 •

edited

Loading

boynux commented May 11, 2016 •

edited

Loading

boynux May 17, 2016 •

edited

Loading

boynux commented Sep 25, 2016 •

edited

Loading

boynux commented Sep 27, 2016 •

edited

Loading

boynux commented Oct 18, 2016 •

edited by thaJeztah

Loading

mlaventure commented Oct 18, 2016 •

edited

Loading

boynux commented Oct 18, 2016 •

edited

Loading

boynux commented Oct 19, 2016 •

edited

Loading