oom detection #365

rphillips · 2022-04-28T16:15:08Z

Added support for OOM detection. Need to figure out how to test it.

cgroup v2
cgroup v1
add oom exit paths

Fixes #297

codecov-commenter · 2022-04-28T16:26:25Z

Codecov Report

Merging #365 (5d30fbf) into main (7bcb7e2) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #365   +/-   ##
=====================================
  Coverage   6.89%   6.89%           
=====================================
  Files          2       2           
  Lines         29      29           
  Branches      17      17           
=====================================
  Hits           2       2           
  Misses        27      27

conmon-rs/server/src/child_reaper.rs

rphillips · 2022-04-28T20:28:27Z

going to fix the cgroup v1 flake.

rphillips · 2022-04-28T21:36:35Z

Fixed... I had to import the tokio-eventfd library to work with cgroupv1 events.

rphillips · 2022-04-29T00:25:06Z

Going to integrate a test and figure out cgroup v1... Something is still wrong.

conmon-rs/server/src/child_reaper.rs

conmon-rs/server/src/oom_watcher.rs

rphillips · 2022-04-29T15:43:46Z

I added all the requested fixes. We will need much more scaffolding to test OOMs within conmon-rs. I think we should rely on crio's test for now and add a TODO to natively test OOMs with this repo.

saschagrunert

Just two nits, otherwise LGTM :)

conmon-rs/server/src/oom_watcher.rs

conmon-rs/server/src/child_reaper.rs

rphillips · 2022-05-02T19:03:02Z

@saschagrunert can you try this on a cgroupv1 machine and see if it fails for you?

saschagrunert · 2022-05-03T07:22:07Z

pkg/client/client_test.go

