Merge pull request #5253 from thaJeztah/refactor_oom

sys: refactor OOM utilities

runtime/v1/shim/client/client.go

@@ -182,10 +182,7 @@ func setupOOMScore(shimPid int) error {
 		return errors.Wrap(err, "get daemon OOM score")
 	}
 	shimScore := score + 1
-	if shimScore > sys.OOMScoreAdjMax {
+	if err := sys.AdjustOOMScore(shimPid, shimScore); err != nil {
-		shimScore = sys.OOMScoreAdjMax
-	}
-	if err := sys.SetOOMScore(shimPid, shimScore); err != nil {
 		return errors.Wrap(err, "set shim OOM score")
 	}
 	return nil

runtime/v2/shim/util_unix.go

@@ -46,11 +46,6 @@ func getSysProcAttr() *syscall.SysProcAttr {
 	}
 }
 
-// SetScore sets the oom score for a process
-func SetScore(pid int) error {
-	return sys.SetOOMScore(pid, sys.OOMScoreMaxKillable)
-}
-
 // AdjustOOMScore sets the OOM score for the process to the parents OOM score +1
 // to ensure that they parent has a lower* score than the shim
 // if not already at the maximum OOM Score
@@ -61,10 +56,7 @@ func AdjustOOMScore(pid int) error {
 		return errors.Wrap(err, "get parent OOM score")
 	}
 	shimScore := score + 1
