Log warning if repo has too many packfile bitmaps

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 {