Merge pull request #3380 from crosbymichael/oom-scores

Set shim OOM scores to +1 containerd daemon score

container_linux_test.go

@@ -42,6 +42,7 @@ import (
 	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/runtime/linux/runctypes"
 	"github.com/containerd/containerd/runtime/v2/runc/options"
+	"github.com/containerd/containerd/sys"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
@@ -1731,3 +1732,75 @@ func TestContainerNoSTDIN(t *testing.T) {
 		t.Errorf("expected status 0 from wait but received %d", code)
 	}
 }
+
+func TestShimOOMScore(t *testing.T) {
+	containerdPid := ctrd.cmd.Process.Pid
+	containerdScore, err := sys.GetOOMScoreAdj(containerdPid)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	client, err := newClient(t, address)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.Close()
+
+	var (
+		image       Image
+		ctx, cancel = testContext()
+		id          = t.Name()
+	)
+	defer cancel()
+
+	path := "/containerd/oomshim"
+	cg, err := cgroups.New(cgroups.V1, cgroups.StaticPath(path), &specs.LinuxResources{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cg.Delete()
+
+	image, err = client.GetImage(ctx, testImage)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	container, err := client.NewContainer(ctx, id, WithNewSnapshot(id, image), WithNewSpec(oci.WithImageConfig(image), withProcessArgs("sleep", "30")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer container.Delete(ctx, WithSnapshotCleanup)
+
+	task, err := container.NewTask(ctx, empty(), WithShimCgroup(path))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer task.Delete(ctx)
+
+	statusC, err := task.Wait(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	processes, err := cg.Processes(cgroups.Devices, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	expectedScore := containerdScore + 1
+	// find the shim's pid
+	for _, p := range processes {
+		score, err := sys.GetOOMScoreAdj(p.Pid)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if score != expectedScore {
+			t.Errorf("expected score %d but got %d for shim process", expectedScore, score)
+		}
+	}
+
+	if err := task.Kill(ctx, unix.SIGKILL); err != nil {
+		t.Fatal(err)
+	}
+
+	<-statusC
+}

runtime/v1/shim/client/client.go

@@ -127,8 +127,8 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa
 				"address": address,
 			}).Infof("shim placed in cgroup %s", cgroup)
 		}
-		if err = sys.SetOOMScore(cmd.Process.Pid, sys.OOMScoreMaxKillable); err != nil {
+		if err = setupOOMScore(cmd.Process.Pid); err != nil {
-			return nil, nil, errors.Wrap(err, "failed to set OOM Score on shim")
+			return nil, nil, err
 		}
 		c, clo, err := WithConnect(address, func() {})(ctx, config)
 		if err != nil {
@@ -138,6 +138,21 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa
 	}
 }
 
+// setupOOMScore gets containerd's oom score and adds +1 to it
+// to ensure a shim has a lower* score than the daemons
+func setupOOMScore(shimPid int) error {
+	pid := os.Getpid()
+	score, err := sys.GetOOMScoreAdj(pid)
+	if err != nil {
+		return errors.Wrap(err, "get daemon OOM score")
+	}
+	shimScore := score + 1
+	if err := sys.SetOOMScore(shimPid, shimScore); err != nil {
+		return errors.Wrap(err, "set shim OOM score")
+	}
+	return nil
+}
+
 func newCommand(binary, daemonAddress string, debug bool, config shim.Config, socket *os.File, stdout, stderr io.Writer) (*exec.Cmd, error) {
 	selfExe, err := os.Executable()
 	if err != nil {

runtime/v2/runc/v1/service.go

@@ -190,8 +190,8 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 			}
 		}
 	}
-	if err := shim.SetScore(cmd.Process.Pid); err != nil {
+	if err := shim.AdjustOOMScore(cmd.Process.Pid); err != nil {
-		return "", errors.Wrap(err, "failed to set OOM Score on shim")
+		return "", errors.Wrap(err, "failed to adjust OOM score for shim")
 	}
 	return address, nil
 }

runtime/v2/runc/v2/service.go

@@ -233,8 +233,8 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 			}
 		}
 	}
-	if err := shim.SetScore(cmd.Process.Pid); err != nil {
+	if err := shim.AdjustOOMScore(cmd.Process.Pid); err != nil {
-		return "", errors.Wrap(err, "failed to set OOM Score on shim")
+		return "", errors.Wrap(err, "failed to adjust OOM score for shim")
 	}
 	return address, nil
 }

runtime/v2/shim/util_unix.go

@@ -23,6 +23,7 @@ import (
 	"crypto/sha256"
 	"fmt"
 	"net"
+	"os"
 	"path/filepath"
 	"strings"
 	"syscall"
@@ -46,6 +47,21 @@ 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
+func AdjustOOMScore(pid int) error {
+	parent := os.Getppid()
+	score, err := sys.GetOOMScoreAdj(parent)
+	if err != nil {
+		return errors.Wrap(err, "get parent OOM score")
+	}
+	shimScore := score + 1
+	if err := sys.SetOOMScore(pid, shimScore); err != nil {
+		return errors.Wrap(err, "set shim OOM score")
+	}
+	return nil
+}
+
 // SocketAddress returns an abstract socket address
 func SocketAddress(ctx context.Context, id string) (string, error) {
 	ns, err := namespaces.NamespaceRequired(ctx)

runtime/v2/shim/util_windows.go

@@ -40,6 +40,12 @@ func SetScore(pid int) error {
 	return nil
 }
 
+// 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
+func AdjustOOMScore(pid int) error {
+	return nil
+}
+
 // SocketAddress returns a npipe address
 func SocketAddress(ctx context.Context, id string) (string, error) {
 	ns, err := namespaces.NamespaceRequired(ctx)