Skip to content

Daemon: passdown the --oom-kill-disable option to containerd #36201

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 7, 2018
Merged

Daemon: passdown the --oom-kill-disable option to containerd #36201

merged 1 commit into from
Feb 7, 2018

Conversation

arm64b
Copy link
Contributor

@arm64b arm64b commented Feb 5, 2018

Current implementaion of docke daemon doesn't pass down the
--oom-kill-disable option specified by the end user to the containerd
when spawning a new docker instance with help from runc component, which
results in the --oom-kill-disable doesn't work no matter the flag is true
or false.

This PR will fix this issue reported by #36090

Signed-off-by: Dennis Chen dennis.chen@arm.com
Signed-off-by: Jianyong Wu jianyong.wu@arm.com

- What I did
Fix --oom-kill-disable issue
- How I did it
Pass down the actual value of the --oom-kill-disable specified by the end user to the containerd
- How to verify it
Run a container which will use up all the limited memory soon, like this:
/home/dennis/test# docker run -d -v "$PWD:/tmp" --oom-kill-disable=true -m 10MB --name oomk-disable busybox sh -c /tmp/oom.sh

oom.sh is a script to use up the memory quickly:

#!/bin/sh
x=a
while true
do
        x=$x$x$x$x
        echo "hello world"
        sleep 4
done

We can verify this in 2 methods:

  1. When the new docker instance spawned, there is a new generated folder under the /sys/fs/cgroup/memory/docker looks like /sys/fs/cgroup/memory/docker/423797dba327678cd5c3e4eb5e8723019c1aaeface3fab3755be3fd454b36076 in my system, then we can cat memory.oom_control file in this folder to verify if the --oom-kill-disable works or not.
    Before this PR, the oom_kill_disable filed in the file memory.oom_control is always 0 no matter we docker run a container with --oom-kill-disable as true or false. After this PR applied, the memory.oom_control will be changed according to the actual value assigned by the end user.

  2. Launch another terminal then run docker events to watch the events when running /home/dennis/test# docker run -d -v "$PWD:/tmp" --oom-kill-disable=true -m 10MB --name oomk-disable busybox sh -c /tmp/oom.sh
    Without this PR, the events will be:

...
container start
container oom
container die
...

With this PR, the events will be:

container start
container oom
container oom
container oom
...

We can see that the --oom-kill-disable=true works after applied this PR, consequently the DockerSuite.TestEventsOOMDisableFalse test case passed.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)
summit

@arm64b
Daemon: passdown the --oom-kill-disable option to containerd
44b074d 
Current implementaion of docke daemon doesn't pass down the
`--oom-kill-disable` option specified by the end user to the containerd
when spawning a new docker instance with help from `runc` component, which
results in the `--oom-kill-disable` doesn't work no matter the flag is `true`
or `false`.

This PR will fix this issue reported by moby#36090

Signed-off-by: Dennis Chen <dennis.chen@arm.com>
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
This was referenced Feb 5, 2018
Closed
Merged
dnephin
dnephin approved these changes Feb 5, 2018
View reviewed changes
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tophj-ibm
Copy link
Contributor

Just tested, fix works for me.

@arm64b
Copy link
Contributor Author

arm64b commented Feb 6, 2018

Thanks @dnephin for the review 👍
Let's ping the maintainers who commit this file recently for review to get another approval . @cpuguy83 @thaJeztah

@thaJeztah
Copy link
Member

Looks like this was introduced in 45d85c9 (#34356), which is part of 17.09

ping @mlaventure

Should we have a unit-test for this?

@arm64b
Copy link
Contributor Author

arm64b commented Feb 7, 2018

Hello @thaJeztah , seems we've already have a corresponding test file for the daemon_unix.go, this RP just passes a oom- flag into containerd, we certainly have a DockerSuite.TestEventsOOMDisableFalse in
integration which can have effective test for the change, so IMO we can postpone this unit now. But I flexible for this if we do need.

@thaJeztah
Copy link
Member

we certainly have a DockerSuite.TestEventsOOMDisableFalse in
integration which can have effective test for the change

Yes, so I was wondering why none of the tests caught this

@arm64b
Copy link
Contributor Author

arm64b commented Feb 7, 2018

Ah, the reason I have mentioned in PR #36185 Interesting all the arches (amd64, s390, ppc...) have skipped this test, I don't know the context/story here... 🤕 Let's abstract the test result of the TestEventsOOMDisableTrue on log of Janky:

18:28:26 SKIP: docker_cli_events_unix_test.go:51: DockerSuite.TestEventsOOMDisableFalse (unmatched requirement swapMemorySupport)
18:28:26 SKIP: docker_cli_events_unix_test.go:81: DockerSuite.TestEventsOOMDisableTrue (unmatched requirement swapMemorySupport)

That comes from the testRequires of this test case:

func (s *DockerSuite) TestEventsOOMDisableTrue(c *check.C) {
	testRequires(c, DaemonIsLinux, oomControl, memoryLimitSupport, NotArm, swapMemorySupport)

I think we can remove the swapMemorySupport and see what happens after this PR merged.

@arm64b
Copy link
Contributor Author

arm64b commented Feb 7, 2018

BTW: on our Linux/arm64 system, swapMemorySupport is true, so that triggers the issue.

thaJeztah
thaJeztah approved these changes Feb 7, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right; that would explain

LGTM

@thaJeztah thaJeztah merged commit 2e8ccbb into moby:master Feb 7, 2018
@arm64b arm64b deleted the oom-kill-disable-fixing branch February 7, 2018 05:58
@arm64b
Copy link
Contributor Author

arm64b commented Feb 7, 2018

Thanks @thaJeztah for the quick response 👍

@thaJeztah
Copy link
Member

Yw! Can you do a follow up to update the test conditions (as discussed above?)

arm64b added a commit to arm64b/moby that referenced this pull request Feb 7, 2018
@arm64b
Enable OOM Kill events integration test
f218275 
Now that the `-m` flag can work in case of cgroup swap memory limit
isn't enabled, also we have PR moby#36201 merged, so now we can remove
the `swapMemorySupport` from `testRequires()` order to trigger the real
test on those platforms which don't enable cgroup swap mem limit.

Signed-off-by: Dennis Chen <dennis.chen@arm.com>
@arm64b arm64b mentioned this pull request Feb 7, 2018
Closed
@arm64b
Copy link
Contributor Author

arm64b commented Feb 7, 2018

Yw! Can you do a follow up to update the test conditions (as discussed above?)

Absolutely. Please see PR #36229 😄

@arm64b arm64b mentioned this pull request Feb 11, 2018
Closed
@arm64b arm64b mentioned this pull request Jun 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnephin dnephin dnephin approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
impact/changelog status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@arm64b @tophj-ibm @thaJeztah @dnephin @yongtang @AkihiroSuda @GordonTheTurtle