objectpool: Remove feature flag guarding heuristical repo maintenancepks-objectpool-remove-optimize-repository-ff

In 410295810 (objectpool: Switch to use OptimizeRepository for pool
repos, 2022-07-12) we have migrated maintenance of object pools in
`FetchIntoObjectPool()` to use the same repository maintenance as we use
for normal repositories. This deduplicates the logic, makes sure we keep
more data structures up to date in object pools and also unifies nightly
and normal maintenance for pool repositories.

We have rolled out this feautre flag to production without any issues
observed so far. So let's remove the feature flag and unconditionally
enable heuristical repository maintenance for object pools.

Changelog: changed

4 files changed, 10 insertions, 108 deletions

diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 44c940dd2..2c491d9a0 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -15,9 +15,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 )
 
 // FetchFromOrigin initializes the pool and fetches the objects from its origin repository
@@ -73,21 +71,8 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo
 		return fmt.Errorf("computing stats after fetch: %w", err)
 	}
 
-	if featureflag.FetchIntoObjectPoolOptimizeRepository.IsEnabled(ctx) {
-		if err := o.housekeepingManager.OptimizeRepository(ctx, o.Repo); err != nil {
-			return fmt.Errorf("optimizing pool repo: %w", err)
-		}
-	} else {
-		if err := o.Repo.ExecAndWait(ctx, git.SubCmd{
-			Name:  "pack-refs",
-			Flags: []git.Option{git.Flag{Name: "--all"}},
-		}); err != nil {
-			return fmt.Errorf("packing pool refs: %w", err)
-		}
-
-		if err := o.repackPool(ctx, o); err != nil {
-			return fmt.Errorf("repacking pool: %w", err)
-		}
+	if err := o.housekeepingManager.OptimizeRepository(ctx, o.Repo); err != nil {
+		return fmt.Errorf("optimizing pool repo: %w", err)
 	}
 
 	return nil
@@ -145,30 +130,6 @@ func (o *ObjectPool) rescueDanglingObjects(ctx context.Context) error {
 	return updater.Commit()
 }
 
-func (o *ObjectPool) repackPool(ctx context.Context, pool repository.GitRepo) error {
-	config := []git.ConfigPair{
-		{Key: "pack.island", Value: git.ObjectPoolRefNamespace + "/he(a)ds"},
-		{Key: "pack.island", Value: git.ObjectPoolRefNamespace + "/t(a)gs"},
-		{Key: "pack.islandCore", Value: "a"},
-		{Key: "pack.writeBitmapHashCache", Value: "true"},
-	}
-
-	if err := o.Repo.ExecAndWait(ctx, git.SubCmd{
-		Name: "repack",
-		Flags: []git.Option{
-			git.Flag{Name: "-aidb"},
-			// This can be removed as soon as we have upstreamed a
-			// `repack.updateServerInfo` config option. See gitlab-org/git#105 for more
-			// details.
-			git.Flag{Name: "-n"},
-		},
-	}, git.WithConfig(config...)); err != nil {
-		return err
-	}
-
-	return nil
-}
-
 func (o *ObjectPool) logStats(ctx context.Context, when string) error {
 	fields := logrus.Fields{
 		"when": when,
diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go
index 880fa2dd3..6e8bb53e7 100644
--- a/internal/git/objectpool/fetch_test.go
+++ b/internal/git/objectpool/fetch_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -17,18 +16,13 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"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.FetchIntoObjectPoolOptimizeRepository).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)
 
@@ -98,12 +92,8 @@ func testFetchFromOriginDangling(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_fsck(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).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 := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
@@ -127,12 +117,8 @@ func testFetchFromOriginFsck(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_deltaIslands(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).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)
@@ -158,12 +144,8 @@ func testFetchFromOriginDeltaIslands(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_bitmapHashCache(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).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)
 
@@ -188,12 +170,8 @@ func testFetchFromOriginBitmapHashCache(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refUpdates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).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)
 	repoPath := filepath.Join(cfg.Storages[0].Path, repo.GetRelativePath())
@@ -246,12 +224,8 @@ func testFetchFromOriginRefUpdates(t *testing.T, ctx context.Context) {
 
 func TestFetchFromOrigin_refs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).Run(t, testFetchFromOriginRefs)
-}
-
-func testFetchFromOriginRefs(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, pool, _ := setupObjectPool(t, ctx)
 	poolPath := pool.FullPath()
 
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 e9a4320b0..c9c6aaa10 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -3,7 +3,6 @@
 package objectpool
 
 import (
-	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -20,7 +19,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/housekeeping"
 	"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/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"
@@ -34,12 +32,8 @@ import (
 
 func TestFetchIntoObjectPool_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).Run(t, testFetchIntoObjectPoolSuccess)
-}
-
-func testFetchIntoObjectPoolSuccess(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, locator, client := setup(ctx, t)
 
 	repoCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(t.Name()))
@@ -70,19 +64,6 @@ func testFetchIntoObjectPoolSuccess(t *testing.T, ctx context.Context) {
 	// Verify that the newly written commit exists in the repository now.
 	gittest.Exec(t, cfg, "-C", pool.FullPath(), "rev-parse", "--verify", repoCommit.String())
 
-	if featureflag.FetchIntoObjectPoolOptimizeRepository.IsDisabled(ctx) {
-		// We used to verify here how objects have been packed. This doesn't make a lot of
-		// sense anymore in the new world though, where we rely on the housekeeping package
-		// to do repository maintenance for us. We thus only check for this when the feature
-		// flag is disabled.
-		packFiles, err := filepath.Glob(filepath.Join(pool.FullPath(), "objects", "pack", "pack-*.pack"))
-		require.NoError(t, err)
-		require.Len(t, packFiles, 1, "ensure commits got packed")
-
-		packContents := gittest.Exec(t, cfg, "-C", pool.FullPath(), "verify-pack", "-v", packFiles[0])
-		require.Contains(t, string(packContents), repoCommit.String())
-	}
-
 	_, err = client.FetchIntoObjectPool(ctx, req)
 	require.NoError(t, err, "calling FetchIntoObjectPool twice should be OK")
 	require.True(t, pool.IsValid(), "ensure that pool is valid")
@@ -144,12 +125,8 @@ func TestFetchIntoObjectPool_hooks(t *testing.T) {
 
 func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchIntoObjectPoolOptimizeRepository).Run(t, testFetchIntoObjectPoolCollectLogStatistics)
-}
-
-func testFetchIntoObjectPoolCollectLogStatistics(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 
diff --git a/internal/metadata/featureflag/ff_fetch_into_object_pool_optimize_repository.go b/internal/metadata/featureflag/ff_fetch_into_object_pool_optimize_repository.go
deleted file mode 100644
index 0d47c50df..000000000
--- a/internal/metadata/featureflag/ff_fetch_into_object_pool_optimize_repository.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// FetchIntoObjectPoolOptimizeRepository will cause FetchIntoObjectPool to use OptimizeRepository to
-// maintain the object pool instead of manually performing repository maintenance.
-var FetchIntoObjectPoolOptimizeRepository = NewFeatureFlag(
-	"fetch_into_object_pool_optimize_repository",
-	"v15.2.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4342",
-	false,
-)