diff options
author | Paul Okstad <pokstad@gitlab.com> | 2019-04-30 00:11:44 +0300 |
---|---|---|
committer | Paul Okstad <pokstad@gitlab.com> | 2019-04-30 00:11:44 +0300 |
commit | 927ec238de1fa903cb7f9b3f2d91e837cd29d19d (patch) | |
tree | ff653dc1ade97c953cdd96fdd8350be971d1c710 | |
parent | 8287b73ccfafb785963dd4087f916c59203e3343 (diff) | |
parent | 0ab8ea8e311e80ddc9fe911815274aa9dd2733cd (diff) |
Merge branch 'has-gitalternates' into 'master'
Add DisconnectGitAlternates RPC
See merge request gitlab-org/gitaly!1141
-rw-r--r-- | changelogs/unreleased/has-gitalternates.yml | 5 | ||||
-rw-r--r-- | internal/service/objectpool/alternates.go | 241 | ||||
-rw-r--r-- | internal/service/objectpool/alternates_test.go | 148 | ||||
-rw-r--r-- | internal/service/objectpool/server.go | 7 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 2 |
5 files changed, 395 insertions, 8 deletions
diff --git a/changelogs/unreleased/has-gitalternates.yml b/changelogs/unreleased/has-gitalternates.yml new file mode 100644 index 000000000..6d94d750d --- /dev/null +++ b/changelogs/unreleased/has-gitalternates.yml @@ -0,0 +1,5 @@ +--- +title: Add DisconnectGitAlternates RPC +merge_request: 1141 +author: +type: added diff --git a/internal/service/objectpool/alternates.go b/internal/service/objectpool/alternates.go new file mode 100644 index 000000000..09d2a3453 --- /dev/null +++ b/internal/service/objectpool/alternates.go @@ -0,0 +1,241 @@ +package objectpool + +import ( + "context" + "crypto/rand" + "errors" + "fmt" + "io" + "io/ioutil" + "os" + "path/filepath" + "sort" + "strings" + "time" + + grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" + "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/helper" +) + +// DisconnectGitAlternates is a slightly dangerous RPC. It optimistically +// hard-links all alternate objects we might need, and then temporarily +// removes (renames) objects/info/alternates and runs 'git fsck'. If we +// are unlucky that leaves the repository in a broken state during 'git +// fsck'. If we are very unlucky and Gitaly crashes, the repository stays +// in a broken state until an administrator intervenes and restores the +// backed-up copy of objects/info/alternates. +func (s *server) DisconnectGitAlternates(ctx context.Context, req *gitalypb.DisconnectGitAlternatesRequest) (*gitalypb.DisconnectGitAlternatesResponse, error) { + repo := req.Repository + + if repo == nil { + return nil, helper.ErrInvalidArgument(errors.New("no repository")) + } + + if err := disconnectAlternates(ctx, repo); err != nil { + return nil, helper.ErrInternal(err) + } + + return &gitalypb.DisconnectGitAlternatesResponse{}, nil +} + +func disconnectAlternates(ctx context.Context, repo *gitalypb.Repository) error { + repoPath, err := helper.GetRepoPath(repo) + if err != nil { + return err + } + + altFile, err := git.InfoAlternatesPath(repo) + if err != nil { + return err + } + + altContents, err := ioutil.ReadFile(altFile) + if err != nil { + if os.IsNotExist(err) { + return nil + } + + return err + } + + altDir := strings.TrimSpace(string(altContents)) + if strings.Contains(altDir, "\n") { + return fmt.Errorf("found multiple lines in %s", altFile) + } + + if !filepath.IsAbs(altDir) { + altDir = filepath.Join(repoPath, "objects", altDir) + } + + backupFile, err := newBackupFile(altFile) + if err != nil { + return err + } + + stat, err := os.Stat(altDir) + if err != nil { + if os.IsNotExist(err) { + return nil + } + + return err + } + + if !stat.IsDir() { + // The existing alternates file is bogus. There can be no objects on the + // other end that the current repository relies on, so we could just + // delete objects/info/alternates. To be a little more careful, instead of + // deleting it we rename it. + return os.Rename(altFile, backupFile) + } + + objectFiles, err := findObjectFiles(altDir) + if err != nil { + return err + } + + for _, path := range objectFiles { + source := filepath.Join(altDir, path) + target := filepath.Join(repoPath, "objects", path) + + if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil { + return err + } + + if err := os.Link(source, target); err != nil { + if os.IsExist(err) { + continue + } + + return err + } + } + + return removeAlternatesIfOk(ctx, repo, altFile, backupFile) +} + +func newBackupFile(altFile string) (string, error) { + randSuffix := make([]byte, 6) + if _, err := io.ReadFull(rand.Reader, randSuffix); err != nil { + return "", err + } + + return fmt.Sprintf("%s.%d.%x", altFile, time.Now().Unix(), randSuffix), nil +} + +func findObjectFiles(altDir string) ([]string, error) { + var objectFiles []string + if walkErr := filepath.Walk(altDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + rel, err := filepath.Rel(altDir, path) + if err != nil { + return err + } + + if strings.HasPrefix(rel, "info/") { + return nil + } + + if info.IsDir() { + return nil + } + + objectFiles = append(objectFiles, rel) + + return nil + }); walkErr != nil { + return nil, walkErr + } + + sort.Sort(objectPaths(objectFiles)) + + return objectFiles, nil +} + +type fsckError struct{ error } + +func (fe *fsckError) Error() string { + return fmt.Sprintf("git fsck error while disconnected: %v", fe.error) +} + +// removeAlternatesIfOk is dangerous. We optimistically temporarily +// rename objects/info/alternates, and run `git fsck` to see if the +// resulting repo is connected. If this fails we restore +// objects/info/alternates. If the repo is not connected for whatever +// reason, then until this function returns, probably **all concurrent +// RPC calls to the repo will fail**. Also, if Gitaly crashes in the +// middle of this function, the repo is left in a broken state. We do +// take care to leave a copy of the alternates file, so that it can be +// manually restored by an administrator if needed. +func removeAlternatesIfOk(ctx context.Context, repo *gitalypb.Repository, altFile, backupFile string) error { + if err := os.Rename(altFile, backupFile); err != nil { + return err + } + + rollback := true + defer func() { + if !rollback { + return + } + + logger := grpc_logrus.Extract(ctx) + + // If we would do a os.Rename, and then someone else comes and clobbers + // our file, it's gone forever. This trick with os.Link and os.Rename + // is equivalent to "cp $backupFile $altFile", meaning backupFile is + // preserved for possible forensic use. + tmp := backupFile + ".2" + + if err := os.Link(backupFile, tmp); err != nil { + logger.WithError(err).Error("copy backup alternates file") + return + } + + if err := os.Rename(tmp, altFile); err != nil { + logger.WithError(err).Error("restore backup alternates file") + } + }() + + cmd, err := git.Command(ctx, repo, "fsck", "--connectivity-only") + if err != nil { + return err + } + + if err := cmd.Wait(); err != nil { + return &fsckError{error: err} + } + + rollback = false + return nil +} + +type objectPaths []string + +func (o objectPaths) Len() int { return len(o) } +func (o objectPaths) Swap(i, j int) { o[i], o[j] = o[j], o[i] } + +func (o objectPaths) Less(i, j int) bool { + return objectPriority(o[i]) <= objectPriority(o[j]) +} + +// Based on pack_copy_priority in git/tmp-objdir.c +func objectPriority(name string) int { + if !strings.HasPrefix(name, "pack") { + return 0 + } + if strings.HasSuffix(name, ".keep") { + return 1 + } + if strings.HasSuffix(name, ".pack") { + return 2 + } + if strings.HasSuffix(name, ".idx") { + return 3 + } + return 4 +} diff --git a/internal/service/objectpool/alternates_test.go b/internal/service/objectpool/alternates_test.go new file mode 100644 index 000000000..a8269f037 --- /dev/null +++ b/internal/service/objectpool/alternates_test.go @@ -0,0 +1,148 @@ +package objectpool + +import ( + "fmt" + "io/ioutil" + "os" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +func TestDisconnectGitAlternates(t *testing.T) { + server, serverSocketPath := runObjectPoolServer(t) + defer server.Stop() + + client, conn := newObjectPoolClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + pool, err := objectpool.NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + defer pool.Remove(ctx) + + require.NoError(t, pool.Create(ctx, testRepo)) + require.NoError(t, pool.Link(ctx, testRepo)) + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "gc") + + existingObjectID := "55bc176024cfa3baaceb71db584c7e5df900ea65" + + // Corrupt the repository to check that existingObjectID can no longer be found + altPath, err := git.InfoAlternatesPath(testRepo) + require.NoError(t, err, "find info/alternates") + require.NoError(t, os.RemoveAll(altPath)) + + cmd, err := git.Command(ctx, testRepo, "cat-file", "-e", existingObjectID) + require.NoError(t, err) + require.Error(t, cmd.Wait(), "expect cat-file to fail because object cannot be found") + + require.NoError(t, pool.Link(ctx, testRepo)) + require.FileExists(t, altPath, "objects/info/alternates should be back") + + // At this point we know that the repository has access to + // existingObjectID, but only if objects/info/alternates is in place. + + _, err = client.DisconnectGitAlternates(ctx, &gitalypb.DisconnectGitAlternatesRequest{Repository: testRepo}) + require.NoError(t, err, "call DisconnectGitAlternates") + + // Check that the object can still be found, even though + // objects/info/alternates is gone. This is the purpose of + // DisconnectGitAlternates. + testhelper.AssertFileNotExists(t, altPath) + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "cat-file", "-e", existingObjectID) +} + +func TestDisconnectGitAlternatesNoAlternates(t *testing.T) { + server, serverSocketPath := runObjectPoolServer(t) + defer server.Stop() + + client, conn := newObjectPoolClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + altPath, err := git.InfoAlternatesPath(testRepo) + require.NoError(t, err, "find info/alternates") + testhelper.AssertFileNotExists(t, altPath) + + _, err = client.DisconnectGitAlternates(ctx, &gitalypb.DisconnectGitAlternatesRequest{Repository: testRepo}) + require.NoError(t, err, "call DisconnectGitAlternates on repository without alternates") + + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "fsck") +} + +func TestDisconnectGitAlternatesMultipleAlternates(t *testing.T) { + server, serverSocketPath := runObjectPoolServer(t) + defer server.Stop() + + client, conn := newObjectPoolClient(t, serverSocketPath) + defer conn.Close() + + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + altPath, err := git.InfoAlternatesPath(testRepo) + require.NoError(t, err, "find info/alternates") + + altContent := "/foo/bar\n/qux/baz\n" + require.NoError(t, ioutil.WriteFile(altPath, []byte(altContent), 0644)) + + _, err = client.DisconnectGitAlternates(ctx, &gitalypb.DisconnectGitAlternatesRequest{Repository: testRepo}) + require.Error(t, err, "call DisconnectGitAlternates on repository with multiple alternates") + + contentAfterRPC, err := ioutil.ReadFile(altPath) + require.NoError(t, err, "read back objects/info/alternates") + require.Equal(t, altContent, string(contentAfterRPC), "objects/info/alternates content should not have changed") +} + +func TestRemoveAlternatesIfOk(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + altPath, err := git.InfoAlternatesPath(testRepo) + require.NoError(t, err, "find info/alternates") + altContent := "/var/empty\n" + require.NoError(t, ioutil.WriteFile(altPath, []byte(altContent), 0644), "write alternates file") + + // Intentionally break the repository, so that 'git fsck' will fail later. + testhelper.MustRunCommand(t, nil, "sh", "-c", fmt.Sprintf("rm %s/objects/pack/*.pack", testRepoPath)) + + altBackup := altPath + ".backup" + + err = removeAlternatesIfOk(ctx, testRepo, altPath, altBackup) + require.Error(t, err, "removeAlternatesIfOk should fail") + require.IsType(t, &fsckError{}, err, "error must be because of fsck") + + // We expect objects/info/alternates to have been restored when + // removeAlternatesIfOk returned. + assertAlternates(t, altPath, altContent) + + // We expect the backup alternates file to still exist. + assertAlternates(t, altBackup, altContent) +} + +func assertAlternates(t *testing.T, altPath string, altContent string) { + actualContent, err := ioutil.ReadFile(altPath) + require.NoError(t, err, "read %s after fsck failure", altPath) + + require.Equal(t, altContent, string(actualContent), "%s content after fsck failure", altPath) +} diff --git a/internal/service/objectpool/server.go b/internal/service/objectpool/server.go index d3cab887c..e6dbb1c6b 100644 --- a/internal/service/objectpool/server.go +++ b/internal/service/objectpool/server.go @@ -1,10 +1,7 @@ package objectpool import ( - "context" - "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" - "gitlab.com/gitlab-org/gitaly/internal/helper" ) type server struct{} @@ -13,7 +10,3 @@ type server struct{} func NewServer() gitalypb.ObjectPoolServiceServer { return &server{} } - -func (s *server) DisconnectGitAlternates(ctx context.Context, req *gitalypb.DisconnectGitAlternatesRequest) (*gitalypb.DisconnectGitAlternatesResponse, error) { - return nil, helper.Unimplemented -} diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index d4d5dbe11..71242fa0c 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -460,7 +460,7 @@ func GetRepositoryRefs(t *testing.T, repoPath string) string { // AssertFileNotExists asserts true if the file doesn't exist, false otherwise func AssertFileNotExists(t *testing.T, path string) { _, err := os.Stat(path) - assert.True(t, os.IsNotExist(err)) + assert.True(t, os.IsNotExist(err), "file should not exist: %s", path) } // NewTestObjectPoolName returns a random pool repository name. |