cgroups: Remove repository-specific logic

The cgroups manager accepts a `repository.GitRepo` as argument, which is
used to compute a cgroup key that will always be the same for multiple
different Git commands as long as they're executed in the same repo. As
a result we will always put them into the same cgroup bucket.

Having this knowledge in the cgroups manager is a layering violation
though: the manager really shouldn't know about repositories at all, but
only care about commands and the bucket they're put into.

Introduce a new option `WithCgroupKey()` that allows callers to override
the cgroup key used to derive the bucket. Like this, we can pass through
the per-repository cgroup key from the Git command factory through the
command package into the cgroups manager without anybody but the Git
command factory actually knowing about repositories.

9 files changed, 48 insertions, 42 deletions

diff --git a/internal/cgroups/cgroups.go b/internal/cgroups/cgroups.go
index 566c0f0b5..5948b2245 100644
--- a/internal/cgroups/cgroups.go
+++ b/internal/cgroups/cgroups.go
@@ -6,11 +6,25 @@ import (
 
 	"github.com/prometheus/client_golang/prometheus"
 	log "github.com/sirupsen/logrus"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 )
 
+type addCommandCfg struct {
+	cgroupKey string
+}
+
+// AddCommandOption is an option that can be passed to AddCommand.
+type AddCommandOption func(*addCommandCfg)
+
+// WithCgroupKey overrides the key used to derive the Cgroup bucket. If not passed, then the command
+// arguments will be used as the cgroup key.
+func WithCgroupKey(cgroupKey string) AddCommandOption {
+	return func(cfg *addCommandCfg) {
+		cfg.cgroupKey = cgroupKey
+	}
+}
+
 // Manager supplies an interface for interacting with cgroups
 type Manager interface {
 	// Setup creates cgroups and assigns configured limitations.
@@ -18,7 +32,7 @@ type Manager interface {
 	// instance of the Manager.
 	Setup() error
 	// AddCommand adds a Cmd to a cgroup.
-	AddCommand(*exec.Cmd, repository.GitRepo) (string, error)
+	AddCommand(*exec.Cmd, ...AddCommandOption) (string, error)
 	// Cleanup cleans up cgroups created in Setup.
 	// It is expected to be called once at Gitaly shutdown from any
 	// instance of the Manager.
diff --git a/internal/cgroups/noop.go b/internal/cgroups/noop.go
index cd299ef2c..302dc75f5 100644
--- a/internal/cgroups/noop.go
+++ b/internal/cgroups/noop.go
@@ -4,7 +4,6 @@ import (
 	"os/exec"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 )
 
 // NoopManager is a cgroups manager that does nothing
@@ -16,7 +15,7 @@ func (cg *NoopManager) Setup() error {
 }
 
 //nolint:revive // This is unintentionally missing documentation.
-func (cg *NoopManager) AddCommand(cmd *exec.Cmd, repo repository.GitRepo) (string, error) {
+func (cg *NoopManager) AddCommand(*exec.Cmd, ...AddCommandOption) (string, error) {
 	return "", nil
 }
 
diff --git a/internal/cgroups/v1_linux.go b/internal/cgroups/v1_linux.go
index 8440feb03..86577c4af 100644
--- a/internal/cgroups/v1_linux.go
+++ b/internal/cgroups/v1_linux.go
@@ -10,7 +10,6 @@ import (
 	"github.com/containerd/cgroups"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	cgroupscfg "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/log"
@@ -104,13 +103,16 @@ func (cg *CGroupV1Manager) Setup() error {
 // exited.
 func (cg *CGroupV1Manager) AddCommand(
 	cmd *exec.Cmd,
-	repo repository.GitRepo,
+	opts ...AddCommandOption,
 ) (string, error) {
-	var key string
-	if repo == nil {
+	var cfg addCommandCfg
+	for _, opt := range opts {
+		opt(&cfg)
+	}
+
+	key := cfg.cgroupKey
+	if key == "" {
 		key = strings.Join(cmd.Args, "/")
-	} else {
-		key = repo.GetStorageName() + "/" + repo.GetRelativePath()
 	}
 
 	checksum := crc32.ChecksumIEEE(
diff --git a/internal/cgroups/v1_linux_test.go b/internal/cgroups/v1_linux_test.go
index bf093bb5b..863b9adcf 100644
--- a/internal/cgroups/v1_linux_test.go
+++ b/internal/cgroups/v1_linux_test.go
@@ -18,7 +18,6 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
 func defaultCgroupsConfig() cgroups.Config {
@@ -63,11 +62,6 @@ func TestSetup(t *testing.T) {
 func TestAddCommand(t *testing.T) {
 	mock := newMock(t)
 
-	repo := &gitalypb.Repository{
-		StorageName:  "default",
-		RelativePath: "path/to/repo.git",
-	}
-
 	config := defaultCgroupsConfig()
 	config.Repositories.Count = 10
 	config.Repositories.MemoryBytes = 1024
@@ -91,8 +85,8 @@ func TestAddCommand(t *testing.T) {
 		pid:       pid,
 	}
 
-	t.Run("without a repository", func(t *testing.T) {
-		_, err := v1Manager2.AddCommand(cmd2, nil)
+	t.Run("without overridden key", func(t *testing.T) {
+		_, err := v1Manager2.AddCommand(cmd2)
 		require.NoError(t, err)
 
 		checksum := crc32.ChecksumIEEE([]byte(strings.Join(cmd2.Args, "/")))
@@ -110,14 +104,11 @@ func TestAddCommand(t *testing.T) {
 		}
 	})
 
-	t.Run("with a repository", func(t *testing.T) {
-		_, err := v1Manager2.AddCommand(cmd2, repo)
+	t.Run("with overridden key", func(t *testing.T) {
+		_, err := v1Manager2.AddCommand(cmd2, WithCgroupKey("foobar"))
 		require.NoError(t, err)
 
-		checksum := crc32.ChecksumIEEE([]byte(strings.Join([]string{
-			"default",
-			"path/to/repo.git",
-		}, "/")))
+		checksum := crc32.ChecksumIEEE([]byte("foobar"))
 		groupID := uint(checksum) % config.Repositories.Count
 
 		for _, s := range mock.subsystems {
@@ -158,10 +149,6 @@ func TestMetrics(t *testing.T) {
 	t.Parallel()
 
 	mock := newMock(t)
-	repo := &gitalypb.Repository{
-		StorageName:  "default",
-		RelativePath: "path/to/repo.git",
-	}
 
 	config := defaultCgroupsConfig()
 	config.Repositories.Count = 1
@@ -179,17 +166,17 @@ func TestMetrics(t *testing.T) {
 
 	cmd := exec.CommandContext(ctx, "ls", "-hal", ".")
 	require.NoError(t, cmd.Start())
-	_, err := v1Manager1.AddCommand(cmd, repo)
+	_, err := v1Manager1.AddCommand(cmd)
 	require.NoError(t, err)
 
 	gitCmd1 := exec.CommandContext(ctx, "ls", "-hal", ".")
 	require.NoError(t, gitCmd1.Start())
-	_, err = v1Manager1.AddCommand(gitCmd1, repo)
+	_, err = v1Manager1.AddCommand(gitCmd1)
 	require.NoError(t, err)
 
 	gitCmd2 := exec.CommandContext(ctx, "ls", "-hal", ".")
 	require.NoError(t, gitCmd2.Start())
-	_, err = v1Manager1.AddCommand(gitCmd2, repo)
+	_, err = v1Manager1.AddCommand(gitCmd2)
 	require.NoError(t, err)
 	defer func() {
 		require.NoError(t, gitCmd2.Wait())
diff --git a/internal/command/command.go b/internal/command/command.go
index b018f29f9..0ada77777 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -301,7 +301,7 @@ func New(ctx context.Context, nameAndArgs []string, opts ...Option) (*Command, e
 	}()
 
 	if featureflag.RunCommandsInCGroup.IsEnabled(ctx) && cfg.cgroupsManager != nil {
-		cgroupPath, err := cfg.cgroupsManager.AddCommand(command.cmd, cfg.cgroupsRepo)
+		cgroupPath, err := cfg.cgroupsManager.AddCommand(command.cmd, cfg.cgroupsAddCommandOpts...)
 		if err != nil {
 			return nil, err
 		}
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 384b0a250..50f9ca06f 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -20,7 +20,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/cgroups"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
@@ -405,7 +404,7 @@ type mockCgroupManager struct {
 	path string
 }
 
-func (m mockCgroupManager) AddCommand(*exec.Cmd, repository.GitRepo) (string, error) {
+func (m mockCgroupManager) AddCommand(*exec.Cmd, ...cgroups.AddCommandOption) (string, error) {
 	return m.path, nil
 }
 
diff --git a/internal/command/option.go b/internal/command/option.go
index 0ee854ea5..8f1d64f1d 100644
--- a/internal/command/option.go
+++ b/internal/command/option.go
@@ -4,7 +4,6 @@ import (
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/cgroups"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 )
 
 type config struct {
@@ -20,8 +19,8 @@ type config struct {
 	subcommandName string
 	gitVersion     string
 
-	cgroupsManager cgroups.Manager
-	cgroupsRepo    repository.GitRepo
+	cgroupsManager        cgroups.Manager
+	cgroupsAddCommandOpts []cgroups.AddCommandOption
 }
 
 // Option is an option that can be passed to `New()` for controlling how the command is being
@@ -89,10 +88,10 @@ func WithCommandGitVersion(gitCmdVersion string) Option {
 
 // WithCgroup adds the spawned command to a Cgroup. The bucket used will be derived from the
 // command's arguments and/or from the repository.
-func WithCgroup(cgroupsManager cgroups.Manager, repo repository.GitRepo) Option {
+func WithCgroup(cgroupsManager cgroups.Manager, opts ...cgroups.AddCommandOption) Option {
 	return func(cfg *config) {
 		cfg.cgroupsManager = cgroupsManager
-		cfg.cgroupsRepo = repo
+		cfg.cgroupsAddCommandOpts = opts
 	}
 }
 
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index d288929c8..41f43948e 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -413,11 +413,18 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 		return nil, fmt.Errorf("getting Git version: %w", err)
 	}
 
+	var cgroupsAddCommandOpts []cgroups.AddCommandOption
+	if repo != nil {
+		cgroupsAddCommandOpts = []cgroups.AddCommandOption{
+			cgroups.WithCgroupKey(repo.GetStorageName() + "/" + repo.GetRelativePath()),
+		}
+	}
+
 	command, err := command.New(ctx, append([]string{execEnv.BinaryPath}, args...), append(
 		config.commandOpts,
 		command.WithEnvironment(env),
 		command.WithCommandName("git", sc.Name),
-		command.WithCgroup(cf.cgroupsManager, repo),
+		command.WithCgroup(cf.cgroupsManager, cgroupsAddCommandOpts...),
 		command.WithCommandGitVersion(cmdGitVersion.String()),
 	)...)
 	if err != nil {
diff --git a/internal/git/command_factory_cgroup_test.go b/internal/git/command_factory_cgroup_test.go
index e002fbb59..b37da13a0 100644
--- a/internal/git/command_factory_cgroup_test.go
+++ b/internal/git/command_factory_cgroup_test.go
@@ -10,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/cgroups"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
@@ -21,7 +20,7 @@ type mockCgroupsManager struct {
 	commands []*exec.Cmd
 }
 
-func (m *mockCgroupsManager) AddCommand(c *exec.Cmd, repo repository.GitRepo) (string, error) {
+func (m *mockCgroupsManager) AddCommand(c *exec.Cmd, _ ...cgroups.AddCommandOption) (string, error) {
 	m.commands = append(m.commands, c)
 	return "", nil
 }