diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2020-05-21 21:51:46 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2020-05-22 11:46:57 +0300 |
commit | 01e8fe840a1a02ffba64b9eb7b0482012b16c087 (patch) | |
tree | f9b4745032cdffa05ce3823bd183c8b5d9a1728c | |
parent | 8122214a1052416fdd94cb547752cf4090c5d68d (diff) |
Stale packed-refs.new files prevents branches from being deleted
Cleanup RPC must remove stale packed-refs.new files that Git
doesn't remove when update-ref operation fails.
This required in order to fix 'File exists' error on the next
run of update-ref operation.
Closes https://gitlab.com/gitlab-org/gitaly/-/issues/2627
-rw-r--r-- | changelogs/unreleased/ps-clean-packed-refs-new.yml | 5 | ||||
-rw-r--r-- | internal/service/repository/cleanup.go | 34 | ||||
-rw-r--r-- | internal/service/repository/cleanup_test.go | 72 | ||||
-rw-r--r-- | internal/service/repository/gc_test.go | 18 |
4 files changed, 110 insertions, 19 deletions
diff --git a/changelogs/unreleased/ps-clean-packed-refs-new.yml b/changelogs/unreleased/ps-clean-packed-refs-new.yml new file mode 100644 index 000000000..2dd16f127 --- /dev/null +++ b/changelogs/unreleased/ps-clean-packed-refs-new.yml @@ -0,0 +1,5 @@ +--- +title: Stale packed-refs.new files prevents branches from being deleted +merge_request: 2206 +author: +type: fixed diff --git a/internal/service/repository/cleanup.go b/internal/service/repository/cleanup.go index 54469938a..e18e9f401 100644 --- a/internal/service/repository/cleanup.go +++ b/internal/service/repository/cleanup.go @@ -48,11 +48,16 @@ func cleanupRepo(ctx context.Context, repo *gitalypb.Repository) error { return status.Errorf(codes.Internal, "Cleanup: cleanDisconnectedWorktrees: %v", err) } - configLockThreshod := time.Now().Add(-15 * time.Minute) - if err := cleanFileLocks(repoPath, configLockThreshod); err != nil { + older15min := time.Now().Add(-15 * time.Minute) + + if err := cleanFileLocks(repoPath, older15min); err != nil { return status.Errorf(codes.Internal, "Cleanup: cleanupConfigLock: %v", err) } + if err := cleanPackedRefsNew(repoPath, older15min); err != nil { + return status.Errorf(codes.Internal, "Cleanup: cleanPackedRefsNew: %v", err) + } + return nil } @@ -166,3 +171,28 @@ func cleanFileLocks(repoPath string, threshold time.Time) error { return nil } + +func cleanPackedRefsNew(repoPath string, threshold time.Time) error { + path := filepath.Join(repoPath, "packed-refs.new") + + fileInfo, err := os.Stat(path) + if err != nil { + if os.IsNotExist(err) { + return nil // file is already gone, nothing to do! + } + return err + } + + if fileInfo.ModTime().After(threshold) { + return nil // it is fresh enough + } + + if err := os.Remove(path); err != nil { + if os.IsNotExist(err) { + return nil // file is already gone, nothing to do! + } + return err + } + + return nil +} diff --git a/internal/service/repository/cleanup_test.go b/internal/service/repository/cleanup_test.go index 6b36eae92..7b8256c17 100644 --- a/internal/service/repository/cleanup_test.go +++ b/internal/service/repository/cleanup_test.go @@ -31,17 +31,17 @@ func TestCleanupDeletesRefsLocks(t *testing.T) { refsPath := filepath.Join(testRepoPath, "refs") keepRefPath := filepath.Join(refsPath, "heads", "keepthis") - createFileWithTimes(keepRefPath, freshTime) + mustCreateFileWithTimes(t, keepRefPath, freshTime) keepOldRefPath := filepath.Join(refsPath, "heads", "keepthisalso") - createFileWithTimes(keepOldRefPath, oldTime) + mustCreateFileWithTimes(t, keepOldRefPath, oldTime) keepDeceitfulRef := filepath.Join(refsPath, "heads", " .lock.not-actually-a-lock.lock ") - createFileWithTimes(keepDeceitfulRef, oldTime) + mustCreateFileWithTimes(t, keepDeceitfulRef, oldTime) keepLockPath := filepath.Join(refsPath, "heads", "keepthis.lock") - createFileWithTimes(keepLockPath, freshTime) + mustCreateFileWithTimes(t, keepLockPath, freshTime) deleteLockPath := filepath.Join(refsPath, "heads", "deletethis.lock") - createFileWithTimes(deleteLockPath, oldTime) + mustCreateFileWithTimes(t, deleteLockPath, oldTime) c, err := client.Cleanup(ctx, req) assert.NoError(t, err) @@ -100,7 +100,7 @@ func TestCleanupDeletesPackedRefsLock(t *testing.T) { lockPath := filepath.Join(testRepoPath, "packed-refs.lock") if tc.lockTime != nil { - createFileWithTimes(lockPath, *tc.lockTime) + mustCreateFileWithTimes(t, lockPath, *tc.lockTime) } ctx, cancel := testhelper.Context() @@ -279,15 +279,71 @@ func TestCleanupFileLocks(t *testing.T) { assert.NoError(t, err) // Fresh lock should remain - createFileWithTimes(lockPath, freshTime) + mustCreateFileWithTimes(t, lockPath, freshTime) _, err = client.Cleanup(ctx, req) assert.NoError(t, err) assert.FileExists(t, lockPath) // Old lock should be removed - createFileWithTimes(lockPath, oldTime) + mustCreateFileWithTimes(t, lockPath, oldTime) _, err = client.Cleanup(ctx, req) assert.NoError(t, err) testhelper.AssertPathNotExists(t, lockPath) } } + +func TestCleanupDeletesPackedRefsNew(t *testing.T) { + serverSocketPath, stop := runRepoServer(t) + defer stop() + + client, conn := newRepositoryClient(t, serverSocketPath) + defer conn.Close() + + testCases := []struct { + desc string + lockTime *time.Time + shouldExist bool + }{ + { + desc: "created recently", + lockTime: &freshTime, + shouldExist: true, + }, + { + desc: "exists for too long", + lockTime: &oldTime, + shouldExist: false, + }, + { + desc: "nothing to clean up", + shouldExist: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + req := &gitalypb.CleanupRequest{Repository: testRepo} + packedRefsNewPath := filepath.Join(testRepoPath, "packed-refs.new") + + if tc.lockTime != nil { + mustCreateFileWithTimes(t, packedRefsNewPath, *tc.lockTime) + } + + ctx, cancel := testhelper.Context() + defer cancel() + + c, err := client.Cleanup(ctx, req) + require.NotNil(t, c) + require.NoError(t, err) + + if tc.shouldExist { + require.FileExists(t, packedRefsNewPath) + } else { + testhelper.AssertPathNotExists(t, packedRefsNewPath) + } + }) + } +} diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go index 1295e3275..256a517fd 100644 --- a/internal/service/repository/gc_test.go +++ b/internal/service/repository/gc_test.go @@ -173,17 +173,17 @@ func TestGarbageCollectDeletesRefsLocks(t *testing.T) { // deleted are all due to our *.lock cleanup step before gc runs (since // `git gc` also deletes files from /refs when packing). keepRefPath := filepath.Join(refsPath, "heads", "keepthis") - createFileWithTimes(keepRefPath, freshTime) + mustCreateFileWithTimes(t, keepRefPath, freshTime) keepOldRefPath := filepath.Join(refsPath, "heads", "keepthisalso") - createFileWithTimes(keepOldRefPath, oldTime) + mustCreateFileWithTimes(t, keepOldRefPath, oldTime) keepDeceitfulRef := filepath.Join(refsPath, "heads", " .lock.not-actually-a-lock.lock ") - createFileWithTimes(keepDeceitfulRef, oldTime) + mustCreateFileWithTimes(t, keepDeceitfulRef, oldTime) keepLockPath := filepath.Join(refsPath, "heads", "keepthis.lock") - createFileWithTimes(keepLockPath, freshTime) + mustCreateFileWithTimes(t, keepLockPath, freshTime) deleteLockPath := filepath.Join(refsPath, "heads", "deletethis.lock") - createFileWithTimes(deleteLockPath, oldTime) + mustCreateFileWithTimes(t, deleteLockPath, oldTime) c, err := client.GarbageCollect(ctx, req) testhelper.RequireGrpcError(t, err, codes.Internal) @@ -322,10 +322,10 @@ func TestCleanupInvalidKeepAroundRefs(t *testing.T) { } } -func createFileWithTimes(path string, mTime time.Time) { - os.MkdirAll(filepath.Dir(path), 0755) - ioutil.WriteFile(path, nil, 0644) - os.Chtimes(path, mTime, mTime) +func mustCreateFileWithTimes(t testing.TB, path string, mTime time.Time) { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755)) + require.NoError(t, ioutil.WriteFile(path, nil, 0644)) + require.NoError(t, os.Chtimes(path, mTime, mTime)) } func TestGarbageCollectDeltaIslands(t *testing.T) { |