Merge branch 'pks-objectpool-revert-dont-init-on-fetch' into 'master'

objectpool: Revert error if object does not exist on fetch

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

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

4 files changed, 14 insertions, 105 deletions

diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 6cc0e19a7..7f3e72472 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -17,8 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 )
 
@@ -26,14 +24,8 @@ var objectPoolRefspec = fmt.Sprintf("+refs/*:%s/*", git.ObjectPoolRefNamespace)
 
 // FetchFromOrigin initializes the pool and fetches the objects from its origin repository
 func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo) error {
-	if featureflag.ObjectPoolDontInitOnFetch.IsDisabled(ctx) {
-		if err := o.Init(ctx); err != nil {
-			return fmt.Errorf("initializing object pool: %w", err)
-		}
-	} else {
-		if !o.Exists() {
-			return helper.ErrInvalidArgumentf("object pool does not exist")
-		}
+	if err := o.Init(ctx); err != nil {
+		return fmt.Errorf("initializing object pool: %w", err)
 	}
 
 	originPath, err := origin.Path()
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index c0c4f17ed..fe0a32d57 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"strconv"
@@ -14,23 +13,18 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestFetchFromOrigin_dangling(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginDangling)
-}
-
-func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath, err := repo.Path()
+
 	require.NoError(t, err)
 
 	// Write some reachable objects into the object pool member and fetch them into the pool.
@@ -42,7 +36,6 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 		gittest.WithTree(treeID),
 		gittest.WithBranch("master"),
 	)
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
 	// We now write a bunch of objects into the object pool that are not referenced by anything.
@@ -88,19 +81,14 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginFsck)
-}
-
-func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath, err := repo.Path()
 	require.NoError(t, err)
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
 
 	// We're creating a new commit which has a root tree with duplicate entries. git-mktree(1)
@@ -120,19 +108,14 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginDeltaIslands)
-}
-
-func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 	repoPath, err := repo.Path()
 	require.NoError(t, err)
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo), "seed pool")
 	require.NoError(t, pool.Link(ctx, repo))
 
@@ -146,12 +129,8 @@ func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginBitmapHashCache)
-}
-
-func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -159,7 +138,6 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 	require.NoError(t, err)
 	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 
 	bitmaps, err := filepath.Glob(filepath.Join(pool.FullPath(), "objects", "pack", "*.bitmap"))
@@ -171,12 +149,8 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginRefUpdates)
-}
-
-func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -191,7 +165,6 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 	oldRefs["tags/v1.1.0"] = gittest.WriteTag(t, cfg, repoPath, "v1.1.0", oldRefs["heads/csv"].Revision())
 
 	// We now fetch that data into the object pool and verify that it exists as expected.
-	require.NoError(t, pool.Init(ctx))
 	require.NoError(t, pool.FetchFromOrigin(ctx, repo))
 	for ref, oid := range oldRefs {
 		require.Equal(t, oid, gittest.ResolveRevision(t, cfg, poolPath, "refs/remotes/origin/"+ref))
@@ -225,12 +198,8 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginRefs)
-}
-
-func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 
 	// Initialize the object pool and verify that it ain't yet got any references.
@@ -268,24 +237,3 @@ func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
 	// write out in Git.
 	require.NoFileExists(t, filepath.Join(poolPath, "FETCH_HEAD"))
 }
-
-func TestFetchFromOrigin_missingPool(t *testing.T) {
-	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchFromOriginMissingPool)
-}
-
-func testFetchFromOriginMissingPool(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
-	cfg, pool, repoProto := setupObjectPool(t, ctx)
-	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-
-	err := pool.FetchFromOrigin(ctx, repo)
-	if featureflag.ObjectPoolDontInitOnFetch.IsEnabled(ctx) {
-		require.Equal(t, helper.ErrInvalidArgumentf("object pool does not exist"), err)
-		require.False(t, pool.Exists())
-	} else {
-		require.NoError(t, err)
-		require.True(t, pool.Exists())
-	}
-}
diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
index 2ac4d8208..217fee28e 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -24,7 +24,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -40,12 +39,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolsuccess)
-}
-
-func testFetchIntoObjectPoolsuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	parentID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
@@ -102,11 +97,6 @@ func testFetchIntoObjectPoolsuccess(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_transactional(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolTransactional)
-}
-
-func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	var votes []voting.Vote
 	var votesMutex sync.Mutex
@@ -119,6 +109,7 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 		},
 	}
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	cfg.SocketPath = runObjectPoolServer(
 		t, cfg, config.NewLocator(cfg),
@@ -230,11 +221,6 @@ func testFetchIntoObjectPoolTransactional(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolcollectLogStatistics)
-}
-
-func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
@@ -244,6 +230,7 @@ func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Conte
 	logger, hook := test.NewNullLogger()
 	cfg.SocketPath = runObjectPoolServer(t, cfg, locator, logger)
 
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, log.WithField("test", "logging"))
 	repo, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -280,11 +267,6 @@ func testFetchIntoObjectPoolcollectLogStatistics(t *testing.T, ctx context.Conte
 
 func TestFetchIntoObjectPool_Failure(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolfailure)
-}
-
-func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
 	cfgBuilder := testcfg.NewGitalyCfgBuilder()
 	cfg, repos := cfgBuilder.BuildWithRepoAt(t, t.Name())
@@ -302,6 +284,7 @@ func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
 		txManager,
 		housekeeping.NewManager(cfg.Prometheus, txManager),
 	)
+	ctx := testhelper.Context(t)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
 
@@ -352,12 +335,8 @@ func testFetchIntoObjectPoolfailure(t *testing.T, ctx context.Context) {
 
 func TestFetchIntoObjectPool_dfConflict(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.ObjectPoolDontInitOnFetch).Run(t, testFetchIntoObjectPoolDfConflict)
-}
-
-func testFetchIntoObjectPoolDfConflict(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, _, client := setup(t, ctx)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
diff --git a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go b/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
deleted file mode 100644
index aa7bf57ca..000000000
--- a/internal/metadata/featureflag/ff_object_pool_dont_init_on_fetch.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// ObjectPoolDontInitOnFetch will cause FetchIntoObjectPool to not initialize the object pool when
-// it does not yet exist.
-var ObjectPoolDontInitOnFetch = NewFeatureFlag(
-	"object_pool_dont_init_on_fetch",
-	"v15.6.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4560",
-	false,
-)