Merge branch 'smh-tx-object-pool' into 'master'

Enable transactions for object pools

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6562

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

3 files changed, 40 insertions, 54 deletions

diff --git a/internal/praefect/get_object_pool_test.go b/internal/praefect/get_object_pool_test.go
index 83585d7ba..60143fc8f 100644
--- a/internal/praefect/get_object_pool_test.go
+++ b/internal/praefect/get_object_pool_test.go
@@ -273,47 +273,6 @@ func TestGetObjectPoolHandler(t *testing.T) {
 				}
 			},
 		},
-		{
-			desc: "object pool cluster path does not contain valid repository ID",
-			setup: func(t *testing.T) setupData {
-				client, repoStore := setupPraefect(t)
-
-				// Create repositories that will be liked to object pools on each Gitaly node with
-				// replica path and register them in Praefect.
-				relativePath := gittest.NewRepositoryName(t)
-				replicaPath := storage.DeriveReplicaPath(5)
-				repo1, _ := gittest.CreateRepository(t, ctx, gitaly1Cfg, gittest.CreateRepositoryConfig{
-					RelativePath: replicaPath,
-				})
-				repo2, _ := gittest.CreateRepository(t, ctx, gitaly2Cfg, gittest.CreateRepositoryConfig{
-					RelativePath: replicaPath,
-				})
-				require.NoError(t, repoStore.CreateRepository(ctx, 5, virtualStorage, relativePath, replicaPath, gitaly1Storage, []string{gitaly2Storage}, nil, false, false))
-
-				// Create object pool repositories that link to the previously created repositories
-				// with the invalid cluster pool path and register them with Praefect. Praefect
-				// relies on the cluster path to get the repository ID which is needed to fetch
-				// repository metadata. If a valid repository ID cannot be parsed from the object
-				// pool cluster path, an error is returned.
-				poolRelativePath := gittest.NewObjectPoolName(t)
-				poolReplicaPath := storage.DerivePoolPath(6) + "foobar"
-				gittest.CreateObjectPool(t, ctx, gitaly1Cfg, repo1, gittest.CreateObjectPoolConfig{
-					RelativePath:               poolReplicaPath,
-					LinkRepositoryToObjectPool: true,
-				})
-				gittest.CreateObjectPool(t, ctx, gitaly2Cfg, repo2, gittest.CreateObjectPoolConfig{
-					RelativePath:               poolReplicaPath,
-					LinkRepositoryToObjectPool: true,
-				})
-				require.NoError(t, repoStore.CreateRepository(ctx, 6, virtualStorage, poolRelativePath, poolRelativePath, gitaly1Storage, []string{gitaly2Storage}, nil, false, false))
-
-				return setupData{
-					client:        client,
-					repository:    &gitalypb.Repository{StorageName: virtualStorage, RelativePath: relativePath},
-					expectedError: structerr.NewInternal("parsing repository ID: strconv.ParseInt: parsing \"6foobar\": invalid syntax"),
-				}
-			},
-		},
 	} {
 		tc := tc
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/internal/praefect/replicator.go b/internal/praefect/replicator.go
index bc5f1e6ca..0028e2ac7 100644
--- a/internal/praefect/replicator.go
+++ b/internal/praefect/replicator.go
@@ -4,12 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/repository"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/config"
@@ -136,7 +138,16 @@ func (dr defaultReplicator) Replicate(ctx context.Context, event datastore.Repli
 			ObjectPool: targetObjectPool,
 			Repository: targetRepository,
 		}); err != nil {
-			return err
+			if !strings.Contains(err.Error(), storagemgr.ErrRepositoriesAreInDifferentPartitions.Error()) {
+				return err
+			}
+
+			// The pool and the member repository were not in the same partition and thus failed to be linked.
+			// When transactions are enabled, the pool and the member repository must be in the same partition
+			// are only serialized within partitions. Moving a repository into a different partition is difficult
+			// as one would have to create a new repository in the same partition as the pool, and delete the old
+			// one. We don't intend to support moving repositories between partitions with Praefect. If we hit this
+			// error, we'll leave the repository without the alternate link.
 		}
 	}
 
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 414f27b3a..68ba3f314 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -292,18 +292,26 @@ func TestDefaultReplicator_Replicate(t *testing.T) {
 					RelativePath: sourcePool.Repository.RelativePath,
 				})
 
+				expectedPool := &gitalypb.ObjectPool{
+					Repository: &gitalypb.Repository{
+						StorageName:  targetStorage,
+						RelativePath: sourcePool.Repository.RelativePath,
+					},
+				}
+
+				if testhelper.IsWALEnabled() {
+					// See the replicator's replicate method for reasoning behind the pool
+					// not being connected with transactions.
+					expectedPool = nil
+				}
+
 				return setupData{
 					job: datastore.ReplicationJob{
 						ReplicaPath:       sourceRepo.RelativePath,
 						TargetNodeStorage: targetStorage,
 						SourceNodeStorage: sourceStorage,
 					},
-					expectedPool: &gitalypb.ObjectPool{
-						Repository: &gitalypb.Repository{
-							StorageName:  targetStorage,
-							RelativePath: sourcePool.Repository.RelativePath,
-						},
-					},
+					expectedPool: expectedPool,
 				}
 			},
 		},
@@ -357,18 +365,26 @@ func TestDefaultReplicator_Replicate(t *testing.T) {
 					RelativePath: sourcePool.Repository.RelativePath,
 				})
 
+				expectedPool := &gitalypb.ObjectPool{
+					Repository: &gitalypb.Repository{
+						StorageName:  targetStorage,
+						RelativePath: sourcePool.Repository.RelativePath,
+					},
+				}
+
+				if testhelper.IsWALEnabled() {
+					// See the replicator's replicate method for reasoning behind the pool
+					// not being connected with transactions.
+					expectedPool = nil
+				}
+
 				return setupData{
 					job: datastore.ReplicationJob{
 						ReplicaPath:       sourceRepo.RelativePath,
 						TargetNodeStorage: targetStorage,
 						SourceNodeStorage: sourceStorage,
 					},
-					expectedPool: &gitalypb.ObjectPool{
-						Repository: &gitalypb.Repository{
-							StorageName:  targetStorage,
-							RelativePath: sourcePool.Repository.RelativePath,
-						},
-					},
+					expectedPool: expectedPool,
 				}
 			},
 		},