Merge branch 'pks-repository-size-remove-disk-usage-implementation' into 'master'

repository: Always use walk-based implementation for RepositorySize

Closes #4976

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5569

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>

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,
-)