cache/diskcache: Inject logger instead of using global one

We're using the global logging instance provided by the logrus package,
which is discouraged. Inject a logger such that we can use a properly
configured logger instead.

9 files changed, 32 insertions, 30 deletions

diff --git a/internal/cache/diskcache.go b/internal/cache/diskcache.go
index 60c3d59fd..84e75f49f 100644
--- a/internal/cache/diskcache.go
+++ b/internal/cache/diskcache.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/dontpanic"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
@@ -75,6 +76,7 @@ func withDisabledWalker() Option {
 
 // DiskCache stores and retrieves byte streams for repository related RPCs
 type DiskCache struct {
+	logger      logrus.FieldLogger
 	locator     storage.Locator
 	storages    []config.Storage
 	keyer       leaseKeyer
@@ -98,13 +100,14 @@ type DiskCache struct {
 }
 
 // New will create a new DiskCache with the given Keyer.
-func New(cfg config.Cfg, locator storage.Locator, opts ...Option) *DiskCache {
+func New(cfg config.Cfg, locator storage.Locator, logger logrus.FieldLogger, opts ...Option) *DiskCache {
 	var cacheConfig cacheConfig
 	for _, opt := range opts {
 		opt(&cacheConfig)
 	}
 
 	cache := &DiskCache{
+		logger:   logger,
 		locator:  locator,
 		storages: cfg.Storages,
 		af: activeFiles{
diff --git a/internal/cache/diskcache_test.go b/internal/cache/diskcache_test.go
index 2db13ecde..3c97d9eb6 100644
--- a/internal/cache/diskcache_test.go
+++ b/internal/cache/diskcache_test.go
@@ -58,6 +58,7 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	})
 	gittest.WriteCommit(t, cfg, repoPath2, gittest.WithMessage("two"), gittest.WithBranch("master"))
 
+	logger := testhelper.NewDiscardingLogEntry(t)
 	locator := config.NewLocator(cfg)
 
 	req1 := &gitalypb.InfoRefsRequest{
@@ -68,20 +69,20 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	}
 
 	t.Run("empty cache", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		expectGetMiss(ctx, cache, req1)
 		expectGetMiss(ctx, cache, req2)
 	})
 
 	t.Run("store and retrieve", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 		storeAndRetrieve(ctx, cache, req1, "content-1")
 		storeAndRetrieve(ctx, cache, req2, "content-2")
 	})
 
 	t.Run("invalidation", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		storeAndRetrieve(ctx, cache, req1, "content-1")
 		storeAndRetrieve(ctx, cache, req2, "content-2")
@@ -93,7 +94,7 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	})
 
 	t.Run("overwrite existing entry", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		storeAndRetrieve(ctx, cache, req1, "content-1")
 
@@ -102,7 +103,7 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	})
 
 	t.Run("feature flags affect caching", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		ctxWithFF := featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.FeatureFlag{
 			Name: "meow",
@@ -118,7 +119,7 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	})
 
 	t.Run("critical section", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		storeAndRetrieve(ctx, cache, req2, "unrelated")
 
@@ -146,7 +147,7 @@ func TestStreamDBNaiveKeyer(t *testing.T) {
 	})
 
 	t.Run("nonexisteng repository", func(t *testing.T) {
-		cache := New(cfg, locator)
+		cache := New(cfg, locator, logger)
 
 		nonexistentRepo := &gitalypb.Repository{
 			StorageName:  repo1.StorageName,
@@ -166,7 +167,7 @@ func TestLoserCount(t *testing.T) {
 	cfg := cfgBuilder.Build(t)
 
 	locator := config.NewLocator(cfg)
-	cache := New(cfg, locator)
+	cache := New(cfg, locator, testhelper.NewDiscardingLogEntry(t))
 
 	req := &gitalypb.InfoRefsRequest{
 		Repository: &gitalypb.Repository{
diff --git a/internal/cache/walker.go b/internal/cache/walker.go
index fdeca47fe..b9f15b8b4 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -13,17 +13,14 @@ import (
 	"path/filepath"
 	"time"
 
-	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/dontpanic"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 )
 
 func (c *DiskCache) logWalkErr(err error, path, msg string) {
 	c.walkerErrorTotal.Inc()
-	log.Default().
-		WithField("path", path).
+	c.logger.WithField("path", path).
 		WithError(err).
 		Warn(msg)
 }
@@ -106,7 +103,7 @@ func (c *DiskCache) cleanWalk(path string) error {
 const cleanWalkFrequency = 10 * time.Minute
 
 func (c *DiskCache) walkLoop(walkPath string) {
-	logger := logrus.WithField("path", walkPath)
+	logger := c.logger.WithField("path", walkPath)
 	logger.Infof("Starting file walker for %s", walkPath)
 
 	walkTick := time.NewTicker(cleanWalkFrequency)
@@ -149,7 +146,7 @@ func (c *DiskCache) moveAndClear(storage config.Storage) error {
 		return nil
 	}
 
-	logger := logrus.WithField("storage", storage.Name)
+	logger := c.logger.WithField("storage", storage.Name)
 	logger.Info("clearing disk cache object folder")
 
 	tempPath, err := c.locator.TempDir(storage.Name)
diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go
index c6a6fce72..7b71ca73d 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -52,7 +52,7 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 		}
 	}
 
-	cache := New(cfg, locator, withDisabledMoveAndClear())
+	cache := New(cfg, locator, testhelper.NewDiscardingLogEntry(t), withDisabledMoveAndClear())
 	require.NoError(t, cache.StartWalkers())
 	defer cache.StopWalkers()
 
@@ -81,7 +81,7 @@ func TestDiskCacheInitialClear(t *testing.T) {
 	require.NoError(t, os.MkdirAll(filepath.Dir(canary), perm.SharedDir))
 	require.NoError(t, os.WriteFile(canary, []byte("chirp chirp"), perm.PublicFile))
 
-	cache := New(cfg, locator, withDisabledWalker())
+	cache := New(cfg, locator, testhelper.NewDiscardingLogEntry(t), withDisabledWalker())
 	require.NoError(t, cache.StartWalkers())
 	defer cache.StopWalkers()
 
@@ -91,7 +91,7 @@ func TestDiskCacheInitialClear(t *testing.T) {
 func TestCleanWalkDirNotExists(t *testing.T) {
 	cfg := testcfg.Build(t)
 
-	cache := New(cfg, config.NewLocator(cfg))
+	cache := New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t))
 
 	err := cache.cleanWalk("/path/that/does/not/exist")
 	assert.NoError(t, err, "cleanWalk returned an error for a non existing directory")
@@ -124,7 +124,7 @@ func TestCleanWalkEmptyDirs(t *testing.T) {
 	}
 
 	cfg := testcfg.Build(t)
-	cache := New(cfg, config.NewLocator(cfg))
+	cache := New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t))
 
 	require.NoError(t, cache.cleanWalk(tmp))
 
diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index 6ef94e642..8c20a5a05 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -265,7 +265,7 @@ func run(cfg config.Cfg, logger logrus.FieldLogger) error {
 	defer catfileCache.Stop()
 	prometheus.MustRegister(catfileCache)
 
-	diskCache := cache.New(cfg, locator)
+	diskCache := cache.New(cfg, locator, logger)
 	prometheus.MustRegister(diskCache)
 	if err := diskCache.StartWalkers(); err != nil {
 		return fmt.Errorf("disk cache walkers: %w", err)
diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go
index 6333b5942..385a494d2 100644
--- a/internal/gitaly/server/auth_test.go
+++ b/internal/gitaly/server/auth_test.go
@@ -189,6 +189,7 @@ func newOperationClient(t *testing.T, token, serverSocketPath string) (gitalypb.
 func runServer(t *testing.T, cfg config.Cfg) string {
 	t.Helper()
 
+	logger := testhelper.NewDiscardingLogEntry(t)
 	registry := backchannel.NewRegistry()
 	conns := client.NewPool()
 	t.Cleanup(func() { conns.Close() })
@@ -200,11 +201,11 @@ func runServer(t *testing.T, cfg config.Cfg) string {
 	))
 	catfileCache := catfile.NewCache(cfg)
 	t.Cleanup(catfileCache.Stop)
-	diskCache := cache.New(cfg, locator)
+	diskCache := cache.New(cfg, locator, logger)
 	limitHandler := limithandler.New(cfg, limithandler.LimitConcurrencyByRepo, limithandler.WithConcurrencyLimiters)
 	updaterWithHooks := updateref.NewUpdaterWithHooks(cfg, locator, hookManager, gitCmdFactory, catfileCache)
 
-	srv, err := NewGitalyServerFactory(cfg, testhelper.NewDiscardingLogEntry(t), registry, diskCache, []*limithandler.LimiterMiddleware{limitHandler}).New(false)
+	srv, err := NewGitalyServerFactory(cfg, logger, registry, diskCache, []*limithandler.LimiterMiddleware{limitHandler}).New(false)
 	require.NoError(t, err)
 
 	setup.RegisterAll(srv, &service.Dependencies{
@@ -243,7 +244,7 @@ func runSecureServer(t *testing.T, cfg config.Cfg) string {
 		cfg,
 		testhelper.NewDiscardingLogEntry(t),
 		backchannel.NewRegistry(),
-		cache.New(cfg, config.NewLocator(cfg)),
+		cache.New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t)),
 		[]*limithandler.LimiterMiddleware{limithandler.New(cfg, limithandler.LimitConcurrencyByRepo, limithandler.WithConcurrencyLimiters)},
 	).New(true)
 	require.NoError(t, err)
diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go
index fba2a4613..fa77ff93e 100644
--- a/internal/gitaly/server/server_factory_test.go
+++ b/internal/gitaly/server/server_factory_test.go
@@ -83,7 +83,7 @@ func TestGitalyServerFactory(t *testing.T) {
 			cfg,
 			testhelper.NewDiscardingLogEntry(t),
 			backchannel.NewRegistry(),
-			cache.New(cfg, config.NewLocator(cfg)),
+			cache.New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t)),
 			nil,
 		)
 		t.Cleanup(sf.Stop)
@@ -103,7 +103,7 @@ func TestGitalyServerFactory(t *testing.T) {
 			cfg,
 			testhelper.NewDiscardingLogEntry(t),
 			backchannel.NewRegistry(),
-			cache.New(cfg, config.NewLocator(cfg)),
+			cache.New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t)),
 			nil,
 		)
 		t.Cleanup(sf.Stop)
@@ -117,7 +117,7 @@ func TestGitalyServerFactory(t *testing.T) {
 			cfg,
 			testhelper.NewDiscardingLogEntry(t),
 			backchannel.NewRegistry(),
-			cache.New(cfg, config.NewLocator(cfg)),
+			cache.New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t)),
 			nil,
 		)
 		t.Cleanup(sf.Stop)
@@ -147,7 +147,7 @@ func TestGitalyServerFactory(t *testing.T) {
 			cfg,
 			logger.WithContext(ctx),
 			backchannel.NewRegistry(),
-			cache.New(cfg, config.NewLocator(cfg)),
+			cache.New(cfg, config.NewLocator(cfg), logger),
 			nil,
 		)
 		t.Cleanup(sf.Stop)
@@ -182,7 +182,7 @@ func TestGitalyServerFactory_closeOrder(t *testing.T) {
 		cfg,
 		testhelper.NewDiscardingLogEntry(t),
 		backchannel.NewRegistry(),
-		cache.New(cfg, config.NewLocator(cfg)),
+		cache.New(cfg, config.NewLocator(cfg), testhelper.NewDiscardingLogEntry(t)),
 		nil,
 	)
 	defer sf.Stop()
diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go
index 18caca364..b8d8d3794 100644
--- a/internal/gitaly/service/smarthttp/inforefs_test.go
+++ b/internal/gitaly/service/smarthttp/inforefs_test.go
@@ -398,7 +398,7 @@ func TestInfoRefsUploadPack_cache(t *testing.T) {
 	cfg := testcfg.Build(t)
 
 	locator := config.NewLocator(cfg)
-	cache := cache.New(cfg, locator)
+	cache := cache.New(cfg, locator, testhelper.NewDiscardingLogEntry(t))
 
 	streamer := mockStreamer{
 		Streamer: cache,
diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go
index 51d29bcdc..5cb357878 100644
--- a/internal/testhelper/testserver/gitaly.go
+++ b/internal/testhelper/testserver/gitaly.go
@@ -318,7 +318,7 @@ func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, cfg config.Cfg) *
 	}
 
 	if gsd.diskCache == nil {
-		gsd.diskCache = cache.New(cfg, gsd.locator)
+		gsd.diskCache = cache.New(cfg, gsd.locator, gsd.logger)
 	}
 
 	if gsd.packObjectsCache == nil {