diff options
author | John Cai <jcai@gitlab.com> | 2022-08-24 20:59:12 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2022-09-02 23:15:57 +0300 |
commit | f0973cd2c9201fe867fb6ecacfb1910dac418c3d (patch) | |
tree | 5da8dae6426ebd8872090d8fc0df16823fc972ab | |
parent | c9fe65d7c76cc69abc2ed3eeede198bfc614906e (diff) |
repository: Add log only for new repository calculation methods
The RepositorySize is a high risk API, since an inaccurate number
(especially if it's much higher) can cause projects to be locked due to
usage quotas. In order to safely roll out a new calculation, add a log
only mode such that a feature flag has to be enabled in order to send
the value back to the Rails client. Without this feature flag, we merely
log the new calculation and continue to send the value calculated with
du.
-rw-r--r-- | internal/gitaly/service/repository/size.go | 32 | ||||
-rw-r--r-- | internal/gitaly/service/repository/size_test.go | 65 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go | 10 |
3 files changed, 82 insertions, 25 deletions
diff --git a/internal/gitaly/service/repository/size.go b/internal/gitaly/service/repository/size.go index 6b3002c15..9f19b5372 100644 --- a/internal/gitaly/service/repository/size.go +++ b/internal/gitaly/service/repository/size.go @@ -8,6 +8,7 @@ import ( "strconv" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" + "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/v15/internal/command" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" @@ -23,28 +24,32 @@ import ( func (s *server) RepositorySize(ctx context.Context, in *gitalypb.RepositorySizeRequest) (*gitalypb.RepositorySizeResponse, error) { repo := s.localrepo(in.GetRepository()) - var size int64 + var newSize int64 var err error + var logger *logrus.Entry path, err := repo.Path() if err != nil { return nil, err } - duSize := getPathSize(ctx, path) + size := getPathSize(ctx, path) + + logger = ctxlogrus.Extract(ctx).WithField("repo_size_du", size) if featureflag.RevlistForRepoSize.IsEnabled(ctx) { - size, err = calculateSizeWithRevlist(ctx, repo) + newSize, err = calculateSizeWithRevlist(ctx, repo) if err != nil { return nil, fmt.Errorf("calculating repository size with git-rev-list: %w,", err) } - ctxlogrus.Extract(ctx). - WithField("repo_size_revlist", size). - WithField("repo_size_du", duSize). - Info("repository size calculated") + logger.WithField("repo_size_revlist", newSize).Info("repository size calculated") + + if featureflag.UseNewRepoSize.IsEnabled(ctx) { + size = newSize + } } else if featureflag.CatfileRepoSize.IsEnabled(ctx) { - size, err = calculateSizeWithCatfile( + newSize, err = calculateSizeWithCatfile( ctx, repo, s.locator, @@ -57,12 +62,11 @@ func (s *server) RepositorySize(ctx context.Context, in *gitalypb.RepositorySize return nil, fmt.Errorf("calculating repository size with git-cat-file: %w", err) } - ctxlogrus.Extract(ctx). - WithField("repo_size_catfile", size). - WithField("repo_size_du", duSize). - Info("repository size calculated") - } else { - size = duSize + logger.WithField("repo_size_catfile", newSize).Info("repository size calculated") + + if featureflag.UseNewRepoSize.IsEnabled(ctx) { + size = newSize + } } return &gitalypb.RepositorySizeResponse{Size: size}, nil diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go index 98f129bea..521ce9de0 100644 --- a/internal/gitaly/service/repository/size_test.go +++ b/internal/gitaly/service/repository/size_test.go @@ -7,6 +7,8 @@ import ( "context" "testing" + "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" @@ -18,6 +20,7 @@ import ( "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/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/protobuf/proto" @@ -33,6 +36,7 @@ func TestRepositorySize_SuccessfulRequest(t *testing.T) { featureSet := testhelper.NewFeatureSets( featureflag.RevlistForRepoSize, featureflag.CatfileRepoSize, + featureflag.UseNewRepoSize, ) featureSet.Run(t, testSuccessfulRepositorySizeRequest) @@ -97,8 +101,9 @@ func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Con response, err = repoClient.RepositorySize(ctx, sizeRequest) require.NoError(t, err) - if featureflag.RevlistForRepoSize.IsEnabled(ctx) || - featureflag.CatfileRepoSize.IsEnabled(ctx) { + if featureflag.UseNewRepoSize.IsEnabled(ctx) && + (featureflag.RevlistForRepoSize.IsEnabled(ctx) || + featureflag.CatfileRepoSize.IsEnabled(ctx)) { assert.Equal(t, int64(0), response.GetSize()) } else { assert.Less(t, response.GetSize(), sizeBeforePool) @@ -106,7 +111,8 @@ func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Con } func testSuccessfulRepositorySizeRequest(t *testing.T, ctx context.Context) { - cfg, repo, repoPath, client := setupRepositoryService(ctx, t) + logger, hook := test.NewNullLogger() + cfg, repo, repoPath, client := setupRepositoryService(ctx, t, testserver.WithLogger(logger)) request := &gitalypb.RepositorySizeRequest{Repository: repo} response, err := client.RepositorySize(ctx, request) @@ -136,14 +142,51 @@ func testSuccessfulRepositorySizeRequest(t *testing.T, ctx context.Context) { responseAfterRefs, err := client.RepositorySize(ctx, request) require.NoError(t, err) - if featureflag.RevlistForRepoSize.IsEnabled(ctx) { - assert.Equal( - t, - response.Size, - responseAfterRefs.Size, - "excluded refs do not contribute to the repository size", - ) - } else { + // We expect two log entries with "repository size calculated" + // since we called RepositorySize twice, but we are really interested + // in the results of the second log. + var entries []*logrus.Entry + switch { + case featureflag.RevlistForRepoSize.IsEnabled(ctx): + for _, entry := range hook.AllEntries() { + _, ok := entry.Data["repo_size_revlist"] + if ok { + entries = append(entries, entry) + } + } + + require.Len(t, entries, 2) + sizeInLog, ok := entries[1].Data["repo_size_revlist"] + require.True(t, ok) + require.Equal(t, "repository size calculated", entries[1].Message) + + if featureflag.UseNewRepoSize.IsEnabled(ctx) { + assert.Equal( + t, + response.Size, + responseAfterRefs.Size, + "excluded refs do not contribute to the repository size", + ) + require.Equal(t, responseAfterRefs.Size, sizeInLog) + } + case featureflag.CatfileRepoSize.IsEnabled(ctx): + for _, entry := range hook.AllEntries() { + _, ok := entry.Data["repo_size_catfile"] + if ok { + entries = append(entries, entry) + } + } + + require.Len(t, entries, 2) + sizeInLog, ok := entries[1].Data["repo_size_catfile"] + require.True(t, ok) + require.Equal(t, "repository size calculated", entries[1].Message) + + if featureflag.UseNewRepoSize.IsEnabled(ctx) { + require.Less(t, response.Size, sizeInLog) + assert.Less(t, response.Size, responseAfterRefs.Size) + } + default: assert.Less(t, response.Size, responseAfterRefs.Size) } } diff --git a/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go b/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go new file mode 100644 index 000000000..960e213c1 --- /dev/null +++ b/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go @@ -0,0 +1,10 @@ +package featureflag + +// UseNewRepoSize will send the new repository size values in RepositorySize to +// the client. +var UseNewRepoSize = NewFeatureFlag( + "use_new_repository_size", + "v15.4.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4448", + false, +) |