Merge branch '665-avoid-locking-when-updating-mirrors' into 'master'

Distribute RepositoryUpdateMirror jobs with exclusive lease

See merge request !462

CHANGELOG-EE

@@ -5,6 +5,7 @@ v 8.9.0 (unreleased)
   - Allow LDAP to mark users as external based on their group membership. !432
   - Instrument instance methods of Gitlab::InsecureKeyFingerprint class
   - Add API endpoint for Merge Request Approvals !449
+  - Distribute RepositoryUpdateMirror jobs in time and add exclusive lease on them by project_id
 
 v 8.8.4
   - Remove license overusage message

app/models/project.rb

@@ -544,7 +544,7 @@ class Project < ActiveRecord::Base
     mirror_updated? && self.mirror_last_successful_update_at
   end
 
-  def update_mirror
+  def update_mirror(delay: 0)
     return unless mirror? && repository_exists?
 
     return if import_in_progress?
@@ -555,7 +555,7 @@ class Project < ActiveRecord::Base
       import_start
     end
 
-    RepositoryUpdateMirrorWorker.perform_async(self.id)
+    RepositoryUpdateMirrorWorker.perform_in(delay, self.id)
   end
 
   def mark_import_as_failed(error_message)

app/workers/repository_update_mirror_worker.rb

@@ -2,11 +2,15 @@ class RepositoryUpdateMirrorWorker
   include Sidekiq::Worker
   include Gitlab::ShellAdapter
 
+  LEASE_TIMEOUT = 300
+  
   sidekiq_options queue: :gitlab_shell
 
   attr_accessor :project, :repository, :current_user
 
   def perform(project_id)
+    return unless try_obtain_lease(project_id)
+
     @project = Project.find(project_id)
     @current_user = @project.mirror_user || @project.creator
 
@@ -18,4 +22,12 @@ class RepositoryUpdateMirrorWorker
 
     project.import_finish
   end
+
+  private
+
+  def try_obtain_lease(project_id)
+    # Using 5 minutes timeout based on the 95th percent of timings (currently max of 25 seconds)
+    lease = ::Gitlab::ExclusiveLease.new("repository_update_mirror:#{project_id}", timeout: LEASE_TIMEOUT)
+    lease.try_obtain
+  end
 end

app/workers/update_all_mirrors_worker.rb

 class UpdateAllMirrorsWorker
   include Sidekiq::Worker
 
+  LEASE_TIMEOUT = 3600
+
   def perform
+    return unless try_obtain_lease
+
     fail_stuck_mirrors!
 
-    Project.mirror.each(&:update_mirror)
+    Project.mirror.find_each(batch_size: 200) do |project|
+      project.update_mirror(delay: rand(30.minutes))
+    end
   end
 
   def fail_stuck_mirrors!
@@ -16,4 +22,12 @@ class UpdateAllMirrorsWorker
       project.mark_import_as_failed('The mirror update took too long to complete.')
     end
   end
+
+  private
+
+  def try_obtain_lease
+    # Using 30 minutes timeout based on the 95th percent of timings (currently max of 10 minutes)
+    lease = ::Gitlab::ExclusiveLease.new("update_all_mirrors", timeout: LEASE_TIMEOUT)
+    lease.try_obtain
+  end
 end

spec/workers/repository_update_mirror_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryUpdateMirrorWorker do
+  describe '#perform' do
+    it "just returns if cannot obtain a lease" do
+      worker = described_class.new
+      project_id = 15
+
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(false)
+
+      expect(Projects::UpdateMirrorService).not_to receive(:execute)
+
+      worker.perform(project_id)
+    end
+
+    context "when obtain the lease" do
+      before do
+        allow_any_instance_of(Gitlab::ExclusiveLease)
+          .to receive(:try_obtain).and_return(true)
+      end
+
+      it "sets import as finished when update mirror service executes successfully" do
+        project = create(:empty_project, :mirror)
+
+        expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :success)
+
+        expect do
+          described_class.new.perform(project.id)
+        end.to change { project.reload.import_status }.to("finished")
+      end
+
+      it "sets import as failed when update mirror service executes with errors" do
+        project = create(:empty_project, :mirror)
+
+        expect_any_instance_of(Projects::UpdateMirrorService).to receive(:execute).and_return(status: :error, message: 'fail!')
+
+        expect do
+          described_class.new.perform(project.id)
+        end.to change { project.reload.import_status }.to("failed")
+      end
+    end
+  end
+end

spec/workers/update_all_mirrors_worker_spec.rb

 require 'rails_helper'
 
 describe UpdateAllMirrorsWorker do
+  before do
+    allow_any_instance_of(Gitlab::ExclusiveLease)
+      .to receive(:try_obtain).and_return(true)
+  end
+
   describe '#perform' do
     it 'fails stuck mirrors' do
       worker = described_class.new
@@ -11,12 +16,27 @@ describe UpdateAllMirrorsWorker do
     end
 
     it 'updates all mirrored Projects' do
+      worker = described_class.new
+
       create(:empty_project, :mirror)
       create(:empty_project)
 
-      expect_any_instance_of(Project).to receive(:update_mirror).once
+      expect(worker).to receive(:rand).with(30.minutes).and_return(10)
+      expect_any_instance_of(Project).to receive(:update_mirror).with(delay: 10).once
+
+      worker.perform
+    end
 
-      described_class.new.perform
+    it 'does not execute if cannot get the lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(false)
+
+      worker = described_class.new
+      create(:empty_project, :mirror)
+
+      expect(worker).not_to receive(:fail_stuck_mirrors!)
+
+      worker.perform
     end
   end