diff options
| -rw-r--r-- | internal/gitaly/service/repository/optimize.go | 8 | ||||
| -rw-r--r-- | internal/gitaly/service/repository/optimize_test.go | 98 |
2 files changed, 106 insertions, 0 deletions
diff --git a/internal/gitaly/service/repository/optimize.go b/internal/gitaly/service/repository/optimize.go index b13566036..f80506e2e 100644 --- a/internal/gitaly/service/repository/optimize.go +++ b/internal/gitaly/service/repository/optimize.go @@ -341,6 +341,14 @@ func pruneIfNeeded(ctx context.Context, repo *localrepo.Repo) (bool, error) { if err := repo.ExecAndWait(ctx, git.SubCmd{ Name: "prune", + Flags: []git.Option{ + // By default, this prunes all unreachable objects regardless of when they + // have last been accessed. This opens us up for races when there are + // concurrent commands which are just at the point of writing objects into + // the repository, but which haven't yet updated any references to make them + // reachable. We thus use the same two-week grace period as git-gc(1) does. + git.ValueFlag{Name: "--expire", Value: "two.weeks.ago"}, + }, }); err != nil { return false, fmt.Errorf("pruning objects: %w", err) } diff --git a/internal/gitaly/service/repository/optimize_test.go b/internal/gitaly/service/repository/optimize_test.go index f7e4152e0..916aa433f 100644 --- a/internal/gitaly/service/repository/optimize_test.go +++ b/internal/gitaly/service/repository/optimize_test.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "strings" "testing" "time" @@ -15,6 +16,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v14/internal/git/stats" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "google.golang.org/grpc/codes" ) @@ -743,3 +745,99 @@ func TestEstimateLooseObjectCount(t *testing.T) { require.Equal(t, int64(512), looseObjects) }) } + +func TestPruneIfNeededWithRealObjects(t *testing.T) { + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + + t.Run("empty repo does not prune", func(t *testing.T) { + repoProto, _ := gittest.InitRepo(t, cfg, cfg.Storages[0]) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + didPrune, err := pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.False(t, didPrune) + }) + + t.Run("repo with single object does not prune", func(t *testing.T) { + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + gittest.WriteBlob(t, cfg, repoPath, []byte("something")) + + didPrune, err := pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.False(t, didPrune) + }) + + t.Run("repo with single 17-prefixed objects does not prune", func(t *testing.T) { + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + blobID := gittest.WriteBlob(t, cfg, repoPath, []byte("32")) + require.True(t, strings.HasPrefix(blobID.String(), "17")) + + didPrune, err := pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.False(t, didPrune) + }) + + t.Run("repo with four 17-prefixed objects does not prune", func(t *testing.T) { + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + for _, contents := range []string{"32", "119", "334", "782"} { + blobID := gittest.WriteBlob(t, cfg, repoPath, []byte(contents)) + require.True(t, strings.HasPrefix(blobID.String(), "17")) + } + + didPrune, err := pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.False(t, didPrune) + }) + + t.Run("repo with five 17-prefixed objects does prune", func(t *testing.T) { + repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + objectPath := func(oid git.ObjectID) string { + return filepath.Join(repoPath, "objects", oid.String()[0:2], oid.String()[2:]) + } + + // Contents with 17-prefix were brute forced with git-hash-object(1). + var blobs []git.ObjectID + for _, contents := range []string{"32", "119", "334", "782", "907"} { + blobID := gittest.WriteBlob(t, cfg, repoPath, []byte(contents)) + require.True(t, strings.HasPrefix(blobID.String(), "17")) + blobs = append(blobs, blobID) + } + + didPrune, err := pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.True(t, didPrune) + + // We shouldn't prune any objects which aren't older than the grace period of two + // weeks. + for _, blob := range blobs { + require.FileExists(t, objectPath(blob)) + } + + // Now we modify the object's times to be older than two weeks. + twoWeeksAgo := time.Now().Add(-1 * 2 * 7 * 24 * time.Hour) + for _, blob := range blobs { + require.NoError(t, os.Chtimes(objectPath(blob), twoWeeksAgo, twoWeeksAgo)) + } + + // Because we didn't prune objects before due to the grace period, the still exist + // and thus we would still want to prune here. + didPrune, err = pruneIfNeeded(ctx, repo) + require.NoError(t, err) + require.True(t, didPrune) + + // But this time the objects shouldn't exist anymore because they were older than + // the grace period. + for _, blob := range blobs { + require.NoFileExists(t, objectPath(blob)) + } + }) +} |