repository: Always use walk-based implementation for RepositorySize

In ac600d02a (repository: Refactor RepositorySize to not rely on du(1),
2023-03-16), we have refactored the RepositorySize RPC to stop shelling
out to du(1) in favor of a new `filepath.Walk()`-based implementation.
We have since rolled out this change to production and fixed an initial
race condition where the new implementation failed when directories are
deleted concurrently.

Since that bug has been fixed things are going smooth with the new
implementation. As expected, the median and P50 dropped given that the
new implementation avoids the overhead of having to spawn a separate
process. On the other hand, P99 increased slightly as expected as the
implementation of du(1) is more efficient. In practice, this tradeoff is
acceptable and even beneficial as the new code both avoids resource
contention around the command spawn queue and is simpler to extend.

Remove the feature flag and unconditionally enable the new
implementation.

Changelog: changed

3 files changed, 36 insertions, 137 deletions

diff --git a/internal/gitaly/service/repository/size.go b/internal/gitaly/service/repository/size.go
index 195738e10..461312a50 100644
--- a/internal/gitaly/service/repository/size.go
+++ b/internal/gitaly/service/repository/size.go
@@ -1,20 +1,14 @@
 package repository
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
-	"io"
 	"io/fs"
 	"os"
 	"path/filepath"
-	"strconv"
 
-	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -33,19 +27,12 @@ func (s *server) RepositorySize(ctx context.Context, in *gitalypb.RepositorySize
 		return nil, err
 	}
 
