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

Add custom choom helper to lower crocochrome's OOM score #122

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nadiamoe
Copy link
Member

@nadiamoe nadiamoe commented Jan 30, 2025

This PR follows a very similar approach to #104: Making the oom score of crocochrome low so it becomes very hard to kill than others in the cgroup, namely the chromium process.

Unlike #104, this PR uses a separate binary to perform the actual adjustment, which allows the main process to skip that adjustment in environments where it isn't possible, failing gracefully (logging an error) instead of hard crashing. The reasons for this are detailed in cmd/choom/README.md.

Closes #104
Closes #105

@nadiamoe nadiamoe requested a review from a team as a code owner January 30, 2025 12:56
@nadiamoe nadiamoe changed the title Oom adjust helper Add custom choom helper to lower crocochrome's OOM score Jan 30, 2025
@nadiamoe nadiamoe force-pushed the oom-adjust-helper branch 4 times, most recently from d239a7b to 8924469 Compare January 31, 2025 11:53
Comment on lines +70 to +71
os.Exit(1)
return
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated to this PR, just couldn't resist.

@@ -0,0 +1,15 @@
# `choom`

`choom` is a portable, simplified, not compatible version of [`chroom(1)`](https://man7.org/linux/man-pages/man1/choom.1.html), which allows to change a process' `oom_score_adj` to make it more or less attractive to the kernel's OOM killer.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel you are leaving something out here, and I cannot spot what that might be.

If I look at choom.c it's as simple as it gets. The only difference with the program you are adding is 1) the ability to display a PID's score and adjust value; 2) reporting what the previous adjust value was before changing it. That later point is arguably a useful piece of data.

So... what am I missing?

Copy link
Member Author

@nadiamoe nadiamoe Feb 1, 2025

Choose a reason for hiding this comment

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

There is nothing special on this choom implementation w.r.t. choom.c, they do the same thing. It exists because choom is not installed on alpine by default, and thought that implementing this simple thing was going to be just as easy as apk installing something, keeping the Dockerfile simple, and not having to deal with however alpine wants to run choom (I suspect they will rely on setuid, or just being root, instead of granting the specific capabilities so a regular user can use them).

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no good reason for doing this, then just install the corresponding package providing choom (util-linux-misc) and copy over the file to the final image.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a very strong reason to do it, but I neither see a strong reason to not do it.

Copying the file is tricky: There might be dependencies that need to be brought along, it might need special requirements, it might use setuid at some point without us noticing, etc. It may not be doing these things today, but it may do them tomorrow. The way I see it, procfs is a stable API, the inner workings of that particular choom implementation (which we are coupled with) are not.

I still see the homemade one as the safest option here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copying the file is tricky: There might be dependencies that need to be brought along, it might need special requirements, it might use setuid at some point without us noticing, etc. It may not be doing these things today, but it may do them tomorrow. The way I see it, procfs is a stable API, the inner workings of that particular choom implementation (which we are coupled with) are not.

Uhm...

You are putting forward a hypothetical issue with backwards compatibility for a utility that hasn't seen any significant changes in over 6 years while introducing a custom incompatible version of that utility... seriously?

If this was less trivial (external libraries, complex logic, etc), I might be open to the argument, but at this level of triviality it doesn't make sense. I honestly thought there was something I was missing and that's why I asked.

@nadiamoe nadiamoe requested a review from mem February 1, 2025 13:33
Copy link
Contributor

@mem mem left a comment

Choose a reason for hiding this comment

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

Please remove the custom version of choom and install the corresponding binary from alpine.

@nadiamoe
Copy link
Member Author

nadiamoe commented Feb 6, 2025

@mem I am still unconvinced that there is any advantage on using the alpine binary over the custom one, and I see (minor, maybe hypothetical, but still) disadvantages on using alpine's.

However the more important thing to me is that I chose to do it this way. I went through the effort of understanding the details of this particular approach, and did the relevant testing for it.
If you'd rather have this done differently, and cannot to convince me this way was big problems, that's okay, but I kindly ask you to do it yourself, and do the same for your approach. I do not think it's fair to ask me to shoulder that effort for an approach I do not honestly believe is significantly better.

Feel free to tag me as a reviewer and I'll stamp your approach. If you'd rather not do it, feel free to approve this one.

@nadiamoe nadiamoe requested a review from mem February 6, 2025 11:33
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.

2 participants