Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

cgroups: add support for oom control #417

Merged
merged 2 commits into from
Mar 6, 2015
Merged

Conversation

HuKeping
Copy link
Contributor

Redo #412
This patch add support for diable OOM Killer.

Signed-off-by: Hu Keping hukeping@huawei.com

@LK4D4
Copy link
Contributor

LK4D4 commented Feb 27, 2015

@HuKeping I'm not sure but should be pretty easy to write test for this. At least in docker we have test for oom :)

@HuKeping
Copy link
Contributor Author

ok, test will come soon

@HuKeping
Copy link
Contributor Author

hi @LK4D4 I am not quite sure what you would like this test to do. I think the oom control test would be better in docker/docker?

@HuKeping
Copy link
Contributor Author

do we have plan how often will docker/docker merge libcontainer? it seems a lot of docker's PR depend on libcontainer

@LK4D4
Copy link
Contributor

LK4D4 commented Feb 28, 2015

@HuKeping I'm working on move docker to new libcontainer API. There are some problems :) After that we can update libcontainer as often as we need it in docker.

@HuKeping
Copy link
Contributor Author

@LK4D4 OK and by the way what do you want for this PR's test? I didn't see much to do.

@LK4D4
Copy link
Contributor

LK4D4 commented Feb 28, 2015

@HuKeping Actually I prefer to have good coverage for libcontainer too. There was pretty bad problems because of tests lack. But if you don't want write a test, then we can leave it for docker birthday party :)

@HuKeping
Copy link
Contributor Author

@LK4D4 yes, of cause I'd like to post a test for this , just don't know what should it be like. Is it to check the writeable of file memory.oom_control?

@HuKeping HuKeping force-pushed the master branch 2 times, most recently from 2e8cab9 to 4332ffc Compare March 6, 2015 10:49
@HuKeping
Copy link
Contributor Author

HuKeping commented Mar 6, 2015

test added, ping @LK4D4

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 6, 2015

LGTM

1 similar comment
@vmarmol
Copy link
Contributor

vmarmol commented Mar 6, 2015

LGTM

vmarmol added a commit that referenced this pull request Mar 6, 2015
cgroups: add support for oom control
@vmarmol vmarmol merged commit dd3cb88 into docker-archive:master Mar 6, 2015
This patch add support for diable OOM Killer.

Signed-off-by: Hu Keping <hukeping@huawei.com>
Signed-off-by: Hu Keping <hukeping@huawei.com>
@HuKeping
Copy link
Contributor Author

hi @LK4D4 , would you please help me to review the related PR on docker/docker
moby/moby#11034

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jul 12, 2017
It's backed by memory.oom_control, so this commit moves it in with
the rest of the memory-controller config.

Looking at the history, the initial request landing a setting for this
in the Docker/OCI ecosystem seems to be [1], which added
Cgroup.OomKillDisable.  That commit was carried from libcontainer into
runC [2] where it is now Resources.OomKillDisable [3].  From runC it
was carried into this repo (with some renaming) in [4].  Subsequent
early doc updates landed in [5,6].  In none of those can I find
discussion about why the setting is not already under memory.  I
expect the reason is that the runC structures are flat, so "under
memory" is not a thing there.  But in this spec, resources has
per-controller sub-properties.  The fact that disableOOMKiller
belonged to the memory controller may have been overlooked in [4] and
never revisited until now.

[1]: docker-archive/libcontainer#417
     Subject: cgroups: add support for oom control
[2]: opencontainers/runc@295c708
     Subject: cgroups: add support for oom control
[3]: https://github.com/opencontainers/runc/blob/v1.0.0-rc3/libcontainer/configs/cgroup_unix.go#L113-L114
[4]: opencontainers#51
     Subject: Add Go types for specification
[5]: opencontainers#137
     Subject: Adding cgroups path to the Spec.
[6]: opencontainers#199
     Subject: runtime: config: linux: add cgroups informations

Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants