diff options
author | James Fargher <proglottis@gmail.com> | 2022-04-26 01:36:19 +0300 |
---|---|---|
committer | James Fargher <proglottis@gmail.com> | 2022-04-26 01:36:19 +0300 |
commit | 20ad9ff995caa23b3e0c3195175193260adbf0de (patch) | |
tree | 74b13859fd534bfb96f3dd9b1db2c8f231c0e775 | |
parent | 3482d0e7dd3df87b5724d587a331a9409c228fc4 (diff) | |
parent | 58825d1f725f0c04677aeca6ba65521f2e52217f (diff) |
Merge branch 'pks-user-squash-drop-quarantined-voting-ff' into 'master'
operations: Remove feature flag for transactional voting in UserSquash
Closes #4119
See merge request gitlab-org/gitaly!4496
-rw-r--r-- | internal/gitaly/service/operations/squash.go | 82 | ||||
-rw-r--r-- | internal/gitaly/service/operations/squash_test.go | 98 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_user_squash_quarantined_voting.go | 7 |
3 files changed, 54 insertions, 133 deletions
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 1129d2658..9b3adaba9 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -7,13 +7,10 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/v14/internal/git" - "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/v14/internal/git/quarantine" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/text" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) @@ -82,32 +79,21 @@ func validateUserSquashRequest(req *gitalypb.UserSquashRequest) error { } func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (string, error) { - repo := s.localrepo(req.GetRepository()) - repoPath, err := repo.Path() - if err != nil { - return "", helper.ErrInternalf("cannot resolve repo path: %w", err) - } - // All new objects are staged into a quarantine directory first so that we can do // transactional voting before we commit data to disk. - var quarantineDir *quarantine.Dir - if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) { - var quarantineRepo *localrepo.Repo - quarantineDir, quarantineRepo, err = s.quarantinedRepo(ctx, req.GetRepository()) - if err != nil { - return "", helper.ErrInternalf("creating quarantine: %w", err) - } + quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + if err != nil { + return "", helper.ErrInternalf("creating quarantine: %w", err) + } - repo = quarantineRepo - repoPath, err = quarantineRepo.Path() - if err != nil { - return "", helper.ErrInternalf("getting quarantine path: %w", err) - } + quarantineRepoPath, err := quarantineRepo.Path() + if err != nil { + return "", helper.ErrInternalf("getting quarantine path: %w", err) } // We need to retrieve the start commit such that we can create the new commit with // all parents of the start commit. - startCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetStartSha()+"^{commit}")) + startCommit, err := quarantineRepo.ResolveRevision(ctx, git.Revision(req.GetStartSha()+"^{commit}")) if err != nil { detailedErr, err := helper.ErrWithDetails( helper.ErrInvalidArgumentf("resolving start revision: %w", err), @@ -127,7 +113,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest } // And we need to take the tree of the end commit. This tree already is the result - endCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}")) + endCommit, err := quarantineRepo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}")) if err != nil { detailedErr, err := helper.ErrWithDetails( helper.ErrInvalidArgumentf("resolving end revision: %w", err), @@ -153,8 +139,8 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest // We're now rebasing the end commit on top of the start commit. The resulting tree // is then going to be the tree of the squashed commit. - rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, repo, git2go.RebaseCommand{ - Repository: repoPath, + rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{ + Repository: quarantineRepoPath, Committer: git2go.NewSignature( string(req.GetUser().Name), string(req.GetUser().Email), commitDate, ), @@ -191,7 +177,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest return "", helper.ErrInternalf("rebasing commits: %w", err) } - treeID, err := repo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}") + treeID, err := quarantineRepo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}") if err != nil { return "", fmt.Errorf("cannot resolve rebased tree: %w", err) } @@ -217,7 +203,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest } var stdout, stderr bytes.Buffer - if err := repo.ExecAndWait(ctx, git.SubCmd{ + if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{ Name: "commit-tree", Flags: flags, Args: []string{ @@ -234,28 +220,26 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest // objects, vote on them and migrate them into the main directory if quorum was reached so // that we don't pollute the object directory with objects we don't want to have in the // first place. - if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) { - if err := transaction.VoteOnContext( - ctx, - s.txManager, - voting.VoteFromData([]byte(commitID)), - voting.Prepared, - ); err != nil { - return "", helper.ErrAbortedf("preparatory vote on squashed commit: %w", err) - } - - if err := quarantineDir.Migrate(); err != nil { - return "", helper.ErrInternalf("migrating quarantine directory: %w", err) - } - - if err := transaction.VoteOnContext( - ctx, - s.txManager, - voting.VoteFromData([]byte(commitID)), - voting.Committed, - ); err != nil { - return "", helper.ErrAbortedf("committing vote on squashed commit: %w", err) - } + if err := transaction.VoteOnContext( + ctx, + s.txManager, + voting.VoteFromData([]byte(commitID)), + voting.Prepared, + ); err != nil { + return "", helper.ErrAbortedf("preparatory vote on squashed commit: %w", err) + } + + if err := quarantineDir.Migrate(); err != nil { + return "", helper.ErrInternalf("migrating quarantine directory: %w", err) + } + + if err := transaction.VoteOnContext( + ctx, + s.txManager, + voting.VoteFromData([]byte(commitID)), + voting.Committed, + ); err != nil { + return "", helper.ErrAbortedf("committing vote on squashed commit: %w", err) } return commitID, nil diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go index f412fa335..57cb362fe 100644 --- a/internal/gitaly/service/operations/squash_test.go +++ b/internal/gitaly/service/operations/squash_test.go @@ -17,7 +17,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v14/internal/metadata" - "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo" @@ -41,11 +40,8 @@ var ( func TestUserSquash_successful(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashSuccessful) -} -func testUserSquashSuccessful(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) for _, tc := range []struct { desc string @@ -101,11 +97,9 @@ func testUserSquashSuccessful(t *testing.T, ctx context.Context) { func TestUserSquash_transactional(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashTransactional) -} -func testUserSquashTransactional(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) + txManager := transaction.MockManager{} ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx, @@ -196,46 +190,29 @@ func testUserSquashTransactional(t *testing.T, ctx context.Context) { Timestamp: ×tamppb.Timestamp{Seconds: 1234512345}, }) - if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) { - if tc.expectedErr == nil { - require.NoError(t, err) - require.Equal(t, squashedCommitID, response.SquashSha) - } else { - testhelper.RequireGrpcError(t, tc.expectedErr, err) - } - - require.Equal(t, tc.expectedVotes, votes) - } else { - // There is no transactional voting when the feature flag is - // disabled, so we'd never return an error from it either. + if tc.expectedErr == nil { require.NoError(t, err) - require.Empty(t, votes) + require.Equal(t, squashedCommitID, response.SquashSha) + } else { + testhelper.RequireGrpcError(t, tc.expectedErr, err) } + require.Equal(t, tc.expectedVotes, votes) exists, err := repo.HasRevision(ctx, git.Revision(squashedCommitID+"^{commit}")) require.NoError(t, err) - // In case the feature flag is enabled we use a quarantine directory to - // stage the new objects. So if we fail to reach quorum in the preparatory - // vote we expect the object to not have been migrated to the repository. On - // the other hand, if the feature flag is disabled, the object would always - // exist no matter whether the RPC is successful or not. - if featureflag.UserSquashQuarantinedVoting.IsEnabled(ctx) { - require.Equal(t, tc.expectedExists, exists) - } else { - require.True(t, exists) - } + // We use a quarantine directory to stage the new objects. So if we fail to + // reach quorum in the preparatory vote we expect the object to not have + // been migrated to the repository. + require.Equal(t, tc.expectedExists, exists) }) } } func TestUserSquash_stableID(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashStableID) -} -func testUserSquashStableID(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx) @@ -294,11 +271,8 @@ func ensureSplitIndexExists(t *testing.T, cfg config.Cfg, repoDir string) bool { func TestUserSquash_threeWayMerge(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashThreeWayMerge) -} -func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx) @@ -333,12 +307,8 @@ func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) { func TestUserSquash_splitIndex(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashSplitIndex) -} - -func testUserSquashSplitIndex(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) require.False(t, ensureSplitIndexExists(t, cfg, repoPath)) @@ -360,12 +330,9 @@ func testUserSquashSplitIndex(t *testing.T, ctx context.Context) { } func TestUserSquash_renames(t *testing.T) { - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashRenames) -} - -func testUserSquashRenames(t *testing.T, ctx context.Context) { t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) gittest.AddWorktree(t, cfg, repoPath, "worktree") @@ -424,12 +391,8 @@ func testUserSquashRenames(t *testing.T, ctx context.Context) { func TestUserSquash_missingFileOnTargetBranch(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashMissingFileOnTargetBranch) -} - -func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, _, repo, _, client := setupOperationsService(t, ctx) conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3" @@ -451,12 +414,8 @@ func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) func TestUserSquash_emptyCommit(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashEmptyCommit) -} - -func testUserSquashEmptyCommit(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -653,14 +612,9 @@ func TestUserSquash_validation(t *testing.T) { } func TestUserSquash_conflicts(t *testing.T) { - testhelper.NewFeatureSets( - featureflag.UserSquashQuarantinedVoting, - ).Run(t, testUserSquashConflicts) -} - -func testUserSquashConflicts(t *testing.T, ctx context.Context) { t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( @@ -704,12 +658,8 @@ func testUserSquashConflicts(t *testing.T, ctx context.Context) { func TestUserSquash_ancestry(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserSquashQuarantinedVoting).Run(t, testUserSquashAncestry) -} - -func testUserSquashAncestry(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) // We create an empty parent commit and two commits which both branch off from it. As a @@ -742,14 +692,8 @@ func testUserSquashAncestry(t *testing.T, ctx context.Context) { func TestUserSquash_gitError(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets( - featureflag.UserSquashQuarantinedVoting, - ).Run(t, testUserSquashGitError) -} - -func testUserSquashGitError(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, _, repo, _, client := setupOperationsService(t, ctx) testCases := []struct { diff --git a/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go b/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go deleted file mode 100644 index baf41ebf0..000000000 --- a/internal/metadata/featureflag/ff_user_squash_quarantined_voting.go +++ /dev/null @@ -1,7 +0,0 @@ -package featureflag - -// UserSquashQuarantinedVoting enables the use of a quarantine directory to stage all objects -// created by UserSquash into a temporary directory. This quarantine directory will only be migrated -// into the final repository when the RPC is successful, including a new transactional vote on the -// object ID of the resulting squashed commit. -var UserSquashQuarantinedVoting = NewFeatureFlag("user_squash_quarantined_voting", true) |