Skip to content
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.

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

Feature: Add oom monitor for cgroupv2. #257

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aa624545345
Copy link
Contributor

Add oom monitor for cgroupv2.

@github-actions github-actions bot added the C-runc-shim Runc shim label Apr 3, 2024
@aa624545345
Copy link
Contributor Author

@kzys @dims @caniszczyk @tianon Hi, cgroup v2 version of oom monitor is pushed, please review, thx!

@aa624545345
Copy link
Contributor Author

aa624545345 commented Apr 7, 2024

#229 Implementation of cgroup v1 oom monitor is here.

@aa624545345
Copy link
Contributor Author

Copy link
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!
I left a few comments.
Also clippy on CI suggests a few refactorings.

let key = key.to_string();

tokio::spawn(async move {
let inotify = memory_event_fd(&cg_dir).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

You can move this before spawn to handle error properly.
And then move to thread.

Copy link
Member

Choose a reason for hiding this comment

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

Also possibly you could extend the function to return a stream of inotify_event events to simplify the logic of this loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, sorry, I don't get you. Are you suggesting that I should move certain parts of this loop into a separate function in order to simplify it? If that's the case, how to make the function returns a stream of inotify_event? Using a channel for that purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mxpv or to modify here as:

fn new_read_inotify_file_function(inotify_file) {
         nread = read(inotify_file)
        // verify events here
        if nread >= size_of::<libc::inotify_event>()
        ....
}
pub async fn register_memory_event_v2() {
    // create inotify file
    inotify_file = ...
    tokio::spawn() {
        loop {
            // mv all verifications of events to a new function
            if new_read_inotify_file_function(inotify_file) {
                            parse_kv_file()
                            .......
            } 
        }
    }
}

None => &0,
};

let oom_kill = match mem_map.get("oom_kill") {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

let Some(oom_kill) = mem_map.get("oom_kill") else {
    return;
}

same for below.

if nread >= size_of::<libc::inotify_event>() {
match parse_kv_file(&cg_dir, "memory.events").await {
Ok(mem_map) => {
let last_oom_kill = match lastoom_map.get(&key) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: try lastoom_map.get(&key).copied().unwrap_or(0)

let val = val.parse::<u32>()?;
map.insert(key.to_string(), val);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

More rusty variation of this:

    let map = file_string
        .lines()
        .filter(|line| line.split_once(" "))
        .map(|(k, v) {
            let val = val.parse::<u32>()?;
            Ok((k.to_string(), val))
        })
        .collect::<Result<HashMap<_, _>>>()?;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-runc-shim Runc shim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants