diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2019-06-21 17:05:46 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-06-21 17:05:46 +0300 |
commit | f8c4207f56adadc66178b168af79999868493c0d (patch) | |
tree | 3ed759a3f675beb5a0b7efaf925816409e9fa522 | |
parent | 8a954b0de6e7f715682b4f01aadd5fe8c29dd622 (diff) |
Maintain pool packfiles
-rw-r--r-- | changelogs/unreleased/jv-dedup-delta-islands.yml | 5 | ||||
-rw-r--r-- | internal/git/gittest/delta_islands.go (renamed from internal/service/repository/delta_islands_test.go) | 25 | ||||
-rw-r--r-- | internal/git/gittest/hashcache.go | 27 | ||||
-rw-r--r-- | internal/git/objectpool/fetch.go | 33 | ||||
-rw-r--r-- | internal/git/objectpool/fetch_test.go | 107 | ||||
-rw-r--r-- | internal/service/repository/gc_test.go | 21 | ||||
-rw-r--r-- | internal/service/repository/repack_test.go | 43 |
7 files changed, 185 insertions, 76 deletions
diff --git a/changelogs/unreleased/jv-dedup-delta-islands.yml b/changelogs/unreleased/jv-dedup-delta-islands.yml new file mode 100644 index 000000000..2b03f1dc6 --- /dev/null +++ b/changelogs/unreleased/jv-dedup-delta-islands.yml @@ -0,0 +1,5 @@ +--- +title: Maintain pool packfiles +merge_request: 1316 +author: +type: performance diff --git a/internal/service/repository/delta_islands_test.go b/internal/git/gittest/delta_islands.go index 5efd49451..7640be6d3 100644 --- a/internal/service/repository/delta_islands_test.go +++ b/internal/git/gittest/delta_islands.go @@ -1,4 +1,4 @@ -package repository +package gittest import ( "bytes" @@ -14,16 +14,9 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/testhelper" ) -type deltaIslandOutcome int - -const ( - expectDeltaIslands deltaIslandOutcome = iota - expectNoDeltaIslands -) - -// testDeltaIslands is based on the tests in +// TestDeltaIslands is based on the tests in // https://github.com/git/git/blob/master/t/t5320-delta-islands.sh . -func testDeltaIslands(t *testing.T, repoPath string, outcome deltaIslandOutcome, repack func() error) { +func TestDeltaIslands(t *testing.T, repoPath string, repack func() error) { gitVersion, err := git.Version() require.NoError(t, err) @@ -60,16 +53,10 @@ func testDeltaIslands(t *testing.T, repoPath string, outcome deltaIslandOutcome, require.NoError(t, repack(), "repack after delta island setup") - switch outcome { - case expectDeltaIslands: - assert.Equal(t, blob2ID, deltaBase(t, repoPath, blob1ID), "blob 1 delta base should be blob 2 after repack") + assert.Equal(t, blob2ID, deltaBase(t, repoPath, blob1ID), "blob 1 delta base should be blob 2 after repack") - // blob2 is the bigger of the two so it should be the delta base - assert.Equal(t, git.NullSHA, deltaBase(t, repoPath, blob2ID), "blob 2 should not be delta compressed after repack") - case expectNoDeltaIslands: - assert.Equal(t, badBlobID, deltaBase(t, repoPath, blob1ID), "expect blob 1 delta base to still be bad blob after repack") - assert.Equal(t, badBlobID, deltaBase(t, repoPath, blob2ID), "expect blob 2 delta base to still be bad blob after repack") - } + // blob2 is the bigger of the two so it should be the delta base + assert.Equal(t, git.NullSHA, deltaBase(t, repoPath, blob2ID), "blob 2 should not be delta compressed after repack") } func commitBlob(t *testing.T, repoPath, ref string, content []byte) string { diff --git a/internal/git/gittest/hashcache.go b/internal/git/gittest/hashcache.go new file mode 100644 index 000000000..b09c58dfc --- /dev/null +++ b/internal/git/gittest/hashcache.go @@ -0,0 +1,27 @@ +package gittest + +import ( + "encoding/binary" + "io" + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestBitmapHasHashcache checks if the named pack bitmap file contains +// "hash cache" data. See +// https://github.com/git/git/blob/master/Documentation/technical/bitmap-format.txt +func TestBitmapHasHashcache(t *testing.T, bitmap string) { + bitmapFile, err := os.Open(bitmap) + require.NoError(t, err) + defer bitmapFile.Close() + + b := make([]byte, 8) + _, err = io.ReadFull(bitmapFile, b) + require.NoError(t, err) + + const hashCacheFlag = 0x4 + flags := binary.BigEndian.Uint16(b[6:]) + require.Equal(t, uint16(hashCacheFlag), flags&hashCacheFlag, "expect BITMAP_OPT_HASH_CACHE to be set") +} diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go index 30df30ba1..b173e02b8 100644 --- a/internal/git/objectpool/fetch.go +++ b/internal/git/objectpool/fetch.go @@ -14,6 +14,11 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/helper" ) +const ( + sourceRemote = "origin" + sourceRefNamespace = "refs/remotes/" + sourceRemote +) + // FetchFromOrigin initializes the pool and fetches the objects from its origin repository func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repository) error { if err := o.Init(ctx); err != nil { @@ -35,7 +40,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos var originExists bool for remoteReader.Scan() { - if remoteReader.Text() == "origin" { + if remoteReader.Text() == sourceRemote { originExists = true } } @@ -45,12 +50,12 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos var setOriginCmd *command.Command if originExists { - setOriginCmd, err = git.Command(ctx, o, "remote", "set-url", "origin", originPath) + setOriginCmd, err = git.Command(ctx, o, "remote", "set-url", sourceRemote, originPath) if err != nil { return err } } else { - setOriginCmd, err = git.Command(ctx, o, "remote", "add", "origin", originPath) + setOriginCmd, err = git.Command(ctx, o, "remote", "add", sourceRemote, originPath) if err != nil { return err } @@ -60,7 +65,8 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos return err } - fetchCmd, err := git.Command(ctx, o, "fetch", "--quiet", "origin") + refSpec := fmt.Sprintf("+refs/*:%s/*", sourceRefNamespace) + fetchCmd, err := git.Command(ctx, o, "fetch", "--quiet", sourceRemote, refSpec) if err != nil { return err } @@ -73,7 +79,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos return err } - return nil + return repackPool(ctx, o) } const danglingObjectNamespace = "refs/dangling" @@ -122,7 +128,22 @@ func rescueDanglingObjects(ctx context.Context, repo repository.GitRepo) error { return fmt.Errorf("git fsck: %v", err) } - if err := updater.Wait(); err != nil { + return updater.Wait() +} + +func repackPool(ctx context.Context, pool repository.GitRepo) error { + repackArgs := []string{ + "-c", "pack.island=" + sourceRefNamespace + "/heads", + "-c", "pack.island=" + sourceRefNamespace + "/tags", + "-c", "pack.writeBitmapHashCache=true", + "repack", "-aidb", + } + repackCmd, err := git.Command(ctx, pool, repackArgs...) + if err != nil { + return err + } + + if err := repackCmd.Wait(); err != nil { return err } diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go index 85dfb144a..5af01b220 100644 --- a/internal/git/objectpool/fetch_test.go +++ b/internal/git/objectpool/fetch_test.go @@ -5,10 +5,13 @@ import ( "encoding/hex" "fmt" "io" + "io/ioutil" + "path/filepath" "strings" "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/internal/helper/text" "gitlab.com/gitlab-org/gitaly/internal/testhelper" ) @@ -86,3 +89,107 @@ func TestFetchFromOriginDangling(t *testing.T) { require.Contains(t, refsAfterLines, fmt.Sprintf("refs/dangling/%s %s", id, id)) } } + +func TestFetchFromOriginDeltaIslands(t *testing.T) { + source, sourcePath, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + + ctx, cancel := testhelper.Context() + defer cancel() + + require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool") + require.NoError(t, pool.Link(ctx, source)) + + gittest.TestDeltaIslands(t, sourcePath, func() error { + // This should create a new packfile with good delta chains in the pool + if err := pool.FetchFromOrigin(ctx, source); err != nil { + return err + } + + // Make sure the old packfile, with bad delta chains, is deleted from the source repo + testhelper.MustRunCommand(t, nil, "git", "-C", sourcePath, "repack", "-ald") + + return nil + }) +} + +func TestFetchFromOriginBitmapHashCache(t *testing.T) { + source, _, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + + ctx, cancel := testhelper.Context() + defer cancel() + + require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool") + + packDir := filepath.Join(pool.FullPath(), "objects/pack") + packEntries, err := ioutil.ReadDir(packDir) + require.NoError(t, err) + + var bitmap string + for _, ent := range packEntries { + if name := ent.Name(); strings.HasSuffix(name, ".bitmap") { + bitmap = filepath.Join(packDir, name) + break + } + } + + require.NotEmpty(t, bitmap, "path to bitmap file") + + gittest.TestBitmapHasHashcache(t, bitmap) +} + +func TestFetchFromOriginRefUpdates(t *testing.T) { + source, sourcePath, cleanup := testhelper.NewTestRepo(t) + defer cleanup() + + pool, err := NewObjectPool(source.StorageName, testhelper.NewTestObjectPoolName(t)) + require.NoError(t, err) + poolPath := pool.FullPath() + + ctx, cancel := testhelper.Context() + defer cancel() + + require.NoError(t, pool.FetchFromOrigin(ctx, source), "seed pool") + + oldRefs := map[string]string{ + "heads/csv": "3dd08961455abf80ef9115f4afdc1c6f968b503c", + "tags/v1.1.0": "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b", + } + + for ref, oid := range oldRefs { + require.Equal(t, oid, resolveRef(t, sourcePath, "refs/"+ref), "look up %q in source", ref) + require.Equal(t, oid, resolveRef(t, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool", ref) + } + + newRefs := map[string]string{ + "heads/csv": "46abbb087fcc0fd02c340f0f2f052bd2c7708da3", + "tags/v1.1.0": "646ece5cfed840eca0a4feb21bcd6a81bb19bda3", + } + + for ref, newOid := range newRefs { + require.NotEqual(t, newOid, oldRefs[ref], "sanity check of new refs") + } + + for ref, oid := range newRefs { + testhelper.MustRunCommand(t, nil, "git", "-C", sourcePath, "update-ref", "refs/"+ref, oid) + require.Equal(t, oid, resolveRef(t, sourcePath, "refs/"+ref), "look up %q in source after update", ref) + } + + require.NoError(t, pool.FetchFromOrigin(ctx, source), "update pool") + + for ref, oid := range newRefs { + require.Equal(t, oid, resolveRef(t, poolPath, "refs/remotes/origin/"+ref), "look up %q in pool after update", ref) + } +} + +func resolveRef(t *testing.T, repo string, ref string) string { + out := testhelper.MustRunCommand(t, nil, "git", "-C", repo, "rev-parse", ref) + return text.ChompBytes(out) +} diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go index 029d5e885..0cf9e5d58 100644 --- a/internal/service/repository/gc_test.go +++ b/internal/service/repository/gc_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/internal/helper/text" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/codes" @@ -307,20 +308,8 @@ func TestGarbageCollectDeltaIslands(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - testCases := []struct { - desc string - outcome deltaIslandOutcome - ctx context.Context - }{ - {desc: "are created by default", outcome: expectDeltaIslands, ctx: ctx}, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - testDeltaIslands(t, testRepoPath, tc.outcome, func() error { - _, err := client.GarbageCollect(tc.ctx, &gitalypb.GarbageCollectRequest{Repository: testRepo}) - return err - }) - }) - } + gittest.TestDeltaIslands(t, testRepoPath, func() error { + _, err := client.GarbageCollect(ctx, &gitalypb.GarbageCollectRequest{Repository: testRepo}) + return err + }) } diff --git a/internal/service/repository/repack_test.go b/internal/service/repository/repack_test.go index 84f875591..08d94960b 100644 --- a/internal/service/repository/repack_test.go +++ b/internal/service/repository/repack_test.go @@ -2,8 +2,6 @@ package repository import ( "context" - "encoding/binary" - "os" "os/exec" "path" "path/filepath" @@ -13,6 +11,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/codes" ) @@ -154,7 +153,7 @@ func TestRepackFullSuccess(t *testing.T) { if len(bmPath) == 0 { t.Errorf("No bitmaps found") } - require.True(t, doBitmapsContainHashCache(t, bmPath), "bitmap file should contain hash cache") + doBitmapsContainHashCache(t, bmPath) } else { if len(bmPath) != 0 { t.Errorf("Bitmap found: %v", bmPath) @@ -164,26 +163,12 @@ func TestRepackFullSuccess(t *testing.T) { } } -func doBitmapsContainHashCache(t *testing.T, bitmapPaths []string) bool { +func doBitmapsContainHashCache(t *testing.T, bitmapPaths []string) { // for each bitmap file, check the 2-byte flag as documented in // https://github.com/git/git/blob/master/Documentation/technical/bitmap-format.txt for _, bitmapPath := range bitmapPaths { - b := make([]byte, 8) - bitmapFile, err := os.Open(bitmapPath) - require.NoError(t, err) - defer bitmapFile.Close() - - read, err := bitmapFile.Read(b) - require.NoError(t, err) - require.Equal(t, 8, read) - - flags := binary.BigEndian.Uint16(b[6:]) - if flags != 5 { - return false - } + gittest.TestBitmapHasHashcache(t, bitmapPath) } - - return true } func TestRepackFullFailure(t *testing.T) { @@ -227,20 +212,8 @@ func TestRepackFullDeltaIslands(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - testCases := []struct { - desc string - outcome deltaIslandOutcome - ctx context.Context - }{ - {desc: "are created by default", outcome: expectDeltaIslands, ctx: ctx}, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - testDeltaIslands(t, testRepoPath, tc.outcome, func() error { - _, err := client.RepackFull(tc.ctx, &gitalypb.RepackFullRequest{Repository: testRepo}) - return err - }) - }) - } + gittest.TestDeltaIslands(t, testRepoPath, func() error { + _, err := client.RepackFull(ctx, &gitalypb.RepackFullRequest{Repository: testRepo}) + return err + }) } |