Merge branch 'jacobvosmaer-gitlab/gitlab-ce-git-gc-improvements' into 'master'

Use more than one kind of Git garbage collection

Replaces https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6588 by @jacobvosmaer to get the builds to pass :)

Closes #22729

See merge request !7321

5 files changed, 177 insertions, 18 deletions

diff --git a/spec/lib/gitlab/backend/shell_spec.rb b/spec/lib/gitlab/backend/shell_spec.rb
index f826d0d1b04..4b08a02ec73 100644
--- a/spec/lib/gitlab/backend/shell_spec.rb
+++ b/spec/lib/gitlab/backend/shell_spec.rb
@@ -14,7 +14,6 @@ describe Gitlab::Shell, lib: true do
   it { is_expected.to respond_to :add_repository }
   it { is_expected.to respond_to :remove_repository }
   it { is_expected.to respond_to :fork_repository }
-  it { is_expected.to respond_to :gc }
   it { is_expected.to respond_to :add_namespace }
   it { is_expected.to respond_to :rm_namespace }
   it { is_expected.to respond_to :mv_namespace }
diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb
index 6b3bd08b978..a366d68a146 100644
--- a/spec/lib/gitlab/exclusive_lease_spec.rb
+++ b/spec/lib/gitlab/exclusive_lease_spec.rb
@@ -5,32 +5,47 @@ describe Gitlab::ExclusiveLease, type: :redis do
 
   describe '#try_obtain' do
     it 'cannot obtain twice before the lease has expired' do
-      lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
-      expect(lease.try_obtain).to eq(true)
+      lease = described_class.new(unique_key, timeout: 3600)
+      expect(lease.try_obtain).to be_present
       expect(lease.try_obtain).to eq(false)
     end
 
     it 'can obtain after the lease has expired' do
       timeout = 1
-      lease = Gitlab::ExclusiveLease.new(unique_key, timeout: timeout)
+      lease = described_class.new(unique_key, timeout: timeout)
       lease.try_obtain # start the lease
       sleep(2 * timeout) # lease should have expired now
-      expect(lease.try_obtain).to eq(true)
+      expect(lease.try_obtain).to be_present
     end
   end
 
   describe '#exists?' do
     it 'returns true for an existing lease' do
-      lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
+      lease = described_class.new(unique_key, timeout: 3600)
       lease.try_obtain
 
       expect(lease.exists?).to eq(true)
     end
 
     it 'returns false for a lease that does not exist' do
-      lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
+      lease = described_class.new(unique_key, timeout: 3600)
 
       expect(lease.exists?).to eq(false)
     end
   end
+
+  describe '.cancel' do
+    it 'can cancel a lease' do
+      uuid = new_lease(unique_key)
+      expect(uuid).to be_present
+      expect(new_lease(unique_key)).to eq(false)
+
+      described_class.cancel(unique_key, uuid)
+      expect(new_lease(unique_key)).to be_present
+    end
+
+    def new_lease(key)
+      described_class.new(key, timeout: 3600).try_obtain
+    end
+  end
 end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 2b76e056f3c..b950fcdd81a 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -98,6 +98,24 @@ describe ApplicationSetting, models: true do
         end
       end
     end
+
+    context 'housekeeping settings' do
+      it { is_expected.not_to allow_value(0).for(:housekeeping_incremental_repack_period) }
+
+      it 'wants the full repack period to be longer than the incremental repack period' do
+        subject.housekeeping_incremental_repack_period = 2
+        subject.housekeeping_full_repack_period = 1
+
+        expect(subject).not_to be_valid
+      end
+
+      it 'wants the gc period to be longer than the full repack period' do
+        subject.housekeeping_full_repack_period = 2
+        subject.housekeeping_gc_period = 1
+
+        expect(subject).not_to be_valid
+      end
+    end
   end
 
   context 'restricted signup domains' do
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index cf90b33dfb4..57a5aa5cedc 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -14,8 +14,10 @@ describe Projects::HousekeepingService do
 
   describe '#execute' do
     it 'enqueues a sidekiq job' do
-      expect(subject).to receive(:try_obtain_lease).and_return(true)
-      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id)
+      expect(subject).to receive(:try_obtain_lease).and_return(:the_uuid)
+      expect(subject).to receive(:lease_key).and_return(:the_lease_key)
+      expect(subject).to receive(:task).and_return(:the_task)
+      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :the_task, :the_lease_key, :the_uuid)
 
       subject.execute
       expect(project.reload.pushes_since_gc).to eq(0)
@@ -58,4 +60,26 @@ describe Projects::HousekeepingService do
       end.to change { project.pushes_since_gc }.from(0).to(1)
     end
   end
