Generate unique replica paths for repositories

Renaming, creating and deleting repositories is racy in Praefect.
They can also partially fail in awkward ways due to Praefect first
applying the operations on the disks of the Gitalys and later updating
its metadata store. In order to make these operations atomic, there's
been ongoing work in the background to make it possible to perform these
in the database in a manner that races are not possible and partial
failures do not end up conflicting with future operations.

Renames can be fixed by doing the rename atomically in the database
without moving any repositories on the disks.

Deletes can be modeled as simply deleting the repository's database record.

Creates are atomic by creating the repository's database record as the
last step in the process.

The last piece of the puzzle is to ensure repositories always land in different
directories on the disk. This ensures that a partial failure doesn't block
a future operation from succeeding. This commit implements that piece by deriving
a unique path for each repository to store their replicas. Existing repositories
stay where they are but newly created repositories will land in unique directories.

Create might succeed on a disk but fail to be persisted in the database. Prior to
this change, attempting to recreate a repository might fail due to the stale state
on the disk. With this in place, the next attempt at creating the repository would
still succeed as the new attempt would land the repository in a different directory
on the disk.

Deletes have the same problem prior to this commit. The repository's metadata may
be successfully deleted but if we fail to delete the repository from the disk, future
creations and renames may fail if they conflict with the stale state. As creates now
always land in a different directory, the stale state no longer causes problems.

Renames will work purely in the database, so any stale state on the disk will not affect
them.

Changelog: fixed

7 files changed, 58 insertions, 17 deletions

diff --git a/internal/gitaly/service/repository/rename_test.go b/internal/gitaly/service/repository/rename_test.go
index 6c4f2b520..f28657fb8 100644
--- a/internal/gitaly/service/repository/rename_test.go
+++ b/internal/gitaly/service/repository/rename_test.go
@@ -1,6 +1,7 @@
 package repository
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -11,6 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
+	"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/proto/go/gitalypb"
@@ -19,8 +21,11 @@ import (
 )
 
 func TestRenameRepository_success(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.PraefectGeneratedReplicaPaths).Run(t, testRenameRepositorySuccess)
+}
+
+func testRenameRepositorySuccess(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	// Praefect does not move repositories on the disk so this test case is not run with Praefect.
 	cfg, repo, _, client := setupRepositoryService(ctx, t, testserver.WithDisablePraefect())
@@ -43,8 +48,11 @@ func TestRenameRepository_success(t *testing.T) {
 }
 
 func TestRenameRepository_DestinationExists(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.PraefectGeneratedReplicaPaths).Run(t, testRenameRepositoryDestinationExists)
+}
+
+func testRenameRepositoryDestinationExists(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
@@ -70,8 +78,11 @@ func TestRenameRepository_DestinationExists(t *testing.T) {
 }
 
 func TestRenameRepository_invalidRequest(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.PraefectGeneratedReplicaPaths).Run(t, testRenameRepositoryInvalidRequest)
+}
+
+func testRenameRepositoryInvalidRequest(t *testing.T, ctx context.Context) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
 	_, repo, repoPath, client := setupRepositoryService(ctx, t)
 	storagePath := strings.TrimSuffix(repoPath, "/"+repo.RelativePath)
diff --git a/internal/metadata/featureflag/ff_praefect_generated_paths.go b/internal/metadata/featureflag/ff_praefect_generated_paths.go
new file mode 100644
index 000000000..4109bd636
--- /dev/null
+++ b/internal/metadata/featureflag/ff_praefect_generated_paths.go
@@ -0,0 +1,4 @@
+package featureflag
+
+// PraefectGeneratedReplicaPaths will enable Praefect generated replica paths for new repositories.
+var PraefectGeneratedReplicaPaths = NewFeatureFlag("praefect_generated_replica_paths", false)
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 77e83ec5a..3821db602 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -488,6 +488,7 @@ func (c *Coordinator) mutatorStreamParameters(ctx context.Context, call grpcCall
 				route.RepositoryID,
 				virtualStorage,
 				targetRepo,
+				route.ReplicaPath,
 				route.Primary.Storage,
 				nil,
 				append(routerNodesToStorages(route.Secondaries), route.ReplicationTargets...),
@@ -835,7 +836,7 @@ func (c *Coordinator) createTransactionFinalizer(
 		}
 
 		return c.newRequestFinalizer(
-			ctx, route.RepositoryID, virtualStorage, targetRepo, route.Primary.Storage,
+			ctx, route.RepositoryID, virtualStorage, targetRepo, route.ReplicaPath, route.Primary.Storage,
 			updated, outdated, change, params, cause)()
 	}
 }
