Cgroup: Add an option to include Gitaly process in Cgroup hierarchyqmnguyen0711/fix-cgroup-main-process-configuration

When cgroup feature is enabled in Gitaly, all children Git commands are
assigned with a corresponding per-repository cgroup. All of those
cgroups are also under the control of a parent cgroup. This hierarchy
limits the resource usage of those commands and prevents them from
dominating the node resources.

In an incident, we discovered that the main Gitaly process is not a part
of that cgroup hierarchy. It's fine most of the time because the Gitaly
process itself is not resource-hungry. Unfortunately, in the incident,
the Gitaly process leaks memory and grows its memory usage gradually.
After a certain point, the process becomes unresponsive. This situation
cannot be self-resolved without human intervention.

This commit adds an "IncludeGitalyProcess" config to include the Gitaly
process in the parent cgroup.

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. 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.

For https://gitlab.com/gitlab-org/gitaly/-/issues/5535

6 files changed, 195 insertions, 3 deletions

diff --git a/internal/cgroups/manager_linux.go b/internal/cgroups/manager_linux.go
index 13e742db2..360a14f2d 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 b533b5ad1..8c4ef6417 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 043d8f708..38fd7ff07 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 8990358d2..3ee009465 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 16963d234..cfa7f68b8 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 ff19650f9..88f14b33d 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 <Mountpoint>/memory
 	// and/or <Mountpoint>/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