diff options
author | Justin Tobler <jtobler@gitlab.com> | 2023-12-14 23:12:27 +0300 |
---|---|---|
committer | GitLab <noreply@gitlab.com> | 2023-12-14 23:12:27 +0300 |
commit | 694ed8c84ab21a53426bccee500c1fca3800d1f2 (patch) | |
tree | fb192962d6116819c5679fae1c2ef892188cb547 /internal/praefect | |
parent | 6214bf505356bff62260485c662f719cb490244a (diff) | |
parent | a15c2cee76f804919a6d7eee33acef1446205fbd (diff) |
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>
Diffstat (limited to 'internal/praefect')
-rw-r--r-- | internal/praefect/get_object_pool_test.go | 41 | ||||
-rw-r--r-- | internal/praefect/replicator.go | 13 | ||||
-rw-r--r-- | internal/praefect/replicator_test.go | 40 |
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, } }, }, |