enable cgroups memory.oom_control by HuKeping · Pull Request #11034 · moby/moby

HuKeping · 2015-02-26T08:11:06Z

This PR is related to #11026

closes #11026

hqhq · 2015-02-26T08:56:19Z

You shouldn't add libcontainer changes here, do it in libcontainer project.

hqhq · 2015-02-26T08:59:57Z

Conflict with #10298 , better hold a while for now :)

hqhq · 2015-02-26T09:03:06Z

memory.oom_control is with memory cgroup, I think it is not needed to check this file separately.

yes, this check is redundancy :), but it is a little wired we make sysInfo.OomKillDisable= true without doing anything.

HuKeping · 2015-02-26T10:55:06Z

I've added it in libcontainer, but it seems libcontainer in docker/docker is far behind the one in docker/libcontainer

HuKeping · 2015-02-27T10:17:12Z

@hqhq updated, and this PR needs docker-archive/libcontainer#417 to be merged.

HuKeping · 2015-03-09T01:17:06Z

updated and docker-archive/libcontainer#417 merged.

Since the docker/docker's libcontainer has not changed yet, this PR will fail for build :(

HuKeping · 2015-03-18T06:53:44Z

@hqhq , As #10298 , you said "Cgroup resources are host dependent, they should be in hostConfig."

Do you think this oom_control should also be in hostConfig?

hqhq · 2015-03-18T08:06:24Z

Yes, of course it is, all cgroup related configs are host dependent, because different hosts could have different cgroup subsystems mounted.

HuKeping · 2015-03-18T09:57:46Z

updated as @hqhq 's advice.
Since the change on libcontainer has been merged into docker, I think this PR can be reviewed now.
ping @jfrazelle @icecrime @crosbymichael @LK4D4

HuKeping · 2015-03-19T19:47:26Z

ping @icecrime @jamtur01

LK4D4 · 2015-03-19T20:00:37Z

@HuKeping The main problem that soon there won't be execdriver. We're working on it now and then you'll be able to rebase your changes on top.

hqhq · 2015-03-24T08:55:29Z

We should not add it here, all these codes below if job.EnvExists("HostConfig") is for backward compatibility, the new comings should always in HostConfig in json, or it won't work. We should lead users to use it the right way.

This is not in the if job.EnvExists block

You got me wrong, normally ContainerHostConfigFromJob should be as simple as just return job.GetenvJson("HostConfig", &hostConfig), all other codes are for backward compatibility, because we didn't have hostConfig at the first time.
For the new fields put into hostConfig, users should only use them through hostConfig, so if users want to use it, there would always have HostConfig env, and we can get the field from it, so you don't need to job.GetenvBool("OomKillDisable").

Ah you are right.

hqhq · 2015-03-24T09:05:19Z

You need to add docs as well.

hqhq · 2015-03-26T07:47:05Z

As I said you need docs updated, new options always need docs, you can take #11802 as an example.
But I think both of us need to wait for the execdriver change as @LK4D4 said.

HuKeping · 2015-03-26T09:50:42Z

OK, docs will come soon

icecrime · 2015-04-02T21:25:33Z

Although execdriver is meant to be removed, I think we can proceed reviewing this PR until it is. I'm pushing to 2-needs-code-review.

LK4D4 · 2015-04-14T22:25:09Z

LGTM
but needs rebase

cpuguy83 · 2015-04-30T16:42:04Z

No need for this \n I think.

cpuguy83 · 2015-05-01T13:52:58Z

LGTM moving to docs review

jamtur01 · 2015-05-01T14:17:04Z

Docs LGTM ping @moxiegirl @fredlf @SvenDowideit @thaJeztah

thaJeztah · 2015-05-01T14:23:06Z

The docs so far look good, but some info has to be added to the Run reference that has a section on constraining Memory.

HuKeping · 2015-05-02T16:26:17Z

@thaJeztah updated

thaJeztah · 2015-05-03T04:31:46Z

This should be the same text as the other documents;

--oom-kill-disable=true|false: Whether to disable OOM Killer for the container or not.

HuKeping · 2015-05-03T15:30:33Z

Thanks for @thaJeztah advise, updated.

thaJeztah · 2015-05-03T15:58:45Z

