Merge branch 'pks-enable-atomic-rename-repository' into 'master'

repository: Always enable locking RenameRepository RPC

Closes #3950

See merge request gitlab-org/gitaly!4194

5 files changed, 19 insertions, 62 deletions

diff --git a/internal/gitaly/service/repository/rename.go b/internal/gitaly/service/repository/rename.go
index 861f99b90..b161b4a9e 100644
--- a/internal/gitaly/service/repository/rename.go
+++ b/internal/gitaly/service/repository/rename.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"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/proto/go/gitalypb"
 )
@@ -19,38 +18,12 @@ func (s *server) RenameRepository(ctx context.Context, in *gitalypb.RenameReposi
 		return nil, helper.ErrInvalidArgument(err)
 	}
 
-	if featureflag.RenameRepositoryLocking.IsEnabled(ctx) {
-		targetRepo := &gitalypb.Repository{
-			StorageName:  in.GetRepository().GetStorageName(),
-			RelativePath: in.GetRelativePath(),
-		}
-
-		if err := s.renameRepository(ctx, in.GetRepository(), targetRepo); err != nil {
-			return nil, helper.ErrInternal(err)
-		}
-
-		return &gitalypb.RenameRepositoryResponse{}, nil
-	}
-
-	fromFullPath, err := s.locator.GetRepoPath(in.GetRepository())
-	if err != nil {
-		return nil, helper.ErrInvalidArgument(err)
-	}
-
-	toFullPath, err := s.locator.GetPath(&gitalypb.Repository{StorageName: in.GetRepository().GetStorageName(), RelativePath: in.GetRelativePath()})
-	if err != nil {
-		return nil, helper.ErrInvalidArgument(err)
-	}
-
-	if _, err = os.Stat(toFullPath); !os.IsNotExist(err) {
-		return nil, helper.ErrFailedPreconditionf("destination already exists")
-	}
-
-	if err = os.MkdirAll(filepath.Dir(toFullPath), 0o755); err != nil {
-		return nil, helper.ErrInternal(err)
+	targetRepo := &gitalypb.Repository{
+		StorageName:  in.GetRepository().GetStorageName(),
+		RelativePath: in.GetRelativePath(),
 	}
 
-	if err = os.Rename(fromFullPath, toFullPath); err != nil {
+	if err := s.renameRepository(ctx, in.GetRepository(), targetRepo); err != nil {
 		return nil, helper.ErrInternal(err)
 	}
 
diff --git a/internal/gitaly/service/repository/rename_test.go b/internal/gitaly/service/repository/rename_test.go
index 834864511..17c20606f 100644
--- a/internal/gitaly/service/repository/rename_test.go
+++ b/internal/gitaly/service/repository/rename_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -18,10 +17,10 @@ import (
 
 func TestRenameRepository_success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RenameRepositoryLocking).Run(t, testRenameRepositorySuccess)
-}
 
-func testRenameRepositorySuccess(t *testing.T, ctx context.Context) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	// Praefect does not move repositories on the disk so this test case is not run with Praefect.
 	cfg, repo, _, client := setupRepositoryService(t, testserver.WithDisablePraefect())
 
@@ -44,11 +43,9 @@ func testRenameRepositorySuccess(t *testing.T, ctx context.Context) {
 
 func TestRenameRepository_DestinationExists(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RenameRepositoryLocking).Run(t, testRenameRepositoryDestinationExists)
-}
 
-func testRenameRepositoryDestinationExists(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
@@ -69,11 +66,7 @@ func testRenameRepositoryDestinationExists(t *testing.T, ctx context.Context) {
 		Repository:   renamedRepo,
 		RelativePath: existingDestinationRepo.RelativePath,
 	})
-	if featureflag.RenameRepositoryLocking.IsEnabled(ctx) {
-		testhelper.RequireGrpcCode(t, err, codes.AlreadyExists)
-	} else {
-		testhelper.RequireGrpcCode(t, err, codes.FailedPrecondition)
-	}
+	testhelper.RequireGrpcCode(t, err, codes.AlreadyExists)
 
 	// ensure the git directory that already existed didn't get overwritten
 	gittest.GitObjectMustExist(t, cfg.Git.BinPath, destinationRepoPath, commitID.String())
@@ -81,10 +74,10 @@ func testRenameRepositoryDestinationExists(t *testing.T, ctx context.Context) {
 
 func TestRenameRepository_invalidRequest(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RenameRepositoryLocking).Run(t, testRenameRepositoryInvalidRequest)
-}
 
-func testRenameRepositoryInvalidRequest(t *testing.T, ctx context.Context) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	_, repo, _, client := setupRepositoryService(t)
 
 	testCases := []struct {
diff --git a/internal/metadata/featureflag/ff_locking_rename_repository.go b/internal/metadata/featureflag/ff_locking_rename_repository.go
deleted file mode 100644
index 79e3ea645..000000000
--- a/internal/metadata/featureflag/ff_locking_rename_repository.go
+++ /dev/null
@@ -1,4 +0,0 @@
-package featureflag
-
-// RenameRepositoryLocking enables locking semantics for the RenameRepository RPC.
-var RenameRepositoryLocking = NewFeatureFlag("rename_repository_locking", false)
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index ad59e09ca..2dbf02045 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -25,7 +25,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"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/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
@@ -760,10 +759,10 @@ func TestProcessBacklog_FailedJobs(t *testing.T) {
 
 func TestProcessBacklog_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RenameRepositoryLocking).Run(t, testProcessBacklogsuccess)
-}
 
-func testProcessBacklogsuccess(t *testing.T, ctx context.Context) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	primaryCfg, testRepo, _ := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))
 	primaryCfg.SocketPath = testserver.RunGitalyServer(t, primaryCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 	testcfg.BuildGitalySSH(t, primaryCfg)
@@ -796,9 +795,6 @@ func testProcessBacklogsuccess(t *testing.T, ctx context.Context) {
 		},
 	}
 
-	ctx, cancel := context.WithCancel(ctx)
-	defer cancel()
-
 	queueInterceptor := datastore.NewReplicationEventQueueInterceptor(datastore.NewPostgresReplicationEventQueue(glsql.NewDB(t)))
 	queueInterceptor.OnAcknowledge(func(ctx context.Context, state datastore.JobState, ids []uint64, queue datastore.ReplicationEventQueue) ([]uint64, error) {
 		ackIDs, err := queue.Acknowledge(ctx, state, ids)
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index 9e58a96af..c54714143 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -601,11 +601,10 @@ func pollUntilRemoved(t testing.TB, path string, deadline <-chan time.Time) {
 
 func TestRenameRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RenameRepositoryLocking).Run(t, testRenameRepository)
-}
 
-func testRenameRepository(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	gitalyStorages := []string{"gitaly-1", "gitaly-2", "gitaly-3"}
 	repoPaths := make([]string, len(gitalyStorages))
 	praefectCfg := config.Config{