-	if shimScore > sys.OOMScoreAdjMax {
+	if err := sys.AdjustOOMScore(pid, shimScore); err != nil {
-		shimScore = sys.OOMScoreAdjMax
-	}
-	if err := sys.SetOOMScore(pid, shimScore); err != nil {
 		return errors.Wrap(err, "set shim OOM score")
 	}
 	return nil

sys/env.go

@@ -1,33 +0,0 @@
-// +build !windows
-
-/*
-   Copyright The containerd Authors.
-
-   Licensed under the Apache License, Version 2.0 (the "License");
-   you may not use this file except in compliance with the License.
-   You may obtain a copy of the License at
-
-       http://www.apache.org/licenses/LICENSE-2.0
-
-   Unless required by applicable law or agreed to in writing, software
-   distributed under the License is distributed on an "AS IS" BASIS,
-   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-   See the License for the specific language governing permissions and
-   limitations under the License.
-*/
-
-package sys
-
-import "golang.org/x/sys/unix"
-
-// RunningPrivileged returns true if the effective user ID of the
-// calling process is 0
-func RunningPrivileged() bool {
-	return unix.Geteuid() == 0
-}
-
-// RunningUnprivileged returns true if the effective user ID of the
-// calling process is not 0
-func RunningUnprivileged() bool {
-	return !RunningPrivileged()
-}

sys/mount_linux_test.go

@@ -32,7 +32,7 @@ import (
 type fMountatCaseFunc func(t *testing.T, root string)
 
 func TestFMountat(t *testing.T) {
-	if RunningUnprivileged() {
+	if !runningPrivileged() {
 		t.Skip("Needs to be run as root")
 		return
 	}

sys/oom_linux.go

@@ -24,17 +24,32 @@ import (
 	"strings"
 
 	"github.com/containerd/containerd/pkg/userns"
+	"golang.org/x/sys/unix"
 )
 
 const (
-	// OOMScoreMaxKillable is the maximum score keeping the process killable by the oom killer
+	// OOMScoreAdjMin is from OOM_SCORE_ADJ_MIN https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L9
-	OOMScoreMaxKillable = -999
+	OOMScoreAdjMin = -1000
-	// OOMScoreAdjMax is from OOM_SCORE_ADJ_MAX https://github.com/torvalds/linux/blob/master/include/uapi/linux/oom.h
+	// OOMScoreAdjMax is from OOM_SCORE_ADJ_MAX https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L10
 	OOMScoreAdjMax = 1000
 )
 
+// AdjustOOMScore sets the oom score for the provided pid. If the provided score
+// is out of range (-1000 - 1000), it is clipped to the min/max value.
+func AdjustOOMScore(pid, score int) error {
+	if score > OOMScoreAdjMax {
+		score = OOMScoreAdjMax
+	} else if score < OOMScoreAdjMin {
+		score = OOMScoreAdjMin
+	}
+	return SetOOMScore(pid, score)
+}
+
 // SetOOMScore sets the oom score for the provided pid
 func SetOOMScore(pid, score int) error {
+	if score > OOMScoreAdjMax || score < OOMScoreAdjMin {
+		return fmt.Errorf("value out of range (%d): OOM score must be between %d and %d", score, OOMScoreAdjMin, OOMScoreAdjMax)
+	}
 	path := fmt.Sprintf("/proc/%d/oom_score_adj", pid)
 	f, err := os.OpenFile(path, os.O_WRONLY, 0)
 	if err != nil {
@@ -42,7 +57,7 @@ func SetOOMScore(pid, score int) error {
 	}
 	defer f.Close()
 	if _, err = f.WriteString(strconv.Itoa(score)); err != nil {
-		if os.IsPermission(err) && (userns.RunningInUserNS() || RunningUnprivileged()) {
+		if os.IsPermission(err) && (!runningPrivileged() || userns.RunningInUserNS()) {
 			return nil
 		}
 		return err
@@ -59,3 +74,9 @@ func GetOOMScoreAdj(pid int) (int, error) {
 	}
 	return strconv.Atoi(strings.TrimSpace(string(data)))
 }
+
+// runningPrivileged returns true if the effective user ID of the
+// calling process is 0
+func runningPrivileged() bool {
+	return unix.Geteuid() == 0
+}

sys/oom_linux_test.go

@@ -18,52 +18,68 @@ package sys
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"os/exec"
 	"testing"
 	"time"
 
+	"github.com/containerd/containerd/pkg/userns"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 )
 
 func TestSetPositiveOomScoreAdjustment(t *testing.T) {
+	// Setting a *positive* OOM score adjust does not require privileged
 	_, adjustment, err := adjustOom(123)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Error(err)
-		return
-	}
 	assert.Check(t, is.Equal(adjustment, 123))
 }
 
 func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) {
-	if RunningUnprivileged() {
+	if !runningPrivileged() || userns.RunningInUserNS() {
-		t.Skip("Needs to be run as root")
+		t.Skip("requires root and not running in user namespace")
 		return
 	}
 
 	_, adjustment, err := adjustOom(-123)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Error(err)
-		return
-	}
 	assert.Check(t, is.Equal(adjustment, -123))
 }
 
 func TestSetNegativeOomScoreAdjustmentWhenUnprivilegedHasNoEffect(t *testing.T) {
-	if RunningPrivileged() {
+	if runningPrivileged() && !userns.RunningInUserNS() {
-		t.Skip("Needs to be run as non-root")
+		t.Skip("needs to be run as non-root or in user namespace")
 		return
 	}
 
 	initial, adjustment, err := adjustOom(-123)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Error(err)
-		return
-	}
 	assert.Check(t, is.Equal(adjustment, initial))
 }
 
+func TestSetOOMScoreBoundaries(t *testing.T) {
+	err := SetOOMScore(0, OOMScoreAdjMax+1)
+	assert.ErrorContains(t, err, fmt.Sprintf("value out of range (%d): OOM score must be between", OOMScoreAdjMax+1))
+
+	err = SetOOMScore(0, OOMScoreAdjMin-1)
+	assert.ErrorContains(t, err, fmt.Sprintf("value out of range (%d): OOM score must be between", OOMScoreAdjMin-1))
+
+	_, adjustment, err := adjustOom(OOMScoreAdjMax)
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(adjustment, OOMScoreAdjMax))
+
+	score, err := GetOOMScoreAdj(os.Getpid())
+	assert.NilError(t, err)
+	if score == 0 || score == OOMScoreAdjMin {
+		// we won't be able to set the score lower than the parent process,
+		// so only test if parent process does not have a oom-score-adj
+		_, adjustment, err = adjustOom(OOMScoreAdjMin)
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(adjustment, OOMScoreAdjMin))
+	}
+}
+
 func adjustOom(adjustment int) (int, int, error) {
 	cmd := exec.Command("sleep", "100")
 	if err := cmd.Start(); err != nil {

sys/oom_unsupported.go

@@ -25,6 +25,14 @@ const (
 	OOMScoreAdjMax = 0
 )
 
+// AdjustOOMScore sets the oom score for the provided pid. If the provided score
+// is out of range (-1000 - 1000), it is clipped to the min/max value.
+//
+// Not implemented on Windows
+func AdjustOOMScore(pid, score int) error {
+	return nil
+}
+
 // SetOOMScore sets the oom score for the process
 //
 // Not implemented on Windows