diff --git a/internal/cgroups/manager_linux.go b/internal/cgroups/manager_linux.go index 13e742db29a7ae941045a9652bc9329dcbe1908d..360a14f2d28173110a3635e93c9cc33361d13f26 100644 --- a/internal/cgroups/manager_linux.go +++ b/internal/cgroups/manager_linux.go @@ -25,6 +25,7 @@ type cgroupHandler interface { collect(ch chan<- prometheus.Metric) cleanup() error currentProcessCgroup() string + mainPath() string repoPath(groupID int) string stats() (Stats, error) } @@ -70,6 +71,32 @@ func (cgm *CGroupManager) Setup() error { if err := cgm.handler.setupRepository(cgm.configRepositoryResources()); err != nil { return err } + // When the IncludeGitalyProcess configuration is enabled, the main cgroup process's PID is added to Gitaly's + // cgroup hierarchy. The process doesn't possess its own exclusive limit. Rather, it shares the limit with all + // spawned Git processes, which are also managed by the repository cgroup, under the umbrella of the parent + // cgroup. This configuration offers superior protection and flexibility compared to setting a fixed limit + // specifically for the Gitaly process: + // - If certain Git commands monopolize resources, they will be terminated by the repository cgroup. + // - If the cumulative resource usage of all Git commands surpasses the parent cgroup limit, the Out-Of-Memory + // (OOM) killer is triggered which terminates the most resource-intensive processes. Resource-heavy Git commands + // are more likely to be targeted. + // - If the Gitaly process consumes excessive memory (possibly due to a memory leak), it is allowed to do so if + // the parent cgroup has available resources. However, if the parent cgroup's limit is reached, the Gitaly + // process will be terminated. + // Consequently, the Gitaly process typically has more resource leeway than the Git processes and is likely to + // be terminated last in the event of a problem. + // + // Example cgroup hierarchy if this config is enabled (Cgroup V2): + // /sys/fs/cgroup/gitaly/gitaly-121843 + // |_ main + // |_ repos-0 + // |_ repos-1 + // |_ ... + if cgm.cfg.IncludeGitalyProcess { + if err := cgm.handler.addToCgroup(cgm.pid, cgm.handler.mainPath()); err != nil { + return err + } + } cgm.enabled = true return nil diff --git a/internal/cgroups/v1_linux.go b/internal/cgroups/v1_linux.go index b533b5ad10eff646bfc17d23e2dd16d8d6b1d950..8c4ef6417b63021fc76aedc2ff3fc8347f033485 100644 --- a/internal/cgroups/v1_linux.go +++ b/internal/cgroups/v1_linux.go @@ -45,6 +45,17 @@ func (cvh *cgroupV1Handler) setupParent(parentResources *specs.LinuxResources) e ); err != nil { return fmt.Errorf("failed creating parent cgroup: %w", err) } + // Setup a "main" path with empty resources. This setup lets the pids added to this path share the + // resources with all processes under the control of the parent cgroup. + if cvh.cfg.IncludeGitalyProcess { + if _, err := cgroup1.New( + cgroup1.StaticPath(cvh.mainPath()), + &specs.LinuxResources{}, + cgroup1.WithHiearchy(cvh.hierarchy), + ); err != nil { + return fmt.Errorf("failed creating main cgroup: %w", err) + } + } return nil } @@ -178,6 +189,10 @@ func (cvh *cgroupV1Handler) repoPath(groupID int) string { return filepath.Join(cvh.currentProcessCgroup(), fmt.Sprintf("repos-%d", groupID)) } +func (cvh *cgroupV1Handler) mainPath() string { + return filepath.Join(cvh.currentProcessCgroup(), "main") +} + func (cvh *cgroupV1Handler) currentProcessCgroup() string { return config.GetGitalyProcessTempDir(cvh.cfg.HierarchyRoot, cvh.pid) } diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go index 043d8f708a6b6f41e59c3cc5332ebf5c2004df97..38fd7ff07ac266981c74b7f0de2f744547d09c2d 100644 --- a/internal/cgroups/v1_linux_test.go +++ b/internal/cgroups/v1_linux_test.go @@ -215,6 +215,69 @@ func TestSetup_RepoCgroups(t *testing.T) { } } +func TestSetup_IncludeGitalyProcess(t *testing.T) { + tests := []struct { + name string + cfg cgroups.Config + shouldIncludeGitalyProcess bool + }{ + { + name: "IncludeGitalyProcess is false", + cfg: cgroups.Config{ + MemoryBytes: 102400, + CPUShares: 256, + CPUQuotaUs: 200, + }, + shouldIncludeGitalyProcess: false, + }, + { + name: "IncludeGitalyProcess is true", + cfg: cgroups.Config{ + MemoryBytes: 102400, + CPUShares: 256, + CPUQuotaUs: 200, + IncludeGitalyProcess: true, + }, + shouldIncludeGitalyProcess: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mock := newMock(t) + pid := 666 + tt.cfg.HierarchyRoot = "gitaly" + tt.cfg.Mountpoint = mock.root + + v1Manager := mock.newCgroupManager(tt.cfg, testhelper.NewDiscardingLogEntry(t), pid) + require.False(t, v1Manager.Ready()) + require.NoError(t, v1Manager.Setup()) + require.True(t, v1Manager.Ready()) + + for _, s := range mock.subsystems { + path := filepath.Join(mock.root, string(s.Name()), "gitaly", fmt.Sprintf("gitaly-%d", pid), "main") + if tt.shouldIncludeGitalyProcess { + content := readCgroupFile(t, filepath.Join(path, "cgroup.procs")) + cmdPid, err := strconv.Atoi(string(content)) + + require.NoError(t, err) + require.Equal(t, pid, cmdPid) + + // The main process should not have memory and cpu configured. Although the OS + // will set it up automatically, cgroup mock does not. + require.NoFileExists(t, filepath.Join(path, "memory.limit_in_bytes")) + require.NoFileExists(t, filepath.Join(path, "cpu.shares")) + } else { + require.NoDirExists(t, path) + } + } + }) + } +} + func TestAddCommand(t *testing.T) { mock := newMock(t) diff --git a/internal/cgroups/v2_linux.go b/internal/cgroups/v2_linux.go index 8990358d24250e2dc7587b485dfb8096ac7fd00f..3ee009465221cb8a828cdd56e5b9f74e15107c84 100644 --- a/internal/cgroups/v2_linux.go +++ b/internal/cgroups/v2_linux.go @@ -36,10 +36,26 @@ func newV2Handler(cfg cgroupscfg.Config, logger logrus.FieldLogger, pid int) *cg } func (cvh *cgroupV2Handler) setupParent(parentResources *specs.LinuxResources) error { - if _, err := cgroup2.NewManager(cvh.cfg.Mountpoint, "/"+cvh.currentProcessCgroup(), cgroup2.ToResources(parentResources)); err != nil { + if _, err := cgroup2.NewManager( + cvh.cfg.Mountpoint, + "/"+cvh.currentProcessCgroup(), + cgroup2.ToResources(parentResources), + ); err != nil { return fmt.Errorf("failed creating parent cgroup: %w", err) } + // Setup a "main" path with empty resources. This setup lets the pids added to this path share the + // resources with all processes under the control of the parent cgroup. + if cvh.cfg.IncludeGitalyProcess { + if _, err := cgroup2.NewManager( + cvh.cfg.Mountpoint, + "/"+cvh.mainPath(), + &cgroup2.Resources{}, + ); err != nil { + return fmt.Errorf("failed creating main cgroup: %w", err) + } + } + return nil } @@ -159,6 +175,10 @@ func (cvh *cgroupV2Handler) repoPath(groupID int) string { return filepath.Join(cvh.currentProcessCgroup(), fmt.Sprintf("repos-%d", groupID)) } +func (cvh *cgroupV2Handler) mainPath() string { + return filepath.Join(cvh.currentProcessCgroup(), "main") +} + func (cvh *cgroupV2Handler) currentProcessCgroup() string { return config.GetGitalyProcessTempDir(cvh.cfg.HierarchyRoot, cvh.pid) } diff --git a/internal/cgroups/v2_linux_test.go b/internal/cgroups/v2_linux_test.go index 16963d23491b2cc775d2352aba421af1b11e3112..cfa7f68b89adc5a7577ea8a6ca84ecd7f6de2396 100644 --- a/internal/cgroups/v2_linux_test.go +++ b/internal/cgroups/v2_linux_test.go @@ -201,6 +201,70 @@ func TestSetup_RepoCgroupsV2(t *testing.T) { } } +func TestSetup_IncludeGitalyProcessV2(t *testing.T) { + tests := []struct { + name string + cfg cgroups.Config + shouldIncludeGitalyProcess bool + }{ + { + name: "IncludeGitalyProcess is false", + cfg: cgroups.Config{ + MemoryBytes: 102400, + CPUShares: 256, + CPUQuotaUs: 200, + }, + shouldIncludeGitalyProcess: false, + }, + { + name: "IncludeGitalyProcess is true", + cfg: cgroups.Config{ + MemoryBytes: 102400, + CPUShares: 256, + CPUQuotaUs: 200, + IncludeGitalyProcess: true, + }, + shouldIncludeGitalyProcess: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mock := newMockV2(t) + + pid := 666 + tt.cfg.HierarchyRoot = "gitaly" + tt.cfg.Mountpoint = mock.root + + v2Manager := mock.newCgroupManager(tt.cfg, testhelper.NewDiscardingLogEntry(t), pid) + mock.setupMockCgroupFiles(t, v2Manager) + + require.False(t, v2Manager.Ready()) + require.NoError(t, v2Manager.Setup()) + require.True(t, v2Manager.Ready()) + + path := filepath.Join(mock.root, "gitaly", fmt.Sprintf("gitaly-%d", pid), "main") + if tt.shouldIncludeGitalyProcess { + content := readCgroupFile(t, filepath.Join(path, "cgroup.procs")) + cmdPid, err := strconv.Atoi(string(content)) + + require.NoError(t, err) + require.Equal(t, pid, cmdPid) + + // The main process should not have memory and cpu configured. Although the OS + // will set it up automatically, cgroup mock does not. + require.NoFileExists(t, filepath.Join(path, "memory.max")) + require.NoFileExists(t, filepath.Join(path, "cpu.max")) + } else { + require.NoDirExists(t, path) + } + }) + } +} + func TestAddCommandV2(t *testing.T) { mock := newMockV2(t) diff --git a/internal/gitaly/config/cgroups/cgroups.go b/internal/gitaly/config/cgroups/cgroups.go index ff19650f9fecf56a9ac29953f292aaf930f1c079..88f14b33d787afa91868acbb1786ff57f7bcd9e2 100644 --- a/internal/gitaly/config/cgroups/cgroups.go +++ b/internal/gitaly/config/cgroups/cgroups.go @@ -12,8 +12,11 @@ type Config struct { // A system administrator is expected to create such cgroup/directory under /memory // and/or /cpu depending on which resource is enabled. HierarchyRoot is expected to // be owned by the user and group Gitaly runs as. - HierarchyRoot string `toml:"hierarchy_root"` - Repositories Repositories `toml:"repositories"` + HierarchyRoot string `toml:"hierarchy_root"` + // IncludeGitalyProcess determines whether the main Gitaly process should be a part of Gitaly's cgroup + // hierarchy. + IncludeGitalyProcess bool `toml:"include_gitaly_process"` + Repositories Repositories `toml:"repositories"` // MemoryBytes is the memory limit for the parent cgroup. 0 implies no memory limit. MemoryBytes int64 `toml:"memory_bytes"` // CPUShares are the shares of CPU the parent cgroup is allowed to utilize. A value of 1024