housekeeping: Implement repository housekeeping via the manager

Now that we have the housekeeping manager globally injected as required,
move the logic to clean up repositories and optimize them to this
manager.

7 files changed, 29 insertions, 23 deletions

diff --git a/internal/git/housekeeping/housekeeping.go b/internal/git/housekeeping/clean_stale_data.go
index d157fb08d..c1c2c9797 100644
--- a/internal/git/housekeeping/housekeeping.go
+++ b/internal/git/housekeeping/clean_stale_data.go
@@ -15,7 +15,6 @@ import (
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"google.golang.org/grpc/codes"
@@ -55,8 +54,8 @@ func init() {
 
 type staleFileFinderFn func(context.Context, string) ([]string, error)
 
-// Perform will perform housekeeping duties on a repository
-func Perform(ctx context.Context, repo *localrepo.Repo, txManager transaction.Manager) error {
+// CleanStaleData cleans up any stale data in the repository.
+func (m *RepositoryManager) CleanStaleData(ctx context.Context, repo *localrepo.Repo) error {
 	repoPath, err := repo.Path()
 	if err != nil {
 		myLogger(ctx).WithError(err).Warn("housekeeping failed to get repo path")
@@ -110,7 +109,7 @@ func Perform(ctx context.Context, repo *localrepo.Repo, txManager transaction.Ma
 	// TODO: https://gitlab.com/gitlab-org/gitaly/-/issues/3987
 	// This is a temporary code and needs to be removed once it will be run on all repositories at least once.
 	unnecessaryConfigRegex := "^(http\\..+\\.extraheader|remote\\..+\\.(fetch|mirror|prune|url)|core\\.(commitgraph|sparsecheckout|splitindex))$"
-	if err := repo.UnsetMatchingConfig(ctx, unnecessaryConfigRegex, txManager); err != nil {
+	if err := repo.UnsetMatchingConfig(ctx, unnecessaryConfigRegex, m.txManager); err != nil {
 		if !errors.Is(err, git.ErrNotFound) {
 			return fmt.Errorf("housekeeping could not unset unnecessary config lines: %w", err)
 		}
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/clean_stale_data_test.go
index 00c794bf1..881b1c374 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/clean_stale_data_test.go
@@ -217,7 +217,7 @@ func TestPerform(t *testing.T) {
 				e.create(t, repoPath)
 			}
 
-			require.NoError(t, Perform(ctx, repo, nil))
+			require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -302,7 +302,7 @@ func TestPerform_references(t *testing.T) {
 			}
 			ctx := testhelper.Context(t)
 
-			require.NoError(t, Perform(ctx, repo, nil))
+			require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 
 			var actual []string
 			require.NoError(t, filepath.Walk(filepath.Join(repoPath, "refs"), func(path string, info os.FileInfo, _ error) error {
@@ -403,7 +403,7 @@ func TestPerform_emptyRefDirs(t *testing.T) {
 				e.create(t, repoPath)
 			}
 
-			require.NoError(t, Perform(ctx, repo, nil))
+			require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -428,8 +428,9 @@ func testPerformWithSpecificFile(t *testing.T, file string, finder staleFileFind
 
 	cfg, repoProto, repoPath := testcfg.BuildWithRepo(t)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	mgr := NewManager(nil)
 
-	require.NoError(t, Perform(ctx, repo, nil))
+	require.NoError(t, mgr.CleanStaleData(ctx, repo))
 	for _, tc := range []struct {
 		desc          string
 		entries       []entry
@@ -476,7 +477,7 @@ func testPerformWithSpecificFile(t *testing.T, file string, finder staleFileFind
 			require.NoError(t, err)
 			require.ElementsMatch(t, tc.expectedFiles, staleFiles)
 
-			require.NoError(t, Perform(ctx, repo, nil))
+			require.NoError(t, mgr.CleanStaleData(ctx, repo))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -559,7 +560,7 @@ func TestPerform_referenceLocks(t *testing.T) {
 			require.NoError(t, err)
 			require.ElementsMatch(t, expectedReferenceLocks, staleLockfiles)
 
-			require.NoError(t, Perform(ctx, repo, nil))
+			require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 
 			for _, e := range tc.entries {
 				e.validate(t, repoPath)
@@ -649,7 +650,7 @@ func TestPerformRepoDoesNotExist(t *testing.T) {
 
 	require.NoError(t, os.RemoveAll(repoPath))
 
-	require.NoError(t, Perform(ctx, repo, nil))
+	require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 }
 
 func TestPerform_UnsetConfiguration(t *testing.T) {
@@ -685,7 +686,7 @@ func TestPerform_UnsetConfiguration(t *testing.T) {
 	unrelated = untouched
 `), 0o644))
 
-	require.NoError(t, Perform(ctx, repo, nil))
+	require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 	require.Equal(t,
 		`[core]
 	repositoryformatversion = 0
@@ -718,7 +719,7 @@ func TestPerform_UnsetConfiguration_transactional(t *testing.T) {
 		AuthInfo: backchannel.WithID(nil, 1234),
 	})
 
-	require.NoError(t, Perform(ctx, repo, txManager))
+	require.NoError(t, NewManager(txManager).CleanStaleData(ctx, repo))
 	require.Equal(t, 2, len(txManager.Votes()))
 
 	configKeys := gittest.Exec(t, cfg, "-C", repoPath, "config", "--list", "--local", "--name-only")
@@ -762,7 +763,7 @@ func TestPerform_cleanupConfig(t *testing.T) {
 [remote "tmp-8c948ca94832c2725733e48cb2902287"]
 `), 0o644))
 
-	require.NoError(t, Perform(ctx, repo, nil))
+	require.NoError(t, NewManager(nil).CleanStaleData(ctx, repo))
 	require.Equal(t, `[core]
 	repositoryformatversion = 0
 	filemode = true
diff --git a/internal/git/housekeeping/manager.go b/internal/git/housekeeping/manager.go
index e19021f7e..17e5911f3 100644
--- a/internal/git/housekeeping/manager.go
+++ b/internal/git/housekeeping/manager.go
@@ -1,13 +1,22 @@
 package housekeeping
 
 import (
+	"context"
+
 	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 )
 
 // Manager is a housekeeping manager. It is supposed to handle housekeeping tasks for repositories
 // such as the cleanup of unneeded files and optimizations for the repository's data structures.
-type Manager interface{}
+type Manager interface {
+	// CleanStaleData removes any stale data in the repository.
+	CleanStaleData(context.Context, *localrepo.Repo) error
+	// OptimizeRepository optimizes the repository's data structures such that it can be more
+	// efficiently served.
+	OptimizeRepository(context.Context, *localrepo.Repo) error
+}
 
 // RepositoryManager is an implementation of the Manager interface.
 type RepositoryManager struct {
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 7fdb76f8a..d1f4da740 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -15,13 +15,12 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/stats"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 )
 
 // OptimizeRepository performs optimizations on the repository. Whether optimizations are performed
 // or not depends on a set of heuristics.
-func OptimizeRepository(ctx context.Context, repo *localrepo.Repo, txManager transaction.Manager) error {
+func (m *RepositoryManager) OptimizeRepository(ctx context.Context, repo *localrepo.Repo) error {
 	optimizations := struct {
 		PackedObjects bool `json:"packed_objects"`
 		PrunedObjects bool `json:"pruned_objects"`
@@ -31,7 +30,7 @@ func OptimizeRepository(ctx context.Context, repo *localrepo.Repo, txManager tra
 		ctxlogrus.Extract(ctx).WithField("optimizations", optimizations).Info("optimized repository")
 	}()
 
-	if err := Perform(ctx, repo, txManager); err != nil {
+	if err := m.CleanStaleData(ctx, repo); err != nil {
 		return fmt.Errorf("could not execute houskeeping: %w", err)
 	}
 
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index bf03e9496..c04406d6b 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -15,7 +15,6 @@ import (
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref"
@@ -35,7 +34,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo
 		return err
 	}
 
-	if err := housekeeping.Perform(ctx, o.Repo, o.txManager); err != nil {
+	if err := o.housekeepingManager.CleanStaleData(ctx, o.Repo); err != nil {
 		return err
 	}
 
diff --git a/internal/gitaly/service/repository/gc.go b/internal/gitaly/service/repository/gc.go
index 0e45a3534..b92842d06 100644
--- a/internal/gitaly/service/repository/gc.go
+++ b/internal/gitaly/service/repository/gc.go
@@ -38,7 +38,7 @@ func (s *server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollect
 	}
 
 	// Perform housekeeping to cleanup stale lockfiles that may block GC
-	if err := housekeeping.Perform(ctx, repo, s.txManager); err != nil {
+	if err := s.housekeepingManager.CleanStaleData(ctx, repo); err != nil {
 		ctxlogger.WithError(err).Warn("Pre gc housekeeping failed")
 	}
 
diff --git a/internal/gitaly/service/repository/optimize.go b/internal/gitaly/service/repository/optimize.go
index cea96b6de..bbf1ee406 100644
--- a/internal/gitaly/service/repository/optimize.go
+++ b/internal/gitaly/service/repository/optimize.go
@@ -3,7 +3,6 @@ package repository
 import (
 	"context"
 
-	"gitlab.com/gitlab-org/gitaly/v14/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
@@ -15,7 +14,7 @@ func (s *server) OptimizeRepository(ctx context.Context, in *gitalypb.OptimizeRe
 
 	repo := s.localrepo(in.GetRepository())
 
-	if err := housekeeping.OptimizeRepository(ctx, repo, s.txManager); err != nil {
+	if err := s.housekeepingManager.OptimizeRepository(ctx, repo); err != nil {
 		return nil, helper.ErrInternal(err)
 	}