Thanks! Could you put the --oom-kill-disable and -m/--memory flags in back-ticks? (` ``) then LGTM a

@jamtur01 @moxiegirl @fredlf @SvenDowideit PTAL for the added docs

HuKeping · 2015-05-03T16:07:46Z

@thaJeztah updated

moxiegirl · 2015-05-03T22:38:53Z

Based on the reference you wrote, I expected to see:

$ docker run -ti -m 100M --oom-kill-disable=true ubuntu:14.04 /bin/bash

Why is that =true (or false) missing here?

It's because of the (IMO) confusing way that Boolean flags are documented. Basically, --enable-foo=true is the same as --enable-foo, and --enable-foo=false is the same as not setting the --enable-foo flag. For more info, see #9406.

In general, you don't actually add the true/false value, even though you can.
If you're okay with not documenting the =true|false here, I'd be +1, but it would be inconsistent with other parts of the docs 😞

I'd like to +1 for just --oom-kill-disable

@thaJeztah I was afraid of this; wow, I think that is confusing even without the lemon drops I've been drinking. Uh, ok. I'll tweak my comments. We do need to fix this though, I think you are right.

HuKeping · 2015-05-04T05:10:47Z

hi @moxiegirl updated as your advise but not use the =true, hope this is OK to you

Add cgroup support for disable OOM killer. Signed-off-by: Hu Keping <[email protected]>

moxiegirl · 2015-05-04T14:32:02Z

Thanks for the work @HuKeping. LGTM

thaJeztah · 2015-05-04T15:52:56Z

Thanks @HuKeping LGTM

enable cgroups memory.oom_control

GordonTheTurtle added the status/0-triage label Feb 26, 2015

hqhq reviewed Feb 26, 2015
View reviewed changes

jessfraz added status/1-design-review and removed status/0-triage labels Mar 2, 2015

HuKeping mentioned this pull request Mar 19, 2015

cgroups: add support for oom control docker-archive/libcontainer#417

Merged

hqhq reviewed Mar 24, 2015
View reviewed changes

icecrime added status/2-code-review and removed status/1-design-review labels Apr 2, 2015

hqhq mentioned this pull request Apr 9, 2015

add cpu.cfs_period_us support #12180

Merged

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

cpuguy83 reviewed Apr 30, 2015
View reviewed changes

cpuguy83 added status/3-docs-review and removed status/2-code-review labels May 1, 2015

thaJeztah reviewed May 3, 2015
View reviewed changes

moxiegirl reviewed May 3, 2015
View reviewed changes

Feature: option for disable OOM killer

a4a924e

Add cgroup support for disable OOM killer. Signed-off-by: Hu Keping <[email protected]>

moxiegirl added status/4-merge and removed status/3-docs-review labels May 4, 2015

thaJeztah added a commit that referenced this pull request May 4, 2015

Merge pull request #11034 from HuKeping/oom_kill_disable

ac324e5

enable cgroups memory.oom_control

thaJeztah merged commit ac324e5 into moby:master May 4, 2015

HuKeping deleted the oom_kill_disable branch May 5, 2015 01:19

thaJeztah mentioned this pull request May 11, 2015

Support setting OOM score #13116

Closed

phemmer mentioned this pull request Jul 7, 2015

oom_kill_disable poorly understood & implemented #14440

Closed

Conversation

HuKeping commented Feb 26, 2015

Uh oh!

hqhq commented Feb 26, 2015

Uh oh!

hqhq commented Feb 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuKeping commented Feb 26, 2015

Uh oh!

HuKeping commented Feb 27, 2015

Uh oh!

HuKeping commented Mar 9, 2015

Uh oh!

HuKeping commented Mar 18, 2015

Uh oh!

hqhq commented Mar 18, 2015

Uh oh!

HuKeping commented Mar 18, 2015

Uh oh!

HuKeping commented Mar 19, 2015

Uh oh!

LK4D4 commented Mar 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Mar 24, 2015

Uh oh!

hqhq commented Mar 26, 2015

Uh oh!

HuKeping commented Mar 26, 2015

Uh oh!

icecrime commented Apr 2, 2015

Uh oh!

LK4D4 commented Apr 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 1, 2015

Uh oh!

jamtur01 commented May 1, 2015

Uh oh!

thaJeztah commented May 1, 2015

Uh oh!

HuKeping commented May 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuKeping commented May 3, 2015

Uh oh!

thaJeztah commented May 3, 2015

Uh oh!

HuKeping commented May 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuKeping commented May 4, 2015

Uh oh!

moxiegirl commented May 4, 2015

Uh oh!

thaJeztah commented May 4, 2015

Uh oh!

Reviewers