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

Handle memory swappiness default properly #580

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

estesp
Copy link
Contributor

@estesp estesp commented Feb 21, 2016

This prior fix was lost in other refactorings of the codebase.

Docker-DCO-1.1-Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com (github: estesp)

Fixes: #543
Fixes: #575

@estesp
Copy link
Contributor Author

estesp commented Feb 21, 2016

I think this is reasonable as the only way to have an "unset" value is to explicitly default to "-1" in the actual implementation, which is the libcontainer cgroups.Resources struct. Without this you end up with (a) a fatal error on kernels before the cgroups fix for setting memory.swappiness in children, or (b) you end up always setting swappiness to "0" as a default, which is most likely unintended by the user.

@crosbymichael
Copy link
Member

What do you think about making the type on the libcontainer config a pointer so that it can actually be nil and we treat nil and -1 as the same thing for backwards compat?

@estesp
Copy link
Contributor Author

estesp commented Feb 22, 2016

@crosbymichael sounds good.. I went for simplest approach, but backwards compat sounds like a good thing. I'll rework

@estesp estesp force-pushed the swappiness-fix branch 3 times, most recently from ca41492 to 94ece21 Compare February 24, 2016 14:53
This prior fix to set "-1" explicitly was lost, and it is simpler to use
the same pointer type from the OCI spec to handle nil pointer == -1 ==
unset case.

Also, as a nearly humorous aside, there was a test for MemorySwappiness
that was actually setting Memory, and it was passing because of this
bug (as it was always setting everyone's MemorySwappiness to zero!)

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@estesp estesp changed the title Setup correct memory swappiness default to -1 Handle memory swappiness default properly Feb 24, 2016
@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael added this to the 0.0.9 milestone Feb 24, 2016
@LK4D4
Copy link
Contributor

LK4D4 commented Feb 24, 2016

LGTM

LK4D4 added a commit that referenced this pull request Feb 24, 2016
Handle memory swappiness default properly
@LK4D4 LK4D4 merged commit f94eb27 into opencontainers:master Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants