housekeeping: Extend OptimizeRepository tests to test object pools

We already have some differences here because we never prune objects in
pool repositories, and we're about to tweak pool maintenance a bit more.

Extend tests for OptimizeRepository and related functionality to
explicitly verify behaviour with object pools.

2 files changed, 144 insertions, 65 deletions

diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index 096394d4f..08aa7ee13 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -32,22 +32,26 @@ func TestNeedsRepacking(t *testing.T) {
 
 	for _, tc := range []struct {
 		desc           string
-		setup          func(t *testing.T) *gitalypb.Repository
+		setup          func(t *testing.T, relativePath string) *gitalypb.Repository
 		expectedErr    error
 		expectedNeeded bool
 		expectedConfig RepackObjectsConfig
 	}{
 		{
 			desc: "empty repo does nothing",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+					WithRelativePath: relativePath,
+				})
 				return repoProto
 			},
 		},
 		{
 			desc: "missing bitmap",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				return repoProto
 			},
 			expectedNeeded: true,
@@ -58,8 +62,10 @@ func TestNeedsRepacking(t *testing.T) {
 		},
 		{
 			desc: "missing bitmap with alternate",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 
 				// Create the alternates file. If it exists, then we shouldn't try
 				// to generate a bitmap.
@@ -75,8 +81,10 @@ func TestNeedsRepacking(t *testing.T) {
 		},
 		{
 			desc: "missing commit-graph",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-Ad", "--write-bitmap-index")
 
@@ -90,8 +98,10 @@ func TestNeedsRepacking(t *testing.T) {
 		},
 		{
 			desc: "commit-graph without bloom filters",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-Ad", "--write-bitmap-index")
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write")
@@ -106,8 +116,10 @@ func TestNeedsRepacking(t *testing.T) {
 		},
 		{
 			desc: "no repack needed",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repoProto, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-Ad", "--write-bitmap-index")
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--changed-paths", "--split")
@@ -121,13 +133,15 @@ func TestNeedsRepacking(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			t.Parallel()
 
-			repoProto := tc.setup(t)
-			repo := localrepo.NewTestRepo(t, cfg, repoProto)
+			testRepoAndPool(t, tc.desc, func(t *testing.T, relativePath string) {
+				repoProto := tc.setup(t, relativePath)
+				repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
-			repackNeeded, repackCfg, err := needsRepacking(repo)
-			require.Equal(t, tc.expectedErr, err)
-			require.Equal(t, tc.expectedNeeded, repackNeeded)
-			require.Equal(t, tc.expectedConfig, repackCfg)
+				repackNeeded, repackCfg, err := needsRepacking(repo)
+				require.Equal(t, tc.expectedErr, err)
+				require.Equal(t, tc.expectedNeeded, repackNeeded)
+				require.Equal(t, tc.expectedConfig, repackCfg)
+			})
 		})
 	}
 
@@ -167,8 +181,10 @@ func TestNeedsRepacking(t *testing.T) {
 		},
 		// Let's not go any further than this, we're thrashing the temporary directory.
 	} {
-		t.Run(fmt.Sprintf("packfile with %d bytes", tc.packfileSize), func(t *testing.T) {
-			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+		testRepoAndPool(t, fmt.Sprintf("packfile with %d bytes", tc.packfileSize), func(t *testing.T, relativePath string) {
+			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+				WithRelativePath: relativePath,
+			})
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 			packDir := filepath.Join(repoPath, "objects", "pack")
 
@@ -272,8 +288,10 @@ func TestNeedsRepacking(t *testing.T) {
 			expectedRepack: true,
 		},
 	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+		testRepoAndPool(t, tc.desc, func(t *testing.T, relativePath string) {
+			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+				WithRelativePath: relativePath,
+			})
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 			// Emulate the existence of a bitmap and a commit-graph with bloom filters.
@@ -348,8 +366,10 @@ func TestPackRefsIfNeeded(t *testing.T) {
 			requiredRefs:   99,
 		},
 	} {
-		t.Run(fmt.Sprintf("packed-refs with %d bytes", tc.packedRefsSize), func(t *testing.T) {
-			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+		testRepoAndPool(t, fmt.Sprintf("packed-refs with %d bytes", tc.packedRefsSize), func(t *testing.T, relativePath string) {
+			repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+				WithRelativePath: relativePath,
+			})
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 			// Write an empty commit such that we can create valid refs.
@@ -483,15 +503,18 @@ func TestOptimizeRepository(t *testing.T) {
 	txManager := transaction.NewManager(cfg, backchannel.NewRegistry())
 
 	for _, tc := range []struct {
-		desc            string
-		setup           func(t *testing.T) *gitalypb.Repository
-		expectedErr     error
-		expectedMetrics string
+		desc                   string
+		setup                  func(t *testing.T, relativePath string) *gitalypb.Repository
+		expectedErr            error
+		expectedMetrics        string
+		expectedMetricsForPool string
 	}{
 		{
 			desc: "empty repository does nothing",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+					WithRelativePath: relativePath,
+				})
 				return repo
 			},
 			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
@@ -501,8 +524,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 		},
 		{
 			desc: "repository without bitmap repacks objects",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				return repo
 			},
 			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
@@ -514,8 +539,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 		},
 		{
 			desc: "repository without commit-graph repacks objects",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "--write-bitmap-index")
 				return repo
 			},
@@ -528,8 +555,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 		},
 		{
 			desc: "well-packed repository does not optimize",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "--write-bitmap-index")
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--split", "--changed-paths")
 				return repo
@@ -541,8 +570,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 		},
 		{
 			desc: "recent loose objects don't get pruned",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "--write-bitmap-index")
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--split", "--changed-paths")
 
@@ -571,8 +602,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 		},
 		{
 			desc: "old loose objects get pruned",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0], gittest.CloneRepoOpts{
+					RelativePath: relativePath,
+				})
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "--write-bitmap-index")
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--split", "--changed-paths")
 
