Fix update memory without memoryswap #25461
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| If you want update a memory limit bigger than the already set swap memory limit, | ||
| you should update swap memroy limit at the same time. If you don't set swap memory | ||
| limit on docker create/run but only memory limit, the swap memory is double of | ||
| the memory limit. |
There was a problem hiding this comment.
remove the "of" here, or use "double that of", it reads a bit odd.
There was a problem hiding this comment.
fixed, thanks :)
coolljt0725
force-pushed
the
fix_update_mem
branch
from
96cd3cb
to
a705989
Compare
| @@ -67,6 +67,12 @@ a running container with kernel memory initialized. | |||
| **-m**, **--memory**="" | |||
| Memory limit (format: <number><optional unit>, where unit = b, k, m or g) | |||
|
|
|||
| Note that the memory should be smaller than the already set swap memory limit. | |||
| If you want update a memory limit bigger than the already set swap memory limit, | |||
| you should update swap memroy limit at the same time. If you don't set swap memory | |||
The memory should always be smaller than memoryswap, we should error out with message that user know how to do rather than just an invalid argument error if user update the memory limit bigger than already set memory swap. Signed-off-by: Lei Jitang <leijitang@huawei.com>
coolljt0725
force-pushed
the
fix_update_mem
branch
from
a705989
to
9239478
Compare
|
LGTM |
| @@ -283,6 +283,11 @@ func (container *Container) UpdateContainer(hostConfig *containertypes.HostConfi | |||
| cResources.CpusetMems = resources.CpusetMems | |||
| } | |||
| if resources.Memory != 0 { | |||
| // if memory limit smaller than already set memoryswap limit and doesn't | |||
| // update the memoryswap limit, then error out. | |||
| if resources.Memory > cResources.MemorySwap && resources.MemorySwap == 0 { | |||
There was a problem hiding this comment.
Should this be
if resources.Memory > cResources.MemorySwap && resources.Memory > resources.MemorySwap {
To check if a new MemorySwap is set, it's a valid new combination as well?
There was a problem hiding this comment.
er, sorry, my example wasn't right, hold on
There was a problem hiding this comment.
think this;
if resources.Memory > cResources.MemorySwap && (resources.MemorySwap == 0 || resources.Memory > resources.MemorySwap) {There was a problem hiding this comment.
LOL, actually, my first example may be correct
There was a problem hiding this comment.
@thaJeztah We already has resources.Memory > resources.MemorySwap validation before we get here in https://github.com/docker/docker/blob/master/daemon/daemon_unix.go#L273. So I think it's ok just check if the memoryswap is set here.
There was a problem hiding this comment.
@coolljt0725 ah, makes sense, thanks!
|
LGTM |
liusdu
pushed a commit
to liusdu/moby
that referenced
this pull request
Oct 30, 2017
The memory should always be smaller than memoryswap, we should error out with message that user know how to do rather than just an invalid argument error if user update the memory limit bigger than already set memory swap. cherry-pick from: moby#25461 conflict: integration-cli/docker_cli_update_unix_test.go Signed-off-by: Lei Jitang <leijitang@huawei.com>
liusdu
pushed a commit
to liusdu/moby
that referenced
this pull request
Oct 30, 2017
Fix update memory without memoryswap The memory should always be smaller than memoryswap, we should error out with message that user know how to do rather than just an invalid argument error if user update the memory limit bigger than already set memory swap. cherry-pick from: moby#25461 conflict: integration-cli/docker_cli_update_unix_test.go Signed-off-by: Lei Jitang <leijitang@huawei.com> See merge request docker/docker!389
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
fixes #25460
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
The memory should always be smaller than memoryswap,
we should error out with message that user know how
to do rather than just an invalid argument error if
user update the memory limit bigger than already set
memory swap.
Signed-off-by: Lei Jitang leijitang@huawei.com