From 321504664f3580f0f7f16d8538519b32a0601420 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 21:58:50 -0500 Subject: [PATCH 01/17] cgroups: Rename mockCgroup to mockCgroupV1 We will shortly be creating an interface to cover the mock cgroups for v1 and v2. Rename `mockCgroup` to `mockCgroupV1` to clear the way. --- internal/cgroups/mock_linux_test.go | 12 ++++++------ internal/cgroups/v1_linux_test.go | 28 ++++++++++++++-------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/cgroups/mock_linux_test.go b/internal/cgroups/mock_linux_test.go index ac54418a0b..bd546545c3 100644 --- a/internal/cgroups/mock_linux_test.go +++ b/internal/cgroups/mock_linux_test.go @@ -35,12 +35,12 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" ) -type mockCgroup struct { +type mockCgroupV1 struct { root string subsystems []cgroup1.Subsystem } -func newMock(t *testing.T) *mockCgroup { +func newMockV1(t *testing.T) *mockCgroupV1 { t.Helper() root := testhelper.TempDir(t) @@ -52,7 +52,7 @@ func newMock(t *testing.T) *mockCgroup { require.NoError(t, os.MkdirAll(filepath.Join(root, string(s.Name())), perm.SharedDir)) } - return &mockCgroup{ + return &mockCgroupV1{ root: root, subsystems: subsystems, } @@ -93,7 +93,7 @@ nr_throttled 20 throttled_time 1000000`, } -func (m *mockCgroup) setupMockCgroupFiles( +func (m *mockCgroupV1) setupMockCgroupFiles( t *testing.T, manager *CGroupManager, shards []uint, @@ -142,11 +142,11 @@ func (m *mockCgroup) setupMockCgroupFiles( } } -func (m *mockCgroup) newCgroupManager(cfg cgroupscfg.Config, logger log.Logger, pid int) *CGroupManager { +func (m *mockCgroupV1) newCgroupManager(cfg cgroupscfg.Config, logger log.Logger, pid int) *CGroupManager { return newCgroupManagerWithMode(cfg, logger, pid, cgrps.Legacy) } -func (m *mockCgroup) pruneOldCgroups(cfg cgroupscfg.Config, logger log.Logger) { +func (m *mockCgroupV1) pruneOldCgroups(cfg cgroupscfg.Config, logger log.Logger) { pruneOldCgroupsWithMode(cfg, logger, cgrps.Legacy) } diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 38d81293a4..2f240dfbb2 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -95,7 +95,7 @@ func TestSetup_ParentCgroups(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - mock := newMock(t) + mock := newMockV1(t) pid := 1 tt.cfg.HierarchyRoot = "gitaly" tt.cfg.Mountpoint = mock.root @@ -174,7 +174,7 @@ func TestSetup_RepoCgroups(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - mock := newMock(t) + mock := newMockV1(t) pid := 1 cfg := defaultCgroupsConfig() @@ -249,7 +249,7 @@ func TestSetup_RepoCgroups(t *testing.T) { } func TestAddCommand(t *testing.T) { - mock := newMock(t) + mock := newMockV1(t) config := defaultCgroupsConfig() config.Repositories.Count = 10 @@ -307,7 +307,7 @@ func TestAddCommand(t *testing.T) { } func TestCleanup(t *testing.T) { - mock := newMock(t) + mock := newMockV1(t) pid := 1 cfg := defaultCgroupsConfig() @@ -371,7 +371,7 @@ gitaly_cgroup_cpu_cfs_throttled_seconds_total{path="%s"} 0.001 tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - mock := newMock(t) + mock := newMockV1(t) config := defaultCgroupsConfig() config.Repositories.Count = 1 @@ -429,7 +429,7 @@ func TestPruneOldCgroups(t *testing.T) { cfg cgroups.Config expectedPruned bool // setup returns a pid - setup func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int + setup func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int }{ { desc: "process belongs to another user", @@ -441,7 +441,7 @@ func TestPruneOldCgroups(t *testing.T) { CPUShares: 1024, }, }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int { + setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { pid := 1 cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) require.NoError(t, cgroupManager.Setup()) @@ -460,7 +460,7 @@ func TestPruneOldCgroups(t *testing.T) { CPUShares: 1024, }, }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int { + setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { pid := 1 cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) require.NoError(t, cgroupManager.Setup()) @@ -478,7 +478,7 @@ func TestPruneOldCgroups(t *testing.T) { CPUShares: 1024, }, }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int { + setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { cmd := exec.Command("ls") require.NoError(t, cmd.Run()) pid := cmd.Process.Pid @@ -508,7 +508,7 @@ func TestPruneOldCgroups(t *testing.T) { CPUShares: 1024, }, }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int { + setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { pid := os.Getpid() cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) @@ -528,7 +528,7 @@ func TestPruneOldCgroups(t *testing.T) { CPUShares: 1024, }, }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroup) int { + setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), 0) require.NoError(t, cgroupManager.Setup()) @@ -540,7 +540,7 @@ func TestPruneOldCgroups(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - mock := newMock(t) + mock := newMockV1(t) tc.cfg.Mountpoint = mock.root memoryRoot := filepath.Join( @@ -641,7 +641,7 @@ active_file 135000000`}, }, } { t.Run(tc.desc, func(t *testing.T) { - mock := newMock(t) + mock := newMockV1(t) config := defaultCgroupsConfig() config.Repositories.Count = 1 @@ -661,7 +661,7 @@ active_file 135000000`}, } } -func requireShardsV1(t *testing.T, mock *mockCgroup, mgr *CGroupManager, pid int, expectedShards ...uint) { +func requireShardsV1(t *testing.T, mock *mockCgroupV1, mgr *CGroupManager, pid int, expectedShards ...uint) { t.Helper() for shard := uint(0); shard < mgr.cfg.Repositories.Count; shard++ { -- GitLab From 51cfb034224b8104a9646c8ad9ec36222c116aa2 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:00:26 -0500 Subject: [PATCH 02/17] cgroups: Introduce mockCgroup interface Introduce a `mockCgroup` interface to allow tests to create a v1 or v2 mock depending on a test parameter, rather than duplicating the entire test. This requires accessing the `root` path via the new `rootPath()` method, and a convenience method for calculating the mock's repository cgroup paths is also added. --- internal/cgroups/mock_linux_test.go | 64 +++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/internal/cgroups/mock_linux_test.go b/internal/cgroups/mock_linux_test.go index bd546545c3..16f7c19004 100644 --- a/internal/cgroups/mock_linux_test.go +++ b/internal/cgroups/mock_linux_test.go @@ -35,6 +35,35 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" ) +type mockCgroup interface { + setupMockCgroupFiles( + t *testing.T, + manager *CGroupManager, + shards []uint, + inputContent ...mockCgroupFile, + ) + newCgroupManager(cfg cgroupscfg.Config, logger log.Logger, pid int) *CGroupManager + pruneOldCgroups(cfg cgroupscfg.Config, logger log.Logger) + // rootPath returns the mock's root directory. + rootPath() string + // repoPaths returns the full disk path for each subcomponent, e.g. memory, cpu, of + // a repository cgroup. On v2 this is a single path. + repoPaths(pid int, repoID uint) []string + // version returns the cgroup version number, 1 or 2. + version() int +} + +func newMock(t *testing.T, version int) mockCgroup { + var mock mockCgroup + if version == 1 { + mock = newMockV1(t) + } else { + mock = newMockV2(t) + } + + return mock +} + type mockCgroupV1 struct { root string subsystems []cgroup1.Subsystem @@ -150,6 +179,26 @@ func (m *mockCgroupV1) pruneOldCgroups(cfg cgroupscfg.Config, logger log.Logger) pruneOldCgroupsWithMode(cfg, logger, cgrps.Legacy) } +func (m *mockCgroupV1) rootPath() string { + return m.root +} + +func (m *mockCgroupV1) repoPaths(pid int, repoID uint) []string { + paths := make([]string, 0, len(m.subsystems)) + + for _, s := range m.subsystems { + path := filepath.Join(m.root, string(s.Name()), "gitaly", + fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", repoID)) + paths = append(paths, path) + } + + return paths +} + +func (m *mockCgroupV1) version() int { + return 1 +} + type mockCgroupV2 struct { root string } @@ -221,3 +270,18 @@ func (m *mockCgroupV2) newCgroupManager(cfg cgroupscfg.Config, logger log.Logger func (m *mockCgroupV2) pruneOldCgroups(cfg cgroupscfg.Config, logger log.Logger) { pruneOldCgroupsWithMode(cfg, logger, cgrps.Unified) } + +func (m *mockCgroupV2) rootPath() string { + return m.root +} + +func (m *mockCgroupV2) repoPaths(pid int, repoID uint) []string { + path := filepath.Join(m.root, "gitaly", + fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", repoID)) + + return []string{path} +} + +func (m *mockCgroupV2) version() int { + return 2 +} -- GitLab From 0a07f3e3d4b5e24e5525d5f4c03919ad032e811a Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 21:45:48 -0500 Subject: [PATCH 03/17] cgroups: Unify tests for TestNewManager Rename `TestNewManager` to `TestNewManagerNoop` so that we can use the more generic name for our combined test. Unify the separate manager tests for v1 and v2 cgroups into a single `TestNewManager` test. --- internal/cgroups/cgroups_linux_test.go | 2 +- internal/cgroups/handler_linux_test.go | 25 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 12 ------------ internal/cgroups/v2_linux_test.go | 8 -------- 4 files changed, 26 insertions(+), 21 deletions(-) create mode 100644 internal/cgroups/handler_linux_test.go diff --git a/internal/cgroups/cgroups_linux_test.go b/internal/cgroups/cgroups_linux_test.go index c3e7e143d7..80a307a941 100644 --- a/internal/cgroups/cgroups_linux_test.go +++ b/internal/cgroups/cgroups_linux_test.go @@ -10,6 +10,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" ) -func TestNewManager(t *testing.T) { +func TestNewManagerNoop(t *testing.T) { require.IsType(t, &NoopManager{}, NewManager(cgroups.Config{}, testhelper.SharedLogger(t), 1)) } diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go new file mode 100644 index 0000000000..12ac52b4ec --- /dev/null +++ b/internal/cgroups/handler_linux_test.go @@ -0,0 +1,25 @@ +//go:build linux + +package cgroups + +import ( + "testing" + + cgrps "github.com/containerd/cgroups/v3" + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" +) + +func TestNewManager(t *testing.T) { + cfg := cgroups.Config{Repositories: cgroups.Repositories{Count: 10}} + + manager := newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Legacy) + require.IsType(t, &cgroupV1Handler{}, manager.handler) + manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Hybrid) + require.IsType(t, &cgroupV1Handler{}, manager.handler) + manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unified) + require.IsType(t, &cgroupV2Handler{}, manager.handler) + manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unavailable) + require.Nil(t, manager) +} diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 2f240dfbb2..73be99f9d5 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -12,7 +12,6 @@ import ( "strings" "testing" - cgrps "github.com/containerd/cgroups/v3" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,17 +33,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestNewManagerV1(t *testing.T) { - cfg := cgroups.Config{Repositories: cgroups.Repositories{Count: 10}} - - manager := newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Legacy) - require.IsType(t, &cgroupV1Handler{}, manager.handler) - manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Hybrid) - require.IsType(t, &cgroupV1Handler{}, manager.handler) - manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unavailable) - require.Nil(t, manager) -} - func TestSetup_ParentCgroups(t *testing.T) { tests := []struct { name string diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index e6a788b4ea..88004c1fe6 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -12,7 +12,6 @@ import ( "strings" "testing" - cgrps "github.com/containerd/cgroups/v3" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,13 +33,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestNewManagerV2(t *testing.T) { - cfg := cgroups.Config{Repositories: cgroups.Repositories{Count: 10}} - - manager := newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unified) - require.IsType(t, &cgroupV2Handler{}, manager.handler) -} - func TestSetup_ParentCgroupsV2(t *testing.T) { tests := []struct { name string -- GitLab From d7d074c8800fd1ec26af2c06c0cc0dbb332d18f6 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:07:45 -0500 Subject: [PATCH 04/17] cgroups: Unify TestSetup_ParentCgroups Coalesce `TestSetup_ParentCgroups()` and `TestSetup_ParentCgroupsV2()` into a single test that handles both v1 and v2 cgroups. Note that the `requireCgroupComponents()` helper introduced slightly changes the conditions checked versus the v1 test. The v2 manager requires that `setupMockCgroupFiles()` be executed before running `Setup()`. The v1 test used `requireCgroup()`, which would verify that a file was _not_ present, which is no longer be the case now that we have setup the cgroup files. We switch to `requireCgroupWithInt()` to avoid this failure. --- internal/cgroups/handler_linux_test.go | 139 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 83 --------------- internal/cgroups/v2_linux_test.go | 78 -------------- 3 files changed, 139 insertions(+), 161 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index 12ac52b4ec..ab22af6f1c 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -3,6 +3,9 @@ package cgroups import ( + "fmt" + "path/filepath" + "strconv" "testing" cgrps "github.com/containerd/cgroups/v3" @@ -23,3 +26,139 @@ func TestNewManager(t *testing.T) { manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unavailable) require.Nil(t, manager) } + +type expectedCgroup struct { + wantMemoryBytes int + wantCPUShares int + wantCPUQuotaUs int + wantCFSPeriod int + wantCPUWeight int + wantCPUMax string +} + +func TestSetup_ParentCgroups(t *testing.T) { + tests := []struct { + name string + cfg cgroups.Config + expectedV1 expectedCgroup + expectedV2 expectedCgroup + }{ + { + name: "all config specified", + cfg: cgroups.Config{ + MemoryBytes: 102400, + CPUShares: 256, + CPUQuotaUs: 200, + }, + expectedV1: expectedCgroup{ + wantMemoryBytes: 102400, + wantCPUShares: 256, + wantCPUQuotaUs: 200, + wantCFSPeriod: int(cfsPeriodUs), + }, + expectedV2: expectedCgroup{ + wantMemoryBytes: 102400, + wantCPUWeight: 256, + wantCPUMax: "200 100000", + }, + }, + { + name: "only memory limit set", + cfg: cgroups.Config{ + MemoryBytes: 102400, + }, + expectedV1: expectedCgroup{ + wantMemoryBytes: 102400, + }, + expectedV2: expectedCgroup{ + wantMemoryBytes: 102400, + }, + }, + { + name: "only cpu shares set", + cfg: cgroups.Config{ + CPUShares: 512, + }, + expectedV1: expectedCgroup{ + wantCPUShares: 512, + }, + expectedV2: expectedCgroup{ + wantCPUWeight: 512, + }, + }, + { + name: "only cpu quota set", + cfg: cgroups.Config{ + CPUQuotaUs: 200, + }, + expectedV1: expectedCgroup{ + wantCPUQuotaUs: 200, + wantCFSPeriod: int(cfsPeriodUs), + }, + expectedV2: expectedCgroup{ + wantCPUMax: "200 100000", + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + for _, version := range []int{1, 2} { + version := version + t.Run("cgroups-v"+strconv.Itoa(version), func(t *testing.T) { + t.Parallel() + + mock := newMock(t, version) + + pid := 1 + + cfg := tt.cfg + cfg.HierarchyRoot = "gitaly" + cfg.Mountpoint = mock.rootPath() + + manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, manager, []uint{}) + + require.False(t, manager.Ready()) + require.NoError(t, manager.Setup()) + require.True(t, manager.Ready()) + + cgroupPath := filepath.Join("gitaly", fmt.Sprintf("gitaly-%d", pid)) + if version == 1 { + requireCgroupComponents(t, version, mock.rootPath(), cgroupPath, tt.expectedV1) + } else { + requireCgroupComponents(t, version, mock.rootPath(), cgroupPath, tt.expectedV2) + } + }) + } + }) + } +} + +func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { + t.Helper() + + if version == 1 { + memoryLimitPath := filepath.Join(root, "memory", cgroupPath, "memory.limit_in_bytes") + requireCgroupWithInt(t, memoryLimitPath, expected.wantMemoryBytes) + + cpuSharesPath := filepath.Join(root, "cpu", cgroupPath, "cpu.shares") + requireCgroupWithInt(t, cpuSharesPath, expected.wantCPUShares) + + cpuCFSQuotaPath := filepath.Join(root, "cpu", cgroupPath, "cpu.cfs_quota_us") + requireCgroupWithInt(t, cpuCFSQuotaPath, expected.wantCPUQuotaUs) + + cpuCFSPeriodPath := filepath.Join(root, "cpu", cgroupPath, "cpu.cfs_period_us") + requireCgroupWithInt(t, cpuCFSPeriodPath, expected.wantCFSPeriod) + } else { + memoryMaxPath := filepath.Join(root, cgroupPath, "memory.max") + requireCgroupWithInt(t, memoryMaxPath, expected.wantMemoryBytes) + + cpuWeightPath := filepath.Join(root, cgroupPath, "cpu.weight") + requireCgroupWithInt(t, cpuWeightPath, calculateWantCPUWeight(expected.wantCPUWeight)) + + cpuMaxPath := filepath.Join(root, cgroupPath, "cpu.max") + requireCgroupWithString(t, cpuMaxPath, expected.wantCPUMax) + } +} diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 73be99f9d5..11ae09209a 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -33,89 +33,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestSetup_ParentCgroups(t *testing.T) { - tests := []struct { - name string - cfg cgroups.Config - wantMemoryBytes int - wantCPUShares int - wantCPUQuotaUs int - wantCFSPeriod int - }{ - { - name: "all config specified", - cfg: cgroups.Config{ - MemoryBytes: 102400, - CPUShares: 256, - CPUQuotaUs: 200, - }, - wantMemoryBytes: 102400, - wantCPUShares: 256, - wantCPUQuotaUs: 200, - wantCFSPeriod: int(cfsPeriodUs), - }, - { - name: "only memory limit set", - cfg: cgroups.Config{ - MemoryBytes: 102400, - }, - wantMemoryBytes: 102400, - }, - { - name: "only cpu shares set", - cfg: cgroups.Config{ - CPUShares: 512, - }, - wantCPUShares: 512, - }, - { - name: "only cpu quota set", - cfg: cgroups.Config{ - CPUQuotaUs: 200, - }, - wantCPUQuotaUs: 200, - wantCFSPeriod: int(cfsPeriodUs), - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - mock := newMockV1(t) - pid := 1 - tt.cfg.HierarchyRoot = "gitaly" - tt.cfg.Mountpoint = mock.root - - v1Manager := mock.newCgroupManager(tt.cfg, testhelper.SharedLogger(t), pid) - require.False(t, v1Manager.Ready()) - require.NoError(t, v1Manager.Setup()) - require.True(t, v1Manager.Ready()) - - memoryLimitPath := filepath.Join( - mock.root, "memory", "gitaly", fmt.Sprintf("gitaly-%d", pid), "memory.limit_in_bytes", - ) - requireCgroup(t, memoryLimitPath, tt.wantMemoryBytes) - - cpuSharesPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), "cpu.shares", - ) - requireCgroup(t, cpuSharesPath, tt.wantCPUShares) - - cpuCFSQuotaPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), "cpu.cfs_quota_us", - ) - requireCgroup(t, cpuCFSQuotaPath, tt.wantCPUQuotaUs) - - cpuCFSPeriodPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), "cpu.cfs_period_us", - ) - requireCgroup(t, cpuCFSPeriodPath, tt.wantCFSPeriod) - }) - } -} - func TestSetup_RepoCgroups(t *testing.T) { tests := []struct { name string diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index 88004c1fe6..5bbd626698 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -33,84 +33,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestSetup_ParentCgroupsV2(t *testing.T) { - tests := []struct { - name string - cfg cgroups.Config - wantMemoryBytes int - wantCPUWeight int - wantCPUMax string - }{ - { - name: "all config specified", - cfg: cgroups.Config{ - MemoryBytes: 102400, - CPUShares: 256, - CPUQuotaUs: 2000, - }, - wantMemoryBytes: 102400, - wantCPUWeight: 256, - wantCPUMax: "2000 100000", - }, - { - name: "only memory limit set", - cfg: cgroups.Config{ - MemoryBytes: 102400, - }, - wantMemoryBytes: 102400, - }, - { - name: "only cpu shares set", - cfg: cgroups.Config{ - CPUShares: 512, - }, - wantCPUWeight: 512, - }, - { - name: "only cpu quota set", - cfg: cgroups.Config{ - CPUQuotaUs: 2000, - }, - wantCPUMax: "2000 100000", - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - mock := newMockV2(t) - - pid := 1 - tt.cfg.HierarchyRoot = "gitaly" - tt.cfg.Mountpoint = mock.root - - v2Manager := mock.newCgroupManager(tt.cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, v2Manager, []uint{}) - - require.False(t, v2Manager.Ready()) - require.NoError(t, v2Manager.Setup()) - require.True(t, v2Manager.Ready()) - - memoryMaxPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), "memory.max", - ) - requireCgroupWithInt(t, memoryMaxPath, tt.wantMemoryBytes) - - cpuWeightPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), "cpu.weight", - ) - requireCgroupWithInt(t, cpuWeightPath, calculateWantCPUWeight(tt.wantCPUWeight)) - - cpuMaxPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), "cpu.max", - ) - requireCgroupWithString(t, cpuMaxPath, tt.wantCPUMax) - }) - } -} - func TestSetup_RepoCgroupsV2(t *testing.T) { tests := []struct { name string -- GitLab From 518dadf2bbb9dce3c7992a9b90192b0b07d3cdcb Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:34:57 -0500 Subject: [PATCH 05/17] cgroups: Unify TestSetup_RepoCgroups Convert the v1 and v2 copies of `TestSetup_RepoCgroups` to a single test that covers both. We drop the `Setup` from the test name as repository cgroups are no longer created during setup. --- internal/cgroups/handler_linux_test.go | 148 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 120 -------------------- internal/cgroups/v2_linux_test.go | 116 ------------------- 3 files changed, 148 insertions(+), 236 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index ab22af6f1c..5e5a039528 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -4,6 +4,7 @@ package cgroups import ( "fmt" + "os/exec" "path/filepath" "strconv" "testing" @@ -12,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" + "golang.org/x/exp/slices" ) func TestNewManager(t *testing.T) { @@ -136,6 +138,133 @@ func TestSetup_ParentCgroups(t *testing.T) { } } +func TestRepoCgroups(t *testing.T) { + tests := []struct { + name string + cfg cgroups.Repositories + expectedV1 expectedCgroup + expectedV2 expectedCgroup + }{ + { + name: "all config specified", + cfg: defaultCgroupsConfig().Repositories, + expectedV1: expectedCgroup{ + wantMemoryBytes: 1024000, + wantCPUShares: 256, + wantCPUQuotaUs: 200, + wantCFSPeriod: int(cfsPeriodUs), + }, + expectedV2: expectedCgroup{ + wantMemoryBytes: 1024000, + wantCPUWeight: 256, + wantCPUMax: "200 100000", + }, + }, + { + name: "only memory limit set", + cfg: cgroups.Repositories{ + MemoryBytes: 1024000, + }, + expectedV1: expectedCgroup{ + wantMemoryBytes: 1024000, + }, + expectedV2: expectedCgroup{ + wantMemoryBytes: 1024000, + }, + }, + { + name: "only cpu shares set", + cfg: cgroups.Repositories{ + CPUShares: 512, + }, + expectedV1: expectedCgroup{ + wantCPUShares: 512, + }, + expectedV2: expectedCgroup{ + wantCPUWeight: 512, + }, + }, + { + name: "only cpu quota set", + cfg: cgroups.Repositories{ + CPUQuotaUs: 100, + }, + expectedV1: expectedCgroup{ + wantCPUQuotaUs: 100, + wantCFSPeriod: int(cfsPeriodUs), + }, + expectedV2: expectedCgroup{ + wantCPUMax: "100 100000", + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + for _, version := range []int{1, 2} { + version := version + t.Run("cgroups-v"+strconv.Itoa(version), func(t *testing.T) { + t.Parallel() + + mock := newMock(t, version) + + pid := 1 + cfg := defaultCgroupsConfig() + cfg.Repositories = tt.cfg + cfg.Repositories.Count = 3 + cfg.HierarchyRoot = "gitaly" + cfg.Mountpoint = mock.rootPath() + + manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + + // Validate no shards have been created. We deliberately do not call + // `setupMockCgroupFiles()` here to confirm that the cgroup controller + // is creating repository directories in the correct location. + requireShards(t, version, mock, manager, pid) + + groupID := calcGroupID(cmdArgs, cfg.Repositories.Count) + + mock.setupMockCgroupFiles(t, manager, []uint{groupID}) + + require.False(t, manager.Ready()) + require.NoError(t, manager.Setup()) + require.True(t, manager.Ready()) + + ctx := testhelper.Context(t) + + // Create a command to force Gitaly to create the repo cgroup. + cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) + require.NoError(t, cmd.Run()) + _, err := manager.AddCommand(cmd) + require.NoError(t, err) + + requireShards(t, version, mock, manager, pid, groupID) + + var expected expectedCgroup + if version == 1 { + expected = tt.expectedV1 + } else { + expected = tt.expectedV2 + } + + for shard := uint(0); shard < cfg.Repositories.Count; shard++ { + // The negative case where no directory should exist is asserted + // by `requireShards()`. + if shard == groupID { + cgRelPath := filepath.Join( + "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", shard), + ) + + requireCgroupComponents(t, version, mock.rootPath(), cgRelPath, expected) + } + } + }) + } + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() @@ -162,3 +291,22 @@ func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath requireCgroupWithString(t, cpuMaxPath, expected.wantCPUMax) } } + +func requireShards(t *testing.T, version int, mock mockCgroup, mgr *CGroupManager, pid int, expectedShards ...uint) { + for shard := uint(0); shard < mgr.cfg.Repositories.Count; shard++ { + shouldExist := slices.Contains(expectedShards, shard) + + cgroupPath := filepath.Join("gitaly", + fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", shard)) + cgLock := mgr.status.getLock(cgroupPath) + require.Equal(t, shouldExist, cgLock.isCreated()) + + for _, diskPath := range mock.repoPaths(pid, shard) { + if shouldExist { + require.DirExists(t, diskPath) + } else { + require.NoDirExists(t, diskPath) + } + } + } +} diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 11ae09209a..e8840054f5 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -33,126 +33,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestSetup_RepoCgroups(t *testing.T) { - tests := []struct { - name string - cfg cgroups.Repositories - wantMemoryBytes int - wantCPUShares int - wantCPUQuotaUs int - wantCFSPeriod int - }{ - { - name: "all config specified", - cfg: defaultCgroupsConfig().Repositories, - wantMemoryBytes: 1024000, - wantCPUShares: 256, - wantCPUQuotaUs: 200, - wantCFSPeriod: int(cfsPeriodUs), - }, - { - name: "only memory limit set", - cfg: cgroups.Repositories{ - MemoryBytes: 1024000, - }, - wantMemoryBytes: 1024000, - }, - { - name: "only cpu shares set", - cfg: cgroups.Repositories{ - CPUShares: 512, - }, - wantCPUShares: 512, - }, - { - name: "only cpu quota set", - cfg: cgroups.Repositories{ - CPUQuotaUs: 100, - }, - wantCPUQuotaUs: 100, - wantCFSPeriod: int(cfsPeriodUs), - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - mock := newMockV1(t) - - pid := 1 - cfg := defaultCgroupsConfig() - cfg.Repositories = tt.cfg - cfg.Repositories.Count = 3 - cfg.HierarchyRoot = "gitaly" - cfg.Mountpoint = mock.root - - v1Manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - - require.False(t, v1Manager.Ready()) - require.NoError(t, v1Manager.Setup()) - require.True(t, v1Manager.Ready()) - - ctx := testhelper.Context(t) - - // Validate no shards have been created. We deliberately do not call - // `setupMockCgroupFiles()` here to confirm that the cgroup controller - // is creating repository directories in the correct location. - requireShardsV1(t, mock, v1Manager, pid) - - // Create a command to force Gitaly to create the repo cgroup. - cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd.Run()) - _, err := v1Manager.AddCommand(cmd) - require.NoError(t, err) - - groupID := calcGroupID(cmd.Args, cfg.Repositories.Count) - requireShardsV1(t, mock, v1Manager, pid, groupID) - - for i := 0; i < 3; i++ { - cgroupExists := uint(i) == groupID - - memoryLimitPath := filepath.Join( - mock.root, "memory", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "memory.limit_in_bytes", - ) - if cgroupExists { - requireCgroup(t, memoryLimitPath, tt.wantMemoryBytes) - } else { - require.NoFileExists(t, memoryLimitPath) - } - - cpuSharesPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "cpu.shares", - ) - if cgroupExists { - requireCgroup(t, cpuSharesPath, tt.wantCPUShares) - } else { - require.NoFileExists(t, cpuSharesPath) - } - - cpuCFSQuotaPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "cpu.cfs_quota_us", - ) - if cgroupExists { - requireCgroup(t, cpuCFSQuotaPath, tt.wantCPUQuotaUs) - } else { - require.NoFileExists(t, cpuCFSQuotaPath) - } - - cpuCFSPeriodPath := filepath.Join( - mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "cpu.cfs_period_us", - ) - if cgroupExists { - requireCgroup(t, cpuCFSPeriodPath, tt.wantCFSPeriod) - } else { - require.NoFileExists(t, cpuCFSPeriodPath) - } - } - }) - } -} - func TestAddCommand(t *testing.T) { mock := newMockV1(t) diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index 5bbd626698..d3a459f850 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -33,122 +33,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestSetup_RepoCgroupsV2(t *testing.T) { - tests := []struct { - name string - cfg cgroups.Repositories - wantMemoryBytes int - wantCPUWeight int - wantCPUMax string - }{ - { - name: "all config specified", - cfg: defaultCgroupsV2Config().Repositories, - wantMemoryBytes: 1024000, - wantCPUWeight: 256, - wantCPUMax: "2000 100000", - }, - { - name: "only memory limit set", - cfg: cgroups.Repositories{ - Count: 3, - MemoryBytes: 1024000, - }, - wantMemoryBytes: 1024000, - }, - { - name: "only cpu shares set", - cfg: cgroups.Repositories{ - Count: 3, - CPUShares: 512, - }, - wantCPUWeight: 512, - }, - { - name: "only cpu quota set", - cfg: cgroups.Repositories{ - Count: 3, - CPUQuotaUs: 1000, - }, - wantCPUMax: "1000 100000", - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - mock := newMockV2(t) - - pid := 1 - - cfg := defaultCgroupsV2Config() - cfg.Mountpoint = mock.root - cfg.Repositories = tt.cfg - - groupID := calcGroupID(cmdArgs, cfg.Repositories.Count) - - v2Manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - - // Validate no shards have been created. We deliberately do not call - // `setupMockCgroupFiles()` here to confirm that the cgroup controller - // is creating repository directories in the correct location. - requireShardsV2(t, mock, v2Manager, pid) - - mock.setupMockCgroupFiles(t, v2Manager, []uint{groupID}) - - require.False(t, v2Manager.Ready()) - require.NoError(t, v2Manager.Setup()) - require.True(t, v2Manager.Ready()) - - ctx := testhelper.Context(t) - - // Create a command to force Gitaly to create the repo cgroup. - cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd.Run()) - _, err := v2Manager.AddCommand(cmd) - require.NoError(t, err) - - requireShardsV2(t, mock, v2Manager, pid, groupID) - - for i := 0; i < 3; i++ { - cgroupExists := uint(i) == groupID - - memoryMaxPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "memory.max", - ) - - if cgroupExists { - requireCgroupWithInt(t, memoryMaxPath, tt.wantMemoryBytes) - } else { - require.NoFileExists(t, memoryMaxPath) - } - - cpuWeightPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "cpu.weight", - ) - - if cgroupExists { - requireCgroupWithInt(t, cpuWeightPath, calculateWantCPUWeight(tt.wantCPUWeight)) - } else { - require.NoFileExists(t, cpuWeightPath) - } - - cpuMaxPath := filepath.Join( - mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i), "cpu.max", - ) - - if cgroupExists { - requireCgroupWithString(t, cpuMaxPath, tt.wantCPUMax) - } else { - require.NoFileExists(t, cpuMaxPath) - } - } - }) - } -} - func TestAddCommandV2(t *testing.T) { mock := newMockV2(t) -- GitLab From eac2abe91d875d4852bacc69fefb33560cfc543f Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:40:24 -0500 Subject: [PATCH 06/17] cgroups: Unify TestAddCommand Convert the v1 and v2 versions of `TestAddCommand` to a single shared test. --- internal/cgroups/handler_linux_test.go | 63 ++++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 58 ------------------------ internal/cgroups/v2_linux_test.go | 58 ------------------------ 3 files changed, 63 insertions(+), 116 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index 5e5a039528..9e8b7395c6 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -265,6 +265,69 @@ func TestRepoCgroups(t *testing.T) { } } +func TestAddCommand(t *testing.T) { + for _, version := range []int{1, 2} { + t.Run("cgroups-v"+strconv.Itoa(version), func(t *testing.T) { + mock := newMock(t, version) + + config := defaultCgroupsConfig() + config.Repositories.Count = 10 + config.Repositories.MemoryBytes = 1024 + config.Repositories.CPUShares = 16 + config.HierarchyRoot = "gitaly" + config.Mountpoint = mock.rootPath() + + pid := 1 + manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, manager1, []uint{}) + require.NoError(t, manager1.Setup()) + + ctx := testhelper.Context(t) + + cmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) + require.NoError(t, cmd2.Run()) + + groupID := calcGroupID(cmd2.Args, config.Repositories.Count) + + manager2 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) + + t.Run("without overridden key", func(t *testing.T) { + _, err := manager2.AddCommand(cmd2) + require.NoError(t, err) + requireShards(t, version, mock, manager2, pid, groupID) + + for _, path := range mock.repoPaths(pid, groupID) { + procsPath := filepath.Join(path, "cgroup.procs") + content := readCgroupFile(t, procsPath) + + cmdPid, err := strconv.Atoi(string(content)) + require.NoError(t, err) + + require.Equal(t, cmd2.Process.Pid, cmdPid) + } + }) + + t.Run("with overridden key", func(t *testing.T) { + overriddenGroupID := calcGroupID([]string{"foobar"}, config.Repositories.Count) + + _, err := manager2.AddCommand(cmd2, WithCgroupKey("foobar")) + require.NoError(t, err) + requireShards(t, version, mock, manager2, pid, groupID, overriddenGroupID) + + for _, path := range mock.repoPaths(pid, overriddenGroupID) { + procsPath := filepath.Join(path, "cgroup.procs") + content := readCgroupFile(t, procsPath) + + cmdPid, err := strconv.Atoi(string(content)) + require.NoError(t, err) + + require.Equal(t, cmd2.Process.Pid, cmdPid) + } + }) + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index e8840054f5..531b42aca9 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -33,64 +33,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestAddCommand(t *testing.T) { - mock := newMockV1(t) - - config := defaultCgroupsConfig() - config.Repositories.Count = 10 - config.Repositories.MemoryBytes = 1024 - config.Repositories.CPUShares = 16 - config.HierarchyRoot = "gitaly" - config.Mountpoint = mock.root - - pid := 1 - groupID := calcGroupID(cmdArgs, config.Repositories.Count) - - v1Manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) - require.NoError(t, v1Manager1.Setup()) - ctx := testhelper.Context(t) - - cmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd2.Run()) - - v1Manager2 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) - - t.Run("without overridden key", func(t *testing.T) { - _, err := v1Manager2.AddCommand(cmd2) - require.NoError(t, err) - requireShardsV1(t, mock, v1Manager2, pid, groupID) - - for _, s := range mock.subsystems { - path := filepath.Join(mock.root, string(s.Name()), "gitaly", - fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", groupID), "cgroup.procs") - content := readCgroupFile(t, path) - - cmdPid, err := strconv.Atoi(string(content)) - require.NoError(t, err) - - require.Equal(t, cmd2.Process.Pid, cmdPid) - } - }) - - t.Run("with overridden key", func(t *testing.T) { - overridenGroupID := calcGroupID([]string{"foobar"}, config.Repositories.Count) - - _, err := v1Manager2.AddCommand(cmd2, WithCgroupKey("foobar")) - require.NoError(t, err) - - for _, s := range mock.subsystems { - path := filepath.Join(mock.root, string(s.Name()), "gitaly", - fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", overridenGroupID), "cgroup.procs") - content := readCgroupFile(t, path) - - cmdPid, err := strconv.Atoi(string(content)) - require.NoError(t, err) - - require.Equal(t, cmd2.Process.Pid, cmdPid) - } - }) -} - func TestCleanup(t *testing.T) { mock := newMockV1(t) diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index d3a459f850..f9c5f7bee5 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -33,64 +33,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestAddCommandV2(t *testing.T) { - mock := newMockV2(t) - - config := defaultCgroupsV2Config() - config.Repositories.Count = 10 - config.Repositories.MemoryBytes = 1024 - config.Repositories.CPUShares = 16 - config.Mountpoint = mock.root - - pid := 1 - groupID := calcGroupID(cmdArgs, config.Repositories.Count) - - v2Manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, v2Manager1, []uint{}) - - require.NoError(t, v2Manager1.Setup()) - ctx := testhelper.Context(t) - - cmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd2.Run()) - - v2Manager2 := mock.newCgroupManager(config, testhelper.SharedLogger(t), pid) - - t.Run("without overridden key", func(t *testing.T) { - groupID := calcGroupID(cmd2.Args, config.Repositories.Count) - - _, err := v2Manager2.AddCommand(cmd2) - require.NoError(t, err) - requireShardsV2(t, mock, v2Manager2, pid, groupID) - - path := filepath.Join(mock.root, "gitaly", - fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", groupID), "cgroup.procs") - content := readCgroupFile(t, path) - - cmdPid, err := strconv.Atoi(string(content)) - require.NoError(t, err) - - require.Equal(t, cmd2.Process.Pid, cmdPid) - }) - - t.Run("with overridden key", func(t *testing.T) { - overriddenGroupID := calcGroupID([]string{"foobar"}, config.Repositories.Count) - - _, err := v2Manager2.AddCommand(cmd2, WithCgroupKey("foobar")) - require.NoError(t, err) - requireShardsV2(t, mock, v2Manager2, pid, groupID, overriddenGroupID) - - path := filepath.Join(mock.root, "gitaly", - fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", overriddenGroupID), "cgroup.procs") - content := readCgroupFile(t, path) - - cmdPid, err := strconv.Atoi(string(content)) - require.NoError(t, err) - - require.Equal(t, cmd2.Process.Pid, cmdPid) - }) -} - func TestCleanupV2(t *testing.T) { mock := newMockV2(t) -- GitLab From c9cdf8c3bd20f7c9af37dacb4acb0227128f073d Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:41:58 -0500 Subject: [PATCH 07/17] cgroups: Unify TestCleanup Convert the v1 and v2 versions of TestCleanup to a single unified test. --- internal/cgroups/handler_linux_test.go | 24 ++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 21 --------------------- internal/cgroups/v2_linux_test.go | 18 ------------------ 3 files changed, 24 insertions(+), 39 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index 9e8b7395c6..4f61ab7f65 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -328,6 +328,30 @@ func TestAddCommand(t *testing.T) { } } +func TestCleanup(t *testing.T) { + for _, version := range []int{1, 2} { + t.Run("cgroups-v"+strconv.Itoa(version), func(t *testing.T) { + mock := newMock(t, version) + + pid := 1 + cfg := defaultCgroupsConfig() + cfg.Mountpoint = mock.rootPath() + + manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, manager, []uint{0, 1, 2}) + + require.NoError(t, manager.Setup()) + require.NoError(t, manager.Cleanup()) + + for i := uint(0); i < 3; i++ { + for _, path := range mock.repoPaths(pid, i) { + require.NoDirExists(t, path) + } + } + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 531b42aca9..25c835dcfc 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -33,27 +33,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestCleanup(t *testing.T) { - mock := newMockV1(t) - - pid := 1 - cfg := defaultCgroupsConfig() - cfg.Mountpoint = mock.root - - v1Manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - - require.NoError(t, v1Manager.Setup()) - require.NoError(t, v1Manager.Cleanup()) - - for i := 0; i < 3; i++ { - memoryPath := filepath.Join(mock.root, "memory", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i)) - cpuPath := filepath.Join(mock.root, "cpu", "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i)) - - require.NoDirExists(t, memoryPath) - require.NoDirExists(t, cpuPath) - } -} - func TestMetrics(t *testing.T) { tests := []struct { name string diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index f9c5f7bee5..f1cb82d286 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -33,24 +33,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestCleanupV2(t *testing.T) { - mock := newMockV2(t) - - pid := 1 - cfg := defaultCgroupsV2Config() - cfg.Mountpoint = mock.root - - v2Manager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, v2Manager, []uint{0, 1, 2}) - - require.NoError(t, v2Manager.Setup()) - require.NoError(t, v2Manager.Cleanup()) - - for i := 0; i < 3; i++ { - require.NoDirExists(t, filepath.Join(mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", i))) - } -} - func TestMetricsV2(t *testing.T) { tests := []struct { name string -- GitLab From 15ef84dc1817536e362fe7b0a6b5cbe8c0c4d1ca Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:44:43 -0500 Subject: [PATCH 08/17] cgroups: Unify TestMetrics Convert the separate v1 and v2 `TestMetrics` versions to a single test. --- internal/cgroups/handler_linux_test.go | 126 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 97 ------------------- internal/cgroups/v2_linux_test.go | 95 ------------------- 3 files changed, 126 insertions(+), 192 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index 4f61ab7f65..a9a12a2782 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -7,9 +7,12 @@ import ( "os/exec" "path/filepath" "strconv" + "strings" "testing" cgrps "github.com/containerd/cgroups/v3" + "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" @@ -352,6 +355,129 @@ func TestCleanup(t *testing.T) { } } +func TestMetrics(t *testing.T) { + tests := []struct { + name string + metricsEnabled bool + pid int + expectV1 string + expectV2 string + }{ + { + name: "metrics enabled: true", + metricsEnabled: true, + pid: 1, + expectV1: `# HELP gitaly_cgroup_cpu_usage_total CPU Usage of Cgroup +# TYPE gitaly_cgroup_cpu_usage_total gauge +gitaly_cgroup_cpu_usage_total{path="%s",type="kernel"} 0 +gitaly_cgroup_cpu_usage_total{path="%s",type="user"} 0 +# HELP gitaly_cgroup_memory_reclaim_attempts_total Number of memory usage hits limits +# TYPE gitaly_cgroup_memory_reclaim_attempts_total gauge +gitaly_cgroup_memory_reclaim_attempts_total{path="%s"} 2 +# HELP gitaly_cgroup_procs_total Total number of procs +# TYPE gitaly_cgroup_procs_total gauge +gitaly_cgroup_procs_total{path="%s",subsystem="cpu"} 1 +gitaly_cgroup_procs_total{path="%s",subsystem="memory"} 1 +# HELP gitaly_cgroup_cpu_cfs_periods_total Number of elapsed enforcement period intervals +# TYPE gitaly_cgroup_cpu_cfs_periods_total counter +gitaly_cgroup_cpu_cfs_periods_total{path="%s"} 10 +# HELP gitaly_cgroup_cpu_cfs_throttled_periods_total Number of throttled period intervals +# TYPE gitaly_cgroup_cpu_cfs_throttled_periods_total counter +gitaly_cgroup_cpu_cfs_throttled_periods_total{path="%s"} 20 +# HELP gitaly_cgroup_cpu_cfs_throttled_seconds_total Total time duration the Cgroup has been throttled +# TYPE gitaly_cgroup_cpu_cfs_throttled_seconds_total counter +gitaly_cgroup_cpu_cfs_throttled_seconds_total{path="%s"} 0.001 +`, + expectV2: `# HELP gitaly_cgroup_cpu_cfs_periods_total Number of elapsed enforcement period intervals +# TYPE gitaly_cgroup_cpu_cfs_periods_total counter +gitaly_cgroup_cpu_cfs_periods_total{path="%s"} 10 +# HELP gitaly_cgroup_cpu_cfs_throttled_periods_total Number of throttled period intervals +# TYPE gitaly_cgroup_cpu_cfs_throttled_periods_total counter +gitaly_cgroup_cpu_cfs_throttled_periods_total{path="%s"} 20 +# HELP gitaly_cgroup_cpu_cfs_throttled_seconds_total Total time duration the Cgroup has been throttled +# TYPE gitaly_cgroup_cpu_cfs_throttled_seconds_total counter +gitaly_cgroup_cpu_cfs_throttled_seconds_total{path="%s"} 0.001 +# HELP gitaly_cgroup_cpu_usage_total CPU Usage of Cgroup +# TYPE gitaly_cgroup_cpu_usage_total gauge +gitaly_cgroup_cpu_usage_total{path="%s",type="kernel"} 0 +gitaly_cgroup_cpu_usage_total{path="%s",type="user"} 0 +# HELP gitaly_cgroup_procs_total Total number of procs +# TYPE gitaly_cgroup_procs_total gauge +gitaly_cgroup_procs_total{path="%s",subsystem="cpu"} 1 +gitaly_cgroup_procs_total{path="%s",subsystem="cpuset"} 1 +gitaly_cgroup_procs_total{path="%s",subsystem="memory"} 1 +`, + }, + { + name: "metrics enabled: false", + metricsEnabled: false, + pid: 2, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + for _, version := range []int{1, 2} { + version := version + t.Run("cgroups-v"+strconv.Itoa(version), func(t *testing.T) { + t.Parallel() + mock := newMock(t, version) + + config := defaultCgroupsConfig() + config.Repositories.Count = 1 + config.Repositories.MemoryBytes = 1048576 + config.Repositories.CPUShares = 16 + config.Mountpoint = mock.rootPath() + config.MetricsEnabled = tt.metricsEnabled + + manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), tt.pid) + + var mockFiles []mockCgroupFile + if version == 1 { + mockFiles = append(mockFiles, mockCgroupFile{"memory.failcnt", "2"}) + } + mock.setupMockCgroupFiles(t, manager1, []uint{0}, mockFiles...) + require.NoError(t, manager1.Setup()) + + ctx := testhelper.Context(t) + + cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) + require.NoError(t, cmd.Start()) + _, err := manager1.AddCommand(cmd) + require.NoError(t, err) + + gitCmd1 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) + require.NoError(t, gitCmd1.Start()) + _, err = manager1.AddCommand(gitCmd1) + require.NoError(t, err) + + gitCmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) + require.NoError(t, gitCmd2.Start()) + _, err = manager1.AddCommand(gitCmd2) + require.NoError(t, err) + defer func() { + require.NoError(t, gitCmd2.Wait()) + }() + + require.NoError(t, cmd.Wait()) + require.NoError(t, gitCmd1.Wait()) + + repoCgroupPath := filepath.Join(manager1.currentProcessCgroup(), "repos-0") + + var expected *strings.Reader + if version == 1 { + expected = strings.NewReader(strings.ReplaceAll(tt.expectV1, "%s", repoCgroupPath)) + } else { + expected = strings.NewReader(strings.ReplaceAll(tt.expectV2, "%s", repoCgroupPath)) + } + assert.NoError(t, testutil.CollectAndCompare(manager1, expected)) + }) + } + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 25c835dcfc..35f1563544 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -9,11 +9,8 @@ import ( "os/exec" "path/filepath" "strconv" - "strings" "testing" - "github.com/prometheus/client_golang/prometheus/testutil" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" @@ -33,100 +30,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestMetrics(t *testing.T) { - tests := []struct { - name string - metricsEnabled bool - pid int - expect string - }{ - { - name: "metrics enabled: true", - metricsEnabled: true, - pid: 1, - expect: `# HELP gitaly_cgroup_cpu_usage_total CPU Usage of Cgroup -# TYPE gitaly_cgroup_cpu_usage_total gauge -gitaly_cgroup_cpu_usage_total{path="%s",type="kernel"} 0 -gitaly_cgroup_cpu_usage_total{path="%s",type="user"} 0 -# HELP gitaly_cgroup_memory_reclaim_attempts_total Number of memory usage hits limits -# TYPE gitaly_cgroup_memory_reclaim_attempts_total gauge -gitaly_cgroup_memory_reclaim_attempts_total{path="%s"} 2 -# HELP gitaly_cgroup_procs_total Total number of procs -# TYPE gitaly_cgroup_procs_total gauge -gitaly_cgroup_procs_total{path="%s",subsystem="cpu"} 1 -gitaly_cgroup_procs_total{path="%s",subsystem="memory"} 1 -# HELP gitaly_cgroup_cpu_cfs_periods_total Number of elapsed enforcement period intervals -# TYPE gitaly_cgroup_cpu_cfs_periods_total counter -gitaly_cgroup_cpu_cfs_periods_total{path="%s"} 10 -# HELP gitaly_cgroup_cpu_cfs_throttled_periods_total Number of throttled period intervals -# TYPE gitaly_cgroup_cpu_cfs_throttled_periods_total counter -gitaly_cgroup_cpu_cfs_throttled_periods_total{path="%s"} 20 -# HELP gitaly_cgroup_cpu_cfs_throttled_seconds_total Total time duration the Cgroup has been throttled -# TYPE gitaly_cgroup_cpu_cfs_throttled_seconds_total counter -gitaly_cgroup_cpu_cfs_throttled_seconds_total{path="%s"} 0.001 -`, - }, - { - name: "metrics enabled: false", - metricsEnabled: false, - pid: 2, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - mock := newMockV1(t) - - config := defaultCgroupsConfig() - config.Repositories.Count = 1 - config.Repositories.MemoryBytes = 1048576 - config.Repositories.CPUShares = 16 - config.Mountpoint = mock.root - config.MetricsEnabled = tt.metricsEnabled - - v1Manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), tt.pid) - - groupID := calcGroupID(cmdArgs, config.Repositories.Count) - - mock.setupMockCgroupFiles(t, v1Manager1, []uint{groupID}, mockCgroupFile{"memory.failcnt", "2"}) - require.NoError(t, v1Manager1.Setup()) - - ctx := testhelper.Context(t) - - cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd.Start()) - _, err := v1Manager1.AddCommand(cmd) - require.NoError(t, err) - - gitCmd1 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, gitCmd1.Start()) - _, err = v1Manager1.AddCommand(gitCmd1) - require.NoError(t, err) - - gitCmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, gitCmd2.Start()) - _, err = v1Manager1.AddCommand(gitCmd2) - require.NoError(t, err) - - requireShardsV1(t, mock, v1Manager1, tt.pid, groupID) - - defer func() { - require.NoError(t, gitCmd2.Wait()) - }() - - require.NoError(t, cmd.Wait()) - require.NoError(t, gitCmd1.Wait()) - - repoCgroupPath := filepath.Join(v1Manager1.currentProcessCgroup(), "repos-0") - - expected := strings.NewReader(strings.ReplaceAll(tt.expect, "%s", repoCgroupPath)) - assert.NoError(t, testutil.CollectAndCompare(v1Manager1, expected)) - }) - } -} - func TestPruneOldCgroups(t *testing.T) { t.Parallel() diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index f1cb82d286..0c89f9aeed 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -9,11 +9,8 @@ import ( "os/exec" "path/filepath" "strconv" - "strings" "testing" - "github.com/prometheus/client_golang/prometheus/testutil" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" @@ -33,98 +30,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestMetricsV2(t *testing.T) { - tests := []struct { - name string - metricsEnabled bool - pid int - expect string - }{ - { - name: "metrics enabled: true", - metricsEnabled: true, - pid: 1, - expect: `# HELP gitaly_cgroup_cpu_cfs_periods_total Number of elapsed enforcement period intervals -# TYPE gitaly_cgroup_cpu_cfs_periods_total counter -gitaly_cgroup_cpu_cfs_periods_total{path="%s"} 10 -# HELP gitaly_cgroup_cpu_cfs_throttled_periods_total Number of throttled period intervals -# TYPE gitaly_cgroup_cpu_cfs_throttled_periods_total counter -gitaly_cgroup_cpu_cfs_throttled_periods_total{path="%s"} 20 -# HELP gitaly_cgroup_cpu_cfs_throttled_seconds_total Total time duration the Cgroup has been throttled -# TYPE gitaly_cgroup_cpu_cfs_throttled_seconds_total counter -gitaly_cgroup_cpu_cfs_throttled_seconds_total{path="%s"} 0.001 -# HELP gitaly_cgroup_cpu_usage_total CPU Usage of Cgroup -# TYPE gitaly_cgroup_cpu_usage_total gauge -gitaly_cgroup_cpu_usage_total{path="%s",type="kernel"} 0 -gitaly_cgroup_cpu_usage_total{path="%s",type="user"} 0 -# HELP gitaly_cgroup_procs_total Total number of procs -# TYPE gitaly_cgroup_procs_total gauge -gitaly_cgroup_procs_total{path="%s",subsystem="cpu"} 1 -gitaly_cgroup_procs_total{path="%s",subsystem="cpuset"} 1 -gitaly_cgroup_procs_total{path="%s",subsystem="memory"} 1 -`, - }, - { - name: "metrics enabled: false", - metricsEnabled: false, - pid: 2, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - mock := newMockV2(t) - - config := defaultCgroupsV2Config() - config.Repositories.Count = 1 - config.Repositories.MemoryBytes = 1048576 - config.Repositories.CPUShares = 16 - config.Mountpoint = mock.root - config.MetricsEnabled = tt.metricsEnabled - - groupID := calcGroupID(cmdArgs, config.Repositories.Count) - v2Manager1 := mock.newCgroupManager(config, testhelper.SharedLogger(t), tt.pid) - - mock.setupMockCgroupFiles(t, v2Manager1, []uint{groupID}) - require.NoError(t, v2Manager1.Setup()) - - ctx := testhelper.Context(t) - - cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, cmd.Start()) - _, err := v2Manager1.AddCommand(cmd) - require.NoError(t, err) - - gitCmd1 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, gitCmd1.Start()) - _, err = v2Manager1.AddCommand(gitCmd1) - require.NoError(t, err) - - gitCmd2 := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) - require.NoError(t, gitCmd2.Start()) - _, err = v2Manager1.AddCommand(gitCmd2) - require.NoError(t, err) - - requireShardsV2(t, mock, v2Manager1, tt.pid, groupID) - - defer func() { - require.NoError(t, gitCmd2.Wait()) - }() - - require.NoError(t, cmd.Wait()) - require.NoError(t, gitCmd1.Wait()) - - repoCgroupPath := filepath.Join(v2Manager1.currentProcessCgroup(), "repos-0") - - expected := strings.NewReader(strings.ReplaceAll(tt.expect, "%s", repoCgroupPath)) - - assert.NoError(t, testutil.CollectAndCompare(v2Manager1, expected)) - }) - } -} - func TestPruneOldCgroupsV2(t *testing.T) { t.Parallel() -- GitLab From f850c41e192b0038e4a51f6a74dc330f3dd04217 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:47:59 -0500 Subject: [PATCH 09/17] cgroups: Unify TestPruneOldCgroups Combine the v1 and v2 copies of `TestPruneOldCgroups` into a single test. We are unable to deduplicate most of the actual assertions as they rely heavily on the subcomponent paths. This is still a worthwhile change as the test cases can still be shared. --- internal/cgroups/handler_linux_test.go | 213 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 167 ------------------- internal/cgroups/v2_linux_test.go | 158 ------------------ 3 files changed, 213 insertions(+), 325 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index a9a12a2782..e1fd554d7e 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -4,6 +4,8 @@ package cgroups import ( "fmt" + "io/fs" + "os" "os/exec" "path/filepath" "strconv" @@ -15,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" + "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "golang.org/x/exp/slices" ) @@ -478,6 +481,216 @@ gitaly_cgroup_procs_total{path="%s",subsystem="memory"} 1 } } +func TestPruneOldCgroups(t *testing.T) { + t.Parallel() + + testCases := []struct { + desc string + cfg cgroups.Config + expectedPruned bool + // setup returns a pid + setup func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int + }{ + { + desc: "process belongs to another user", + cfg: cgroups.Config{ + HierarchyRoot: "gitaly", + Repositories: cgroups.Repositories{ + Count: 10, + MemoryBytes: 10 * 1024 * 1024, + CPUShares: 1024, + }, + }, + setup: func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int { + pid := 1 + cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) + require.NoError(t, cgroupManager.Setup()) + + return pid + }, + expectedPruned: true, + }, + { + desc: "no hierarchy root", + cfg: cgroups.Config{ + HierarchyRoot: "", + Repositories: cgroups.Repositories{ + Count: 10, + MemoryBytes: 10 * 1024 * 1024, + CPUShares: 1024, + }, + }, + setup: func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int { + pid := 1 + cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) + require.NoError(t, cgroupManager.Setup()) + return 1 + }, + expectedPruned: false, + }, + { + desc: "pid of finished process", + cfg: cgroups.Config{ + HierarchyRoot: "gitaly", + Repositories: cgroups.Repositories{ + Count: 10, + MemoryBytes: 10 * 1024 * 1024, + CPUShares: 1024, + }, + }, + setup: func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int { + cmd := exec.Command("ls") + require.NoError(t, cmd.Run()) + pid := cmd.Process.Pid + + cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) + + if mock.version() == 1 { + memoryRoot := filepath.Join( + cfg.Mountpoint, + "memory", + cfg.HierarchyRoot, + "memory.limit_in_bytes", + ) + require.NoError(t, os.WriteFile(memoryRoot, []byte{}, fs.ModeAppend)) + } else { + require.NoError(t, cgroupManager.Setup()) + + memoryFile := filepath.Join( + cfg.Mountpoint, + cfg.HierarchyRoot, + "memory.limit_in_bytes", + ) + require.NoError(t, os.WriteFile(memoryFile, []byte{}, fs.ModeAppend)) + } + + return pid + }, + expectedPruned: true, + }, + { + desc: "pid of running process", + cfg: cgroups.Config{ + HierarchyRoot: "gitaly", + Repositories: cgroups.Repositories{ + Count: 10, + MemoryBytes: 10 * 1024 * 1024, + CPUShares: 1024, + }, + }, + setup: func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int { + pid := os.Getpid() + + cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) + mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) + require.NoError(t, cgroupManager.Setup()) + + return pid + }, + expectedPruned: false, + }, + { + desc: "gitaly-0 directory is deleted", + cfg: cgroups.Config{ + HierarchyRoot: "gitaly", + Repositories: cgroups.Repositories{ + Count: 10, + MemoryBytes: 10 * 1024 * 1024, + CPUShares: 1024, + }, + }, + setup: func(t *testing.T, cfg cgroups.Config, mock mockCgroup) int { + cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), 0) + mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) + require.NoError(t, cgroupManager.Setup()) + + return 0 + }, + expectedPruned: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + t.Run("cgroups-v1", func(t *testing.T) { + mock := newMock(t, 1) + tc.cfg.Mountpoint = mock.rootPath() + + memoryRoot := filepath.Join( + tc.cfg.Mountpoint, + "memory", + tc.cfg.HierarchyRoot, + ) + cpuRoot := filepath.Join( + tc.cfg.Mountpoint, + "cpu", + tc.cfg.HierarchyRoot, + ) + + require.NoError(t, os.MkdirAll(cpuRoot, perm.PublicDir)) + require.NoError(t, os.MkdirAll(memoryRoot, perm.PublicDir)) + + pid := tc.setup(t, tc.cfg, mock) + + logger := testhelper.NewLogger(t) + hook := testhelper.AddLoggerHook(logger) + + mock.pruneOldCgroups(tc.cfg, logger) + + // create cgroups directories with a different pid + oldGitalyProcessMemoryDir := filepath.Join( + memoryRoot, + fmt.Sprintf("gitaly-%d", pid), + ) + oldGitalyProcesssCPUDir := filepath.Join( + cpuRoot, + fmt.Sprintf("gitaly-%d", pid), + ) + + if tc.expectedPruned { + require.NoDirExists(t, oldGitalyProcessMemoryDir) + require.NoDirExists(t, oldGitalyProcesssCPUDir) + } else { + require.DirExists(t, oldGitalyProcessMemoryDir) + require.DirExists(t, oldGitalyProcesssCPUDir) + require.Empty(t, hook.AllEntries()) + } + }) + + t.Run("cgroups-v2", func(t *testing.T) { + mock := newMock(t, 2) + tc.cfg.Mountpoint = mock.rootPath() + + root := filepath.Join( + tc.cfg.Mountpoint, + tc.cfg.HierarchyRoot, + ) + require.NoError(t, os.MkdirAll(root, perm.PublicDir)) + + pid := tc.setup(t, tc.cfg, mock) + + logger := testhelper.NewLogger(t) + mock.pruneOldCgroups(tc.cfg, logger) + + // create cgroups directories with a different pid + oldGitalyProcessDir := filepath.Join( + root, + fmt.Sprintf("gitaly-%d", pid), + ) + + if tc.expectedPruned { + require.NoDirExists(t, oldGitalyProcessDir) + } else { + require.DirExists(t, oldGitalyProcessDir) + } + }) + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 35f1563544..0179cc08af 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -4,9 +4,7 @@ package cgroups import ( "fmt" - "io/fs" "os" - "os/exec" "path/filepath" "strconv" "testing" @@ -30,171 +28,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestPruneOldCgroups(t *testing.T) { - t.Parallel() - - testCases := []struct { - desc string - cfg cgroups.Config - expectedPruned bool - // setup returns a pid - setup func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int - }{ - { - desc: "process belongs to another user", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { - pid := 1 - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - require.NoError(t, cgroupManager.Setup()) - - return pid - }, - expectedPruned: true, - }, - { - desc: "no hierarchy root", - cfg: cgroups.Config{ - HierarchyRoot: "", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { - pid := 1 - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - require.NoError(t, cgroupManager.Setup()) - return 1 - }, - expectedPruned: false, - }, - { - desc: "pid of finished process", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { - cmd := exec.Command("ls") - require.NoError(t, cmd.Run()) - pid := cmd.Process.Pid - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - require.NoError(t, cgroupManager.Setup()) - - memoryRoot := filepath.Join( - cfg.Mountpoint, - "memory", - cfg.HierarchyRoot, - "memory.limit_in_bytes", - ) - require.NoError(t, os.WriteFile(memoryRoot, []byte{}, fs.ModeAppend)) - - return pid - }, - expectedPruned: true, - }, - { - desc: "pid of running process", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { - pid := os.Getpid() - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - require.NoError(t, cgroupManager.Setup()) - - return pid - }, - expectedPruned: false, - }, - { - desc: "gitaly-0 directory is deleted", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV1) int { - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), 0) - require.NoError(t, cgroupManager.Setup()) - - return 0 - }, - expectedPruned: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - mock := newMockV1(t) - tc.cfg.Mountpoint = mock.root - - memoryRoot := filepath.Join( - tc.cfg.Mountpoint, - "memory", - tc.cfg.HierarchyRoot, - ) - cpuRoot := filepath.Join( - tc.cfg.Mountpoint, - "cpu", - tc.cfg.HierarchyRoot, - ) - - require.NoError(t, os.MkdirAll(cpuRoot, perm.PublicDir)) - require.NoError(t, os.MkdirAll(memoryRoot, perm.PublicDir)) - - pid := tc.setup(t, tc.cfg, mock) - - logger := testhelper.NewLogger(t) - hook := testhelper.AddLoggerHook(logger) - - mock.pruneOldCgroups(tc.cfg, logger) - - // create cgroups directories with a different pid - oldGitalyProcessMemoryDir := filepath.Join( - memoryRoot, - fmt.Sprintf("gitaly-%d", pid), - ) - oldGitalyProcesssCPUDir := filepath.Join( - cpuRoot, - fmt.Sprintf("gitaly-%d", pid), - ) - - if tc.expectedPruned { - require.NoDirExists(t, oldGitalyProcessMemoryDir) - require.NoDirExists(t, oldGitalyProcesssCPUDir) - } else { - require.DirExists(t, oldGitalyProcessMemoryDir) - require.DirExists(t, oldGitalyProcesssCPUDir) - require.Empty(t, hook.AllEntries()) - } - }) - } -} - func TestStatsV1(t *testing.T) { t.Parallel() diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index 0c89f9aeed..65ee215f3f 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -4,16 +4,12 @@ package cgroups import ( "fmt" - "io/fs" - "os" - "os/exec" "path/filepath" "strconv" "testing" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "golang.org/x/exp/slices" ) @@ -30,160 +26,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestPruneOldCgroupsV2(t *testing.T) { - t.Parallel() - - testCases := []struct { - desc string - cfg cgroups.Config - expectedPruned bool - // setup returns a pid - setup func(*testing.T, cgroups.Config, *mockCgroupV2) int - }{ - { - desc: "process belongs to another user", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV2) int { - pid := 1 - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) - require.NoError(t, cgroupManager.Setup()) - - return pid - }, - expectedPruned: true, - }, - { - desc: "no hierarchy root", - cfg: cgroups.Config{ - HierarchyRoot: "", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV2) int { - pid := 1 - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) - require.NoError(t, cgroupManager.Setup()) - return 1 - }, - expectedPruned: false, - }, - { - desc: "pid of finished process", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV2) int { - cmd := exec.Command("ls") - require.NoError(t, cmd.Run()) - pid := cmd.Process.Pid - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) - require.NoError(t, cgroupManager.Setup()) - - memoryFile := filepath.Join( - cfg.Mountpoint, - cfg.HierarchyRoot, - "memory.limit_in_bytes", - ) - require.NoError(t, os.WriteFile(memoryFile, []byte{}, fs.ModeAppend)) - - return pid - }, - expectedPruned: true, - }, - { - desc: "pid of running process", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV2) int { - pid := os.Getpid() - - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), pid) - mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) - require.NoError(t, cgroupManager.Setup()) - - return pid - }, - expectedPruned: false, - }, - { - desc: "gitaly-0 directory is deleted", - cfg: cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 10, - MemoryBytes: 10 * 1024 * 1024, - CPUShares: 1024, - }, - }, - setup: func(t *testing.T, cfg cgroups.Config, mock *mockCgroupV2) int { - cgroupManager := mock.newCgroupManager(cfg, testhelper.SharedLogger(t), 0) - mock.setupMockCgroupFiles(t, cgroupManager, []uint{0, 1, 2}) - require.NoError(t, cgroupManager.Setup()) - - return 0 - }, - expectedPruned: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - mock := newMockV2(t) - tc.cfg.Mountpoint = mock.root - - root := filepath.Join( - tc.cfg.Mountpoint, - tc.cfg.HierarchyRoot, - ) - require.NoError(t, os.MkdirAll(root, perm.PublicDir)) - - pid := tc.setup(t, tc.cfg, mock) - - logger := testhelper.NewLogger(t) - mock.pruneOldCgroups(tc.cfg, logger) - - // create cgroups directories with a different pid - oldGitalyProcessDir := filepath.Join( - root, - fmt.Sprintf("gitaly-%d", pid), - ) - - if tc.expectedPruned { - require.NoDirExists(t, oldGitalyProcessDir) - } else { - require.DirExists(t, oldGitalyProcessDir) - } - }) - } -} - func TestStatsV2(t *testing.T) { t.Parallel() -- GitLab From da5f6f3af4f7543b7bf231db6f1d258c4cee4d8f Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 22:56:26 -0500 Subject: [PATCH 10/17] cgroups: Unify TestStats Combine the v1 and v2 versions of `TestStats()` to a single test. Here we have the opposite of `TestPruneOldCgroups()`, the test cases are unique to v1 or v2, but the assertions are version agnostic. Add a `version` field to the test cases, rather than iterating over each case with each version as we did with the other tests. --- internal/cgroups/handler_linux_test.go | 103 +++++++++++++++++++++++++ internal/cgroups/v1_linux_test.go | 75 ------------------ internal/cgroups/v2_linux_test.go | 78 ------------------- 3 files changed, 103 insertions(+), 153 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index e1fd554d7e..e419cbf671 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -691,6 +691,109 @@ func TestPruneOldCgroups(t *testing.T) { } } +func TestStats(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + desc string + version int + mockFiles []mockCgroupFile + expectedStats Stats + }{ + { + desc: "empty statistics", + version: 1, + mockFiles: []mockCgroupFile{ + {"memory.limit_in_bytes", "0"}, + {"memory.usage_in_bytes", "0"}, + {"memory.oom_control", ""}, + {"cpu.stat", ""}, + }, + expectedStats: Stats{}, + }, + { + desc: "cgroupfs recorded some stats", + version: 1, + mockFiles: []mockCgroupFile{ + {"memory.limit_in_bytes", "2000000000"}, + {"memory.usage_in_bytes", "1234000000"}, + {"memory.oom_control", `oom_kill_disable 1 +under_oom 1 +oom_kill 3`}, + {"cpu.stat", `nr_periods 10 +nr_throttled 50 +throttled_time 1000000`}, // 0.001 seconds + }, + expectedStats: Stats{ + ParentStats: CgroupStats{ + CPUThrottledCount: 50, + CPUThrottledDuration: 0.001, + MemoryUsage: 1234000000, + MemoryLimit: 2000000000, + OOMKills: 3, + UnderOOM: true, + }, + }, + }, + { + desc: "empty statistics", + version: 2, + mockFiles: []mockCgroupFile{ + {"memory.current", "0"}, + {"memory.max", "0"}, + {"cpu.stat", ""}, + }, + expectedStats: Stats{}, + }, + { + desc: "cgroupfs recorded some stats", + version: 2, + mockFiles: []mockCgroupFile{ + {"memory.max", "2000000000"}, + {"memory.current", "1234000000"}, + {"memory.events", `low 1 +high 2 +max 3 +oom 4 +oom_kill 5`}, + {"nr_throttled", "50"}, + {"throttled_usec", "1000000"}, + {"cpu.stat", `nr_periods 10 +nr_throttled 50 +throttled_usec 1000000`}, // 0.001 seconds + }, + expectedStats: Stats{ + ParentStats: CgroupStats{ + CPUThrottledCount: 50, + CPUThrottledDuration: 0.001, + MemoryUsage: 1234000000, + MemoryLimit: 2000000000, + OOMKills: 5, + }, + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + mock := newMock(t, tc.version) + + config := defaultCgroupsConfig() + config.Repositories.Count = 1 + config.Repositories.MemoryBytes = 2000000000 + config.Repositories.CPUShares = 16 + config.Mountpoint = mock.rootPath() + + manager := mock.newCgroupManager(config, testhelper.SharedLogger(t), 1) + + mock.setupMockCgroupFiles(t, manager, []uint{0}, tc.mockFiles...) + require.NoError(t, manager.Setup()) + + stats, err := manager.Stats() + require.NoError(t, err) + require.Equal(t, tc.expectedStats, stats) + }) + } +} + func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath string, expected expectedCgroup) { t.Helper() diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 0179cc08af..cd00e4fa2f 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -28,81 +28,6 @@ func defaultCgroupsConfig() cgroups.Config { } } -func TestStatsV1(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - desc string - mockFiles []mockCgroupFile - expectedStats Stats - }{ - { - desc: "empty statistics", - mockFiles: []mockCgroupFile{ - {"memory.limit_in_bytes", "0"}, - {"memory.usage_in_bytes", "0"}, - {"memory.oom_control", ""}, - {"cpu.stat", ""}, - }, - expectedStats: Stats{}, - }, - { - desc: "cgroupfs recorded some stats", - mockFiles: []mockCgroupFile{ - {"memory.limit_in_bytes", "2000000000"}, - {"memory.usage_in_bytes", "1234000000"}, - {"memory.oom_control", `oom_kill_disable 1 -under_oom 1 -oom_kill 3`}, - {"cpu.stat", `nr_periods 10 -nr_throttled 50 -throttled_time 1000000`}, // 0.001 seconds - {"memory.stat", `cache 235000000 -rss 234000000 -inactive_anon 200000000 -active_anon 34000000 -inactive_file 100000000 -active_file 135000000`}, - }, - expectedStats: Stats{ - ParentStats: CgroupStats{ - CPUThrottledCount: 50, - CPUThrottledDuration: 0.001, - MemoryUsage: 1234000000, - MemoryLimit: 2000000000, - OOMKills: 3, - UnderOOM: true, - Anon: 234000000, - ActiveAnon: 34000000, - InactiveAnon: 200000000, - File: 235000000, - ActiveFile: 135000000, - InactiveFile: 100000000, - }, - }, - }, - } { - t.Run(tc.desc, func(t *testing.T) { - mock := newMockV1(t) - - config := defaultCgroupsConfig() - config.Repositories.Count = 1 - config.Repositories.MemoryBytes = 2000000000 - config.Repositories.CPUShares = 16 - config.Mountpoint = mock.root - - v1Manager := mock.newCgroupManager(config, testhelper.SharedLogger(t), 1) - - mock.setupMockCgroupFiles(t, v1Manager, []uint{}, tc.mockFiles...) - require.NoError(t, v1Manager.Setup()) - - stats, err := v1Manager.Stats() - require.NoError(t, err) - require.Equal(t, tc.expectedStats, stats) - }) - } -} - func requireShardsV1(t *testing.T, mock *mockCgroupV1, mgr *CGroupManager, pid int, expectedShards ...uint) { t.Helper() diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index 65ee215f3f..0bc7a9ed25 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "golang.org/x/exp/slices" ) @@ -26,83 +25,6 @@ func defaultCgroupsV2Config() cgroups.Config { } } -func TestStatsV2(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - desc string - mockFiles []mockCgroupFile - expectedStats Stats - }{ - { - desc: "empty statistics", - mockFiles: []mockCgroupFile{ - {"memory.current", "0"}, - {"memory.max", "0"}, - {"cpu.stat", ""}, - }, - expectedStats: Stats{}, - }, - { - desc: "cgroupfs recorded some stats", - mockFiles: []mockCgroupFile{ - {"memory.max", "2000000000"}, - {"memory.current", "1234000000"}, - {"memory.events", `low 1 -high 2 -max 3 -oom 4 -oom_kill 5`}, - {"nr_throttled", "50"}, - {"throttled_usec", "1000000"}, - {"cpu.stat", `nr_periods 10 -nr_throttled 50 -throttled_usec 1000000`}, // 0.001 seconds - {"memory.stat", `anon 234000000 -file 235000000 -inactive_anon 200000000 -active_anon 34000000 -inactive_file 100000000 -active_file 135000000`}, - }, - expectedStats: Stats{ - ParentStats: CgroupStats{ - CPUThrottledCount: 50, - CPUThrottledDuration: 0.001, - MemoryUsage: 1234000000, - MemoryLimit: 2000000000, - OOMKills: 5, - Anon: 234000000, - ActiveAnon: 34000000, - InactiveAnon: 200000000, - File: 235000000, - ActiveFile: 135000000, - InactiveFile: 100000000, - }, - }, - }, - } { - t.Run(tc.desc, func(t *testing.T) { - mock := newMockV2(t) - - config := defaultCgroupsConfig() - config.Repositories.Count = 1 - config.Repositories.MemoryBytes = 2000000000 - config.Repositories.CPUShares = 16 - config.Mountpoint = mock.root - - v2Manager := mock.newCgroupManager(config, testhelper.SharedLogger(t), 1) - - mock.setupMockCgroupFiles(t, v2Manager, []uint{0}, tc.mockFiles...) - require.NoError(t, v2Manager.Setup()) - - stats, err := v2Manager.Stats() - require.NoError(t, err) - require.Equal(t, tc.expectedStats, stats) - }) - } -} - func calculateWantCPUWeight(wantCPUWeight int) int { if wantCPUWeight == 0 { return 0 -- GitLab From baa2aeb2a7d69635a79dc7395b62acffd29339ef Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Wed, 8 Nov 2023 23:01:07 -0500 Subject: [PATCH 11/17] cgroups: Move remaining test functions to handler Now that all version-specific tests have been converted to unified tests in `handler_linux_test.go`, move the remaining helper functions into that file as well. Remove the version-specific test files as they are now empty. --- internal/cgroups/handler_linux_test.go | 55 ++++++++++++++++++ internal/cgroups/v1_linux_test.go | 80 -------------------------- internal/cgroups/v2_linux_test.go | 80 -------------------------- 3 files changed, 55 insertions(+), 160 deletions(-) delete mode 100644 internal/cgroups/v1_linux_test.go delete mode 100644 internal/cgroups/v2_linux_test.go diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index e419cbf671..9556b393bb 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -22,6 +22,18 @@ import ( "golang.org/x/exp/slices" ) +func defaultCgroupsConfig() cgroups.Config { + return cgroups.Config{ + HierarchyRoot: "gitaly", + Repositories: cgroups.Repositories{ + Count: 3, + MemoryBytes: 1024000, + CPUShares: 256, + CPUQuotaUs: 200, + }, + } +} + func TestNewManager(t *testing.T) { cfg := cgroups.Config{Repositories: cgroups.Repositories{Count: 10}} @@ -821,6 +833,49 @@ func requireCgroupComponents(t *testing.T, version int, root string, cgroupPath } } +func readCgroupFile(t *testing.T, path string) []byte { + t.Helper() + + // The cgroups package defaults to permission 0 as it expects the file to be existing (the kernel creates the file) + // and its testing override the permission private variable to something sensible, hence we have to chmod ourselves + // so we can read the file. + require.NoError(t, os.Chmod(path, perm.PublicFile)) + + return testhelper.MustReadFile(t, path) +} + +func requireCgroupWithInt(t *testing.T, cgroupFile string, want int) { + t.Helper() + + if want <= 0 { + return + } + + require.Equal(t, + string(readCgroupFile(t, cgroupFile)), + strconv.Itoa(want), + ) +} + +func requireCgroupWithString(t *testing.T, cgroupFile string, want string) { + t.Helper() + + if want == "" { + return + } + require.Equal(t, + string(readCgroupFile(t, cgroupFile)), + want, + ) +} + +func calculateWantCPUWeight(wantCPUWeight int) int { + if wantCPUWeight == 0 { + return 0 + } + return 1 + ((wantCPUWeight-2)*9999)/262142 +} + func requireShards(t *testing.T, version int, mock mockCgroup, mgr *CGroupManager, pid int, expectedShards ...uint) { for shard := uint(0); shard < mgr.cfg.Repositories.Count; shard++ { shouldExist := slices.Contains(expectedShards, shard) diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go deleted file mode 100644 index cd00e4fa2f..0000000000 --- a/internal/cgroups/v1_linux_test.go +++ /dev/null @@ -1,80 +0,0 @@ -//go:build linux - -package cgroups - -import ( - "fmt" - "os" - "path/filepath" - "strconv" - "testing" - - "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" - "golang.org/x/exp/slices" -) - -func defaultCgroupsConfig() cgroups.Config { - return cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 3, - MemoryBytes: 1024000, - CPUShares: 256, - CPUQuotaUs: 200, - }, - } -} - -func requireShardsV1(t *testing.T, mock *mockCgroupV1, mgr *CGroupManager, pid int, expectedShards ...uint) { - t.Helper() - - for shard := uint(0); shard < mgr.cfg.Repositories.Count; shard++ { - for _, s := range mock.subsystems { - cgroupPath := filepath.Join("gitaly", - fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", shard)) - diskPath := filepath.Join(mock.root, string(s.Name()), cgroupPath) - - if slices.Contains(expectedShards, shard) { - require.DirExists(t, diskPath) - - cgLock := mgr.status.getLock(cgroupPath) - require.True(t, cgLock.isCreated()) - } else { - require.NoDirExists(t, diskPath) - - // Confirm we pre-populated this map entry. - _, lockInserted := mgr.status.m[cgroupPath] - require.True(t, lockInserted) - } - } - } -} - -func requireCgroup(t *testing.T, cgroupFile string, want int) { - t.Helper() - - if want <= 0 { - // If files doesn't exist kernel will create it with default values - require.NoFileExistsf(t, cgroupFile, "cgroup file should not exist: %q", cgroupFile) - return - } - - require.Equal(t, - string(readCgroupFile(t, cgroupFile)), - strconv.Itoa(want), - ) -} - -func readCgroupFile(t *testing.T, path string) []byte { - t.Helper() - - // The cgroups package defaults to permission 0 as it expects the file to be existing (the kernel creates the file) - // and its testing override the permission private variable to something sensible, hence we have to chmod ourselves - // so we can read the file. - require.NoError(t, os.Chmod(path, perm.PublicFile)) - - return testhelper.MustReadFile(t, path) -} diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go deleted file mode 100644 index 0bc7a9ed25..0000000000 --- a/internal/cgroups/v2_linux_test.go +++ /dev/null @@ -1,80 +0,0 @@ -//go:build linux - -package cgroups - -import ( - "fmt" - "path/filepath" - "strconv" - "testing" - - "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "golang.org/x/exp/slices" -) - -func defaultCgroupsV2Config() cgroups.Config { - return cgroups.Config{ - HierarchyRoot: "gitaly", - Repositories: cgroups.Repositories{ - Count: 3, - MemoryBytes: 1024000, - CPUShares: 256, - CPUQuotaUs: 2000, - }, - } -} - -func calculateWantCPUWeight(wantCPUWeight int) int { - if wantCPUWeight == 0 { - return 0 - } - return 1 + ((wantCPUWeight-2)*9999)/262142 -} - -func requireShardsV2(t *testing.T, mock *mockCgroupV2, mgr *CGroupManager, pid int, expectedShards ...uint) { - t.Helper() - - for shard := uint(0); shard < mgr.cfg.Repositories.Count; shard++ { - cgroupPath := filepath.Join("gitaly", fmt.Sprintf("gitaly-%d", pid), fmt.Sprintf("repos-%d", shard)) - diskPath := filepath.Join(mock.root, cgroupPath) - - if slices.Contains(expectedShards, shard) { - require.DirExists(t, diskPath) - - cgLock := mgr.status.getLock(cgroupPath) - require.True(t, cgLock.isCreated()) - } else { - require.NoDirExists(t, diskPath) - - // Confirm we pre-populated this map entry. - _, lockInserted := mgr.status.m[cgroupPath] - require.True(t, lockInserted) - } - } -} - -func requireCgroupWithString(t *testing.T, cgroupFile string, want string) { - t.Helper() - - if want == "" { - return - } - require.Equal(t, - string(readCgroupFile(t, cgroupFile)), - want, - ) -} - -func requireCgroupWithInt(t *testing.T, cgroupFile string, want int) { - t.Helper() - - if want <= 0 { - return - } - - require.Equal(t, - string(readCgroupFile(t, cgroupFile)), - strconv.Itoa(want), - ) -} -- GitLab From 722547075ed32ca72a26fa197c958cb2838cfaee Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 9 Nov 2023 14:50:50 -0500 Subject: [PATCH 12/17] cgroups: Add genericHandler Create a new `genericHandler` to unify most of the logic for cgroups-v1 and v2. This is made generic over two parameters: T: The cgroup type, `cgroup1.Cgroup` or `*cgroup2.Manager`. H: The hierarchy or mountpoint passed when creating a cgroup. For `cgroup1.New` this is a `cgroup1.Hierarchy`, for `cgroup2.New` this is a `string` with the path to the cgroup's mountpoint. Four generic functions are added for the operations that are easily abstracted over. Metrics and stats collection are not included as they involve significant version-specific logic. --- internal/cgroups/handler_linux.go | 106 ++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 internal/cgroups/handler_linux.go diff --git a/internal/cgroups/handler_linux.go b/internal/cgroups/handler_linux.go new file mode 100644 index 0000000000..eaf23b2dfa --- /dev/null +++ b/internal/cgroups/handler_linux.go @@ -0,0 +1,106 @@ +//go:build linux + +package cgroups + +import ( + "github.com/containerd/cgroups/v3/cgroup1" + "github.com/containerd/cgroups/v3/cgroup2" + "github.com/opencontainers/runtime-spec/specs-go" + cgroupscfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" + "gitlab.com/gitlab-org/gitaly/v16/internal/log" +) + +type ( + // createCgroupFunc is a function that creates a new cgroup. + createCgroupFunc[T any, H any] func(hierarchy H, resources *specs.LinuxResources, path string) (T, error) + // loadCgroupFunc is a function that loads an existing cgroup. + loadCgroupFunc[T any, H any] func(hierarchy H, path string) (T, error) + // addToCgroupFunc is a function that adds a process to an existing cgroup. + addToCgroupFunc[T any] func(control T, pid int) error + // deleteCgroupFunc is a function that deletes a cgroup. + deleteCgroupFunc[T any] func(control T) error +) + +// genericHandler is a cgroup handler that can be instantiated for either cgroups-v1 +// or cgroups-v2. +type genericHandler[T any, H any] struct { + cfg cgroupscfg.Config + logger log.Logger + pid int + supportsClone bool + + // hierarchy is either a cgroup1.Hierarchy or the cgroup2 Mountpoint path. + hierarchy H + createFunc createCgroupFunc[T, H] + loadFunc loadCgroupFunc[T, H] + addFunc addToCgroupFunc[T] + deleteFunc deleteCgroupFunc[T] + + metrics *cgroupsMetrics +} + +func newV1GenericHandler( + cfg cgroupscfg.Config, + logger log.Logger, + pid int, +) *genericHandler[cgroup1.Cgroup, cgroup1.Hierarchy] { + return &genericHandler[cgroup1.Cgroup, cgroup1.Hierarchy]{ + cfg: cfg, + logger: logger, + pid: pid, + supportsClone: false, + hierarchy: func() ([]cgroup1.Subsystem, error) { + return defaultSubsystems(cfg.Mountpoint) + }, + metrics: newV1CgroupsMetrics(), + createFunc: func(hierarchy cgroup1.Hierarchy, resources *specs.LinuxResources, cgroupPath string) (cgroup1.Cgroup, error) { + return cgroup1.New( + cgroup1.StaticPath(cgroupPath), + resources, + cgroup1.WithHiearchy(hierarchy)) + }, + loadFunc: func(hierarchy cgroup1.Hierarchy, cgroupPath string) (cgroup1.Cgroup, error) { + return cgroup1.Load( + cgroup1.StaticPath(cgroupPath), + cgroup1.WithHiearchy(hierarchy), + ) + }, + addFunc: func(control cgroup1.Cgroup, pid int) error { + return control.Add(cgroup1.Process{Pid: pid}) + }, + deleteFunc: func(control cgroup1.Cgroup) error { + return control.Delete() + }, + } +} + +func newV2GenericHandler( + cfg cgroupscfg.Config, + logger log.Logger, + pid int, +) *genericHandler[*cgroup2.Manager, string] { + return &genericHandler[*cgroup2.Manager, string]{ + cfg: cfg, + logger: logger, + pid: pid, + supportsClone: true, + hierarchy: cfg.Mountpoint, + metrics: newV2CgroupsMetrics(), + createFunc: func(mountpoint string, resources *specs.LinuxResources, cgroupPath string) (*cgroup2.Manager, error) { + return cgroup2.NewManager( + mountpoint, + "/"+cgroupPath, + cgroup2.ToResources(resources), + ) + }, + loadFunc: func(mountpoint string, cgroupPath string) (*cgroup2.Manager, error) { + return cgroup2.Load("/"+cgroupPath, cgroup2.WithMountpoint(mountpoint)) + }, + addFunc: func(control *cgroup2.Manager, pid int) error { + return control.AddProc(uint64(pid)) + }, + deleteFunc: func(control *cgroup2.Manager) error { + return control.Delete() + }, + } +} -- GitLab From 8539b2d10ff41ac9256b8f8c8610d59d6d9fb155 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 9 Nov 2023 15:02:07 -0500 Subject: [PATCH 13/17] cgroups: Implement generic methods for handler Add the methods to `genericHandler` that contain no version-specific logic. --- internal/cgroups/handler_linux.go | 62 +++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/internal/cgroups/handler_linux.go b/internal/cgroups/handler_linux.go index eaf23b2dfa..6af06d332e 100644 --- a/internal/cgroups/handler_linux.go +++ b/internal/cgroups/handler_linux.go @@ -3,9 +3,14 @@ package cgroups import ( + "fmt" + "path/filepath" + "strings" + "github.com/containerd/cgroups/v3/cgroup1" "github.com/containerd/cgroups/v3/cgroup2" "github.com/opencontainers/runtime-spec/specs-go" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" cgroupscfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/log" ) @@ -104,3 +109,60 @@ func newV2GenericHandler( }, } } + +func (cvh *genericHandler[T, H]) currentProcessCgroup() string { + return config.GetGitalyProcessTempDir(cvh.cfg.HierarchyRoot, cvh.pid) +} + +func (cvh *genericHandler[T, H]) createCgroup(repoResources *specs.LinuxResources, cgroupPath string) error { + _, err := cvh.createFunc(cvh.hierarchy, repoResources, cgroupPath) + return err +} + +func (cvh *genericHandler[T, H]) addToCgroup(pid int, cgroupPath string) error { + control, err := cvh.loadFunc(cvh.hierarchy, cgroupPath) + if err != nil { + return err + } + + if err := cvh.addFunc(control, pid); err != nil { + // Command could finish so quickly before we can add it to a cgroup, so + // we don't consider it an error. + if strings.Contains(err.Error(), "no such process") { + return nil + } + return fmt.Errorf("failed adding process to cgroup: %w", err) + } + + return nil +} + +func (cvh *genericHandler[T, H]) setupParent(parentResources *specs.LinuxResources) error { + if _, err := cvh.createFunc(cvh.hierarchy, parentResources, cvh.currentProcessCgroup()); err != nil { + return fmt.Errorf("failed creating parent cgroup: %w", err) + } + return nil +} + +func (cvh *genericHandler[T, H]) cleanup() error { + processCgroupPath := cvh.currentProcessCgroup() + + control, err := cvh.loadFunc(cvh.hierarchy, processCgroupPath) + if err != nil { + return err + } + + if err := cvh.deleteFunc(control); err != nil { + return fmt.Errorf("failed cleaning up cgroup %s: %w", processCgroupPath, err) + } + + return nil +} + +func (cvh *genericHandler[T, H]) repoPath(groupID int) string { + return filepath.Join(cvh.currentProcessCgroup(), fmt.Sprintf("repos-%d", groupID)) +} + +func (cvh *genericHandler[T, H]) supportsCloneIntoCgroup() bool { + return cvh.supportsClone +} -- GitLab From da3e644d31df21ca28654499a4927f280c93ce86 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 9 Nov 2023 15:03:07 -0500 Subject: [PATCH 14/17] cgroups: Add stats method Add the `stats()` method to `genericHandler`. We use reflection here as the stats collected vary significantly between v1 and v2. --- internal/cgroups/handler_linux.go | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/internal/cgroups/handler_linux.go b/internal/cgroups/handler_linux.go index 6af06d332e..5f17843774 100644 --- a/internal/cgroups/handler_linux.go +++ b/internal/cgroups/handler_linux.go @@ -3,9 +3,11 @@ package cgroups import ( + "errors" "fmt" "path/filepath" "strings" + "time" "github.com/containerd/cgroups/v3/cgroup1" "github.com/containerd/cgroups/v3/cgroup2" @@ -166,3 +168,59 @@ func (cvh *genericHandler[T, H]) repoPath(groupID int) string { func (cvh *genericHandler[T, H]) supportsCloneIntoCgroup() bool { return cvh.supportsClone } + +func (cvh *genericHandler[T, H]) stats() (Stats, error) { + processCgroupPath := cvh.currentProcessCgroup() + + control, err := cvh.loadFunc(cvh.hierarchy, processCgroupPath) + if err != nil { + return Stats{}, err + } + + switch c := any(control).(type) { + case cgroup1.Cgroup: + return v1Stats(c, processCgroupPath) + case *cgroup2.Manager: + return v2stats(c, processCgroupPath) + default: + return Stats{}, errors.New("unknown cgroup type") + } +} + +func v1Stats(control cgroup1.Cgroup, processCgroupPath string) (Stats, error) { + metrics, err := control.Stat() + if err != nil { + return Stats{}, fmt.Errorf("failed to fetch metrics %s: %w", processCgroupPath, err) + } + + return Stats{ + ParentStats: CgroupStats{ + CPUThrottledCount: metrics.CPU.Throttling.ThrottledPeriods, + CPUThrottledDuration: float64(metrics.CPU.Throttling.ThrottledTime) / float64(time.Second), + MemoryUsage: metrics.Memory.Usage.Usage, + MemoryLimit: metrics.Memory.Usage.Limit, + OOMKills: metrics.MemoryOomControl.OomKill, + UnderOOM: metrics.MemoryOomControl.UnderOom != 0, + }, + }, nil +} + +func v2stats(control *cgroup2.Manager, processCgroupPath string) (Stats, error) { + metrics, err := control.Stat() + if err != nil { + return Stats{}, fmt.Errorf("failed to fetch metrics %s: %w", processCgroupPath, err) + } + + stats := Stats{ + ParentStats: CgroupStats{ + CPUThrottledCount: metrics.CPU.NrThrottled, + CPUThrottledDuration: float64(metrics.CPU.ThrottledUsec) / float64(time.Second), + MemoryUsage: metrics.Memory.Usage, + MemoryLimit: metrics.Memory.UsageLimit, + }, + } + if metrics.MemoryEvents != nil { + stats.ParentStats.OOMKills = metrics.MemoryEvents.OomKill + } + return stats, nil +} -- GitLab From 1c2e90f0c4e9f7102369103c26db9b8aeebffd2b Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 9 Nov 2023 15:04:02 -0500 Subject: [PATCH 15/17] cgroups: Add collect method Add the `collect()` method to `genericHandler`. As with `stats()`, the types we deal with here vary between v1 and v2, so we keep separate functions for them. --- internal/cgroups/handler_linux.go | 126 ++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/internal/cgroups/handler_linux.go b/internal/cgroups/handler_linux.go index 5f17843774..b9869e5430 100644 --- a/internal/cgroups/handler_linux.go +++ b/internal/cgroups/handler_linux.go @@ -12,6 +12,7 @@ import ( "github.com/containerd/cgroups/v3/cgroup1" "github.com/containerd/cgroups/v3/cgroup2" "github.com/opencontainers/runtime-spec/specs-go" + "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" cgroupscfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" "gitlab.com/gitlab-org/gitaly/v16/internal/log" @@ -187,6 +188,22 @@ func (cvh *genericHandler[T, H]) stats() (Stats, error) { } } +func (cvh *genericHandler[T, H]) collect(repoPath string, ch chan<- prometheus.Metric) { + logger := cvh.logger.WithField("cgroup_path", repoPath) + control, err := cvh.loadFunc(cvh.hierarchy, repoPath) + if err != nil { + logger.WithError(err).Warn("unable to load cgroup controller") + return + } + + switch c := any(control).(type) { + case cgroup1.Cgroup: + v1Collect(c, any(cvh.hierarchy).(cgroup1.Hierarchy), cvh.metrics, repoPath, cvh.logger, ch) + case *cgroup2.Manager: + v2Collect(c, cvh.metrics, repoPath, cvh.logger, ch) + } +} + func v1Stats(control cgroup1.Cgroup, processCgroupPath string) (Stats, error) { metrics, err := control.Stat() if err != nil { @@ -224,3 +241,112 @@ func v2stats(control *cgroup2.Manager, processCgroupPath string) (Stats, error) } return stats, nil } + +func v1Collect(control cgroup1.Cgroup, hierarchy cgroup1.Hierarchy, m *cgroupsMetrics, repoPath string, logger log.Logger, ch chan<- prometheus.Metric) { + if metrics, err := control.Stat(); err != nil { + logger.WithError(err).Warn("unable to get cgroup stats") + } else { + memoryMetric := m.memoryReclaimAttemptsTotal.WithLabelValues(repoPath) + memoryMetric.Set(float64(metrics.Memory.Usage.Failcnt)) + ch <- memoryMetric + + cpuUserMetric := m.cpuUsage.WithLabelValues(repoPath, "user") + cpuUserMetric.Set(float64(metrics.CPU.Usage.User)) + ch <- cpuUserMetric + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSPeriods, + prometheus.CounterValue, + float64(metrics.CPU.Throttling.Periods), + repoPath, + ) + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSThrottledPeriods, + prometheus.CounterValue, + float64(metrics.CPU.Throttling.ThrottledPeriods), + repoPath, + ) + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSThrottledTime, + prometheus.CounterValue, + float64(metrics.CPU.Throttling.ThrottledTime)/float64(time.Second), + repoPath, + ) + + cpuKernelMetric := m.cpuUsage.WithLabelValues(repoPath, "kernel") + cpuKernelMetric.Set(float64(metrics.CPU.Usage.Kernel)) + ch <- cpuKernelMetric + } + + if subsystems, err := hierarchy(); err != nil { + logger.WithError(err).Warn("unable to get cgroup hierarchy") + } else { + for _, subsystem := range subsystems { + processes, err := control.Processes(subsystem.Name(), true) + if err != nil { + logger.WithField("subsystem", subsystem.Name()). + WithError(err). + Warn("unable to get process list") + continue + } + + procsMetric := m.procs.WithLabelValues(repoPath, string(subsystem.Name())) + procsMetric.Set(float64(len(processes))) + ch <- procsMetric + } + } +} + +func v2Collect(control *cgroup2.Manager, m *cgroupsMetrics, repoPath string, logger log.Logger, ch chan<- prometheus.Metric) { + if metrics, err := control.Stat(); err != nil { + logger.WithError(err).Warn("unable to get cgroup stats") + } else { + cpuUserMetric := m.cpuUsage.WithLabelValues(repoPath, "user") + cpuUserMetric.Set(float64(metrics.CPU.UserUsec)) + ch <- cpuUserMetric + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSPeriods, + prometheus.CounterValue, + float64(metrics.CPU.NrPeriods), + repoPath, + ) + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSThrottledPeriods, + prometheus.CounterValue, + float64(metrics.CPU.NrThrottled), + repoPath, + ) + + ch <- prometheus.MustNewConstMetric( + m.cpuCFSThrottledTime, + prometheus.CounterValue, + float64(metrics.CPU.ThrottledUsec)/float64(time.Second), + repoPath, + ) + + cpuKernelMetric := m.cpuUsage.WithLabelValues(repoPath, "kernel") + cpuKernelMetric.Set(float64(metrics.CPU.SystemUsec)) + ch <- cpuKernelMetric + } + + if subsystems, err := control.Controllers(); err != nil { + logger.WithError(err).Warn("unable to get cgroup hierarchy") + } else { + processes, err := control.Procs(true) + if err != nil { + logger.WithError(err). + Warn("unable to get process list") + return + } + + for _, subsystem := range subsystems { + procsMetric := m.procs.WithLabelValues(repoPath, subsystem) + procsMetric.Set(float64(len(processes))) + ch <- procsMetric + } + } +} -- GitLab From 02b3a2d7513a17327789af186210dc17528f1702 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Fri, 10 Nov 2023 11:21:47 -0500 Subject: [PATCH 16/17] cgroups: Start using genericHandler Start using `genericHandler` now that it fully implements the `cgroupHandler` interface. --- internal/cgroups/handler_linux_test.go | 8 +++++--- internal/cgroups/manager_linux.go | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go index 9556b393bb..401151fb30 100644 --- a/internal/cgroups/handler_linux_test.go +++ b/internal/cgroups/handler_linux_test.go @@ -13,6 +13,8 @@ import ( "testing" cgrps "github.com/containerd/cgroups/v3" + "github.com/containerd/cgroups/v3/cgroup1" + "github.com/containerd/cgroups/v3/cgroup2" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -38,11 +40,11 @@ func TestNewManager(t *testing.T) { cfg := cgroups.Config{Repositories: cgroups.Repositories{Count: 10}} manager := newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Legacy) - require.IsType(t, &cgroupV1Handler{}, manager.handler) + require.IsType(t, &genericHandler[cgroup1.Cgroup, cgroup1.Hierarchy]{}, manager.handler) manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Hybrid) - require.IsType(t, &cgroupV1Handler{}, manager.handler) + require.IsType(t, &genericHandler[cgroup1.Cgroup, cgroup1.Hierarchy]{}, manager.handler) manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unified) - require.IsType(t, &cgroupV2Handler{}, manager.handler) + require.IsType(t, &genericHandler[*cgroup2.Manager, string]{}, manager.handler) manager = newCgroupManagerWithMode(cfg, testhelper.SharedLogger(t), 1, cgrps.Unavailable) require.Nil(t, manager) } diff --git a/internal/cgroups/manager_linux.go b/internal/cgroups/manager_linux.go index f55a7c58f6..da10c15d51 100644 --- a/internal/cgroups/manager_linux.go +++ b/internal/cgroups/manager_linux.go @@ -92,9 +92,9 @@ func newCgroupManagerWithMode(cfg cgroupscfg.Config, logger log.Logger, pid int, var handler cgroupHandler switch mode { case cgrps.Legacy, cgrps.Hybrid: - handler = newV1Handler(cfg, logger, pid) + handler = newV1GenericHandler(cfg, logger, pid) case cgrps.Unified: - handler = newV2Handler(cfg, logger, pid) + handler = newV2GenericHandler(cfg, logger, pid) logger.Warn("Gitaly now includes experimental support for CgroupV2. Please proceed with caution and use this experimental feature at your own risk") default: logger.Warn("Gitaly has encountered an issue while trying to detect the version of the system's cgroup. As a result, all subsequent commands will be executed without cgroup support. Please check the system's cgroup configuration and try again") -- GitLab From 2bb704a1deb70925ad1d825a9a75e03d1a218e30 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 9 Nov 2023 15:05:49 -0500 Subject: [PATCH 17/17] cgroups: Remove v1 and v2 handlers Remove the now unused v1 and v2 handler implementations, copying still-used freestanding functions into `handler_linux.go`. --- internal/cgroups/handler_linux.go | 40 +++++ internal/cgroups/v1_linux.go | 237 ------------------------------ internal/cgroups/v2_linux.go | 220 --------------------------- 3 files changed, 40 insertions(+), 457 deletions(-) delete mode 100644 internal/cgroups/v1_linux.go delete mode 100644 internal/cgroups/v2_linux.go diff --git a/internal/cgroups/handler_linux.go b/internal/cgroups/handler_linux.go index b9869e5430..2b92253fb8 100644 --- a/internal/cgroups/handler_linux.go +++ b/internal/cgroups/handler_linux.go @@ -5,6 +5,7 @@ package cgroups import ( "errors" "fmt" + "io/fs" "path/filepath" "strings" "time" @@ -350,3 +351,42 @@ func v2Collect(control *cgroup2.Manager, m *cgroupsMetrics, repoPath string, log } } } + +func defaultSubsystems(root string) ([]cgroup1.Subsystem, error) { + subsystems := []cgroup1.Subsystem{ + cgroup1.NewMemory(root, cgroup1.OptionalSwap()), + cgroup1.NewCpu(root), + } + + return subsystems, nil +} + +func pruneOldCgroupsV1(cfg cgroupscfg.Config, logger log.Logger) { + if err := config.PruneOldGitalyProcessDirectories( + logger, + filepath.Join(cfg.Mountpoint, "memory", + cfg.HierarchyRoot), + ); err != nil { + logger.WithError(err).Error("failed to clean up memory cgroups") + } + + if err := config.PruneOldGitalyProcessDirectories( + logger, + filepath.Join(cfg.Mountpoint, "cpu", + cfg.HierarchyRoot), + ); err != nil { + logger.WithError(err).Error("failed to clean up cpu cgroups") + } +} + +func pruneOldCgroupsV2(cfg cgroupscfg.Config, logger log.Logger) { + if err := config.PruneOldGitalyProcessDirectories( + logger, + filepath.Join(cfg.Mountpoint, cfg.HierarchyRoot), + ); err != nil { + var pathError *fs.PathError + if !errors.As(err, &pathError) { + logger.WithError(err).Error("failed to clean up cpu cgroups") + } + } +} diff --git a/internal/cgroups/v1_linux.go b/internal/cgroups/v1_linux.go deleted file mode 100644 index 53f6cb5a43..0000000000 --- a/internal/cgroups/v1_linux.go +++ /dev/null @@ -1,237 +0,0 @@ -//go:build linux - -package cgroups - -import ( - "fmt" - "path/filepath" - "strings" - "time" - - "github.com/containerd/cgroups/v3/cgroup1" - specs "github.com/opencontainers/runtime-spec/specs-go" - "github.com/prometheus/client_golang/prometheus" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" - cgroupscfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "gitlab.com/gitlab-org/gitaly/v16/internal/log" -) - -type cgroupV1Handler struct { - cfg cgroupscfg.Config - logger log.Logger - hierarchy func() ([]cgroup1.Subsystem, error) - - *cgroupsMetrics - pid int -} - -func newV1Handler(cfg cgroupscfg.Config, logger log.Logger, pid int) *cgroupV1Handler { - return &cgroupV1Handler{ - cfg: cfg, - logger: logger, - pid: pid, - hierarchy: func() ([]cgroup1.Subsystem, error) { - return defaultSubsystems(cfg.Mountpoint) - }, - cgroupsMetrics: newV1CgroupsMetrics(), - } -} - -func (cvh *cgroupV1Handler) setupParent(parentResources *specs.LinuxResources) error { - if _, err := cgroup1.New( - cgroup1.StaticPath(cvh.currentProcessCgroup()), - parentResources, - cgroup1.WithHiearchy(cvh.hierarchy), - ); err != nil { - return fmt.Errorf("failed creating parent cgroup: %w", err) - } - return nil -} - -func (cvh *cgroupV1Handler) createCgroup(reposResources *specs.LinuxResources, cgroupPath string) error { - _, err := cgroup1.New( - cgroup1.StaticPath(cgroupPath), - reposResources, - cgroup1.WithHiearchy(cvh.hierarchy), - ) - - return err -} - -func (cvh *cgroupV1Handler) addToCgroup(pid int, cgroupPath string) error { - control, err := cvh.loadCgroup(cgroupPath) - if err != nil { - return err - } - - if err := control.Add(cgroup1.Process{Pid: pid}); err != nil { - // Command could finish so quickly before we can add it to a cgroup, so - // we don't consider it an error. - if strings.Contains(err.Error(), "no such process") { - return nil - } - return fmt.Errorf("failed adding process to cgroup: %w", err) - } - - return nil -} - -func (cvh *cgroupV1Handler) loadCgroup(cgroupPath string) (cgroup1.Cgroup, error) { - control, err := cgroup1.Load( - cgroup1.StaticPath(cgroupPath), - cgroup1.WithHiearchy(cvh.hierarchy), - ) - if err != nil { - return nil, fmt.Errorf("failed loading %s cgroup: %w", cgroupPath, err) - } - return control, nil -} - -func (cvh *cgroupV1Handler) collect(repoPath string, ch chan<- prometheus.Metric) { - logger := cvh.logger.WithField("cgroup_path", repoPath) - control, err := cvh.loadCgroup(repoPath) - if err != nil { - logger.WithError(err).Warn("unable to load cgroup controller") - return - } - - if metrics, err := control.Stat(); err != nil { - logger.WithError(err).Warn("unable to get cgroup stats") - } else { - memoryMetric := cvh.memoryReclaimAttemptsTotal.WithLabelValues(repoPath) - memoryMetric.Set(float64(metrics.Memory.Usage.Failcnt)) - ch <- memoryMetric - - cpuUserMetric := cvh.cpuUsage.WithLabelValues(repoPath, "user") - cpuUserMetric.Set(float64(metrics.CPU.Usage.User)) - ch <- cpuUserMetric - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSPeriods, - prometheus.CounterValue, - float64(metrics.CPU.Throttling.Periods), - repoPath, - ) - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSThrottledPeriods, - prometheus.CounterValue, - float64(metrics.CPU.Throttling.ThrottledPeriods), - repoPath, - ) - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSThrottledTime, - prometheus.CounterValue, - float64(metrics.CPU.Throttling.ThrottledTime)/float64(time.Second), - repoPath, - ) - - cpuKernelMetric := cvh.cpuUsage.WithLabelValues(repoPath, "kernel") - cpuKernelMetric.Set(float64(metrics.CPU.Usage.Kernel)) - ch <- cpuKernelMetric - } - - if subsystems, err := cvh.hierarchy(); err != nil { - logger.WithError(err).Warn("unable to get cgroup hierarchy") - } else { - for _, subsystem := range subsystems { - processes, err := control.Processes(subsystem.Name(), true) - if err != nil { - logger.WithField("subsystem", subsystem.Name()). - WithError(err). - Warn("unable to get process list") - continue - } - - procsMetric := cvh.procs.WithLabelValues(repoPath, string(subsystem.Name())) - procsMetric.Set(float64(len(processes))) - ch <- procsMetric - } - } -} - -func (cvh *cgroupV1Handler) cleanup() error { - processCgroupPath := cvh.currentProcessCgroup() - - control, err := cvh.loadCgroup(processCgroupPath) - if err != nil { - return err - } - - if err := control.Delete(); err != nil { - return fmt.Errorf("failed cleaning up cgroup %s: %w", processCgroupPath, err) - } - - return nil -} - -func (cvh *cgroupV1Handler) repoPath(groupID int) string { - return filepath.Join(cvh.currentProcessCgroup(), fmt.Sprintf("repos-%d", groupID)) -} - -func (cvh *cgroupV1Handler) currentProcessCgroup() string { - return config.GetGitalyProcessTempDir(cvh.cfg.HierarchyRoot, cvh.pid) -} - -func (cvh *cgroupV1Handler) stats() (Stats, error) { - processCgroupPath := cvh.currentProcessCgroup() - - control, err := cvh.loadCgroup(processCgroupPath) - if err != nil { - return Stats{}, err - } - - metrics, err := control.Stat() - if err != nil { - return Stats{}, fmt.Errorf("failed to fetch metrics %s: %w", processCgroupPath, err) - } - - return Stats{ - ParentStats: CgroupStats{ - CPUThrottledCount: metrics.CPU.Throttling.ThrottledPeriods, - CPUThrottledDuration: float64(metrics.CPU.Throttling.ThrottledTime) / float64(time.Second), - MemoryUsage: metrics.Memory.Usage.Usage, - MemoryLimit: metrics.Memory.Usage.Limit, - OOMKills: metrics.MemoryOomControl.OomKill, - UnderOOM: metrics.MemoryOomControl.UnderOom != 0, - Anon: metrics.Memory.RSS, - ActiveAnon: metrics.Memory.ActiveAnon, - InactiveAnon: metrics.Memory.InactiveAnon, - File: metrics.Memory.Cache, - ActiveFile: metrics.Memory.ActiveFile, - InactiveFile: metrics.Memory.InactiveFile, - }, - }, nil -} - -func (cvh *cgroupV1Handler) supportsCloneIntoCgroup() bool { - return false -} - -func defaultSubsystems(root string) ([]cgroup1.Subsystem, error) { - subsystems := []cgroup1.Subsystem{ - cgroup1.NewMemory(root, cgroup1.OptionalSwap()), - cgroup1.NewCpu(root), - } - - return subsystems, nil -} - -func pruneOldCgroupsV1(cfg cgroupscfg.Config, logger log.Logger) { - if err := config.PruneOldGitalyProcessDirectories( - logger, - filepath.Join(cfg.Mountpoint, "memory", - cfg.HierarchyRoot), - ); err != nil { - logger.WithError(err).Error("failed to clean up memory cgroups") - } - - if err := config.PruneOldGitalyProcessDirectories( - logger, - filepath.Join(cfg.Mountpoint, "cpu", - cfg.HierarchyRoot), - ); err != nil { - logger.WithError(err).Error("failed to clean up cpu cgroups") - } -} diff --git a/internal/cgroups/v2_linux.go b/internal/cgroups/v2_linux.go deleted file mode 100644 index 396cf104fd..0000000000 --- a/internal/cgroups/v2_linux.go +++ /dev/null @@ -1,220 +0,0 @@ -//go:build linux - -package cgroups - -import ( - "errors" - "fmt" - "io/fs" - "path/filepath" - "strings" - "time" - - "github.com/containerd/cgroups/v3/cgroup2" - "github.com/opencontainers/runtime-spec/specs-go" - "github.com/prometheus/client_golang/prometheus" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" - cgroupscfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups" - "gitlab.com/gitlab-org/gitaly/v16/internal/kernel" - "gitlab.com/gitlab-org/gitaly/v16/internal/log" -) - -type cgroupV2Handler struct { - cfg cgroupscfg.Config - logger log.Logger - - *cgroupsMetrics - pid int - cloneIntoCgroup bool -} - -func newV2Handler(cfg cgroupscfg.Config, logger log.Logger, pid int) *cgroupV2Handler { - cloneIntoCgroup, err := kernel.IsAtLeast(kernel.Version{Major: 5, Minor: 7}) - if err != nil { - // Log the error for now as we're only rolling out functionality behind feature flag. - logger.WithError(err).Error("failed detecting kernel version, CLONE_INTO_CGROUP support disabled") - } - - return &cgroupV2Handler{ - cfg: cfg, - logger: logger, - pid: pid, - cgroupsMetrics: newV2CgroupsMetrics(), - cloneIntoCgroup: cloneIntoCgroup, - } -} - -func (cvh *cgroupV2Handler) setupParent(parentResources *specs.LinuxResources) error { - if _, err := cgroup2.NewManager(cvh.cfg.Mountpoint, "/"+cvh.currentProcessCgroup(), cgroup2.ToResources(parentResources)); err != nil { - return fmt.Errorf("failed creating parent cgroup: %w", err) - } - - return nil -} - -func (cvh *cgroupV2Handler) createCgroup(reposResources *specs.LinuxResources, cgroupPath string) error { - _, err := cgroup2.NewManager( - cvh.cfg.Mountpoint, - "/"+cgroupPath, - cgroup2.ToResources(reposResources), - ) - - return err -} - -func (cvh *cgroupV2Handler) addToCgroup(pid int, cgroupPath string) error { - control, err := cvh.loadCgroup(cgroupPath) - if err != nil { - return err - } - - if err := control.AddProc(uint64(pid)); err != nil { - // Command could finish so quickly before we can add it to a cgroup, so - // we don't consider it an error. - if strings.Contains(err.Error(), "no such process") { - return nil - } - return fmt.Errorf("failed adding process to cgroup: %w", err) - } - - return nil -} - -func (cvh *cgroupV2Handler) loadCgroup(cgroupPath string) (*cgroup2.Manager, error) { - control, err := cgroup2.Load("/"+cgroupPath, cgroup2.WithMountpoint(cvh.cfg.Mountpoint)) - if err != nil { - return nil, fmt.Errorf("failed loading %s cgroup: %w", cgroupPath, err) - } - return control, nil -} - -func (cvh *cgroupV2Handler) collect(repoPath string, ch chan<- prometheus.Metric) { - logger := cvh.logger.WithField("cgroup_path", repoPath) - control, err := cvh.loadCgroup(repoPath) - if err != nil { - logger.WithError(err).Warn("unable to load cgroup controller") - return - } - - if metrics, err := control.Stat(); err != nil { - logger.WithError(err).Warn("unable to get cgroup stats") - } else { - cpuUserMetric := cvh.cpuUsage.WithLabelValues(repoPath, "user") - cpuUserMetric.Set(float64(metrics.CPU.UserUsec)) - ch <- cpuUserMetric - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSPeriods, - prometheus.CounterValue, - float64(metrics.CPU.NrPeriods), - repoPath, - ) - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSThrottledPeriods, - prometheus.CounterValue, - float64(metrics.CPU.NrThrottled), - repoPath, - ) - - ch <- prometheus.MustNewConstMetric( - cvh.cpuCFSThrottledTime, - prometheus.CounterValue, - float64(metrics.CPU.ThrottledUsec)/float64(time.Second), - repoPath, - ) - - cpuKernelMetric := cvh.cpuUsage.WithLabelValues(repoPath, "kernel") - cpuKernelMetric.Set(float64(metrics.CPU.SystemUsec)) - ch <- cpuKernelMetric - } - - if subsystems, err := control.Controllers(); err != nil { - logger.WithError(err).Warn("unable to get cgroup hierarchy") - } else { - processes, err := control.Procs(true) - if err != nil { - logger.WithError(err). - Warn("unable to get process list") - return - } - - for _, subsystem := range subsystems { - procsMetric := cvh.procs.WithLabelValues(repoPath, subsystem) - procsMetric.Set(float64(len(processes))) - ch <- procsMetric - } - } -} - -func (cvh *cgroupV2Handler) cleanup() error { - processCgroupPath := cvh.currentProcessCgroup() - - control, err := cvh.loadCgroup(processCgroupPath) - if err != nil { - return err - } - - if err := control.Delete(); err != nil { - return fmt.Errorf("failed cleaning up cgroup %s: %w", processCgroupPath, err) - } - - return nil -} - -func (cvh *cgroupV2Handler) repoPath(groupID int) string { - return filepath.Join(cvh.currentProcessCgroup(), fmt.Sprintf("repos-%d", groupID)) -} - -func (cvh *cgroupV2Handler) currentProcessCgroup() string { - return config.GetGitalyProcessTempDir(cvh.cfg.HierarchyRoot, cvh.pid) -} - -func (cvh *cgroupV2Handler) stats() (Stats, error) { - processCgroupPath := cvh.currentProcessCgroup() - - control, err := cvh.loadCgroup(processCgroupPath) - if err != nil { - return Stats{}, err - } - - metrics, err := control.Stat() - if err != nil { - return Stats{}, fmt.Errorf("failed to fetch metrics %s: %w", processCgroupPath, err) - } - - stats := Stats{ - ParentStats: CgroupStats{ - CPUThrottledCount: metrics.CPU.NrThrottled, - CPUThrottledDuration: float64(metrics.CPU.ThrottledUsec) / float64(time.Second), - MemoryUsage: metrics.Memory.Usage, - MemoryLimit: metrics.Memory.UsageLimit, - Anon: metrics.Memory.Anon, - ActiveAnon: metrics.Memory.ActiveAnon, - InactiveAnon: metrics.Memory.InactiveAnon, - File: metrics.Memory.File, - ActiveFile: metrics.Memory.ActiveFile, - InactiveFile: metrics.Memory.InactiveFile, - }, - } - if metrics.MemoryEvents != nil { - stats.ParentStats.OOMKills = metrics.MemoryEvents.OomKill - } - return stats, nil -} - -func (cvh *cgroupV2Handler) supportsCloneIntoCgroup() bool { - return cvh.cloneIntoCgroup -} - -func pruneOldCgroupsV2(cfg cgroupscfg.Config, logger log.Logger) { - if err := config.PruneOldGitalyProcessDirectories( - logger, - filepath.Join(cfg.Mountpoint, cfg.HierarchyRoot), - ); err != nil { - var pathError *fs.PathError - if !errors.As(err, &pathError) { - logger.WithError(err).Error("failed to clean up cpu cgroups") - } - } -} -- GitLab