Merge branch 'jv-link-remove-bitmap' into 'master'

Remove member bitmaps when linking to objectpool

Closes #1728

See merge request gitlab-org/gitaly!1311

3 files changed, 119 insertions, 1 deletions

diff --git a/changelogs/unreleased/jv-link-remove-bitmap.yml b/changelogs/unreleased/jv-link-remove-bitmap.yml
new file mode 100644
index 000000000..1b4c19039
--- /dev/null
+++ b/changelogs/unreleased/jv-link-remove-bitmap.yml
@@ -0,0 +1,5 @@
+---
+title: Remove member bitmaps when linking to objectpool
+merge_request: 1311
+author:
+type: changed
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index 7c5160ded..731081b8f 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -9,8 +9,10 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
@@ -72,7 +74,69 @@ func (o *ObjectPool) Link(ctx context.Context, repo *gitalypb.Repository) error
 		return err
 	}
 
-	return os.Rename(tmp.Name(), altPath)
+	if err := os.Rename(tmp.Name(), altPath); err != nil {
+		return err
+	}
+
+	return o.removeMemberBitmaps(repo)
+}
+
+// removeMemberBitmaps removes packfile bitmaps from the member
+// repository that just joined the pool. If Git finds two packfiles with
+// bitmaps it will print a warning, which is visible to the end user
+// during a Git clone. Our goal is to avoid that warning. In normal
+// operation, the next 'git gc' or 'git repack -ad' on the member
+// repository will remove its local bitmap file. In other words the
+// situation we eventually converge to is that the pool may have a bitmap
+// but none of its members will. With removeMemberBitmaps we try to
+// change "eventually" to "immediately", so that users won't see the
+// warning. https://gitlab.com/gitlab-org/gitaly/issues/1728
+func (o *ObjectPool) removeMemberBitmaps(repo repository.GitRepo) error {
+	poolBitmaps, err := getBitmaps(o)
+	if err != nil {
+		return err
+	}
+	if len(poolBitmaps) == 0 {
+		return nil
+	}
+
+	memberBitmaps, err := getBitmaps(repo)
+	if err != nil {
+		return err
+	}
+	if len(memberBitmaps) == 0 {
+		return nil
+	}
+
+	for _, bitmap := range memberBitmaps {
+		if err := os.Remove(bitmap); err != nil && !os.IsNotExist(err) {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func getBitmaps(repo repository.GitRepo) ([]string, error) {
+	repoPath, err := helper.GetPath(repo)
+	if err != nil {
+		return nil, err
+	}
+
+	packDir := filepath.Join(repoPath, "objects/pack")
+	entries, err := ioutil.ReadDir(packDir)
+	if err != nil {
+		return nil, err
+	}
+
+	var bitmaps []string
+	for _, entry := range entries {
+		if name := entry.Name(); strings.HasSuffix(name, ".bitmap") && strings.HasPrefix(name, "pack-") {
+			bitmaps = append(bitmaps, filepath.Join(packDir, name))
+		}
+	}
+
+	return bitmaps, nil
 }
 
 func (o *ObjectPool) getRelativeObjectPath(repo *gitalypb.Repository) (string, error) {
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index c409aca0e..8b5dd6612 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -3,6 +3,7 @@ package objectpool
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -48,6 +49,54 @@ func TestLink(t *testing.T) {
 	require.True(t, testhelper.RemoteExists(t, pool.FullPath(), testRepo.GetGlRepository()), "pool remotes should include %v", testRepo)
 }
 
+func TestLinkRemoveBitmap(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	pool, err := NewObjectPool(testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t))
+	require.NoError(t, err)
+
+	require.NoError(t, pool.Init(ctx))
+
+	poolPath := pool.FullPath()
+	testhelper.MustRunCommand(t, nil, "git", "-C", poolPath, "fetch", testRepoPath, "+refs/*:refs/*")
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", poolPath, "repack", "-adb")
+	require.Len(t, listBitmaps(t, pool.FullPath()), 1, "pool bitmaps before")
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "repack", "-adb")
+	require.Len(t, listBitmaps(t, testRepoPath), 1, "member bitmaps before")
+
+	refsBefore := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref")
+
+	require.NoError(t, pool.Link(ctx, testRepo))
+
+	require.Len(t, listBitmaps(t, pool.FullPath()), 1, "pool bitmaps after")
+	require.Len(t, listBitmaps(t, testRepoPath), 0, "member bitmaps after")
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "fsck")
+
+	refsAfter := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref")
+	require.Equal(t, refsBefore, refsAfter, "compare member refs before/after link")
+}
+
+func listBitmaps(t *testing.T, repoPath string) []string {
+	entries, err := ioutil.ReadDir(filepath.Join(repoPath, "objects/pack"))
+	require.NoError(t, err)
+
+	var bitmaps []string
+	for _, entry := range entries {
+		if strings.HasSuffix(entry.Name(), ".bitmap") {
+			bitmaps = append(bitmaps, entry.Name())
+		}
+	}
+
+	return bitmaps
+}
+
 func TestUnlink(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()