Merge branch 'jt-praefect-pool-sync' into 'master'

praefect: Reconcile non-matching object pools during replication

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

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

2 files changed, 67 insertions, 11 deletions

diff --git a/internal/praefect/replicator.go b/internal/praefect/replicator.go
index 26e47b26b..bc5f1e6ca 100644
--- a/internal/praefect/replicator.go
+++ b/internal/praefect/replicator.go
@@ -88,32 +88,49 @@ func (dr defaultReplicator) Replicate(ctx context.Context, event datastore.Repli
 		return fmt.Errorf("failed to create repository: %w", err)
 	}
 
-	// check if the repository has an object pool
+	// Object pool state between the source and target repository should match. Get object pool
+	// information for the source and target repository and reconcile any detected differences.
 	sourceObjectPoolClient := gitalypb.NewObjectPoolServiceClient(sourceCC)
+	targetObjectPoolClient := gitalypb.NewObjectPoolServiceClient(targetCC)
 
-	resp, err := sourceObjectPoolClient.GetObjectPool(ctx, &gitalypb.GetObjectPoolRequest{
+	sourceResp, err := sourceObjectPoolClient.GetObjectPool(ctx, &gitalypb.GetObjectPoolRequest{
 		Repository: sourceRepository,
 	})
 	if err != nil {
 		return err
 	}
 
-	sourceObjectPool := resp.GetObjectPool()
+	targetResp, err := targetObjectPoolClient.GetObjectPool(ctx, &gitalypb.GetObjectPoolRequest{
+		Repository: targetRepository,
+	})
+	if err != nil {
+		return err
+	}
 
-	targetObjectPoolClient := gitalypb.NewObjectPoolServiceClient(targetCC)
+	sourcePool := sourceResp.GetObjectPool()
+	targetPool := targetResp.GetObjectPool()
 
-	if sourceObjectPool == nil {
-		// If the source repository is not linked to an object pool, the target repository
-		// should also not be linked to any object pool to ensure consistency.
+	switch {
+	// If the source and target object pool state already match, there is nothing to sync.
+	case sourcePool.GetRepository().GetRelativePath() == targetPool.GetRepository().GetRelativePath():
+	// If the target repository is linked to a non-matching object pool it must be disconnected.
+	case targetPool != nil:
 		if _, err := targetObjectPoolClient.DisconnectGitAlternates(ctx, &gitalypb.DisconnectGitAlternatesRequest{
 			Repository: targetRepository,
 		}); err != nil {
 			return err
 		}
-	} else {
-		// If the source repository is linked to an object pool, the target repository
-		// should link to the same object pool.
-		targetObjectPool := proto.Clone(sourceObjectPool).(*gitalypb.ObjectPool)
+
+		// If the source repository is not linked to an object pool, the target repository does not
+		// need to be linked to a new object pool. Otherwise, continue to object pool linking.
+		if sourcePool == nil {
+			break
+		}
+		fallthrough
+	// If the source pool is linked to a repository, link the target repository to the matching
+	// target object pool.
+	case targetPool == nil:
+		targetObjectPool := proto.Clone(sourcePool).(*gitalypb.ObjectPool)
 		targetObjectPool.GetRepository().StorageName = targetRepository.GetStorageName()
 		if _, err := targetObjectPoolClient.LinkRepositoryToObjectPool(ctx, &gitalypb.LinkRepositoryToObjectPoolRequest{
 			ObjectPool: targetObjectPool,
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index c3f3c5797..13c940066 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -339,6 +339,45 @@ func testDefaultReplicatorReplicate(t *testing.T, ctx context.Context) {
 				}
 			},
 		},
+		{
+			desc: "target disconnected and linked to match source",
+			setup: func(t *testing.T) setupData {
+				// Create repository on source Gitaly that does not have a link to an object pool.
+				sourceRepo, _ := gittest.CreateRepository(t, ctx, sourceCfg)
+				sourcePool, _ := gittest.CreateObjectPool(t, ctx, sourceCfg, sourceRepo, gittest.CreateObjectPoolConfig{
+					LinkRepositoryToObjectPool: true,
+				})
+
+				// Create repository on target Gitaly with a relative path different from the source
+				// repository and link it to an object pool.
+				targetRepo, _ := gittest.CreateRepository(t, ctx, targetCfg, gittest.CreateRepositoryConfig{
+					RelativePath: sourceRepo.RelativePath,
+				})
+				gittest.CreateObjectPool(t, ctx, targetCfg, targetRepo, gittest.CreateObjectPoolConfig{
+					LinkRepositoryToObjectPool: true,
+				})
+
+				// Create object pool on target Gitaly with the same relative path as the source
+				// object pool. This repository will eventually be linked to the target repository.
+				gittest.CreateRepository(t, ctx, targetCfg, gittest.CreateRepositoryConfig{
+					RelativePath: sourcePool.Repository.RelativePath,
+				})
+
+				return setupData{
+					job: datastore.ReplicationJob{
+						ReplicaPath:       sourceRepo.RelativePath,
+						TargetNodeStorage: targetStorage,
+						SourceNodeStorage: sourceStorage,
+					},
+					expectedPool: &gitalypb.ObjectPool{
+						Repository: &gitalypb.Repository{
+							StorageName:  targetStorage,
+							RelativePath: sourcePool.Repository.RelativePath,
+						},
+					},
+				}
+			},
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			testSetup := tc.setup(t)