Merge branch 'pks-apply-gitattributes-voting-on-removal' into 'master'

repository: Use locking two-phase voting when deleting gitattributes

See merge request gitlab-org/gitaly!4179

3 files changed, 75 insertions, 17 deletions

diff --git a/internal/gitaly/service/repository/apply_gitattributes.go b/internal/gitaly/service/repository/apply_gitattributes.go
index 614e17bbc..82a08d1a1 100644
--- a/internal/gitaly/service/repository/apply_gitattributes.go
+++ b/internal/gitaly/service/repository/apply_gitattributes.go
@@ -8,11 +8,13 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
@@ -39,7 +41,46 @@ func (s *server) applyGitattributes(ctx context.Context, repo *localrepo.Repo, o
 		return err
 	}
 
+	// Create  /info folder if it doesn't exist
+	if err := os.MkdirAll(infoPath, 0o755); err != nil {
+		return err
+	}
+
 	if catfile.IsNotFound(err) || blobObj.Type != "blob" {
+		if featureflag.TxApplyGitattributesTwoPhaseRemoval.IsEnabled(ctx) {
+			locker, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
+				FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
+			})
+			if err != nil {
+				return fmt.Errorf("creating gitattributes lock: %w", err)
+			}
+			defer func() {
+				if err := locker.Close(); err != nil {
+					ctxlogrus.Extract(ctx).WithError(err).Error("unlocking gitattributes")
+				}
+			}()
+
+			if err := locker.Lock(); err != nil {
+				return fmt.Errorf("locking gitattributes: %w", err)
+			}
+
+			// We use the zero OID as placeholder to vote on removal of the
+			// gitattributes file.
+			if err := s.vote(ctx, git.ZeroOID); err != nil {
+				return fmt.Errorf("preimage vote: %w", err)
+			}
+
+			if err := os.Remove(attributesPath); err != nil && !os.IsNotExist(err) {
+				return err
+			}
+
+			if err := s.vote(ctx, git.ZeroOID); err != nil {
+				return fmt.Errorf("postimage vote: %w", err)
+			}
+
+			return nil
+		}
+
 		// If there is no gitattributes file, we simply use the ZeroOID
 		// as a placeholder to vote on the removal.
 		if err := s.vote(ctx, git.ZeroOID); err != nil {
@@ -54,11 +95,6 @@ func (s *server) applyGitattributes(ctx context.Context, repo *localrepo.Repo, o
 		return nil
 	}
 
-	// Create  /info folder if it doesn't exist
-	if err := os.MkdirAll(infoPath, 0o755); err != nil {
-		return err
-	}
-
 	writer, err := safe.NewLockingFileWriter(attributesPath, safe.LockingFileWriterConfig{
 		FileWriterConfig: safe.FileWriterConfig{FileMode: attributesFileMode},
 	})
diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go
index c332ab906..f1d145a93 100644
--- a/internal/gitaly/service/repository/apply_gitattributes_test.go
+++ b/internal/gitaly/service/repository/apply_gitattributes_test.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -28,9 +29,10 @@ import (
 func TestApplyGitattributesSuccess(t *testing.T) {
 	t.Parallel()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets(featureflag.TxApplyGitattributesTwoPhaseRemoval).Run(t, testApplyGitattributesSuccess)
+}
 
+func testApplyGitattributesSuccess(t *testing.T, ctx context.Context) {
 	cfg, repo, _, client := setupRepositoryService(t)
 
 	infoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath(), "info")
@@ -90,9 +92,10 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp
 func TestApplyGitattributesWithTransaction(t *testing.T) {
 	t.Parallel()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets(featureflag.TxApplyGitattributesTwoPhaseRemoval).Run(t, testApplyGitattributesWithTransaction)
+}
 
+func testApplyGitattributesWithTransaction(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
 	transactionServer := &testTransactionServer{}
@@ -122,11 +125,12 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 	)
 
 	for _, tc := range []struct {
-		desc        string
-		revision    []byte
-		voteFn      func(*testing.T, *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error)
-		shouldExist bool
-		expectedErr error
+		desc          string
+		revision      []byte
+		voteFn        func(*testing.T, *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error)
+		shouldExist   bool
+		expectedErr   error
+		expectedVotes func(ctx context.Context) int
 	}{
 		{
 			desc:     "successful vote writes gitattributes",
@@ -140,7 +144,8 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 					State: gitalypb.VoteTransactionResponse_COMMIT,
 				}, nil
 			},
-			shouldExist: true,
+			shouldExist:   true,
+			expectedVotes: func(context.Context) int { return 2 },
 		},
 		{
 			desc:     "aborted vote does not write gitattributes",
@@ -154,6 +159,7 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 			expectedErr: func() error {
 				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: transaction was aborted")
 			}(),
+			expectedVotes: func(context.Context) int { return 1 },
 		},
 		{
 			desc:     "failing vote does not write gitattributes",
@@ -165,6 +171,7 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 			expectedErr: func() error {
 				return status.Error(codes.Unknown, "committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
 			}(),
+			expectedVotes: func(context.Context) int { return 1 },
 		},
 		{
 			desc:     "commit without gitattributes performs vote",
@@ -176,6 +183,12 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 				}, nil
 			},
 			shouldExist: false,
+			expectedVotes: func(ctx context.Context) int {
+				if featureflag.TxApplyGitattributesTwoPhaseRemoval.IsEnabled(ctx) {
+					return 2
+				}
+				return 1
+			},
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -186,7 +199,9 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 			require.NoError(t, err)
 			ctx = metadata.IncomingToOutgoing(ctx)
 
+			var votes int
 			transactionServer.vote = func(request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
+				votes++
 				return tc.voteFn(t, request)
 			}
 
@@ -204,6 +219,7 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 			} else {
 				require.NoFileExists(t, path)
 			}
+			require.Equal(t, tc.expectedVotes(ctx), votes)
 		})
 	}
 }
@@ -211,9 +227,10 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 func TestApplyGitattributesFailure(t *testing.T) {
 	t.Parallel()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets(featureflag.TxApplyGitattributesTwoPhaseRemoval).Run(t, testApplyGitattributesFailure)
+}
 
+func testApplyGitattributesFailure(t *testing.T, ctx context.Context) {
 	_, repo, _, client := setupRepositoryService(t)
 
 	tests := []struct {
diff --git a/internal/metadata/featureflag/ff_tx_apply_gitattributes_two_phase_removal.go b/internal/metadata/featureflag/ff_tx_apply_gitattributes_two_phase_removal.go
new file mode 100644
index 000000000..b43b7d5ee
--- /dev/null
+++ b/internal/metadata/featureflag/ff_tx_apply_gitattributes_two_phase_removal.go
@@ -0,0 +1,5 @@
+package featureflag
+
+// TxApplyGitattributesTwoPhaseRemoval enables two-phase voting on the removal of the gitattributes
+// file.
+var TxApplyGitattributesTwoPhaseRemoval = NewFeatureFlag("tx_apply_gitattributes_two_phase_removal", false)