tempdir: Inject properly configured logger

The tempdir logic uses the global logrus logger to log data, which is
discouraged. Convert the code to instead inject a logger so that we can
use a properly configured one.

3 files changed, 16 insertions, 20 deletions

diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index 24c002f23..24c3b2089 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -233,7 +233,7 @@ func run(cfg config.Cfg, logger logrus.FieldLogger) error {
 	prometheus.MustRegister(repoCounter)
 	repoCounter.StartCountingRepositories(ctx, locator, logger)
 
-	tempdir.StartCleaning(locator, cfg.Storages, time.Hour)
+	tempdir.StartCleaning(logger, locator, cfg.Storages, time.Hour)
 
 	prometheus.MustRegister(gitCmdFactory)
 
diff --git a/internal/tempdir/clean.go b/internal/tempdir/clean.go
index f2efa2c87..548dab199 100644
--- a/internal/tempdir/clean.go
+++ b/internal/tempdir/clean.go
@@ -28,21 +28,21 @@ const (
 )
 
 // StartCleaning starts tempdir cleanup in a goroutine.
-func StartCleaning(locator storage.Locator, storages []config.Storage, d time.Duration) {
+func StartCleaning(logger logrus.FieldLogger, locator storage.Locator, storages []config.Storage, d time.Duration) {
 	dontpanic.Go(func() {
 		for {
-			cleanTempDir(locator, storages)
+			cleanTempDir(logger, locator, storages)
 			time.Sleep(d)
 		}
 	})
 }
 
-func cleanTempDir(locator storage.Locator, storages []config.Storage) {
+func cleanTempDir(logger logrus.FieldLogger, locator storage.Locator, storages []config.Storage) {
 	for _, storage := range storages {
 		start := time.Now()
-		err := clean(locator, storage)
+		err := clean(logger, locator, storage)
 
-		entry := logrus.WithFields(logrus.Fields{
+		entry := logger.WithFields(logrus.Fields{
 			"time_ms": time.Since(start).Milliseconds(),
 			"storage": storage.Name,
 		})
@@ -55,7 +55,7 @@ func cleanTempDir(locator storage.Locator, storages []config.Storage) {
 
 type invalidCleanRoot string
 
-func clean(locator storage.Locator, storage config.Storage) error {
+func clean(logger logrus.FieldLogger, locator storage.Locator, storage config.Storage) error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -67,7 +67,7 @@ func clean(locator storage.Locator, storage config.Storage) error {
 	// If we start "cleaning up" the wrong directory we may delete user data
 	// which is Really Bad.
 	if !strings.HasSuffix(dir, tmpRootPrefix) {
-		logrus.Print(dir)
+		logger.Print(dir)
 		panic(invalidCleanRoot("invalid tempdir clean root: panicking to prevent data loss"))
 	}
 
diff --git a/internal/tempdir/clean_test.go b/internal/tempdir/clean_test.go
index e6975f554..1bd810a60 100644
--- a/internal/tempdir/clean_test.go
+++ b/internal/tempdir/clean_test.go
@@ -1,13 +1,11 @@
 package tempdir
 
 import (
-	"io"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
-	"github.com/sirupsen/logrus"
 	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
@@ -49,11 +47,13 @@ func TestCleanSuccess(t *testing.T) {
 
 	assertEntries(t, locator, cfg.Storages[0], "a", "c", "e", "f")
 
-	require.NoError(t, clean(locator, cfg.Storages[0]), "walk first pass")
+	require.NoError(t, clean(testhelper.NewDiscardingLogEntry(t), locator, cfg.Storages[0]), "walk first pass")
 	assertEntries(t, locator, cfg.Storages[0], "c", "e")
 }
 
 func TestCleanTempDir(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t, testcfg.WithStorages("first", "second"))
 	locator := config.NewLocator(cfg)
@@ -64,12 +64,8 @@ func TestCleanTempDir(t *testing.T) {
 
 	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
 
-	logrus.SetLevel(logrus.InfoLevel)
-	logrus.SetOutput(io.Discard)
-
-	hook := test.NewGlobal()
-
-	cleanTempDir(locator, cfg.Storages)
+	logger, hook := test.NewNullLogger()
+	cleanTempDir(logger, locator, cfg.Storages)
 
 	require.Equal(t, 2, len(hook.Entries), hook.Entries)
 	require.Equal(t, "finished tempdir cleaner walk", hook.LastEntry().Message)
@@ -79,14 +75,14 @@ func TestCleanNoTmpExists(t *testing.T) {
 	cfg := testcfg.Build(t)
 	locator := config.NewLocator(cfg)
 
-	require.NoError(t, clean(locator, cfg.Storages[0]))
+	require.NoError(t, clean(testhelper.NewDiscardingLogEntry(t), locator, cfg.Storages[0]))
 }
 
 func TestCleanNoStorageExists(t *testing.T) {
 	cfg := testcfg.Build(t, testcfg.WithStorages("first"))
 	locator := config.NewLocator(cfg)
 
-	err := clean(locator, config.Storage{Name: "does-not-exist", Path: "/something"})
+	err := clean(testhelper.NewDiscardingLogEntry(t), locator, config.Storage{Name: "does-not-exist", Path: "/something"})
 	require.EqualError(t, err, "temporary dir: tmp dir: no such storage: \"does-not-exist\"")
 }
 
@@ -108,7 +104,7 @@ func TestCleanerSafety(t *testing.T) {
 	}()
 
 	// We need to set up a mock locator which returns an invalid temporary directory path.
-	require.NoError(t, clean(mockLocator{}, config.Storage{}))
+	require.NoError(t, clean(testhelper.NewDiscardingLogEntry(t), mockLocator{}, config.Storage{}))
 
 	t.Fatal("expected panic")
 }