Merge branch...

Merge branch '35558-only-one-garbage-collection-should-be-running-per-project-at-once' into 'master'

Adds exclusive lease to Git garbage collect worker.

Closes #35558

See merge request !14036

app/workers/git_garbage_collect_worker.rb

@@ -5,6 +5,9 @@ class GitGarbageCollectWorker
 
   sidekiq_options retry: false
 
+  # Timeout set to 24h
+  LEASE_TIMEOUT = 86400
+
   GITALY_MIGRATED_TASKS = {
     gc: :garbage_collect,
     full_repack: :repack_full,
@@ -13,8 +16,19 @@ class GitGarbageCollectWorker
 
   def perform(project_id, task = :gc, lease_key = nil, lease_uuid = nil)
     project = Project.find(project_id)
-    task = task.to_sym
+    active_uuid = get_lease_uuid(lease_key)
+
+    if active_uuid
+      return unless active_uuid == lease_uuid
+
+      renew_lease(lease_key, active_uuid)
+    else
+      lease_uuid = try_obtain_lease(lease_key)
+
+      return unless lease_uuid
+    end
 
+    task = task.to_sym
     cmd = command(task)
     repo_path = project.repository.path_to_repo
     description = "'#{cmd.join(' ')}' in #{repo_path}"
@@ -33,11 +47,27 @@ class GitGarbageCollectWorker
     # Refresh the branch cache in case garbage collection caused a ref lookup to fail
     flush_ref_caches(project) if task == :gc
   ensure
-    Gitlab::ExclusiveLease.cancel(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present?
+    cancel_lease(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present?
   end
 
   private
 
+  def try_obtain_lease(key)
+    ::Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain
+  end
+
+  def renew_lease(key, uuid)
+    ::Gitlab::ExclusiveLease.new(key, uuid: uuid, timeout: LEASE_TIMEOUT).renew
+  end
+
+  def cancel_lease(key, uuid)
+    ::Gitlab::ExclusiveLease.cancel(key, uuid)
+  end
+
+  def get_lease_uuid(key)
+    ::Gitlab::ExclusiveLease.get_uuid(key)
+  end
+
   ## `repository` has to be a Gitlab::Git::Repository
   def gitaly_call(task, repository)
     client = Gitlab::GitalyClient::RepositoryService.new(repository)

lib/gitlab/exclusive_lease.rb

@@ -25,6 +25,12 @@ module Gitlab
       end
     EOS
 
+    def self.get_uuid(key)
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.get(redis_shared_state_key(key)) || false
+      end
+    end
+
     def self.cancel(key, uuid)
       Gitlab::Redis::SharedState.with do |redis|
         redis.eval(LUA_CANCEL_SCRIPT, keys: [redis_shared_state_key(key)], argv: [uuid])
@@ -35,10 +41,10 @@ module Gitlab
       "gitlab:exclusive_lease:#{key}"
     end
 
-    def initialize(key, timeout:)
+    def initialize(key, uuid: nil, timeout:)
       @redis_shared_state_key = self.class.redis_shared_state_key(key)
       @timeout = timeout
-      @uuid = SecureRandom.uuid
+      @uuid = uuid || SecureRandom.uuid
     end
 
     # Try to obtain the lease. Return lease UUID on success,

spec/lib/gitlab/exclusive_lease_spec.rb

@@ -47,6 +47,18 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '.get_uuid' do
+    it 'gets the uuid if lease with the key associated exists' do
+      uuid = described_class.new(unique_key, timeout: 3600).try_obtain
+
+      expect(described_class.get_uuid(unique_key)).to eq(uuid)
+    end
+
+    it 'returns false if the lease does not exist' do
+      expect(described_class.get_uuid(unique_key)).to be false
+    end
+  end
+
   describe '.cancel' do
     it 'can cancel a lease' do
       uuid = new_lease(unique_key)

spec/services/projects/housekeeping_service_spec.rb

@@ -20,6 +20,7 @@ describe Projects::HousekeepingService do
       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)
     end
 

spec/workers/git_garbage_collect_worker_spec.rb

@@ -5,28 +5,100 @@ require 'spec_helper'
 describe GitGarbageCollectWorker do
   let(:project) { create(:project, :repository) }
   let(:shell) { Gitlab::Shell.new }
+  let!(:lease_uuid) { SecureRandom.uuid }
+  let!(:lease_key) { "project_housekeeping:#{project.id}" }
 
   subject { described_class.new }
 
   describe "#perform" do
     shared_examples 'flushing ref caches' do |gitaly|
-      it "flushes ref caches when the task if 'gc'" do
-        expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
-
-        if gitaly
-          expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
-            .and_return(nil)
-        else
-          expect(Gitlab::Popen).to receive(:popen)
-            .with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
+      context 'with active lease_uuid' do
+        before do
+          allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
         end
 
-        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(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
-        expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
+        it "flushes ref caches when the task if 'gc'" do
+          expect(subject).to receive(:renew_lease).with(lease_key, lease_uuid).and_call_original
+          expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
 
-        subject.perform(project.id)
+          if gitaly
+            expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
+              .and_return(nil)
+          else
+            expect(Gitlab::Popen).to receive(:popen)
+              .with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
+          end
+
+          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(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
+          expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
+
+          subject.perform(project.id, :gc, lease_key, lease_uuid)
+        end
+      end
+
+      context 'with different lease than the active one' do
+        before do
+          allow(subject).to receive(:get_lease_uuid).and_return(SecureRandom.uuid)
+        end
+
+        it 'returns silently' do
+          expect(subject).not_to receive(:command)
+          expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
+          expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
+          expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
+          expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
+
+          subject.perform(project.id, :gc, lease_key, lease_uuid)
+        end
+      end
+
+      context 'with no active lease' do
+        before do
+          allow(subject).to receive(:get_lease_uuid).and_return(false)
+        end
+
+        context 'when is able to get the lease' do
+          before do
+            allow(subject).to receive(:try_obtain_lease).and_return(SecureRandom.uuid)
+          end
+
+          it "flushes ref caches when the task if 'gc'" do
+            expect(subject).to receive(:command).with(:gc).and_return([:the, :command])
+
+            if gitaly
+              expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect)
+                .and_return(nil)
+            else
+              expect(Gitlab::Popen).to receive(:popen)
+                .with([:the, :command], project.repository.path_to_repo).and_return(["", 0])
+            end
+
+            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(Gitlab::Git::Repository).to receive(:branch_count).and_call_original
+            expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original
+
+            subject.perform(project.id)
+          end
+        end
+
+        context 'when no lease can be obtained' do
+          before do
+            expect(subject).to receive(:try_obtain_lease).and_return(false)
+          end
+
+          it 'returns silently' do
+            expect(subject).not_to receive(:command)
+            expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original
+            expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original
+            expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original
+            expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original
+
+            subject.perform(project.id)
+          end
+        end
       end
     end
 
@@ -39,25 +111,34 @@ describe GitGarbageCollectWorker do
     end
 
     context "repack_full" do
+      before do
+        expect(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
+      end
+
       it "calls Gitaly" do
         expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:repack_full)
           .and_return(nil)
 
-        subject.perform(project.id, :full_repack)
+        subject.perform(project.id, :full_repack, lease_key, lease_uuid)
       end
     end
 
     context "repack_incremental" do
+      before do
+        expect(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
+      end
+
       it "calls Gitaly" do
         expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:repack_incremental)
           .and_return(nil)
 
-        subject.perform(project.id, :incremental_repack)
+        subject.perform(project.id, :incremental_repack, lease_key, lease_uuid)
       end
     end
 
     shared_examples 'gc tasks' do
       before do
+        allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid)
         allow(subject).to receive(:bitmaps_enabled?).and_return(bitmaps_enabled)
       end
 
@@ -67,7 +148,7 @@ describe GitGarbageCollectWorker do
 
         expect(before_packs.count).to be >= 1
 
-        subject.perform(project.id, 'incremental_repack')
+        subject.perform(project.id, 'incremental_repack', lease_key, lease_uuid)
         after_packs = packs(project)
 
         # Exactly one new pack should have been created
@@ -79,12 +160,12 @@ describe GitGarbageCollectWorker do
 
       it 'full repack consolidates into 1 packfile' do
         create_objects(project)
-        subject.perform(project.id, 'incremental_repack')
+        subject.perform(project.id, 'incremental_repack', lease_key, lease_uuid)
         before_packs = packs(project)
 
         expect(before_packs.count).to be >= 2
 
-        subject.perform(project.id, 'full_repack')
+        subject.perform(project.id, 'full_repack', lease_key, lease_uuid)
         after_packs = packs(project)
 
         expect(after_packs.count).to eq(1)
@@ -102,7 +183,7 @@ describe GitGarbageCollectWorker do
 
         expect(before_packs.count).to be >= 1
 
-        subject.perform(project.id, 'gc')
+        subject.perform(project.id, 'gc', lease_key, lease_uuid)
         after_packed_refs = packed_refs(project)
         after_packs = packs(project)