diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-10-08 10:31:17 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-10-08 10:31:17 +0300 |
commit | 58ccfe7fdbde5a3f1d492d7a196ea24f8fab4e34 (patch) | |
tree | 52aa2f1d1e9bd8171533cf163eb86743f9db740f | |
parent | cd55801fe87cb6cf416c5439f85f29570d330519 (diff) | |
parent | 52ff76041c757d07c81cb1d73f1871a47ba5374d (diff) |
Merge branch 'jv-log-double-bitmap' into 'master'
Log warning if repo has too many packfile bitmaps
See merge request gitlab-org/gitaly!1529
18 files changed, 270 insertions, 1 deletions
diff --git a/internal/git/bitmap.go b/internal/git/bitmap.go new file mode 100644 index 000000000..491937b0a --- /dev/null +++ b/internal/git/bitmap.go @@ -0,0 +1,78 @@ +package git + +import ( + "context" + "os" + "strconv" + "strings" + + grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" + grpc_ctxtags "github.com/grpc-ecosystem/go-grpc-middleware/tags" + "github.com/prometheus/client_golang/prometheus" + "gitlab.com/gitlab-org/gitaly/internal/git/packfile" +) + +var badBitmapRequestCount = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_bad_bitmap_request_total", + Help: "RPC calls during which there was not exactly 1 packfile bitmap", + }, + []string{"method", "bitmaps"}, +) + +func init() { prometheus.MustRegister(badBitmapRequestCount) } + +// WarnIfTooManyBitmaps checks for too many (more than one) bitmaps in +// repoPath, and if it finds any, it logs a warning. This is to help us +// investigate https://gitlab.com/gitlab-org/gitaly/issues/1728. +func WarnIfTooManyBitmaps(ctx context.Context, repoPath string) { + logEntry := grpc_logrus.Extract(ctx) + + objdirs, err := ObjectDirectories(ctx, repoPath) + if err != nil { + logEntry.WithError(err).Info("bitmap check failed") + return + } + + var count int + seen := make(map[string]bool) + for _, dir := range objdirs { + if seen[dir] { + continue + } + seen[dir] = true + + packs, err := packfile.List(dir) + if err != nil { + logEntry.WithError(err).Info("bitmap check failed") + return + } + + for _, p := range packs { + fi, err := os.Stat(strings.TrimSuffix(p, ".pack") + ".bitmap") + if err == nil && !fi.IsDir() { + count++ + } + } + } + + if count == 1 { + // Exactly one bitmap: this is how things should be. + return + } + + if count > 1 { + logEntry.WithField("bitmaps", count).Warn("found more than one packfile bitmap in repository") + } + + // The case where count == 0 is likely to occur early in the life of a + // repository. We don't want to spam our logs with that, so we count but + // not log it. + + grpcMethod, ok := grpc_ctxtags.Extract(ctx).Values()["grpc.request.fullMethod"].(string) + if !ok { + return + } + + badBitmapRequestCount.WithLabelValues(grpcMethod, strconv.Itoa(count)).Inc() +} diff --git a/internal/git/dirs.go b/internal/git/dirs.go new file mode 100644 index 000000000..102022dcf --- /dev/null +++ b/internal/git/dirs.go @@ -0,0 +1,77 @@ +package git + +import ( + "context" + "io/ioutil" + "os" + "path/filepath" + "strings" + + grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" +) + +// ObjectDirectories looks for Git object directories, including +// alternates specified in objects/info/alternates. +// +// CAVEAT Git supports quoted strings in here, but we do not. We should +// never need those on a Gitaly server. +func ObjectDirectories(ctx context.Context, repoPath string) ([]string, error) { + objDir := filepath.Join(repoPath, "objects") + dirs, err := altObjectDirs(ctx, objDir, 0) + if err != nil { + return nil, err + } + + return dirs, nil +} + +func altObjectDirs(ctx context.Context, objDir string, depth int) ([]string, error) { + logEntry := grpc_logrus.Extract(ctx) + + const maxAlternatesDepth = 5 // Taken from https://github.com/git/git/blob/v2.23.0/sha1-file.c#L575 + if depth > maxAlternatesDepth { + logEntry.WithField("objdir", objDir).Warn("ignoring deeply nested alternate object directory") + return nil, nil + } + + fi, err := os.Stat(objDir) + if os.IsNotExist(err) { + logEntry.WithField("objdir", objDir).Warn("object directory not found") + return nil, nil + } + if err != nil { + return nil, err + } + if !fi.IsDir() { + return nil, nil + } + + dirs := []string{objDir} + + alternates, err := ioutil.ReadFile(filepath.Join(objDir, "info", "alternates")) + if os.IsNotExist(err) { + return dirs, nil + } + if err != nil { + return nil, err + } + + for _, newDir := range strings.Split(string(alternates), "\n") { + if len(newDir) == 0 || newDir[0] == '#' { + continue + } + + if !filepath.IsAbs(newDir) { + newDir = filepath.Join(objDir, newDir) + } + + nestedDirs, err := altObjectDirs(ctx, newDir, depth+1) + if err != nil { + return nil, err + } + + dirs = append(dirs, nestedDirs...) + } + + return dirs, nil +} diff --git a/internal/git/dirs_test.go b/internal/git/dirs_test.go new file mode 100644 index 000000000..398e2db2f --- /dev/null +++ b/internal/git/dirs_test.go @@ -0,0 +1,28 @@ +package git + +import ( + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +func TestObjectDirs(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + expected := []string{ + "testdata/objdirs/repo0/objects", + "testdata/objdirs/repo1/objects", + "testdata/objdirs/repo2/objects", + "testdata/objdirs/repo3/objects", + "testdata/objdirs/repo4/objects", + "testdata/objdirs/repo5/objects", + "testdata/objdirs/repoB/objects", + } + + out, err := ObjectDirectories(ctx, "testdata/objdirs/repo0") + require.NoError(t, err) + + require.Equal(t, expected, out) +} diff --git a/internal/git/packfile/.gitignore b/internal/git/packfile/.gitignore new file mode 100644 index 000000000..82e77686b --- /dev/null +++ b/internal/git/packfile/.gitignore @@ -0,0 +1 @@ +testdata/empty.git diff --git a/internal/git/packfile/index.go b/internal/git/packfile/index.go index 6597c8949..8f61544c3 100644 --- a/internal/git/packfile/index.go +++ b/internal/git/packfile/index.go @@ -21,8 +21,11 @@ import ( const sumSize = sha1.Size +const regexCore = `(.*/pack-)([0-9a-f]{40})` + var ( - idxFileRegex = regexp.MustCompile(`\A(.*/pack-)([0-9a-f]{40})\.idx\z`) + idxFileRegex = regexp.MustCompile(`\A` + regexCore + `\.idx\z`) + packFileRegex = regexp.MustCompile(`\A` + regexCore + `\.pack\z`) ) // Index is an in-memory representation of a packfile .idx file. diff --git a/internal/git/packfile/packfile.go b/internal/git/packfile/packfile.go new file mode 100644 index 000000000..63716a18c --- /dev/null +++ b/internal/git/packfile/packfile.go @@ -0,0 +1,28 @@ +package packfile + +import ( + "io/ioutil" + "path/filepath" +) + +// List returns the packfiles in objDir. +func List(objDir string) ([]string, error) { + packDir := filepath.Join(objDir, "pack") + entries, err := ioutil.ReadDir(packDir) + if err != nil { + return nil, err + } + + var packs []string + for _, ent := range entries { + if ent.IsDir() { + continue + } + + if p := filepath.Join(packDir, ent.Name()); packFileRegex.MatchString(p) { + packs = append(packs, p) + } + } + + return packs, nil +} diff --git a/internal/git/packfile/packfile_test.go b/internal/git/packfile/packfile_test.go new file mode 100644 index 000000000..94a053213 --- /dev/null +++ b/internal/git/packfile/packfile_test.go @@ -0,0 +1,37 @@ +package packfile + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +func TestList(t *testing.T) { + repoPath0 := "testdata/empty.git" + require.NoError(t, os.RemoveAll(repoPath0)) + testhelper.MustRunCommand(t, nil, "git", "init", "--bare", repoPath0) + + _, repoPath1, cleanup1 := testhelper.NewTestRepo(t) + defer cleanup1() + testhelper.MustRunCommand(t, nil, "git", "-C", repoPath1, "repack", "-ad") + + testCases := []struct { + desc string + path string + numPacks int + }{ + {desc: "empty", path: repoPath0}, + {desc: "1 pack no alternates", path: repoPath1, numPacks: 1}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + out, err := List(filepath.Join(tc.path, "objects")) + require.NoError(t, err) + require.Len(t, out, tc.numPacks) + }) + } +} diff --git a/internal/git/packfile/testdata/.gitkeep b/internal/git/packfile/testdata/.gitkeep new file mode 100644 index 000000000..e69de29bb --- /dev/null +++ b/internal/git/packfile/testdata/.gitkeep diff --git a/internal/git/testdata/objdirs/repo0/objects/info/alternates b/internal/git/testdata/objdirs/repo0/objects/info/alternates new file mode 100644 index 000000000..306b2c526 --- /dev/null +++ b/internal/git/testdata/objdirs/repo0/objects/info/alternates @@ -0,0 +1,4 @@ +../../repo1/objects +# ignored +../../repoB/objects +not found diff --git a/internal/git/testdata/objdirs/repo1/objects/info/alternates b/internal/git/testdata/objdirs/repo1/objects/info/alternates new file mode 100644 index 000000000..bca7f6ff4 --- /dev/null +++ b/internal/git/testdata/objdirs/repo1/objects/info/alternates @@ -0,0 +1,2 @@ +../../repo2/objects/ + diff --git a/internal/git/testdata/objdirs/repo2/objects/info/alternates b/internal/git/testdata/objdirs/repo2/objects/info/alternates new file mode 100644 index 000000000..665cd1c14 --- /dev/null +++ b/internal/git/testdata/objdirs/repo2/objects/info/alternates @@ -0,0 +1,3 @@ +../../repo3/objects + +zzz
\ No newline at end of file diff --git a/internal/git/testdata/objdirs/repo3/objects/info/alternates b/internal/git/testdata/objdirs/repo3/objects/info/alternates new file mode 100644 index 000000000..bb959c2ff --- /dev/null +++ b/internal/git/testdata/objdirs/repo3/objects/info/alternates @@ -0,0 +1 @@ +../../repo4/objects
\ No newline at end of file diff --git a/internal/git/testdata/objdirs/repo4/objects/info/alternates b/internal/git/testdata/objdirs/repo4/objects/info/alternates new file mode 100644 index 000000000..94928d9b4 --- /dev/null +++ b/internal/git/testdata/objdirs/repo4/objects/info/alternates @@ -0,0 +1 @@ +../../repo5/objects diff --git a/internal/git/testdata/objdirs/repo5/objects/info/alternates b/internal/git/testdata/objdirs/repo5/objects/info/alternates new file mode 100644 index 000000000..3899f677e --- /dev/null +++ b/internal/git/testdata/objdirs/repo5/objects/info/alternates @@ -0,0 +1 @@ +../../repo6/objects diff --git a/internal/git/testdata/objdirs/repo6/objects/info/alternates b/internal/git/testdata/objdirs/repo6/objects/info/alternates new file mode 100644 index 000000000..8cd424b08 --- /dev/null +++ b/internal/git/testdata/objdirs/repo6/objects/info/alternates @@ -0,0 +1 @@ +../../repo7/objects diff --git a/internal/git/testdata/objdirs/repoB/objects/.gitkeep b/internal/git/testdata/objdirs/repoB/objects/.gitkeep new file mode 100644 index 000000000..e69de29bb --- /dev/null +++ b/internal/git/testdata/objdirs/repoB/objects/.gitkeep diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go index 6d1232f76..559d0a2a6 100644 --- a/internal/service/smarthttp/upload_pack.go +++ b/internal/service/smarthttp/upload_pack.go @@ -62,6 +62,8 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS return err } + git.WarnIfTooManyBitmaps(ctx, repoPath) + args := []string{} if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) { args = append(args, "-c", "uploadpack.allowFilter=true", "-c", "uploadpack.allowAnySHA1InWant=true") diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go index 969764285..b4881a618 100644 --- a/internal/service/ssh/upload_pack.go +++ b/internal/service/ssh/upload_pack.go @@ -48,6 +48,8 @@ func sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, req *gitalypb return err } + git.WarnIfTooManyBitmaps(ctx, repoPath) + args := []string{} for _, params := range req.GitConfigOptions { |