@@ -992,6 +993,7 @@ func (c *Coordinator) newRequestFinalizer(
 	repositoryID int64,
 	virtualStorage string,
 	targetRepo *gitalypb.Repository,
+	replicaPath string,
 	primary string,
 	updatedSecondaries []string,
 	outdatedSecondaries []string,
@@ -1048,7 +1050,7 @@ func (c *Coordinator) newRequestFinalizer(
 				repositoryID,
 				virtualStorage,
 				targetRepo.GetRelativePath(),
-				targetRepo.GetRelativePath(),
+				replicaPath,
 				primary,
 				updatedSecondaries,
 				outdatedSecondaries,
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index fce4d5e5b..e0927ae69 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -2673,6 +2673,7 @@ func TestNewRequestFinalizer_contextIsDisjointedFromTheRPC(t *testing.T) {
 					0,
 					"virtual storage",
 					&gitalypb.Repository{},
+					"replica-path",
 					"primary",
 					[]string{},
 					[]string{"secondary"},
diff --git a/internal/praefect/router_per_repository.go b/internal/praefect/router_per_repository.go
index 6f1b4e7bd..7067cb36e 100644
--- a/internal/praefect/router_per_repository.go
+++ b/internal/praefect/router_per_repository.go
@@ -5,8 +5,11 @@ import (
 	"errors"
 	"fmt"
 
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/housekeeping"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/nodes"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/praefectutil"
 	"google.golang.org/grpc"
 )
 
@@ -307,11 +310,19 @@ func (r *PerRepositoryRouter) RouteRepositoryCreation(ctx context.Context, virtu
 		return RepositoryMutatorRoute{}, fmt.Errorf("reserve repository id: %w", err)
 	}
 
+	replicaPath := relativePath
+	if featureflag.PraefectGeneratedReplicaPaths.IsEnabled(ctx) {
+		replicaPath = praefectutil.DeriveReplicaPath(id)
+		if housekeeping.IsRailsPoolPath(relativePath) {
+			replicaPath = praefectutil.DerivePoolPath(id)
+		}
+	}
+
 	replicationFactor := r.defaultReplicationFactors[virtualStorage]
 	if replicationFactor == 1 {
 		return RepositoryMutatorRoute{
 			RepositoryID: id,
-			ReplicaPath:  relativePath,
+			ReplicaPath:  replicaPath,
 			Primary:      primary,
 		}, nil
 	}
@@ -360,7 +371,7 @@ func (r *PerRepositoryRouter) RouteRepositoryCreation(ctx context.Context, virtu
 
 	return RepositoryMutatorRoute{
 		RepositoryID:          id,
-		ReplicaPath:           relativePath,
+		ReplicaPath:           replicaPath,
 		AdditionalReplicaPath: additionalReplicaPath,
 		Primary:               primary,
 		Secondaries:           secondaries,
diff --git a/internal/praefect/router_per_repository_test.go b/internal/praefect/router_per_repository_test.go
index 44d8b10ce..6e620ffe2 100644
--- a/internal/praefect/router_per_repository_test.go
+++ b/internal/praefect/router_per_repository_test.go
@@ -8,9 +8,11 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/commonerr"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/nodes"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/praefectutil"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testdb"
 	"google.golang.org/grpc"
@@ -662,6 +664,10 @@ func TestPerRepositoryRouter_RouteRepositoryMaintenance(t *testing.T) {
 }
 
 func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
+	testhelper.NewFeatureSets(featureflag.PraefectGeneratedReplicaPaths).Run(t, testPerRepositoryRouterRouteRepositoryCreation)
+}
+
+func testPerRepositoryRouterRouteRepositoryCreation(t *testing.T, ctx context.Context) {
 	t.Parallel()
 
 	configuredNodes := map[string][]string{
@@ -692,6 +698,11 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 		additionalReplicaPath  = "additional-replica-path"
 	)
 
+	replicaPath := relativePath
+	if featureflag.PraefectGeneratedReplicaPaths.IsEnabled(ctx) {
+		replicaPath = praefectutil.DeriveReplicaPath(1)
+	}
+
 	for _, tc := range []struct {
 		desc                   string
 		virtualStorage         string
@@ -726,7 +737,7 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID:          1,
-					ReplicaPath:           relativePath,
+					ReplicaPath:           replicaPath,
 					AdditionalReplicaPath: additionalReplicaPath,
 					Primary:               RouterNode{Storage: "primary", Connection: primaryConn},
 					ReplicationTargets:    []string{"secondary-1", "secondary-2"},
@@ -742,7 +753,7 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID: 1,
-					ReplicaPath:  relativePath,
+					ReplicaPath:  replicaPath,
 					Primary:      RouterNode{Storage: "primary", Connection: primaryConn},
 					Secondaries: []RouterNode{
 						{Storage: "secondary-1", Connection: secondary1Conn},
@@ -760,7 +771,7 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID: 1,
-					ReplicaPath:  relativePath,
+					ReplicaPath:  replicaPath,
 					Primary:      RouterNode{Storage: "primary", Connection: primaryConn},
 					Secondaries: []RouterNode{
 						{Storage: "secondary-1", Connection: secondary1Conn},
@@ -779,7 +790,7 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID: 1,
-					ReplicaPath:  relativePath,
+					ReplicaPath:  replicaPath,
 					Primary:      RouterNode{Storage: "primary", Connection: primaryConn},
 				},
 			),
@@ -795,13 +806,13 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID: 1,
-					ReplicaPath:  relativePath,
+					ReplicaPath:  replicaPath,
 					Primary:      RouterNode{Storage: "primary", Connection: primaryConn},
 					Secondaries:  []RouterNode{{Storage: "secondary-1", Connection: secondary1Conn}},
 				},
 				RepositoryMutatorRoute{
 					RepositoryID: 1,
-					ReplicaPath:  relativePath,
+					ReplicaPath:  replicaPath,
 					Primary:      RouterNode{Storage: "primary", Connection: primaryConn},
 					Secondaries:  []RouterNode{{Storage: "secondary-2", Connection: secondary1Conn}},
 				},
@@ -818,7 +829,7 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 			matchRoute: requireOneOf(
 				RepositoryMutatorRoute{
 					RepositoryID:       1,
-					ReplicaPath:        relativePath,
+					ReplicaPath:        replicaPath,
 					Primary:            RouterNode{Storage: "primary", Connection: primaryConn},
 					Secondaries:        []RouterNode{{Storage: "secondary-1", Connection: secondary1Conn}},
 					ReplicationTargets: []string{"secondary-2"},
@@ -848,14 +859,12 @@ func TestPerRepositoryRouter_RouteRepositoryCreation(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx := testhelper.Context(t)
-
 			db.TruncateAll(t)
 
 			rs := datastore.NewPostgresRepositoryStore(db, nil)
 			if tc.repositoryExists {
 				require.NoError(t,
-					rs.CreateRepository(ctx, 1, "virtual-storage-1", relativePath, relativePath, "primary", nil, nil, true, true),
+					rs.CreateRepository(ctx, 1, "virtual-storage-1", relativePath, replicaPath, "primary", nil, nil, true, true),
 				)
 			}
 
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 281dbc5b8..f3a917c70 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -180,6 +180,9 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	// Randomly inject the Git flag so that we have coverage of tests with both old and new Git
 	// version by pure chance.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.GitV2361Gl1, rnd.Int()%2 == 0)
+	// PraefectGeneratedReplicaPaths affects many tests as it changes the repository creation logic.
+	// Randomly enable the flag to exercise both paths to some extent.
+	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectGeneratedReplicaPaths, rnd.Int()%2 == 0)
 
 	for _, opt := range opts {
 		ctx = opt(ctx)