@@ -58,6 +58,7 @@ var _ = Describe("ConmonClient", func() {
 			})

 			It(testName("should write exit file", terminal), func() {
+				Skip("")


I think this should be removed, right? :)

If I re-enable the test then the Expect(fileContents(tr.exitPath())).To(Equal("0")) does fail below.

saschagrunert · 2022-05-03T08:38:56Z

@saschagrunert can you try this on a cgroupv1 machine and see if it fails for you?

Hm, on Ubuntu 18.04 the setup seems to work:

root@ubuntu1804:~/conmon-rs# ginkgo run -v -r --focus="should write exit file$"
Running Suite: ConmonClient - /root/conmon-rs/pkg/client
========================================================
Random Seed: 1651567071

Will run 1 of 25 specs
SSSSSSSS
------------------------------
ConmonClient CreateContainer
  should write exit file
  /root/conmon-rs/pkg/client/client_test.go:60
2022-05-03T08:37:52.689Z INFO [conmon::server] Using stdout logger
2022-05-03T08:37:52.689Z INFO [conmon::server] Set log level to: DEBUG
2022-05-03T08:37:52.694Z DEBUG [conmon::rpc] Got a version request
2022-05-03T08:37:52.697Z DEBUG [conmon::rpc] Got a create container request for id 145ac9c38327b45b8eb09b9823e2b71e8c43812226ef8abf2ba7d0b463cefb95
2022-05-03T08:37:52.697Z DEBUG [conmon::streams] Creating new IO streams
2022-05-03T08:37:52.698Z DEBUG [conmon::rpc] PID file is /tmp/conmon-client954065182/pidfile
2022-05-03T08:37:52.698Z DEBUG [conmon::server] Runtime args "--root=/tmp/conmon-client954065182/root create --bundle /tmp/conmon-client954065182 --pid-file /tmp/conmon-client954065182/pidfile 145ac9c38327b45b8eb09b9823e2b71e8c43812226ef8abf2ba7d0b463cefb95"
2022-05-03T08:37:52.698Z DEBUG [conmon::cri_logger] Initializing CRI logger in path /tmp/conmon-client954065182/log
2022-05-03T08:37:52.699Z DEBUG [conmon::streams] Start reading from IO streams
2022-05-03T08:37:52.845Z DEBUG [conmon::oom_watcher] 7611: enabled cgroup v1 oom handling
2022-05-03T08:37:52.845Z DEBUG [conmon::oom_watcher] 7611: using cgroup path : /proc/7611/cgroup
2022-05-03T08:37:52.850Z DEBUG [conmon::oom_watcher] 7611: memory_cgroup_file_oom_path /sys/fs/cgroup/memory/user.slice/145ac9c38327b45b8eb09b9823e2b71e8c43812226ef8abf2ba7d0b463cefb95/memory.oom_control
2022-05-03T08:37:52.852Z DEBUG [conmon::oom_watcher] 7611: successfully setup cgroup v1 oom detection
2022-05-03T08:37:52.881Z DEBUG [conmon::container_io] fd:14:read 21 bytes
2022-05-03T08:37:52.910Z DEBUG [conmon::container_io] fd:16:No more to read
2022-05-03T08:37:52.911Z DEBUG [conmon::container_io] fd:14:No more to read
VmRSS for server is 10592
•SSSSSSSSSSSSSSSS

Ran 1 of 25 Specs in 0.255 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 24 Skipped
PASS
2022-05-03T08:37:52.940Z INFO [conmon::server] Received SIGINT
2022-05-03T08:37:52.940Z DEBUG [conmon::child_reaper] 7611: killing pid
2022-05-03T08:37:52.940Z DEBUG [conmon::child_reaper] Failed killing pid 7611 with error ESRCH: No such process
2022-05-03T08:37:52.940Z DEBUG [conmon::server] Removing socket file /tmp/conmon-client954065182/conmon.sock
2022-05-03T08:37:52.940Z DEBUG [conmon::oom_watcher] 7611: done watching for ooms

rphillips · 2022-05-09T16:02:00Z

This is ready now... phew.

haircommander · 2022-05-09T20:32:56Z

.github/workflows/ci.yml

@@ -7,7 +7,7 @@ on:
 env:
  CARGO_TERM_COLOR: always
  GO_VERSION: '1.18'
-  ACTION_MSRV_TOOLCHAIN: 1.59.0
+  ACTION_MSRV_TOOLCHAIN: 1.60.0


is this toolchain available on rhcos?

cc @cgwalters if you know

haircommander · 2022-05-09T20:33:30Z

conmon-rs/server/src/child_reaper.rs

@@ -143,7 +143,7 @@ impl ChildReaper {

    pub fn kill_grandchildren(&self, s: Signal) -> Result<()> {
        for (_, grandchild) in self.grandchildren.lock().unwrap().iter() {
-            debug!("{}: killing grandchild", grandchild.pid);
+            debug!(grandchild.pid, "killing grandchild");


I think this work got mixed with the toolchain bump, can we separate?

I would even prefer we just merge this stuff with the original commit, as the git tree will look as though span support was added before this anyway

fixed that up.

haircommander · 2022-05-09T20:34:11Z

one small nit, otherwise LGTM

what about 1.60 helped us?

great work on this, it's been a long haul!

Signed-off-by: Ryan Phillips <rphillips@redhat.com>

rphillips · 2022-05-09T21:27:06Z

@haircommander We hit this problem with the PR: rust-lang/cargo#10623. The only thing I could find to do is bump the version.

haircommander · 2022-05-09T23:20:26Z

LGTM, summoning @saschagrunert for the merge

saschagrunert

LGTM, I just see some unwraps and can follow-up on them. Great work @rphillips!

rphillips force-pushed the oom_detection branch 4 times, most recently from 4203d05 to 86dd940 Compare April 28, 2022 19:03

rphillips changed the title ~~WIP: oom detection~~ oom detection Apr 28, 2022

haircommander reviewed Apr 28, 2022

View reviewed changes

conmon-rs/server/src/child_reaper.rs Outdated Show resolved Hide resolved

rphillips force-pushed the oom_detection branch 2 times, most recently from 2910d88 to 8e5391e Compare April 28, 2022 20:23

rphillips changed the title ~~oom detection~~ WIP: oom detection Apr 28, 2022

rphillips force-pushed the oom_detection branch 3 times, most recently from 410ebb7 to 736a291 Compare April 28, 2022 21:28

rphillips changed the title ~~WIP: oom detection~~ oom detection Apr 28, 2022

rphillips force-pushed the oom_detection branch from 736a291 to 5c11719 Compare April 28, 2022 21:39

rphillips changed the title ~~oom detection~~ WIP: oom detection Apr 29, 2022

rphillips force-pushed the oom_detection branch from 5c11719 to 99240e0 Compare April 29, 2022 02:33

saschagrunert reviewed Apr 29, 2022

View reviewed changes

conmon-rs/server/src/child_reaper.rs Outdated Show resolved Hide resolved

conmon-rs/server/src/oom_watcher.rs Outdated Show resolved Hide resolved

conmon-rs/server/src/oom_watcher.rs Outdated Show resolved Hide resolved

conmon-rs/server/src/oom_watcher.rs Outdated Show resolved Hide resolved

saschagrunert reviewed Apr 29, 2022

View reviewed changes

conmon-rs/server/src/oom_watcher.rs Outdated Show resolved Hide resolved

rphillips force-pushed the oom_detection branch 6 times, most recently from 9e39623 to a81b77a Compare April 29, 2022 15:31

rphillips changed the title ~~WIP: oom detection~~ oom detection Apr 29, 2022

saschagrunert approved these changes May 2, 2022

View reviewed changes

conmon-rs/server/src/oom_watcher.rs Outdated Show resolved Hide resolved

conmon-rs/server/src/child_reaper.rs Outdated Show resolved Hide resolved

rphillips force-pushed the oom_detection branch 4 times, most recently from 95b2413 to 6d42b79 Compare May 2, 2022 18:21

saschagrunert reviewed May 3, 2022

View reviewed changes

rphillips force-pushed the oom_detection branch from 6d42b79 to e428d23 Compare May 3, 2022 13:19

haircommander mentioned this pull request May 3, 2022

Add support for conmonrs cri-o/cri-o#5839

Merged

rphillips force-pushed the oom_detection branch 7 times, most recently from b85b0b5 to 4c4798a Compare May 9, 2022 15:49

haircommander reviewed May 9, 2022

View reviewed changes

rphillips added 4 commits May 9, 2022 16:24

add support for oom detection

c354aaf

Signed-off-by: Ryan Phillips <rphillips@redhat.com>

update workflow with go symlinks

d34844c

Signed-off-by: Ryan Phillips <rphillips@redhat.com>

bump memory to 3100

b1f974f

Signed-off-by: Ryan Phillips <rphillips@redhat.com>

bump toolchain

5d30fbf

Signed-off-by: Ryan Phillips <rphillips@redhat.com>

rphillips force-pushed the oom_detection branch from 4c4798a to 5d30fbf Compare May 9, 2022 21:25

saschagrunert approved these changes May 10, 2022

View reviewed changes

saschagrunert merged commit 8868738 into containers:main May 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oom detection #365

oom detection #365

rphillips commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading

rphillips commented Apr 28, 2022

rphillips commented Apr 28, 2022

rphillips commented Apr 29, 2022

rphillips commented Apr 29, 2022

saschagrunert left a comment

rphillips commented May 2, 2022

saschagrunert May 3, 2022 •

edited

Loading

saschagrunert commented May 3, 2022

rphillips commented May 9, 2022

haircommander May 9, 2022

haircommander May 9, 2022

haircommander May 9, 2022

haircommander May 9, 2022

rphillips May 9, 2022

haircommander commented May 9, 2022

rphillips commented May 9, 2022

haircommander commented May 9, 2022

saschagrunert left a comment

oom detection #365

oom detection #365

Conversation

rphillips commented Apr 28, 2022 • edited Loading

codecov-commenter commented Apr 28, 2022 • edited Loading

Codecov Report

rphillips commented Apr 28, 2022

rphillips commented Apr 28, 2022

rphillips commented Apr 29, 2022

rphillips commented Apr 29, 2022

saschagrunert left a comment

Choose a reason for hiding this comment

rphillips commented May 2, 2022

saschagrunert May 3, 2022 • edited Loading

Choose a reason for hiding this comment

saschagrunert commented May 3, 2022

rphillips commented May 9, 2022

haircommander May 9, 2022

Choose a reason for hiding this comment

haircommander May 9, 2022

Choose a reason for hiding this comment

haircommander May 9, 2022

Choose a reason for hiding this comment

haircommander May 9, 2022

Choose a reason for hiding this comment

rphillips May 9, 2022

Choose a reason for hiding this comment

haircommander commented May 9, 2022

rphillips commented May 9, 2022

haircommander commented May 9, 2022

saschagrunert left a comment

Choose a reason for hiding this comment

rphillips commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading

saschagrunert May 3, 2022 •

edited

Loading