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

conmon: do not create oom file under cwd #514

Merged

Conversation

giuseppe
Copy link
Member

instead use the bundle path to create the second (shurgh!) file since this is what CRI-O uses.

Closes: #504

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

one nit, otherwise LGTM

src/cgroup.c Outdated
_cleanup_free_ char *ctr_oom_file_path = g_build_filename(base_path, "oom", NULL);
_cleanup_close_ int ctr_oom_fd = open(ctr_oom_file_path, O_CREAT | O_CLOEXEC, 0666);
if (ctr_oom_fd < 0) {
nwarnf("Failed to write oom file to the %s path", path_type);
Copy link
Member

Choose a reason for hiding this comment

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

If I read such a message using ctr_oom_file_path would make the most sense as a user is likely not knowing where this path would be and if there is a full file path a user could check this directory quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

replaced path_type with base_path itself

@giuseppe giuseppe force-pushed the create-oom-file-to-bundle-path branch from ec25962 to 3e77d22 Compare July 10, 2024 14:51
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe force-pushed the create-oom-file-to-bundle-path branch from 3e77d22 to 77f8e63 Compare July 11, 2024 20:02
@haircommander
Copy link
Collaborator

let's try to get ci running to double check this works with crio #515

@haircommander
Copy link
Collaborator

@giuseppe can you rebase to get test fixes please?

instead use the bundle path to create the second (shurgh!) file since
this is what CRI-O uses.

Closes: containers#504

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the create-oom-file-to-bundle-path branch from 77f8e63 to 959cced Compare July 16, 2024 15:01
@giuseppe
Copy link
Member Author

rebased

@haircommander haircommander merged commit 3bc422c into containers:main Jul 16, 2024
26 of 27 checks passed
@haircommander
Copy link
Collaborator

thanks!

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

Successfully merging this pull request may close these issues.

conmon writes oom file to cwd
3 participants