repository: Always enable locking RenameRepository RPC

We have been pushing our RPCs which manage repository locations to
always use locking semantics on the source repository and, if exists, on
the target repository. Like this, we cannot ever have two RPCs which try
to create and/or remove a repository location at the same point in time.
This allows us to properly use transactional voting for these RPCs.

In 0118edab0 (repository: Implement locking for RenameRepository,
2021-12-06), we have introduced locking support for the RenameRepository
RPC. This change has been successfull rolled out to production without
any issues on December 10th, so the change seems to work as intended.

Remove the feature flag and always enable locking semantics for
RenameRepository.

Changelog: changed

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{