From 6124ee8afbf52cb9832a554e7008fdc3d638c7f2 Mon Sep 17 00:00:00 2001 From: Yorick Gersie <6005868+ygersie@users.noreply.github.com> Date: Thu, 4 Apr 2024 22:54:10 +0200 Subject: [PATCH] cpuset fixer: use correct cgroup path for updates (#20276) * cpuset fixer: use correct cgroup path for updates fixes #20275 * docker: flatten switch statement and add test cases * cl: add cl --------- Co-authored-by: Seth Hoenig --- .changelog/20276.txt | 3 ++ drivers/docker/driver.go | 12 ++++++++ drivers/docker/handle.go | 40 +++++++++++++++++---------- drivers/docker/handle_test.go | 52 +++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 15 deletions(-) create mode 100644 .changelog/20276.txt create mode 100644 drivers/docker/handle_test.go diff --git a/.changelog/20276.txt b/.changelog/20276.txt new file mode 100644 index 000000000..ca635a61b --- /dev/null +++ b/.changelog/20276.txt @@ -0,0 +1,3 @@ +```release-note:bug +docker: Fixed a bug where cpuset would not be updated on cgroup v2 systems using cgroupfs +``` diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index ddcd38557..80d4c969c 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -230,6 +230,11 @@ func (d *Driver) RecoverTask(handle *drivers.TaskHandle) error { return fmt.Errorf("failed to get docker client: %w", err) } + dockerInfo, err := dockerClient.Info() + if err != nil { + return fmt.Errorf("failed to fetch docker daemon info: %v", err) + } + infinityClient, err := d.getInfinityClient() if err != nil { return fmt.Errorf("failed to get docker long operations client: %w", err) @@ -244,6 +249,7 @@ func (d *Driver) RecoverTask(handle *drivers.TaskHandle) error { h := &taskHandle{ dockerClient: dockerClient, + dockerCGroupDriver: dockerInfo.CgroupDriver, infinityClient: infinityClient, logger: d.logger.With("container_id", container.ID), task: handle.Config, @@ -323,6 +329,11 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive return nil, nil, fmt.Errorf("Failed to create docker client: %v", err) } + dockerInfo, err := dockerClient.Info() + if err != nil { + return nil, nil, fmt.Errorf("failed to fetch docker daemon info: %v", err) + } + // and also the long operations client infinityClient, err := d.getInfinityClient() if err != nil { @@ -433,6 +444,7 @@ CREATE: // Return a driver handle h := &taskHandle{ dockerClient: dockerClient, + dockerCGroupDriver: dockerInfo.CgroupDriver, infinityClient: infinityClient, dlogger: dlogger, dloggerPluginClient: pluginClient, diff --git a/drivers/docker/handle.go b/drivers/docker/handle.go index 3096796c7..1b4af95ed 100644 --- a/drivers/docker/handle.go +++ b/drivers/docker/handle.go @@ -29,6 +29,8 @@ type taskHandle struct { // for all calls that aren't Wait() or Stop() (and their variations). dockerClient *docker.Client + dockerCGroupDriver string + // infinityClient is useful for // - the Wait docker API call(s) (no limit on container lifetime) // - the Stop docker API call(s) (context with task kill_timeout required) @@ -254,27 +256,35 @@ func (h *taskHandle) startCpusetFixer() { return } - cgroup := h.containerCgroup - if cgroup == "" { - // The api does not actually set this value, so we are left to compute it ourselves. - // Luckily this is documented, - // https://docs.docker.com/config/containers/runmetrics/#find-the-cgroup-for-a-given-container - switch cgroupslib.GetMode() { - case cgroupslib.CG1: - cgroup = "/sys/fs/cgroup/cpuset/docker/" + h.containerID - default: - // systemd driver; not sure if we need to consider cgroupfs driver - cgroup = "/sys/fs/cgroup/system.slice/docker-" + h.containerID + ".scope" - } - } - go (&cpuset{ doneCh: h.doneCh, source: h.task.Resources.LinuxResources.CpusetCgroupPath, - destination: cgroup, + destination: h.dockerCgroup(), }).watch() } +// dockerCgroup returns the path to the cgroup docker will use for the container. +// +// The api does not provide this value, so we are left to compute it ourselves. +// +// https://docs.docker.com/config/containers/runmetrics/#find-the-cgroup-for-a-given-container +func (h *taskHandle) dockerCgroup() string { + cgroup := h.containerCgroup + if cgroup == "" { + mode := cgroupslib.GetMode() + usingCgroupfs := h.dockerCGroupDriver == "cgroupfs" + switch { + case mode == cgroupslib.CG1: + cgroup = "/sys/fs/cgroup/cpuset/docker/" + h.containerID + case mode == cgroupslib.CG2 && usingCgroupfs: + cgroup = "/sys/fs/cgroup/docker/" + h.containerID + default: + cgroup = "/sys/fs/cgroup/system.slice/docker-" + h.containerID + ".scope" + } + } + return cgroup +} + func (h *taskHandle) run() { defer h.shutdownLogger() diff --git a/drivers/docker/handle_test.go b/drivers/docker/handle_test.go new file mode 100644 index 000000000..adeb94032 --- /dev/null +++ b/drivers/docker/handle_test.go @@ -0,0 +1,52 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package docker + +import ( + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/client/testutil" + "github.com/shoenig/test/must" +) + +func Test_dockerCgroup(t *testing.T) { + testutil.RequireRoot(t) + + ci.Parallel(t) + + t.Run("preset", func(t *testing.T) { + testutil.CgroupsCompatible(t) + + h := new(taskHandle) + h.containerCgroup = "/some/preset" + result := h.dockerCgroup() + must.Eq(t, "/some/preset", result) + }) + + t.Run("v1", func(t *testing.T) { + testutil.CgroupsCompatibleV1(t) + h := new(taskHandle) + h.containerID = "abc123" + result := h.dockerCgroup() + must.Eq(t, "/sys/fs/cgroup/cpuset/docker/abc123", result) + }) + + t.Run("v2-systemd", func(t *testing.T) { + testutil.CgroupsCompatibleV2(t) + h := new(taskHandle) + h.containerID = "abc123" + result := h.dockerCgroup() + must.Eq(t, "/sys/fs/cgroup/system.slice/docker-abc123.scope", result) + }) + + t.Run("v2-cgroupfs", func(t *testing.T) { + testutil.CgroupsCompatibleV2(t) + h := new(taskHandle) + h.containerID = "abc123" + h.dockerCGroupDriver = "cgroupfs" + result := h.dockerCgroup() + must.Eq(t, "/sys/fs/cgroup/docker/abc123", result) + }) +}