@@ -596,11 +629,19 @@ gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_incremental",
 gitaly_housekeeping_tasks_total{housekeeping_task="pruned_objects",status="success"} 1
 gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 `,
+			// Object pools never prune objects.
+			expectedMetricsForPool: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_incremental", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "loose refs get packed",
-			setup: func(t *testing.T) *gitalypb.Repository {
-				repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+			setup: func(t *testing.T, relativePath string) *gitalypb.Repository {
+				repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+					WithRelativePath: relativePath,
+				})
 
 				for i := 0; i < 16; i++ {
 					gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(), gittest.WithBranch(fmt.Sprintf("branch-%d", i)))
@@ -618,10 +659,10 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 `,
 		},
 	} {
-		t.Run(tc.desc, func(t *testing.T) {
+		testRepoAndPool(t, tc.desc, func(t *testing.T, relativePath string) {
 			ctx := testhelper.Context(t)
 
-			repoProto := tc.setup(t)
+			repoProto := tc.setup(t, relativePath)
 			repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 			manager := NewManager(cfg.Prometheus, txManager)
@@ -629,9 +670,14 @@ gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
 			err := manager.OptimizeRepository(ctx, repo)
 			require.Equal(t, tc.expectedErr, err)
 
-			assert.NoError(t, testutil.CollectAndCompare(
+			expectedMetrics := tc.expectedMetrics
+			if IsPoolRepository(repoProto) && tc.expectedMetricsForPool != "" {
+				expectedMetrics = tc.expectedMetricsForPool
+			}
+
+			require.NoError(t, testutil.CollectAndCompare(
 				manager.tasksTotal,
-				bytes.NewBufferString(tc.expectedMetrics),
+				bytes.NewBufferString(expectedMetrics),
 				"gitaly_housekeeping_tasks_total",
 			))
 		})
@@ -764,8 +810,10 @@ func TestPruneIfNeeded(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
-	t.Run("empty repo does not prune", func(t *testing.T) {
-		repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	testRepoAndPool(t, "empty repo does not prune", func(t *testing.T, relativePath string) {
+		repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+			WithRelativePath: relativePath,
+		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		didPrune, err := pruneIfNeeded(ctx, repo)
@@ -773,8 +821,10 @@ func TestPruneIfNeeded(t *testing.T) {
 		require.False(t, didPrune)
 	})
 
-	t.Run("repo with single object does not prune", func(t *testing.T) {
-		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	testRepoAndPool(t, "repo with single object does not prune", func(t *testing.T, relativePath string) {
+		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+			WithRelativePath: relativePath,
+		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		gittest.WriteBlob(t, cfg, repoPath, []byte("something"))
@@ -784,8 +834,10 @@ func TestPruneIfNeeded(t *testing.T) {
 		require.False(t, didPrune)
 	})
 
-	t.Run("repo with single 17-prefixed objects does not prune", func(t *testing.T) {
-		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	testRepoAndPool(t, "repo with single 17-prefixed objects does not prune", func(t *testing.T, relativePath string) {
+		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+			WithRelativePath: relativePath,
+		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("32"))
@@ -796,8 +848,10 @@ func TestPruneIfNeeded(t *testing.T) {
 		require.False(t, didPrune)
 	})
 
-	t.Run("repo with four 17-prefixed objects does not prune", func(t *testing.T) {
-		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	testRepoAndPool(t, "repo with four 17-prefixed objects does not prune", func(t *testing.T, relativePath string) {
+		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+			WithRelativePath: relativePath,
+		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		for _, contents := range []string{"32", "119", "334", "782"} {
@@ -810,8 +864,10 @@ func TestPruneIfNeeded(t *testing.T) {
 		require.False(t, didPrune)
 	})
 
-	t.Run("repo with five 17-prefixed objects does prune after grace period", func(t *testing.T) {
-		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	testRepoAndPool(t, "repo with five 17-prefixed objects does prune after grace period", func(t *testing.T, relativePath string) {
+		repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{
+			WithRelativePath: relativePath,
+		})
 		repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 		objectPath := func(oid git.ObjectID) string {
@@ -852,15 +908,24 @@ func TestPruneIfNeeded(t *testing.T) {
 		// and thus we would still want to prune here.
 		didPrune, err = pruneIfNeeded(ctx, repo)
 		require.NoError(t, err)
-		require.True(t, didPrune)
 
-		// But this time the objects shouldn't exist anymore because they were older than
-		// the grace period.
-		for _, blob := range blobs {
-			require.NoFileExists(t, objectPath(blob))
-		}
+		if IsPoolRepository(repoProto) {
+			// Object pools mustn't ever prune objects.
+			require.False(t, didPrune)
+			for _, blob := range append(blobs, recentBlob) {
+				require.FileExists(t, objectPath(blob))
+			}
+		} else {
+			require.True(t, didPrune)
 
-		// The recent blob should continue to exist though.
-		require.FileExists(t, objectPath(recentBlob))
+			// But this time the objects shouldn't exist anymore because they were older than
+			// the grace period.
+			for _, blob := range blobs {
+				require.NoFileExists(t, objectPath(blob))
+			}
+
+			// The recent blob should continue to exist though.
+			require.FileExists(t, objectPath(recentBlob))
+		}
 	})
 }
diff --git a/internal/git/housekeeping/testhelper_test.go b/internal/git/housekeeping/testhelper_test.go
index 3de1870cd..ccb0e1a28 100644
--- a/internal/git/housekeeping/testhelper_test.go
+++ b/internal/git/housekeeping/testhelper_test.go
@@ -3,9 +3,23 @@ package housekeeping
 import (
 	"testing"
 
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
 	testhelper.Run(m)
 }
+
+func testRepoAndPool(t *testing.T, desc string, testFunc func(t *testing.T, relativePath string)) {
+	t.Helper()
+	t.Run(desc, func(t *testing.T) {
+		t.Run("normal repository", func(t *testing.T) {
+			testFunc(t, gittest.NewRepositoryName(t, true))
+		})
+
+		t.Run("object pool", func(t *testing.T) {
+			testFunc(t, gittest.NewObjectPoolName(t))
+		})
+	})
+}