X Tutup
Skip to content

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

Merged
thaJeztah merged 1 commit intomoby:masterfrom
arm64b:oom-kill-disable-fixing
Feb 7, 2018
Merged

Daemon: passdown the --oom-kill-disable option to containerd#36201
thaJeztah merged 1 commit intomoby:masterfrom
arm64b:oom-kill-disable-fixing

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

tophj-ibm commented Feb 5, 2018

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

thaJeztah commented Feb 6, 2018

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

thaJeztah commented Feb 7, 2018

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

thaJeztah commented Feb 7, 2018

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

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@dnephin dnephin dnephin approved these changes

Reviewers whose approvals may not affect merge requirements

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
X Tutup