From 7dfc605fc6d36dd7327420b8956ac3f9ac71f190 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 27 Jun 2019 11:14:14 -0400 Subject: [PATCH] Set shim OOM scores to +1 containerd daemon score This changes the shim's OOM score from a static max killable of -999 to be +1 of the containerd daemon's score. This should allow the shim's to be killed first in an OOM condition but leave the daemon alone for a bit to help cleanup and manage the containers during this situation. Signed-off-by: Michael Crosby --- container_linux_test.go | 73 ++++++++++++++++++++++++++++++++ runtime/v1/shim/client/client.go | 19 ++++++++- runtime/v2/runc/v1/service.go | 4 +- runtime/v2/runc/v2/service.go | 4 +- runtime/v2/shim/util_unix.go | 16 +++++++ runtime/v2/shim/util_windows.go | 6 +++ 6 files changed, 116 insertions(+), 6 deletions(-) diff --git a/container_linux_test.go b/container_linux_test.go index ac1c8c47c..d8e423b53 100644 --- a/container_linux_test.go +++ b/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 +} diff --git a/runtime/v1/shim/client/client.go b/runtime/v1/shim/client/client.go index 9b4b77c37..80edbd36b 100644 --- a/runtime/v1/shim/client/client.go +++ b/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 { - return nil, nil, errors.Wrap(err, "failed to set OOM Score on shim") + if err = setupOOMScore(cmd.Process.Pid); err != nil { + 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 { diff --git a/runtime/v2/runc/v1/service.go b/runtime/v2/runc/v1/service.go index eab1bfa5b..de379d7d7 100644 --- a/runtime/v2/runc/v1/service.go +++ b/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 { - return "", errors.Wrap(err, "failed to set OOM Score on shim") + if err := shim.AdjustOOMScore(cmd.Process.Pid); err != nil { + return "", errors.Wrap(err, "failed to adjust OOM score for shim") } return address, nil } diff --git a/runtime/v2/runc/v2/service.go b/runtime/v2/runc/v2/service.go index dbb6f4fb0..ace71ace1 100644 --- a/runtime/v2/runc/v2/service.go +++ b/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 { - return "", errors.Wrap(err, "failed to set OOM Score on shim") + if err := shim.AdjustOOMScore(cmd.Process.Pid); err != nil { + return "", errors.Wrap(err, "failed to adjust OOM score for shim") } return address, nil } diff --git a/runtime/v2/shim/util_unix.go b/runtime/v2/shim/util_unix.go index 6b9ae4eea..700c5b36b 100644 --- a/runtime/v2/shim/util_unix.go +++ b/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) diff --git a/runtime/v2/shim/util_windows.go b/runtime/v2/shim/util_windows.go index ef26ebe22..65573612e 100644 --- a/runtime/v2/shim/util_windows.go +++ b/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)