diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-09-09 14:31:05 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-09-15 09:45:55 +0300 |
commit | f7bb67439a8e6a1bcd0b2a3e632407dab6479dcf (patch) | |
tree | 6b600a9b1f90e5e360a6904027f0a634b7933e19 | |
parent | eaee4a44c179ec7ebc56a6d11b02e09eebfc1c76 (diff) |
operations: Port UserMergeBranch from Ruby to Go
This commit ports the OperationsService.UserMergeBranch RPC from Ruby to
Go, which is hidden behind a feature flag for now.
5 files changed, 257 insertions, 10 deletions
diff --git a/changelogs/unreleased/pks-git2go-merge-operation-service.yml b/changelogs/unreleased/pks-git2go-merge-operation-service.yml new file mode 100644 index 000000000..430fd1573 --- /dev/null +++ b/changelogs/unreleased/pks-git2go-merge-operation-service.yml @@ -0,0 +1,5 @@ +--- +title: Port OperationService.UserMergeBranch to Go +merge_request: 2540 +author: +type: performance diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 4cdc43d5d..4ff11fea6 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -1,22 +1,55 @@ package operations import ( + "bytes" "context" + "errors" "fmt" + "io" + "os/exec" + "path" "strings" + "gitlab.com/gitlab-org/gitaly/internal/command" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/updateref" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) +type preReceiveError struct { + message string +} + +func (e preReceiveError) Error() string { + return e.message +} + +type updateRefError struct { + reference string +} + +func (e updateRefError) Error() string { + return fmt.Sprintf("Could not update %s. Please refresh and try again.", e.reference) +} + func (s *server) UserMergeBranch(bidi gitalypb.OperationService_UserMergeBranchServer) error { + ctx := bidi.Context() + + if featureflag.IsEnabled(ctx, featureflag.GoUserMergeBranch) { + return s.userMergeBranch(bidi) + } + firstRequest, err := bidi.Recv() if err != nil { return err } - ctx := bidi.Context() client, err := s.ruby.OperationServiceClient(ctx) if err != nil { return err @@ -57,6 +90,189 @@ func (s *server) UserMergeBranch(bidi gitalypb.OperationService_UserMergeBranchS ) } +func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error { + if request.User == nil { + return fmt.Errorf("empty user") + } + + if len(request.Branch) == 0 { + return fmt.Errorf("empty branch name") + } + + if request.CommitId == "" { + return fmt.Errorf("empty commit ID") + } + + if len(request.Message) == 0 { + return fmt.Errorf("empty message") + } + + return nil +} + +func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Repository, user *gitalypb.User, reference, newrev, oldrev string) error { + gitlabshellEnv, err := gitlabshell.Env() + if err != nil { + return err + } + + env := append([]string{ + fmt.Sprintf("GL_ID=%s", user.GetGlId()), + fmt.Sprintf("GL_USERNAME=%s", user.GetGlUsername()), + fmt.Sprintf("GL_REPOSITORY=%s", repo.GetGlRepository()), + fmt.Sprintf("GL_PROJECT_PATH=%s", repo.GetGlProjectPath()), + fmt.Sprintf("GITALY_SOCKET=" + config.GitalyInternalSocketPath()), + fmt.Sprintf("GITALY_REPO=%s", repo), + fmt.Sprintf("GITALY_TOKEN=%s", s.cfg.Auth.Token), + }, gitlabshellEnv...) + + stdin := bytes.NewBufferString(fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference)) + var stdout, stderr bytes.Buffer + + if err := s.hookManager.PreReceiveHook(ctx, repo, env, stdin, &stdout, &stderr); err != nil { + return preReceiveError{message: stdout.String()} + } + if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil { + return preReceiveError{message: stdout.String()} + } + + updater, err := updateref.New(ctx, repo) + if err != nil { + return err + } + + if err := updater.Update(reference, newrev, oldrev); err != nil { + return err + } + + if err := updater.Wait(); err != nil { + return updateRefError{reference: reference} + } + + if err := s.hookManager.PostReceiveHook(ctx, repo, nil, env, stdin, &stdout, &stderr); err != nil { + return err + } + + return nil +} + +// parseRevision parses a Git revision and returns its OID. +func parseRevision(ctx context.Context, repo *gitalypb.Repository, revision string) (string, error) { + revParse, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "rev-parse", + Flags: []git.Option{git.Flag{"--verify"}}, + Args: []string{revision}, + }) + if err != nil { + return "", err + } + + var stdout bytes.Buffer + if _, err := io.Copy(&stdout, revParse); err != nil { + return "", err + } + + if err := revParse.Wait(); err != nil { + return "", err + } + + return text.ChompBytes(stdout.Bytes()), nil +} + +func (s *server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranchServer) error { + ctx := stream.Context() + + firstRequest, err := stream.Recv() + if err != nil { + return err + } + + if err := validateMergeBranchRequest(firstRequest); err != nil { + return helper.ErrInvalidArgument(err) + } + + revision, err := parseRevision(ctx, firstRequest.Repository, string(firstRequest.Branch)) + if err != nil { + return err + } + + repoPath, err := s.locator.GetPath(firstRequest.Repository) + if err != nil { + return err + } + + binary := path.Join(s.cfg.BinDir, "gitaly-git2go") + args := []string{ + "merge", + "-repository", repoPath, + "-author-name", string(firstRequest.User.Name), + "-author-mail", string(firstRequest.User.Email), + "-message", string(firstRequest.Message), + "-ours", firstRequest.CommitId, + "-theirs", revision, + } + + var stderr, stdout bytes.Buffer + mergeCommand, err := command.New(ctx, exec.Command(binary, args...), nil, &stdout, &stderr) + if err != nil { + return err + } + + if err := mergeCommand.Wait(); err != nil { + if _, ok := err.(*exec.ExitError); ok { + return fmt.Errorf("%w: %s", err, stderr.String()) + } + return err + } + + mergeCommit := text.ChompBytes(stdout.Bytes()) + + if err := stream.Send(&gitalypb.UserMergeBranchResponse{ + CommitId: mergeCommit, + }); err != nil { + return err + } + + secondRequest, err := stream.Recv() + if err != nil { + return err + } + if !secondRequest.Apply { + return helper.ErrPreconditionFailedf("merge aborted by client") + } + + branch := "refs/heads/" + text.ChompBytes(firstRequest.Branch) + if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, mergeCommit, revision); err != nil { + var preReceiveError preReceiveError + var updateRefError updateRefError + + if errors.As(err, &preReceiveError) { + err = stream.Send(&gitalypb.UserMergeBranchResponse{ + PreReceiveError: preReceiveError.message, + }) + } else if errors.As(err, &updateRefError) { + // When an error happens updating the reference, e.g. because of a race + // with another update, then Ruby code didn't send an error but just an + // empty response. + err = stream.Send(&gitalypb.UserMergeBranchResponse{}) + } + + return err + } + + if err := stream.Send(&gitalypb.UserMergeBranchResponse{ + BranchUpdate: &gitalypb.OperationBranchUpdate{ + CommitId: mergeCommit, + RepoCreated: false, + BranchCreated: false, + }, + }); err != nil { + return err + } + + return nil +} + func validateFFRequest(in *gitalypb.UserFFBranchRequest) error { if len(in.Branch) == 0 { return fmt.Errorf("empty branch name") diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index 5a9ad356e..691e07a2e 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -13,6 +13,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -24,11 +25,26 @@ var ( mergeBranchHeadBefore = "281d3a76f31c812dbf48abce82ccf6860adedd81" ) -func TestSuccessfulMerge(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() +func testUserMergeBranch(t *testing.T, testcase func(*testing.T, context.Context)) { + featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{ + featureflag.GoUserMergeBranch, + }) + require.NoError(t, err) + + for _, featureSet := range featureSets { + t.Run("disabled "+featureSet.String(), func(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + ctx = featureSet.Disable(ctx) - testSuccessfulMerge(t, ctx) + testcase(t, ctx) + }) + } +} + +func TestSuccessfulMerge(t *testing.T) { + testUserMergeBranch(t, testSuccessfulMerge) } func testSuccessfulMerge(t *testing.T, ctx context.Context) { @@ -105,9 +121,10 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) { } func TestAbortedMerge(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testUserMergeBranch(t, testAbortedMerge) +} +func testAbortedMerge(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -173,9 +190,10 @@ func TestAbortedMerge(t *testing.T) { } func TestFailedMergeConcurrentUpdate(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testUserMergeBranch(t, testFailedMergeConcurrentUpdate) +} +func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -220,6 +238,10 @@ func TestFailedMergeConcurrentUpdate(t *testing.T) { } func TestFailedMergeDueToHooks(t *testing.T) { + testUserMergeBranch(t, testFailedMergeDueToHooks) +} + +func testFailedMergeDueToHooks(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -239,7 +261,7 @@ func TestFailedMergeDueToHooks(t *testing.T) { require.NoError(t, err) defer remove() - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(ctx) defer cancel() mergeBidi, err := client.UserMergeBranch(ctx) diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index 6de86419d..98f124d8b 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -58,6 +58,7 @@ func testMain(m *testing.M) int { config.Config.GitlabShell.Dir = filepath.Join(cwd, "testdata", "gitlab-shell") testhelper.ConfigureGitalySSH() + testhelper.ConfigureGitalyGit2Go() testhelper.ConfigureGitalyHooksBinary() defer func(token string) { diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 7b07355a8..e747fee82 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -31,6 +31,8 @@ var ( ReferenceTransactionHook = FeatureFlag{Name: "reference_transaction_hook", OnByDefault: false} // LogCommandStats will log additional rusage stats for commands LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false} + // GoUserMergeBranch enables the Go implementation of UserMergeBranch + GoUserMergeBranch = FeatureFlag{Name: "go_user_merge_branch", OnByDefault: false} ) // All includes all feature flags. @@ -44,6 +46,7 @@ var All = []FeatureFlag{ ReferenceTransactionsSSHService, ReferenceTransactionsPrimaryWins, ReferenceTransactionHook, + GoUserMergeBranch, } const ( |