Skip to content

Change settings.go to ignore user input for OOM killer if running under Cgroupv2 #151

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.

Sign up for GitHub

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

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

Change settings.go to ignore user input for OOM killer if running under Cgroupv2 #151

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion environment/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package environment
import (
"fmt"
"math"
"os"
"strconv"

"github.com/apex/log"
Expand Down Expand Up @@ -108,15 +109,24 @@ func boolPtr(b bool) *bool {
// that Docker understands.
func (l Limits) AsContainerResources() container.Resources {
pids := l.ProcessLimit()

resources := container.Resources{
Memory: l.BoundedMemoryLimit(),
MemoryReservation: l.MemoryLimit * 1024 * 1024,
MemorySwap: l.ConvertedSwap(),
BlkioWeight: l.IoWeight,
OomKillDisable: boolPtr(!l.OOMKiller),
PidsLimit: &pids,
}

// 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)
}
}
Comment on lines +121 to +128
Copy link

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.IsNotExist to 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.

Suggested change
// 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.


// If the CPU Limit is not set, don't send any of these fields through. Providing
// them seems to break some Java services that try to read the available processors.
//
Expand Down