diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2018-12-06 14:42:13 +0300 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2018-12-06 14:42:13 +0300 |
commit | 1b6e456a8a282d4c6b963c1c92a69ed4eae098f3 (patch) | |
tree | 9496234320721c0ff8a1ff35bca350b7080b19fd | |
parent | 3265234b0d281942d8a02fa5654d76c360ace192 (diff) | |
parent | 3509e2e579552958a4c41cfc3116c940e62b4478 (diff) |
Merge branch 'bvl-cleanup-invalid-keeparounds' into 'master'
Clean up invalid keep-around refs when performing housekeeping
See merge request gitlab-org/gitaly!992
-rw-r--r-- | changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml | 5 | ||||
-rw-r--r-- | internal/service/repository/gc.go | 87 | ||||
-rw-r--r-- | internal/service/repository/gc_test.go | 93 |
3 files changed, 185 insertions, 0 deletions
diff --git a/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml b/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml new file mode 100644 index 000000000..d484b536a --- /dev/null +++ b/changelogs/unreleased/bvl-cleanup-invalid-keeparounds.yml @@ -0,0 +1,5 @@ +--- +title: Clean up invalid keep-around refs when performing housekeeping +merge_request: 992 +author: +type: other diff --git a/internal/service/repository/gc.go b/internal/service/repository/gc.go index d1da2bc48..cd06bce0e 100644 --- a/internal/service/repository/gc.go +++ b/internal/service/repository/gc.go @@ -1,10 +1,17 @@ package repository import ( + "errors" + "fmt" + "io/ioutil" + "os" + "path/filepath" + grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/housekeeping" "golang.org/x/net/context" @@ -27,6 +34,10 @@ func (server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectReq return nil, err } + if err := cleanupKeepArounds(ctx, in.GetRepository()); err != nil { + return nil, err + } + args := []string{"-c"} if in.GetCreateBitmap() { args = append(args, "repack.writeBitmaps=true") @@ -54,3 +65,79 @@ func (server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollectReq return &gitalypb.GarbageCollectResponse{}, nil } + +func cleanupKeepArounds(ctx context.Context, repo *gitalypb.Repository) error { + repoPath, err := helper.GetRepoPath(repo) + if err != nil { + return nil + } + + batch, err := catfile.New(ctx, repo) + if err != nil { + return nil + } + + keepAroundsPrefix := "refs/keep-around" + keepAroundsPath := filepath.Join(repoPath, keepAroundsPrefix) + + refInfos, err := ioutil.ReadDir(keepAroundsPath) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return err + } + + for _, info := range refInfos { + if info.IsDir() { + continue + } + + refName := fmt.Sprintf("%s/%s", keepAroundsPrefix, info.Name()) + path := filepath.Join(repoPath, keepAroundsPrefix, info.Name()) + + if err = checkRef(batch, refName, info); err == nil { + continue + } + + if err := fixRef(ctx, repo, batch, path, refName, info.Name()); err != nil { + return err + } + } + + return nil +} + +func checkRef(batch *catfile.Batch, refName string, info os.FileInfo) error { + if info.Size() == 0 { + return errors.New("checkRef: Ref file is empty") + } + + _, err := batch.Info(refName) + return err +} + +func fixRef(ctx context.Context, repo *gitalypb.Repository, batch *catfile.Batch, refPath string, name string, sha string) error { + // So the ref is broken, let's get rid of it + if err := os.RemoveAll(refPath); err != nil { + return err + } + + // If the sha is not in the the repository, we can't fix it + if _, err := batch.Info(sha); err != nil { + return nil + } + + // The name is a valid sha, recreate the ref + updateRefArgs := []string{"update-ref", name, sha} + cmd, err := git.Command(ctx, repo, updateRefArgs...) + if err != nil { + return err + } + + if err = cmd.Wait(); err != nil { + return err + } + + return nil +} diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go index d718c3229..651a5dfea 100644 --- a/internal/service/repository/gc_test.go +++ b/internal/service/repository/gc_test.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "strings" "testing" "time" @@ -160,6 +161,98 @@ func TestGarbageCollectFailure(t *testing.T) { } +func TestCleanupInvalidKeepAroundRefs(t *testing.T) { + server, serverSocketPath := runRepoServer(t) + defer server.Stop() + + client, conn := newRepositoryClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + // Make the directory, so we can create random reflike things in it + require.NoError(t, os.MkdirAll(filepath.Join(testRepoPath, "refs", "keep-around"), 0755)) + + testCases := []struct { + desc string + refName string + refContent string + shouldExist bool + }{ + { + desc: "A valid ref", + refName: "0b4bc9a49b562e85de7cc9e834518ea6828729b9", + refContent: "0b4bc9a49b562e85de7cc9e834518ea6828729b9", + shouldExist: true, + }, + { + desc: "A ref that does not exist", + refName: "bogus", + refContent: "bogus", + shouldExist: false, + }, { + desc: "Filled with the blank ref", + refName: "0b4bc9a49b562e85de7cc9e834518ea6828729b9", + refContent: "0000000000000000000000000000000000000000", + shouldExist: true, + }, { + desc: "An existing ref with blank content", + refName: "0b4bc9a49b562e85de7cc9e834518ea6828729b9", + refContent: "", + shouldExist: true, + }, { + desc: "A valid sha that does not exist in the repo", + refName: "d669a6f1a70693058cf484318c1cee8526119938", + refContent: "d669a6f1a70693058cf484318c1cee8526119938", + shouldExist: false, + }, + } + + for _, testcase := range testCases { + t.Run(testcase.desc, func(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + // Create a proper keep-around loose ref + existingSha := "1e292f8fedd741b75372e19097c76d327140c312" + existingRefName := fmt.Sprintf("refs/keep-around/%s", existingSha) + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "update-ref", existingRefName, existingSha) + + // Create an invalid ref that should should be removed with the testcase + bogusSha := "b3f5e4adf6277b571b7943a4f0405a6dd7ee7e15" + bogusPath := filepath.Join(testRepoPath, fmt.Sprintf("refs/keep-around/%s", bogusSha)) + require.NoError(t, ioutil.WriteFile(bogusPath, []byte(bogusSha), 0644)) + + // Creating the keeparound without using git so we can create invalid ones in testcases + refPath := filepath.Join(testRepoPath, fmt.Sprintf("refs/keep-around/%s", testcase.refName)) + require.NoError(t, ioutil.WriteFile(refPath, []byte(testcase.refContent), 0644)) + + // Perform the request + req := &gitalypb.GarbageCollectRequest{Repository: testRepo} + _, err := client.GarbageCollect(ctx, req) + require.NoError(t, err) + + // The existing keeparound still exists + commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", existingRefName) + require.Equal(t, existingSha, strings.TrimSpace(string(commitSha))) + + //The invalid one was removed + _, err = os.Stat(bogusPath) + require.True(t, os.IsNotExist(err), "expected 'does not exist' error, got %v", err) + + if testcase.shouldExist { + keepAroundName := fmt.Sprintf("refs/keep-around/%s", testcase.refName) + commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", keepAroundName) + require.Equal(t, testcase.refName, strings.TrimSpace(string(commitSha))) + } else { + _, err := os.Stat(refPath) + require.True(t, os.IsNotExist(err), "expected 'does not exist' error, got %v", err) + } + }) + } +} + func createFileWithTimes(path string, mTime time.Time) { ioutil.WriteFile(path, nil, 0644) os.Chtimes(path, mTime, mTime) |