Merge branch 'wc/prune-noblock' into 'master'

gitaly: Don't cleanup cgroups on exit

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6532

Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

8 files changed, 15 insertions, 87 deletions

diff --git a/internal/cgroups/cgroups.go b/internal/cgroups/cgroups.go
index f8bca0609..8c918399e 100644
--- a/internal/cgroups/cgroups.go
+++ b/internal/cgroups/cgroups.go
@@ -5,6 +5,7 @@ import (
 	"os/exec"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/dontpanic"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/cgroups"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 )
@@ -86,10 +87,6 @@ type Manager interface {
 	// to start the command directly in the correct cgroup. On success, the function returns an io.Closer
 	// that must be closed after the command has been started to close the cgroup's file descriptor.
 	CloneIntoCgroup(*exec.Cmd, ...AddCommandOption) (string, io.Closer, error)
-	// Cleanup cleans up cgroups created in Setup.
-	// It is expected to be called once at Gitaly shutdown from any
-	// instance of the Manager.
-	Cleanup() error
 	// Stats returns cgroup accounting statistics collected by reading
 	// cgroupfs files. Those statistics are generic for both Cgroup V1
 	// and Cgroup V2.
@@ -109,7 +106,10 @@ func NewManager(cfg cgroups.Config, logger log.Logger, pid int) Manager {
 	return &NoopManager{}
 }
 
-// PruneOldCgroups prunes old cgroups for both the memory and cpu subsystems
-func PruneOldCgroups(cfg cgroups.Config, logger log.Logger) {
-	pruneOldCgroups(cfg, logger)
+// StartPruningOldCgroups prunes old cgroups for both the memory and cpu subsystems
+// in a goroutine.
+func StartPruningOldCgroups(cfg cgroups.Config, logger log.Logger) {
+	dontpanic.Go(logger, func() {
+		pruneOldCgroups(cfg, logger)
+	})
 }
diff --git a/internal/cgroups/handler_linux_test.go b/internal/cgroups/handler_linux_test.go
index ac87939e6..cb2a701af 100644
--- a/internal/cgroups/handler_linux_test.go
+++ b/internal/cgroups/handler_linux_test.go
@@ -346,30 +346,6 @@ 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 TestMetrics(t *testing.T) {
 	tests := []struct {
 		name           string
diff --git a/internal/cgroups/manager_linux.go b/internal/cgroups/manager_linux.go
index f55a7c58f..0ee67acc2 100644
--- a/internal/cgroups/manager_linux.go
+++ b/internal/cgroups/manager_linux.go
@@ -30,7 +30,6 @@ type cgroupHandler interface {
 	createCgroup(repoResources *specs.LinuxResources, cgroupPath string) error
 	addToCgroup(pid int, cgroupPath string) error
 	collect(repoPath string, ch chan<- prometheus.Metric)
-	cleanup() error
 	currentProcessCgroup() string
 	repoPath(groupID int) string
 	stats() (Stats, error)
@@ -209,11 +208,6 @@ func (cgm *CGroupManager) cgroupPathForCommand(cmd *exec.Cmd, opts []AddCommandO
 	return cgm.handler.repoPath(int(groupID))
 }
 
-// Cleanup cleans up cgroups created in Setup.
-func (cgm *CGroupManager) Cleanup() error {
-	return cgm.handler.cleanup()
-}
-
 // Describe is used to generate description information for each CGroupManager prometheus metric
 func (cgm *CGroupManager) Describe(ch chan<- *prometheus.Desc) {
 	prometheus.DescribeByCollect(cgm, ch)
diff --git a/internal/cgroups/noop.go b/internal/cgroups/noop.go
index 535c7df4b..ed4b936c0 100644
--- a/internal/cgroups/noop.go
+++ b/internal/cgroups/noop.go
@@ -40,11 +40,6 @@ func (cg *NoopManager) CloneIntoCgroup(*exec.Cmd, ...AddCommandOption) (string,
 	return "", io.NopCloser(nil), nil
 }
 
-//nolint:revive // This is unintentionally missing documentation.
-func (cg *NoopManager) Cleanup() error {
-	return nil
-}
-
 // Describe does nothing
 func (cg *NoopManager) Describe(ch chan<- *prometheus.Desc) {}
 
diff --git a/internal/cgroups/v1_linux.go b/internal/cgroups/v1_linux.go
index e56829c26..22cec8198 100644
--- a/internal/cgroups/v1_linux.go
+++ b/internal/cgroups/v1_linux.go
@@ -151,21 +151,6 @@ func (cvh *cgroupV1Handler) collect(repoPath string, ch chan<- prometheus.Metric
 	}
 }
 
-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))
 }
diff --git a/internal/cgroups/v2_linux.go b/internal/cgroups/v2_linux.go
index 479b2c5ce..8ef717d36 100644
--- a/internal/cgroups/v2_linux.go
+++ b/internal/cgroups/v2_linux.go
@@ -147,21 +147,6 @@ func (cvh *cgroupV2Handler) collect(repoPath string, ch chan<- prometheus.Metric
 	}
 }
 
-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))
 }
diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index 8d6739067..9db91a758 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -169,11 +169,6 @@ func run(cfg config.Cfg, logger log.Logger) error {
 	}
 	cfg.RuntimeDir = runtimeDir
 
-	// When cgroups are configured, we create a directory structure each
-	// time a gitaly process is spawned. Look through the hierarchy root
-	// to find any cgroup directories that belong to old gitaly processes
-	// and remove them.
-	cgroups.PruneOldCgroups(cfg.Cgroups, logger)
 	cgroupMgr := cgroups.NewManager(cfg.Cgroups, logger, os.Getpid())
 
 	began := time.Now()
@@ -183,12 +178,6 @@ func run(cfg config.Cfg, logger log.Logger) error {
 	logger.WithField("duration_ms", time.Since(began).Milliseconds()).Info("finished initializing cgroups")
 
 	defer func() {
-		if err := cgroupMgr.Cleanup(); err != nil {
-			logger.WithError(err).Warn("error cleaning up cgroups")
-		}
-	}()
-
-	defer func() {
 		if err := os.RemoveAll(cfg.RuntimeDir); err != nil {
 			logger.Warn("could not clean up runtime dir")
 		}
@@ -251,9 +240,6 @@ func run(cfg config.Cfg, logger log.Logger) error {
 
 	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 	prometheus.MustRegister(repoCounter)
-	repoCounter.StartCountingRepositories(ctx, locator, logger)
-
-	tempdir.StartCleaning(logger, locator, cfg.Storages, time.Hour)
 
 	prometheus.MustRegister(gitCmdFactory)
 
@@ -499,6 +485,14 @@ func run(cfg config.Cfg, logger log.Logger) error {
 		}
 	}
 
+	// When cgroups are configured, we create a directory structure each
+	// time a gitaly process is spawned. Look through the hierarchy root
+	// to find any cgroup directories that belong to old gitaly processes
+	// and remove them.
+	cgroups.StartPruningOldCgroups(cfg.Cgroups, logger)
+	repoCounter.StartCountingRepositories(ctx, locator, logger)
+	tempdir.StartCleaning(logger, locator, cfg.Storages, time.Hour)
+
 	if err := b.Start(); err != nil {
 		return fmt.Errorf("unable to start the bootstrap: %w", err)
 	}
diff --git a/internal/limiter/watchers/testhelper_test.go b/internal/limiter/watchers/testhelper_test.go
index 81fcef53c..bb4613acc 100644
--- a/internal/limiter/watchers/testhelper_test.go
+++ b/internal/limiter/watchers/testhelper_test.go
@@ -27,7 +27,6 @@ func (m *testCgroupManager) Stats() (cgroups.Stats, error) {
 	return m.statsList[m.statsIndex-1], m.statsErr
 }
 func (m *testCgroupManager) Setup() error                        { return nil }
-func (m *testCgroupManager) Cleanup() error                      { return nil }
 func (m *testCgroupManager) Describe(ch chan<- *prometheus.Desc) {}
 func (m *testCgroupManager) Collect(ch chan<- prometheus.Metric) {}
 func (m *testCgroupManager) AddCommand(*exec.Cmd, ...cgroups.AddCommandOption) (string, error) {