add oom monitor for rustshim

[aa624545345]

Registering and reporting cgroup OOM events for Rust shim。 #227

[Burning1020]

Thanks for your pr, some of the methods already exist, so please change your code, thanks.

[aa624545345]

@mxpv @Burning1020 Hello, are there any further plans for this submission?

[mxpv]

Left a few suggestions.
Also I'd love to see some unit tests if possible.

[Burning1020]

Left a few suggestions. Also I'd love to see some unit tests if possible.

Here I suggest the UT could be:

1. mock a remote publisher
2. create a temp memory cgroup and set the limit, like 10K
3. keep watching this cgroup path
4. fork a child process that would apply for high memory, like 11K

match fork().unwrap() {
            ForkResult::Parent { child } => {
                let pid = child.as_raw() as u64;
                cg.add_task_by_tgid(CgroupPid::from(pid)).unwrap(); // add child to cgroup
                waitpid(Some(child), None).unwrap();
            }
            ForkResult::Child => {
                std::thread::sleep(Duration::from_millis(100));
                let mut v: Vec<u8> = vec![];
                v.resize(11 * 1024 * 1024, 0);
                exit(0);
            }
        };

5. make sure OOM event received
6. reasource cleanup

[aa624545345]

Left a few suggestions. Also I'd love to see some unit tests if possible.

Here I suggest the UT could be:

1. mock a remote publisher
2. create a temp memory cgroup and set the limit, like 10K
3. keep watching this cgroup path
4. fork a child process that would apply for high memory, like 11K

match fork().unwrap() {
            ForkResult::Parent { child } => {
                let pid = child.as_raw() as u64;
                cg.add_task_by_tgid(CgroupPid::from(pid)).unwrap(); // add child to cgroup
                waitpid(Some(child), None).unwrap();
            }
            ForkResult::Child => {
                std::thread::sleep(Duration::from_millis(100));
                let mut v: Vec<u8> = vec![];
                v.resize(11 * 1024 * 1024, 0);
                exit(0);
            }
        };

5. make sure OOM event received
6. reasource cleanup

Thanks for your comments.

[aa624545345]

@Burning1020 @mxpv Hello, there are two points that i]m not quite sure about this PR:

1. This commit only works on cgroup v1, i think it is necessary to add detection for thel cgroup version of the rutime enviroment.(Impelemention of cgroup v2 canbe supported in a future update).
2. Regarding the mentioned UT tests, should i commit it in another PR?

[Burning1020]

@Burning1020 @mxpv Hello, there are two points that i]m not quite sure about this PR:

1. This commit only works on cgroup v1, i think it is necessary to add detection for thel cgroup version of the rutime enviroment.(Impelemention of cgroup v2 canbe supported in a future update).
2. Regarding the mentioned UT tests, should i commit it in another PR?

1. Yes, I agree, cgroup v2 can be supported in future.
2. It's better to keep UT with your code in the same PR.

[codecov-commenter]

Codecov Report

Attention: 154 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@9ac1f26). Click here to learn what that means.

Files	Patch %	Lines
crates/runc-shim/src/cgroup_memory.rs	3.07%	126 Missing ⚠️
crates/runc-shim/src/task.rs	0.00%	28 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #229   +/-   ##
=======================================
  Coverage        ?   36.82%           
=======================================
  Files           ?       56           
  Lines           ?     5241           
  Branches        ?        0           
=======================================
  Hits            ?     1930           
  Misses          ?     3311           
  Partials        ?        0           

Flag	Coverage Δ
unittests	36.82% <2.53%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aa624545345]

@Burning1020 @mxpv @kzys hello, commit contains ut is pushed.

[aa624545345]

@mxpv @Burning1020 Revised according to the feedback, please review, thanks.

[mxpv]

CI needs some work.
Other than that, looks good.

[Burning1020]

LGTM. Thanks

[aa624545345]

@Burning1020 @mxpv
To fix CI, make the following modifications:

1. Use fmt to format code;
2. Add conditional compilation #[cfg(target_os = "linux")];
3. Handling this error scenario, when registering oom monitor, trying to query the cgroup information of the container's main process, if the container has already exited, get_path_from_cgorup can't find the cgroup information of this container's main process, then this line will return the error directly:

let path_from_cgorup = cgroup_memory::get_path_from_cgorup(resp.pid).await?;

Modify the logic of registring the oom monitor to, during the construction of oom monitor, when error happens, not return, but instead log the error msgs.

#[cfg(target_os = "linux")]
async fn monitor_oom(id: &String, pid: u32, tx: EventSender) -> Result<()> {
    if !is_cgroup2_unified_mode() {
        let path_from_cgorup = cgroup_memory::get_path_from_cgorup(pid).await?;
        let (mount_root, mount_point) =
            cgroup_memory::get_existing_cgroup_mem_path(path_from_cgorup).await?;

        let mem_cgroup_path = mount_point + &mount_root;
        let rx = cgroup_memory::register_memory_event(
            id,
            Path::new(&mem_cgroup_path),
            "memory.oom_control",
        )
        .await
        .map_err(other_error!(e, "register_memory_event failed:"))?;

        run_oom_monitor(rx, id.to_string(), tx);
    }
    Ok(())
}
async fn start(&self, _ctx: &TtrpcContext, req: StartRequest) -> TtrpcResult<StartResponse> {
......
            #[cfg(target_os = "linux")]
            if let Err(e) = monitor_oom(&req.id, resp.pid, self.tx.clone()).await {
                error!("monitor_oom failed: {:?}.", e);
            }
......
}

[aa624545345]

@mxpv @Burning1020 Thx for the review, is cgroup2 version of oom monitor needed?

[mxpv]

@aa624545345 what would be nice to have!