Only activate Git pack-objects hook if cache is enabled

In https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3301, we
dropped the `upload_pack_gitaly_hooks` feature flag because it was
confusing to have to enable this feature flag on top of the pack objects
cache setting in `config.toml`.

However, we have found that spawning the gitaly-hooks environment can
add significant CPU load due to overhead from transferring data over
gRPC and spawning gitaly-hooks processes.

We now only enable this hook if the pack objects cache is enabled.

Relates to https://gitlab.com/gitlab-org/gitaly/-/issues/3754

Changelog: performance

4 files changed, 102 insertions, 35 deletions

diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index 7ad3ff9f7..8b03060c7 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -53,6 +53,10 @@ func WithRefTxHook(ctx context.Context, repo repository.GitRepo, cfg config.Cfg)
 // WithPackObjectsHookEnv provides metadata for gitaly-hooks so it can act as a pack-objects hook.
 func WithPackObjectsHookEnv(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) CmdOpt {
 	return func(cc *cmdCfg) error {
+		if !cfg.PackObjectsCache.Enabled {
+			return nil
+		}
+
 		if repo == nil {
 			return fmt.Errorf("missing repo: %w", ErrInvalidArg)
 		}
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index ce6dcf074..bcf67348b 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -29,10 +29,25 @@ const (
 	clientCapabilities = `multi_ack_detailed no-done side-band-64k thin-pack include-tag ofs-delta deepen-since deepen-not filter agent=git/2.18.0`
 )
 
-func TestSuccessfulUploadPackRequest(t *testing.T) {
+func runTestWithAndWithoutConfigOptions(t *testing.T, tf func(t *testing.T, ctx context.Context, opts ...testcfg.Option), opts ...testcfg.Option) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+
+	t.Run("no config options", func(t *testing.T) { tf(t, ctx) })
+
+	if len(opts) > 0 {
+		t.Run("with config options", func(t *testing.T) {
+			tf(t, ctx, opts...)
+		})
+	}
+}
+
+func TestSuccessfulUploadPackRequest(t *testing.T) {
+	runTestWithAndWithoutConfigOptions(t, testSuccessfulUploadPackRequest, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testSuccessfulUploadPackRequest(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 
@@ -101,9 +116,11 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackRequestWithGitConfigOptions, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackRequestWithGitConfigOptions(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 
@@ -164,9 +181,11 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitProtocol(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackRequestWithGitProtocol, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackRequestWithGitProtocol(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	readProto, cfg := gittest.EnableGitProtocolV2Support(t, cfg)
 
@@ -207,9 +226,11 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 // on 'deepen' requests even though the request is being handled just
 // fine from the client perspective.
 func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testSuccessfulUploadPackDeepenRequest, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testSuccessfulUploadPackDeepenRequest(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
@@ -227,7 +248,7 @@ func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
 }
 
 func TestUploadPackWithPackObjectsHook(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, testcfg.WithPackObjectsCacheEnabled())
 	cfg.BinDir = testhelper.TempDir(t)
 
 	outputPath := filepath.Join(cfg.BinDir, "output")
@@ -266,9 +287,11 @@ func TestUploadPackWithPackObjectsHook(t *testing.T) {
 }
 
 func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg := testcfg.Build(t)
+	runTestWithAndWithoutConfigOptions(t, testFailedUploadPackRequestDueToValidationError, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testFailedUploadPackRequestDueToValidationError(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg := testcfg.Build(t, opts...)
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
@@ -355,9 +378,11 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 }
 
 func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, _, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackRequestForPartialCloneSuccess, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackRequestForPartialCloneSuccess(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, _, repoPath := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 0ee9c5d31..b79107bd5 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -36,6 +36,16 @@ type cloneCommand struct {
 	cfg          config.Cfg
 }
 
+func runTestWithAndWithoutConfigOptions(t *testing.T, tf func(t *testing.T, opts ...testcfg.Option), opts ...testcfg.Option) {
+	t.Run("no config options", func(t *testing.T) { tf(t) })
+
+	if len(opts) > 0 {
+		t.Run("with config options", func(t *testing.T) {
+			tf(t, opts...)
+		})
+	}
+}
+
 func (cmd cloneCommand) execute(t *testing.T) error {
 	req := &gitalypb.SSHUploadPackRequest{
 		Repository:  cmd.repository,
@@ -91,7 +101,11 @@ func (cmd cloneCommand) test(t *testing.T, cfg config.Cfg, repoPath string, loca
 }
 
 func TestFailedUploadPackRequestDueToTimeout(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testFailedUploadPackRequestDueToTimeout, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testFailedUploadPackRequestDueToTimeout(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	serverSocketPath := runSSHServerWithOptions(t, cfg, []ServerOpt{WithUploadPackRequestTimeout(10 * time.Microsecond)})
 
@@ -197,7 +211,11 @@ func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccess(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneSuccess, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneSuccess(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 	testhelper.ConfigureGitalySSHBin(t, cfg)
@@ -209,10 +227,9 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 	localRepoPath := testhelper.TempDir(t)
 
 	tests := []struct {
-		cmd          *exec.Cmd
-		desc         string
-		deepen       float64
-		featureFlags []string
+		cmd    *exec.Cmd
+		desc   string
+		deepen float64
 	}{
 		{
 			cmd:    exec.Command(cfg.Git.BinPath, "clone", "git@localhost:test/test.git", localRepoPath),
@@ -231,11 +248,10 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 			negotiationMetrics.Reset()
 
 			cmd := cloneCommand{
-				repository:   repo,
-				command:      tc.cmd,
-				featureFlags: tc.featureFlags,
-				server:       serverSocketPath,
-				cfg:          cfg,
+				repository: repo,
+				command:    tc.cmd,
+				server:     serverSocketPath,
+				cfg:        cfg,
 			}
 			lHead, rHead, _, _ := cmd.test(t, cfg, repoPath, localRepoPath)
 			require.Equal(t, lHead, rHead, "local and remote head not equal")
@@ -248,7 +264,7 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 }
 
 func TestUploadPackWithPackObjectsHook(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	cfg, repo, _ := testcfg.BuildWithRepo(t, testcfg.WithPackObjectsCacheEnabled())
 
 	filterDir := testhelper.TempDir(t)
 	outputPath := filepath.Join(filterDir, "output")
@@ -288,7 +304,11 @@ exec '%s' "$@"
 }
 
 func TestUploadPackWithoutSideband(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackWithoutSideband, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackWithoutSideband(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalySSHBin(t, cfg)
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
@@ -332,7 +352,11 @@ func TestUploadPackWithoutSideband(t *testing.T) {
 }
 
 func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneWithPartialCloneFilter, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalySSHBin(t, cfg)
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
@@ -389,7 +413,11 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneSuccessWithGitProtocol, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.ConfigureGitalySSHBin(t, cfg)
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
diff --git a/internal/testhelper/testcfg/gitaly_builder.go b/internal/testhelper/testcfg/gitaly_builder.go
index 741402fcc..97ea11176 100644
--- a/internal/testhelper/testcfg/gitaly_builder.go
+++ b/internal/testhelper/testcfg/gitaly_builder.go
@@ -41,6 +41,13 @@ func WithRealLinguist() Option {
 	}
 }
 
+// WithPackObjectsCacheEnabled enables the pack object cache.
+func WithPackObjectsCacheEnabled() Option {
+	return func(builder *GitalyCfgBuilder) {
+		builder.packObjectsCacheEnabled = true
+	}
+}
+
 // NewGitalyCfgBuilder returns gitaly configuration builder with configured set of options.
 func NewGitalyCfgBuilder(opts ...Option) GitalyCfgBuilder {
 	cfgBuilder := GitalyCfgBuilder{}
@@ -56,8 +63,9 @@ func NewGitalyCfgBuilder(opts ...Option) GitalyCfgBuilder {
 type GitalyCfgBuilder struct {
 	cfg config.Cfg
 
-	storages     []string
-	realLinguist bool
+	storages                []string
+	realLinguist            bool
+	packObjectsCacheEnabled bool
 }
 
 // Build setups required filesystem structure, creates and returns configuration of the gitaly service.
@@ -121,6 +129,8 @@ func (gc *GitalyCfgBuilder) Build(t testing.TB) config.Cfg {
 		}
 	}
 
+	cfg.PackObjectsCache.Enabled = gc.packObjectsCacheEnabled
+
 	require.NoError(t, testhelper.ConfigureRuby(&cfg))
 	require.NoError(t, cfg.Validate())