Change settings.go to ignore user input for OOM killer if running under Cgroupv2#151
Change settings.go to ignore user input for OOM killer if running under Cgroupv2#151m3ow-dev wants to merge 1 commit intopelican-dev:mainfrom
Conversation
WalkthroughModified OOM killer configuration logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
environment/settings.go(2 hunks)
🔇 Additional comments (2)
environment/settings.go (2)
6-6: LGTM!The
ospackage import is necessary for the cgroup version detection logic.
112-112: LGTM!The blank line improves readability by separating the variable declaration from the struct initialization.
| // If the user wants to disable the OOM killer, we need to handle it differently | ||
| // depending on the cgroup version. | ||
| if !l.OOMKiller { | ||
| if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil { | ||
| } else { | ||
| resources.OomKillDisable = boolPtr(true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling to distinguish file-not-found from other errors.
The current implementation treats any error from os.Stat as an indication of cgroup v1, which could lead to incorrect behavior if the error is due to permission issues, I/O errors, or other filesystem problems rather than the file not existing.
Apply this diff to properly handle errors:
// If the user wants to disable the OOM killer, we need to handle it differently
-// depending on the cgroup version.
+// depending on the cgroup version. On cgroup v2, disabling the OOM killer is not
+// supported (see containers/crun#1416, pterodactyl/panel#4928), so we ignore the
+// user setting and keep it enabled.
if !l.OOMKiller {
- if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil {
- } else {
+ if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); os.IsNotExist(err) {
+ // cgroup v1: honor user preference to disable OOM killer
+ log.Debug("detected cgroup v1, disabling OOM killer as requested")
resources.OomKillDisable = boolPtr(true)
+ } else if err != nil {
+ // Unexpected error (permissions, I/O, etc.) - log and default to safe behavior
+ log.WithError(err).Warn("failed to detect cgroup version, keeping OOM killer enabled")
+ } else {
+ // cgroup v2: ignore user preference and keep OOM killer enabled
+ log.Debug("detected cgroup v2, ignoring OOM killer disable request")
}
}This approach:
- Uses
os.IsNotExistto specifically check for file-not-found - Adds logging for observability and debugging
- Handles unexpected errors safely by keeping OOM killer enabled
- Improves the comment to explain the rationale and reference related issues
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the user wants to disable the OOM killer, we need to handle it differently | |
| // depending on the cgroup version. | |
| if !l.OOMKiller { | |
| if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil { | |
| } else { | |
| resources.OomKillDisable = boolPtr(true) | |
| } | |
| } | |
| // If the user wants to disable the OOM killer, we need to handle it differently | |
| // depending on the cgroup version. On cgroup v2, disabling the OOM killer is not | |
| // supported (see containers/crun#1416, pterodactyl/panel#4928), so we ignore the | |
| // user setting and keep it enabled. | |
| if !l.OOMKiller { | |
| if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); os.IsNotExist(err) { | |
| // cgroup v1: honor user preference to disable OOM killer | |
| log.Debug("detected cgroup v1, disabling OOM killer as requested") | |
| resources.OomKillDisable = boolPtr(true) | |
| } else if err != nil { | |
| // Unexpected error (permissions, I/O, etc.) - log and default to safe behavior | |
| log.WithError(err).Warn("failed to detect cgroup version, keeping OOM killer enabled") | |
| } else { | |
| // cgroup v2: ignore user preference and keep OOM killer enabled | |
| log.Debug("detected cgroup v2, ignoring OOM killer disable request") | |
| } | |
| } |
🤖 Prompt for AI Agents
In environment/settings.go around lines 121 to 128, the os.Stat error is
currently treated as "file missing" for cgroup detection which conflates
missing-file with other errors; update the logic to call os.Stat and then use
os.IsNotExist(err) to detect cgroup v1 explicitly, set resources.OomKillDisable
= boolPtr(true) only when the file does not exist, and in the else branch log
unexpected errors (permission/IO) and leave OOM killer enabled (do not set
OomKillDisable) so we fail safe; also expand the comment to explain the
rationale and reference related issues for future debugging.
There was a problem hiding this comment.
I think this would be the best way check if the file doesn't exist instead of any error.
if err == os.ErrNotExist {
// cgroup v1
}There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go os.IsNotExist vs os.ErrNotExist direct comparison best practice
💡 Result:
Best practice (Go 1.13+): prefer errors.Is(err, fs.ErrNotExist).
- The
osdocs explicitly note thatos.IsNotExistpredateserrors.Isand say: “New code should useerrors.Is(err, fs.ErrNotExist).” [1] os.ErrNotExistis just an alias offs.ErrNotExist(ErrNotExist = fs.ErrNotExist). [2]
Avoid direct comparison (err == os.ErrNotExist or err == fs.ErrNotExist) because filesystem calls typically return wrapped errors like *os.PathError (and may wrap syscall errors), so equality often won’t match even when the underlying condition is “does not exist”. Using errors.Is (or os.IsNotExist) handles that. [1][3]
import (
"errors"
"io/fs"
"os"
)
_, err := os.Stat(path)
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
// missing
} else {
// other error
}
}When is os.IsNotExist OK? For older codebases or when you want the legacy helper; it still works, but errors.Is(..., fs.ErrNotExist) is the modern, general approach. [1]
Sources: [1] https://golang.bg/pkg/os/ (IsNotExist docs) · [2] https://golang.bg/src/os/error.go (ErrNotExist alias) · [3] https://stackoverflow.com/questions/33398110/in-golang-os-openfile-doesnt-return-os-errnotexist-if-file-does-not-exist
@parkervcp, I appreciate the feedback! However, direct comparison with err == os.ErrNotExist unfortunately won't work reliably in this case. Here's why:
os.Stat() returns wrapped errors (typically *os.PathError), not the raw os.ErrNotExist, so direct equality comparison will always be false even when the file doesn't exist.
Modern best practice (Go 1.13+):
if errors.Is(err, fs.ErrNotExist) {
// cgroup v1: file doesn't exist
resources.OomKillDisable = boolPtr(true)
}Also acceptable (legacy helper):
if os.IsNotExist(err) {
// cgroup v1: file doesn't exist
resources.OomKillDisable = boolPtr(true)
}Both errors.Is() and os.IsNotExist() properly unwrap the error to check the underlying condition, while direct comparison does not. The Go documentation for os.IsNotExist explicitly recommends using errors.Is(err, fs.ErrNotExist) for new code.
Would you prefer errors.Is(err, fs.ErrNotExist) (modern approach) or os.IsNotExist(err) (simpler, still works)?
| // If the user wants to disable the OOM killer, we need to handle it differently | ||
| // depending on the cgroup version. | ||
| if !l.OOMKiller { | ||
| if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil { | ||
| } else { | ||
| resources.OomKillDisable = boolPtr(true) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this would be the best way check if the file doesn't exist instead of any error.
if err == os.ErrNotExist {
// cgroup v1
}2 participants
If host is running Cgroupv2, ignore user input and keep OOM killer enabled at all times.
related:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.