+
+  it 'uses all three kinds of housekeeping we offer' do
+    allow(subject).to receive(:try_obtain_lease).and_return(:the_uuid)
+    allow(subject).to receive(:lease_key).and_return(:the_lease_key)
+
+    # At push 200
+    expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid).
+      exactly(1).times
+    # At push 50, 100, 150
+    expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :full_repack, :the_lease_key, :the_uuid).
+      exactly(3).times
+    # At push 10, 20, ... (except those above)
+    expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :incremental_repack, :the_lease_key, :the_uuid).
+      exactly(16).times
+
+    201.times do
+      subject.increment!
+      subject.execute if subject.needed?
+    end
+
+    expect(project.pushes_since_gc).to eq(1)
+  end
 end
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index c9f5aae0815..e471a68a49a 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -1,3 +1,5 @@
+require 'fileutils'
+
 require 'spec_helper'
 
 describe GitGarbageCollectWorker do
@@ -6,16 +8,12 @@ describe GitGarbageCollectWorker do
 
   subject { GitGarbageCollectWorker.new }
 
-  before do
-    allow(subject).to receive(:gitlab_shell).and_return(shell)
-  end
-
   describe "#perform" do
-    it "runs `git gc`" do
-      expect(shell).to receive(:gc).with(
-        project.repository_storage_path,
-        project.path_with_namespace).
-      and_return(true)
+    it "flushes ref caches when the task is 'gc'" do
+      expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
+      expect(Gitlab::Popen).to receive(:popen).
+        with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
+
       expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original
       expect_any_instance_of(Repository).to receive(:branch_names).and_call_original
       expect_any_instance_of(Repository).to receive(:branch_count).and_call_original
@@ -23,5 +21,110 @@ describe GitGarbageCollectWorker do
 
       subject.perform(project.id)
     end
+
+    shared_examples 'gc tasks' do
+      before { allow(subject).to receive(:bitmaps_enabled?).and_return(bitmaps_enabled) }
+
+      it 'incremental repack adds a new packfile' do
+        create_objects(project)
+        before_packs = packs(project)
+
+        expect(before_packs.count).to be >= 1
+
+        subject.perform(project.id, 'incremental_repack')
+        after_packs = packs(project)
+
+        # Exactly one new pack should have been created
+        expect(after_packs.count).to eq(before_packs.count + 1)
+
+        # Previously existing packs are still around
+        expect(before_packs & after_packs).to eq(before_packs)
+      end
+
+      it 'full repack consolidates into 1 packfile' do
+        create_objects(project)
+        subject.perform(project.id, 'incremental_repack')
+        before_packs = packs(project)
+
+        expect(before_packs.count).to be >= 2
+
+        subject.perform(project.id, 'full_repack')
+        after_packs = packs(project)
+
+        expect(after_packs.count).to eq(1)
+
+        # Previously existing packs should be gone now
+        expect(after_packs - before_packs).to eq(after_packs)
+
+        expect(File.exist?(bitmap_path(after_packs.first))).to eq(bitmaps_enabled)
+      end
+
+      it 'gc consolidates into 1 packfile and updates packed-refs' do
+        create_objects(project)
+        before_packs = packs(project)
+        before_packed_refs = packed_refs(project)
+
+        expect(before_packs.count).to be >= 1
+
+        subject.perform(project.id, 'gc')
+        after_packed_refs = packed_refs(project)
+        after_packs = packs(project)
+
+        expect(after_packs.count).to eq(1)
+
+        # Previously existing packs should be gone now
+        expect(after_packs - before_packs).to eq(after_packs)
+
+        # The packed-refs file should have been updated during 'git gc'
+        expect(before_packed_refs).not_to eq(after_packed_refs)
+
+        expect(File.exist?(bitmap_path(after_packs.first))).to eq(bitmaps_enabled)
+      end
+    end
+
+    context 'with bitmaps enabled' do
+      let(:bitmaps_enabled) { true }
+
+      include_examples 'gc tasks'
+    end
+
+    context 'with bitmaps disabled' do
+      let(:bitmaps_enabled) { false }
+
+      include_examples 'gc tasks'
+    end
+  end
+
+  # Create a new commit on a random new branch
+  def create_objects(project)
+    rugged = project.repository.rugged
+    old_commit = rugged.branches.first.target
+    new_commit_sha = Rugged::Commit.create(
+      rugged,
+      message: "hello world #{SecureRandom.hex(6)}",
+      author: Gitlab::Git::committer_hash(email: 'foo@bar', name: 'baz'),
+      committer: Gitlab::Git::committer_hash(email: 'foo@bar', name: 'baz'),
+      tree: old_commit.tree,
+      parents: [old_commit],
+    )
+    project.repository.update_ref!(
+      "refs/heads/#{SecureRandom.hex(6)}",
+      new_commit_sha,
+      Gitlab::Git::BLANK_SHA
+    )
+  end
+
+  def packs(project)
+    Dir["#{project.repository.path_to_repo}/objects/pack/*.pack"]
+  end
+
+  def packed_refs(project)
+    path = "#{project.repository.path_to_repo}/packed-refs"
+    FileUtils.touch(path)
+    File.read(path)
+  end
+
+  def bitmap_path(pack)
+    pack.sub(/\.pack\z/, '.bitmap')
   end
 end