Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem when update memory and swap memory #592

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

hqhq
Copy link
Contributor

@hqhq hqhq commented Feb 26, 2016

Currently, if we start a container with:
docker run -ti --name foo --memory 300M --memory-swap 500M busybox sh

Then we want to update it with:
docker update --memory 600M --memory-swap 800M foo

It'll get error because we can't set memory to 600M with
the 500M limit of swap memory.

Signed-off-by: Qiang Huang h.huangqiang@huawei.com

@dqminh
Copy link
Contributor

dqminh commented Feb 26, 2016

What do you think about setting memsw first before setting memory ?

@hqhq
Copy link
Contributor Author

hqhq commented Feb 27, 2016

@dqminh It won't work for the initial set, where memory is unlimited for a new cgroup, you can't set memsw lower than memory.

@dqminh
Copy link
Contributor

dqminh commented Feb 27, 2016

Hmm you are right. I think the problem with this approach is that it reset the limit momentarily, so theres a chance bad things can happen i.e. process can request more memory than it can.

WDYT about reading the existing value out so we can set the order approriately. If new memsw < old mem, then set new mem first etc ?

@hqhq
Copy link
Contributor Author

hqhq commented Feb 27, 2016

Yeah, I thought about reading existing values and compare them, that logic would be a bit complicated , if in such case that lots of memory are allocated in such short period, after the new limits are set, memory will be reclaimed or cause oom, and this reclaim or oom will happen in such case either we reset the limit momentarily or set the right limit directly, so it's not that bad, but is, there will be a slight difference for user's application.

I'll update it to set the right limit directly.

@cyphar
Copy link
Member

cyphar commented Feb 27, 2016

I agree with @dqminh, we shouldn't reset the limits in order to set new ones. Unfortunately, this will have race conditions no matter how we do it (if we do it in memory, two racing updates would start clobbering each other). Still, I think that "reading the set value for memsw and then changing it if it's too low" is a perfectly fine way of doing this.

@hqhq
Copy link
Contributor Author

hqhq commented Feb 27, 2016

@cyphar Yeah I agree too, too busy today, I'll update next week.

@hqhq hqhq force-pushed the hq_fix_update_memory branch from c8a459f to 2de001f Compare February 29, 2016 03:21
@hqhq
Copy link
Contributor Author

hqhq commented Feb 29, 2016

@dqminh @cyphar Updated.

@hqhq hqhq force-pushed the hq_fix_update_memory branch from 2de001f to 638ead2 Compare March 2, 2016 12:46
@hqhq
Copy link
Contributor Author

hqhq commented Mar 8, 2016

ping @dqminh @mrunalp @LK4D4 @crosbymichael

@cyphar
Copy link
Member

cyphar commented Mar 12, 2016

@hqhq What about the memory soft limit? AFAICS it'll have similar ordering issues.

@hqhq
Copy link
Contributor Author

hqhq commented Mar 14, 2016

@cyphar I don't think there is problem. Soft limit can be set larger than memory limit, we just recommend to set the soft limit always below the hard limit, otherwise the hard limit will take precedence. But there is no force which will cause update fail, so we don't have to worry about the memory soft limit.

@cyphar
Copy link
Member

cyphar commented Mar 14, 2016

@hqhq Yeah, you're right. I thought they'd have mirrored what they did with rlimit (where soft limits must always be smaller than hard limits).

@hqhq hqhq force-pushed the hq_fix_update_memory branch from 638ead2 to e456096 Compare March 25, 2016 11:40
@hqhq
Copy link
Contributor Author

hqhq commented Mar 25, 2016

Rebased, ping @cyphar @dqminh @mrunalp @crosbymichael @LK4D4

@mrunalp
Copy link
Contributor

mrunalp commented Mar 26, 2016

build failed

libcontainer/cgroups/fs/memory_test.go:122: possible formatting directive in Fatal call

Currently, if we start a container with:
`docker run -ti --name foo --memory 300M --memory-swap 500M busybox sh`

Then we want to update it with:
`docker update --memory 600M --memory-swap 800M foo`

It'll get error because we can't set memory to 600M with
the 500M limit of swap memory.

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
@hqhq hqhq force-pushed the hq_fix_update_memory branch from e456096 to d8b8f76 Compare March 28, 2016 03:51
@hqhq
Copy link
Contributor Author

hqhq commented Mar 28, 2016

@mrunalp Updated.

@hqhq
Copy link
Contributor Author

hqhq commented Apr 5, 2016

ping @mrunalp @cyphar PTAL, it's a bug of docker update, hope it can catch up with 1.11

@cyphar
Copy link
Member

cyphar commented Apr 5, 2016

LGTM

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Apr 5, 2016

LGTM

@mrunalp mrunalp merged commit 3f4f442 into opencontainers:master Apr 5, 2016
@hqhq hqhq deleted the hq_fix_update_memory branch April 6, 2016 01:03
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
This adds test coverage for code added by commit d8b8f76
("Fix problem when update memory and swap memory", 2016-04-05).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
Add test coverage for code initially added by commit d8b8f76
("Fix problem when update memory and swap memory", 2016-04-05).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
Add test coverage for code initially added by commit d8b8f76
("Fix problem when update memory and swap memory", 2016-04-05).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
Add integration test coverage for code initially added by commit
d8b8f76 ("Fix problem when update memory and swap memory",
2016-04-05).

This is in addition to existing unit test,
TestMemorySetSwapSmallerThanMemory.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
Add integration test coverage for code initially added by commit
d8b8f76 ("Fix problem when update memory and swap memory",
2016-04-05).

This is in addition to existing unit test,
TestMemorySetSwapSmallerThanMemory.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 19, 2021
Add integration test coverage for code initially added by commit
d8b8f76 ("Fix problem when update memory and swap memory",
2016-04-05).

This is in addition to existing unit test,
TestMemorySetSwapSmallerThanMemory.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Feb 24, 2021
Add integration test coverage for code initially added by commit
d8b8f76 ("Fix problem when update memory and swap memory",
2016-04-05).

This is in addition to existing unit test,
TestMemorySetSwapSmallerThanMemory.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants