diff options
author | Will Chandler <wchandler@gitlab.com> | 2023-11-28 18:56:53 +0300 |
---|---|---|
committer | Will Chandler <wchandler@gitlab.com> | 2023-11-28 18:56:53 +0300 |
commit | 6f824ad13b27677e1a5d74658439ffd777978d1a (patch) | |
tree | ca99df714aa995538f6adfcf3d7d2623ab9388de | |
parent | 4d45db6662456a9018c0ea07f6020bc91dc9394f (diff) | |
parent | 1ca4339feacbc77ccf16dc895687acfe7e93afd7 (diff) |
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>
-rw-r--r-- | internal/featureflag/ff_replicate_repository_object_pool.go | 11 | ||||
-rw-r--r-- | internal/gitaly/service/repository/replicate.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/repository/replicate_test.go | 39 | ||||
-rw-r--r-- | internal/praefect/replicate_repository_test.go | 9 | ||||
-rw-r--r-- | internal/praefect/replicator_test.go | 21 |
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()) |