-	var sizeKiB int64
-	if featureflag.RepositorySizeViaWalk.IsEnabled(ctx) {
-		sizeBytes, err := dirSizeInBytes(path)
-		if err != nil {
-			return nil, fmt.Errorf("calculating directory size: %w", err)
-		}
-
-		sizeKiB = sizeBytes / 1024
-	} else {
-		sizeKiB = getPathSize(ctx, path)
+	sizeInBytes, err := dirSizeInBytes(path)
+	if err != nil {
+		return nil, fmt.Errorf("calculating directory size: %w", err)
 	}
 
-	return &gitalypb.RepositorySizeResponse{Size: sizeKiB}, nil
+	return &gitalypb.RepositorySizeResponse{Size: sizeInBytes / 1024}, nil
 }
 
 func (s *server) GetObjectDirectorySize(ctx context.Context, in *gitalypb.GetObjectDirectorySizeRequest) (*gitalypb.GetObjectDirectorySizeResponse, error) {
@@ -60,52 +47,12 @@ func (s *server) GetObjectDirectorySize(ctx context.Context, in *gitalypb.GetObj
 		return nil, err
 	}
 
-	var sizeKiB int64
-	if featureflag.RepositorySizeViaWalk.IsEnabled(ctx) {
-		sizeBytes, err := dirSizeInBytes(path)
-		if err != nil {
-			return nil, fmt.Errorf("calculating directory size: %w", err)
-		}
-
-		sizeKiB = sizeBytes / 1024
-	} else {
-		sizeKiB = getPathSize(ctx, path)
-	}
-
-	return &gitalypb.GetObjectDirectorySizeResponse{Size: sizeKiB}, nil
-}
-
-func getPathSize(ctx context.Context, path string) int64 {
-	cmd, err := command.New(ctx, []string{"du", "-sk", path})
-	if err != nil {
-		ctxlogrus.Extract(ctx).WithError(err).Warn("ignoring du command error")
-		return 0
-	}
-
-	sizeLine, err := io.ReadAll(cmd)
-	if err != nil {
-		ctxlogrus.Extract(ctx).WithError(err).Warn("ignoring command read error")
-		return 0
-	}
-
-	if err := cmd.Wait(); err != nil {
-		ctxlogrus.Extract(ctx).WithError(err).Warn("ignoring du wait error")
-		return 0
-	}
-
-	sizeParts := bytes.Split(sizeLine, []byte("\t"))
-	if len(sizeParts) != 2 {
-		ctxlogrus.Extract(ctx).Warn(fmt.Sprintf("ignoring du malformed output: %q", sizeLine))
-		return 0
-	}
-
-	size, err := strconv.ParseInt(string(sizeParts[0]), 10, 0)
+	sizeInBytes, err := dirSizeInBytes(path)
 	if err != nil {
-		ctxlogrus.Extract(ctx).WithError(err).Warn("ignoring parsing size error")
-		return 0
+		return nil, fmt.Errorf("calculating directory size: %w", err)
 	}
 
-	return size
+	return &gitalypb.GetObjectDirectorySizeResponse{Size: sizeInBytes / 1024}, nil
 }
 
 func dirSizeInBytes(path string) (int64, error) {
diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go
index 99f87a3c6..126a2ee3c 100644
--- a/internal/gitaly/service/repository/size_test.go
+++ b/internal/gitaly/service/repository/size_test.go
@@ -3,7 +3,6 @@
 package repository
 
 import (
-	"context"
 	"math/rand"
 	"testing"
 
@@ -14,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
-	"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/proto/go/gitalypb"
@@ -27,18 +25,10 @@ import (
 // repository, even in optimally packed state, is greater than this.
 const testRepoMinSizeKB = 10000
 
-func TestRepositorySize_SuccessfulRequest(t *testing.T) {
-	t.Parallel()
-
-	featureSet := testhelper.NewFeatureSets(featureflag.RepositorySizeViaWalk)
-
-	featureSet.Run(t, testSuccessfulRepositorySizeRequest)
-	featureSet.Run(t, testSuccessfulRepositorySizeRequestPoolMember)
-}
-
-func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Context) {
+func TestSuccessfulRepositorySizeRequestPoolMember(t *testing.T) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	repoClient, serverSocketPath := runRepositoryService(t, cfg, nil)
 	cfg.SocketPath = serverSocketPath
@@ -89,9 +79,10 @@ func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Con
 	assert.Less(t, response.GetSize(), sizeBeforePool)
 }
 
-func testSuccessfulRepositorySizeRequest(t *testing.T, ctx context.Context) {
+func TestSuccessfulRepositorySizeRequest(t *testing.T) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repo, repoPath, client := setupRepositoryService(t, ctx)
 
 	request := &gitalypb.RepositorySizeRequest{Repository: repo}
@@ -156,65 +147,40 @@ func TestFailedRepositorySizeRequest(t *testing.T) {
 }
 
 func BenchmarkRepositorySize(b *testing.B) {
+	ctx := testhelper.Context(b)
 	cfg, client := setupRepositoryServiceWithoutRepo(b)
 
-	for _, implementationTC := range []struct {
-		desc         string
-		setupContext func(b *testing.B) context.Context
+	for _, tc := range []struct {
+		desc  string
+		setup func(b *testing.B) *gitalypb.Repository
 	}{
 		{
-			desc: "disk-usage with du",
-			setupContext: func(b *testing.B) context.Context {
-				ctx := testhelper.Context(b)
-				ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RepositorySizeViaWalk, false)
-				return ctx
+			desc: "empty repository",
+			setup: func(b *testing.B) *gitalypb.Repository {
+				repo, _ := gittest.CreateRepository(b, ctx, cfg)
+				return repo
 			},
 		},
 		{
-			desc: "disk-usage with walk",
-			setupContext: func(b *testing.B) context.Context {
-				ctx := testhelper.Context(b)
-				ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RepositorySizeViaWalk, true)
-				return ctx
+			desc: "benchmark repository",
+			setup: func(b *testing.B) *gitalypb.Repository {
+				repo, _ := gittest.CreateRepository(b, ctx, cfg, gittest.CreateRepositoryConfig{
+					Seed: "benchmark.git",
+				})
+				return repo
 			},
 		},
 	} {
-		b.Run(implementationTC.desc, func(b *testing.B) {
-			ctx := implementationTC.setupContext(b)
-
-			for _, tc := range []struct {
-				desc  string
-				setup func(b *testing.B) *gitalypb.Repository
-			}{
-				{
-					desc: "empty repository",
-					setup: func(b *testing.B) *gitalypb.Repository {
-						repo, _ := gittest.CreateRepository(b, ctx, cfg)
-						return repo
-					},
-				},
-				{
-					desc: "benchmark repository",
-					setup: func(b *testing.B) *gitalypb.Repository {
-						repo, _ := gittest.CreateRepository(b, ctx, cfg, gittest.CreateRepositoryConfig{
-							Seed: "benchmark.git",
-						})
-						return repo
-					},
-				},
-			} {
-				b.Run(tc.desc, func(b *testing.B) {
-					repo := tc.setup(b)
-
-					b.StartTimer()
-
-					for i := 0; i < b.N; i++ {
-						_, err := client.RepositorySize(ctx, &gitalypb.RepositorySizeRequest{
-							Repository: repo,
-						})
-						require.NoError(b, err)
-					}
+		b.Run(tc.desc, func(b *testing.B) {
+			repo := tc.setup(b)
+
+			b.StartTimer()
+
+			for i := 0; i < b.N; i++ {
+				_, err := client.RepositorySize(ctx, &gitalypb.RepositorySizeRequest{
+					Repository: repo,
 				})
+				require.NoError(b, err)
 			}
 		})
 	}
@@ -222,10 +188,8 @@ func BenchmarkRepositorySize(b *testing.B) {
 
 func TestRepositorySize_SuccessfulGetObjectDirectorySizeRequest(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RepositorySizeViaWalk).Run(t, testSuccessfulGetObjectDirectorySizeRequest)
-}
 
-func testSuccessfulGetObjectDirectorySizeRequest(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	_, repo, _, client := setupRepositoryService(t, ctx)
 	repo.GitObjectDirectory = "objects/"
 
@@ -241,10 +205,8 @@ func testSuccessfulGetObjectDirectorySizeRequest(t *testing.T, ctx context.Conte
 
 func TestRepositorySize_GetObjectDirectorySize_quarantine(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.RepositorySizeViaWalk).Run(t, testGetObjectDirectorySizeQuarantine)
-}
 
-func testGetObjectDirectorySizeQuarantine(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, client := setupRepositoryServiceWithoutRepo(t)
 	locator := config.NewLocator(cfg)
 
diff --git a/internal/metadata/featureflag/ff_repository_size_via_walk.go b/internal/metadata/featureflag/ff_repository_size_via_walk.go
deleted file mode 100644
index cf10022a4..000000000
--- a/internal/metadata/featureflag/ff_repository_size_via_walk.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// RepositorySizeViaWalk starts to compute the repository size via `filepath.WalkDir()` instead of
-// using du(1).
-var RepositorySizeViaWalk = NewFeatureFlag(
-	"repository_size_via_walk",
-	"15.10.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4976",
-	false,
-)