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

cgroup: do not write twice the oom file #513

Closed
wants to merge 1 commit into from

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Jul 4, 2024

if a persist_path was provided, then use only that to write the oom file instead of creating it also in the current working directory.

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.

Thanks, LGTM

I find it weird that the function is named write and that the error says write too but it really just creates an empty file, but this is pre-existing so I don't mind.

if a persist_path was provided, then use only that to write the oom
file instead of creating it also in the current working directory.

Closes: containers#504

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the do-not-write-oom-twice branch from 46c8252 to 8b50b3a Compare July 4, 2024 10:48
@giuseppe
Copy link
Member Author

giuseppe commented Jul 4, 2024

I find it weird that the function is named write and that the error says write too but it really just creates an empty file, but this is pre-existing so I don't mind.

good idea, I've renamed it to create_oom_file

@giuseppe
Copy link
Member Author

giuseppe commented Jul 9, 2024

@haircommander PTAL

@haircommander
Copy link
Collaborator

hmm unfortunately cri-o does use the path at the bundle path to find the oom file https://github.com/cri-o/cri-o/blob/04500243ec0cd775e76bcec1c822aaa3faa11177/internal/oci/runtime_oci.go#L1100

conmon ci is broken again but I'm pretty sure this would fail cri-o ci if we merged as-is.

would it be better for you if we explicitly used the value of bundle_path rather than assuming that's cwd. but still write to both?

@giuseppe
Copy link
Member Author

maybe we can fix it in Podman, and run the conmon process from the bundle path itself, or the persistent dir

@giuseppe
Copy link
Member Author

alternative fix for Podman: containers/podman#23243

@giuseppe giuseppe closed this Jul 10, 2024
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