Merge branch 'jt-remove-replicate-pool-ff' into 'master'

featureflag: Remove `ReplicateRepositoryObjectPool`

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

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

5 files changed, 12 insertions, 71 deletions

diff --git a/internal/featureflag/ff_replicate_repository_object_pool.go b/internal/featureflag/ff_replicate_repository_object_pool.go
deleted file mode 100644
index c7a5fa0ac..000000000
--- a/internal/featureflag/ff_replicate_repository_object_pool.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package featureflag
-
-// ReplicateRepositoryObjectPool will enable replication of a repository's object pool through the
-// `ReplicateRepository` RPC. This will help to preserve object deduplication between repositories
-// that share an object pool.
-var ReplicateRepositoryObjectPool = NewFeatureFlag(
-	"replicate_repository_object_pool",
-	"v16.2.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/5088",
-	true,
-)
diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go
index 9ee9c05bd..38a90b271 100644
--- a/internal/gitaly/service/repository/replicate.go
+++ b/internal/gitaly/service/repository/replicate.go
@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/command"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/objectpool"
@@ -102,7 +101,7 @@ func (s *server) replicateRepository(ctx context.Context, source, target *gitaly
 		return fmt.Errorf("synchronizing gitattributes: %w", err)
 	}
 
-	if featureflag.ReplicateRepositoryObjectPool.IsEnabled(ctx) && replicateObjectDeduplicationNetworkMembership {
+	if replicateObjectDeduplicationNetworkMembership {
 		// Sync the repository object pool before fetching the repository to avoid additional
 		// duplication. If the object pool already exists on the target node, this will potentially
 		// reduce the amount of time it takes to fetch the repository.
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 701e01496..6b3d78910 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -49,7 +49,6 @@ attributes from HEAD.`)
 
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.ReplicateRepositoryObjectPool,
 		featureflag.InterceptReplicateRepository,
 		featureflag.TransactionalAlternatesDisconnect,
 	).Run(t, testReplicateRepository)
@@ -398,7 +397,7 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 			// Consequently, this test case is executed with Praefect disabled.
 			serverOpts: []testserver.GitalyServerOpt{testserver.WithDisablePraefect()},
 			setup: func(t *testing.T, cfg config.Cfg) setupData {
-				sourceProto, _, targetProto, targetPath := setupSourceAndTarget(t, cfg, true)
+				sourceProto, _, targetProto, _ := setupSourceAndTarget(t, cfg, true)
 
 				// If only the target repository is linked to an object pool, repository replication
 				// results in the target repository disconnecting from its object pool to match the
@@ -407,16 +406,10 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 					LinkRepositoryToObjectPool: true,
 				})
 
-				expectedAltInfo, err := stats.AlternatesInfoForRepository(targetPath)
-				require.NoError(t, err)
-				if featureflag.ReplicateRepositoryObjectPool.IsEnabled(ctx) {
-					expectedAltInfo = stats.AlternatesInfo{Exists: false}
-				}
-
 				return setupData{
 					source:              sourceProto,
 					target:              targetProto,
-					expectedAltInfo:     expectedAltInfo,
+					expectedAltInfo:     stats.AlternatesInfo{Exists: false},
 					replicateObjectPool: true,
 				}
 			},
@@ -460,7 +453,7 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 			// Consequently, this test case is executed with Praefect disabled.
 			serverOpts: []testserver.GitalyServerOpt{testserver.WithDisablePraefect()},
 			setup: func(t *testing.T, cfg config.Cfg) setupData {
-				sourceProto, _, targetProto, targetPath := setupSourceAndTarget(t, cfg, true)
+				sourceProto, _, targetProto, _ := setupSourceAndTarget(t, cfg, true)
 
 				// Both the source and target repositories being linked to different object pools is
 				// an unexpected state. If this occurs replication is aborted and an error returned.
@@ -472,18 +465,6 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 					LinkRepositoryToObjectPool: true,
 				})
 
-				if featureflag.ReplicateRepositoryObjectPool.IsDisabled(ctx) {
-					expectedAltInfo, err := stats.AlternatesInfoForRepository(targetPath)
-					require.NoError(t, err)
-
-					return setupData{
-						source:              sourceProto,
-						target:              targetProto,
-						replicateObjectPool: true,
-						expectedAltInfo:     expectedAltInfo,
-					}
-				}
-
 				return setupData{
 					source:              sourceProto,
 					target:              targetProto,
@@ -518,10 +499,6 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 				expectedAltInfo, err := stats.AlternatesInfoForRepository(sourcePath)
 				require.NoError(t, err)
 
-				if featureflag.ReplicateRepositoryObjectPool.IsDisabled(ctx) {
-					expectedAltInfo = stats.AlternatesInfo{Exists: false}
-				}
-
 				return setupData{
 					source:              sourceProto,
 					target:              targetProto,
@@ -552,10 +529,6 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 				expectedAltInfo, err := stats.AlternatesInfoForRepository(sourcePath)
 				require.NoError(t, err)
 
-				if featureflag.ReplicateRepositoryObjectPool.IsDisabled(ctx) {
-					expectedAltInfo = stats.AlternatesInfo{Exists: false}
-				}
-
 				return setupData{
 					source:              sourceProto,
 					target:              targetProto,
@@ -691,12 +664,8 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 
 func TestReplicateRepository_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ReplicateRepositoryObjectPool).Run(t, testReplicateRepositoryTransactional)
-}
-
-func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages("default", "replica"))
 	cfg := cfgBuilder.Build(t)
 
diff --git a/internal/praefect/replicate_repository_test.go b/internal/praefect/replicate_repository_test.go
index a46e3b397..070ecb8eb 100644
--- a/internal/praefect/replicate_repository_test.go
+++ b/internal/praefect/replicate_repository_test.go
@@ -21,7 +21,7 @@ import (
 
 func TestReplicateRepositoryHandler(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.InterceptReplicateRepository, featureflag.ReplicateRepositoryObjectPool).
+	testhelper.NewFeatureSets(featureflag.InterceptReplicateRepository).
 		Run(t, testReplicateRepositoryHandler)
 }
 
@@ -111,11 +111,8 @@ func testReplicateRepositoryHandler(t *testing.T, ctx context.Context) {
 					RelativePath: gittest.NewRepositoryName(t),
 				}
 
-				// If the RPC is not intercepted, the default behavior RPC handler is invoked. When
-				// object pool replication is also enabled this will lead to an object pool being
-				// created on the target storage. This is problematic because Praefect is not aware
-				// of the object pool.
-				if featureflag.InterceptReplicateRepository.IsDisabled(ctx) && featureflag.ReplicateRepositoryObjectPool.IsEnabled(ctx) {
+				// If the RPC is not intercepted, the default behavior RPC handler is invoked.
+				if featureflag.InterceptReplicateRepository.IsDisabled(ctx) {
 					return setupData{
 						source: sourceProto,
 						target: target,
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 13c940066..414f27b3a 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -14,7 +14,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/v16/auth"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	gitalycfg "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/setup"
@@ -41,10 +40,8 @@ import (
 
 func TestReplMgr_ProcessBacklog(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ReplicateRepositoryObjectPool).Run(t, testReplMgrProcessBacklog)
-}
+	ctx := testhelper.Context(t)
 
-func testReplMgrProcessBacklog(t *testing.T, ctx context.Context) {
 	primaryCfg := testcfg.Build(t, testcfg.WithStorages("primary"))
 	testRepoProto, testRepoPath := gittest.CreateRepository(t, ctx, primaryCfg, gittest.CreateRepositoryConfig{
 		SkipCreationViaService: true,
@@ -203,11 +200,8 @@ gitaly_praefect_replication_jobs{change_type="update",gitaly_storage="backup",vi
 
 func TestDefaultReplicator_Replicate(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ReplicateRepositoryObjectPool).Run(t, testDefaultReplicatorReplicate)
-}
 
-func testDefaultReplicatorReplicate(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	newGitalyConn := func(t *testing.T, config gitalycfg.Cfg) *grpc.ClientConn {
 		t.Helper()
@@ -541,10 +535,8 @@ func getChecksumFunc(ctx context.Context, client gitalypb.RepositoryServiceClien
 
 func TestProcessBacklog_FailedJobs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ReplicateRepositoryObjectPool).Run(t, testProcessBacklogFailedJobs)
-}
+	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
-func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
 	primaryCfg := testcfg.Build(t, testcfg.WithStorages("default"))
 	testRepo, _ := gittest.CreateRepository(t, ctx, primaryCfg, gittest.CreateRepositoryConfig{
 		SkipCreationViaService: true,
@@ -577,7 +569,6 @@ func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
 			},
 		},
 	}
-	ctx, cancel := context.WithCancel(ctx)
 
 	queueInterceptor := datastore.NewReplicationEventQueueInterceptor(datastore.NewPostgresReplicationEventQueue(testdb.New(t)))
 
@@ -650,11 +641,7 @@ func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
 
 func TestProcessBacklog_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ReplicateRepositoryObjectPool).Run(t, testProcessBacklogSuccess)
-}
-
-func testProcessBacklogSuccess(t *testing.T, ctx context.Context) {
-	ctx, cancel := context.WithCancel(ctx)
+	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
 	primaryCfg := testcfg.Build(t, testcfg.WithStorages("primary"))
 	primaryCfg.SocketPath = testserver.RunGitalyServer(t, primaryCfg, setup.RegisterAll, testserver.WithDisablePraefect())