Merge branch 'pks-objectpool-remove-optimize-repository-ff' into 'master'

objectpool: Remove feature flag guarding heuristical repo maintenance

Closes #4342

See merge request gitlab-org/gitaly!4757

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 bb7b5c921..514927555 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())
@@ -245,12 +223,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,
-)