From 9ba982fed484f745e82a22ec385a301883a9575e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 21 Jun 2019 16:18:02 +0200 Subject: [PATCH] Hide object pools .have refs When an object pool has many members, it will have many remotes with references. That makes the ref advertisement large. This changes effectively hides all refs of the alternate and thus shrinks the ref advertisement response. Part of: https://gitlab.com/gitlab-org/gitaly/issues/1747 --- .../unreleased/zj-hide-object-pool-refs.yml | 5 ++ internal/service/smarthttp/inforefs.go | 8 ++- internal/service/smarthttp/inforefs_test.go | 38 +++++++++++ internal/service/smarthttp/receive_pack.go | 1 + internal/service/ssh/receive_pack.go | 8 +++ internal/service/ssh/receive_pack_test.go | 67 +++++++++++++++---- 6 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/zj-hide-object-pool-refs.yml diff --git a/changelogs/unreleased/zj-hide-object-pool-refs.yml b/changelogs/unreleased/zj-hide-object-pool-refs.yml new file mode 100644 index 0000000000..a3a78262b7 --- /dev/null +++ b/changelogs/unreleased/zj-hide-object-pool-refs.yml @@ -0,0 +1,5 @@ +--- +title: Hide object pools .have refs +merge_request: 1323 +author: +type: performance diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go index 3947c1cd94..7b00186796 100644 --- a/internal/service/smarthttp/inforefs.go +++ b/internal/service/smarthttp/inforefs.go @@ -42,7 +42,13 @@ func handleInfoRefs(ctx context.Context, service string, req *gitalypb.InfoRefsR return err } - args := []string{} + // In case the repository belongs to an object pool, we want to prevent + // Git from including the pool's refs in the ref advertisement. We do + // this by rigging core.alternateRefsCommand to produce no output. + // Because Git itself will append the pool repository directory, the + // command ends with a "#". The end result is that Git runs + // `/bin/sh -c 'exit 0 # /path/to/pool.git`. + args := []string{"-c", "core.alternateRefsCommand=exit 0 #"} for _, params := range req.GitConfigOptions { args = append(args, "-c", params) } diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go index 6683a6336e..d1cc8fb89e 100644 --- a/internal/service/smarthttp/inforefs_test.go +++ b/internal/service/smarthttp/inforefs_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -139,6 +140,43 @@ func TestSuccessfulInfoRefsReceivePack(t *testing.T) { }) } +func TestObjectPoolRefAdvertisementHiding(t *testing.T) { + server, serverSocketPath := runSmartHTTPServer(t) + defer server.Stop() + + client, conn := newSmartHTTPClient(t, serverSocketPath) + defer conn.Close() + + repo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + ctx, cancel := testhelper.Context() + defer cancel() + + pool, err := objectpool.NewObjectPool(repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + + require.NoError(t, pool.Create(ctx, repo)) + defer pool.Remove(ctx) + + commitID := testhelper.CreateCommit(t, pool.FullPath(), t.Name(), nil) + + require.NoError(t, pool.Link(ctx, repo)) + + rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo} + + c, err := client.InfoRefsReceivePack(ctx, rpcRequest) + require.NoError(t, err) + + response, err := ioutil.ReadAll(streamio.NewReader(func() ([]byte, error) { + resp, err := c.Recv() + return resp.GetData(), err + })) + + require.NoError(t, err) + require.NotContains(t, string(response), commitID+" .have") +} + func TestFailureRepoNotFoundInfoRefsReceivePack(t *testing.T) { server, serverSocketPath := runSmartHTTPServer(t) defer server.Stop() diff --git a/internal/service/smarthttp/receive_pack.go b/internal/service/smarthttp/receive_pack.go index 5ff0d584e5..fd214f98c2 100644 --- a/internal/service/smarthttp/receive_pack.go +++ b/internal/service/smarthttp/receive_pack.go @@ -62,6 +62,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac env = append(env, command.GitEnv...) opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + gitOptions := git.BuildGitOptions(opts, "receive-pack", "--stateless-rpc", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, nil, env, gitOptions...) diff --git a/internal/service/ssh/receive_pack.go b/internal/service/ssh/receive_pack.go index e952e77779..e8224808c7 100644 --- a/internal/service/ssh/receive_pack.go +++ b/internal/service/ssh/receive_pack.go @@ -70,6 +70,14 @@ func sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, req *gitaly env = append(env, command.GitEnv...) opts := append([]string{fmt.Sprintf("core.hooksPath=%s", hooks.Path())}, req.GitConfigOptions...) + + // In case the repository belongs to an object pool, we want to prevent + // Git from including the pool's refs in the ref advertisement. We do + // this by rigging core.alternateRefsCommand to produce no output. + // Because Git itself will append the pool repository directory, the + // command ends with a "#". The end result is that Git runs + // `/bin/sh -c 'exit 0 # /path/to/pool.git`. + opts = append(opts, "core.alternateRefsCommand=exit 0 #") gitOptions := git.BuildGitOptions(opts, "receive-pack", repoPath) cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, gitOptions...) diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go index 44f0ac1c09..a823b56cdc 100644 --- a/internal/service/ssh/receive_pack_test.go +++ b/internal/service/ssh/receive_pack_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "io" "io/ioutil" "os" "os/exec" @@ -18,7 +19,9 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/config" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/hooks" + "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" "gitlab.com/gitlab-org/gitaly/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" ) @@ -60,15 +63,12 @@ func TestFailedReceivePackRequestDueToValidationError(t *testing.T) { t.Run(test.Desc, func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() + stream, err := client.SSHReceivePack(ctx) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) - if err = stream.Send(test.Req); err != nil { - t.Fatal(err) - } - stream.CloseSend() + require.NoError(t, stream.Send(test.Req)) + require.NoError(t, stream.CloseSend()) err = drainPostReceivePackResponse(stream) testhelper.RequireGrpcError(t, err, test.Code) @@ -130,14 +130,10 @@ func TestReceivePackPushFailure(t *testing.T) { defer server.Stop() _, _, err := testCloneAndPush(t, serverSocketPath, pushParams{storageName: "foobar", glID: "1"}) - if err == nil { - t.Errorf("local and remote head equal. push did not fail") - } + require.Error(t, err, "local and remote head equal. push did not fail") _, _, err = testCloneAndPush(t, serverSocketPath, pushParams{storageName: testRepo.GetStorageName(), glID: ""}) - if err == nil { - t.Errorf("local and remote head equal. push did not fail") - } + require.Error(t, err, "local and remote head equal. push did not fail") } func TestReceivePackPushHookFailure(t *testing.T) { @@ -161,6 +157,51 @@ func TestReceivePackPushHookFailure(t *testing.T) { require.Contains(t, err.Error(), "(pre-receive hook declined)") } +func TestObjectPoolRefAdvertisementHidingSSH(t *testing.T) { + server, serverSocketPath := runSSHServer(t) + defer server.Stop() + + client, conn := newSSHClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.SSHReceivePack(ctx) + require.NoError(t, err) + + repo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + pool, err := objectpool.NewObjectPool(repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + + require.NoError(t, pool.Create(ctx, repo)) + defer pool.Remove(ctx) + + require.NoError(t, pool.Link(ctx, repo)) + + commitID := testhelper.CreateCommit(t, pool.FullPath(), t.Name(), nil) + + // First request + require.NoError(t, stream.Send(&gitalypb.SSHReceivePackRequest{ + Repository: &gitalypb.Repository{StorageName: "default", RelativePath: repo.GetRelativePath()}, GlId: "user-123", + })) + + require.NoError(t, stream.Send(&gitalypb.SSHReceivePackRequest{Stdin: []byte("0000")})) + require.NoError(t, stream.CloseSend()) + + r := streamio.NewReader(func() ([]byte, error) { + msg, err := stream.Recv() + return msg.GetStdout(), err + }) + + var b bytes.Buffer + _, err = io.Copy(&b, r) + require.NoError(t, err) + require.NotContains(t, b.String(), commitID+" .have") +} + func testCloneAndPush(t *testing.T, serverSocketPath string, params pushParams) (string, string, error) { storagePath := testhelper.GitlabTestStoragePath() tempRepo := "gitlab-test-ssh-receive-pack.git" -- GitLab