Merge branch '748-update-mirror' into 'master'

Fix race condition with UpdateMirrorWorker Lease

Any time we modify the project import state to an in progress we're going to execute the sidekiq job not lease to try to obtain in this workflow. This fixes the UI problem when an user try to update a mirror just inside a lease timeout.

Closes #748 

See merge request !641

CHANGELOG-EE

@@ -11,6 +11,9 @@ v 8.11.0 (unreleased)
   - [Elastic] Significant improvement of global search performance
   - [Fix] Push rules check existing commits in some cases
 
+v 8.10.6
+  - Fix race condition with UpdateMirrorWorker Lease
+
 v 8.10.5
   - Used cached value of project count in `Elastic::RepositoriesSearch` to reduce DB load. !637
 

app/models/project.rb

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

app/workers/repository_update_mirror_dispatch_worker.rb

+class RepositoryUpdateMirrorDispatchWorker
+  include Sidekiq::Worker
+
+  LEASE_TIMEOUT = 5.minutes
+
+  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_by_id(project_id)
+    return unless project
+
+    project.update_mirror
+  end
+
+  private
+
+  def try_obtain_lease(project_id)
+    # Using 5 minutes timeout based on the 95th percent of timings (currently max of 25 minutes)
+    lease = ::Gitlab::ExclusiveLease.new("repository_update_mirror_dispatcher:#{project_id}", timeout: LEASE_TIMEOUT)
+    lease.try_obtain
+  end
+end

app/workers/repository_update_mirror_worker.rb

@@ -4,16 +4,12 @@ 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)
     begin
-      return unless try_obtain_lease(project_id)
-
       @project = Project.find(project_id)
       @current_user = @project.mirror_user || @project.creator
 
@@ -30,12 +26,4 @@ class RepositoryUpdateMirrorWorker
       raise UpdateMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}"
     end
   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

@@ -9,7 +9,7 @@ class UpdateAllMirrorsWorker
     fail_stuck_mirrors!
 
     Project.mirror.find_each(batch_size: 200) do |project|
-      project.update_mirror(delay: rand(30.minutes))
+      RepositoryUpdateMirrorDispatchWorker.perform_in(rand(30.minutes), project.id)
     end
   end
 

spec/features/projects/mirror_spec.rb

+require 'spec_helper'
+
+feature 'Project mirror', feature: true do
+  let(:project) { create(:project, :mirror, :import_finished, creator: user, name: 'Victorialand') }
+  let(:user) { create(:user) }
+
+  describe 'On a project', js: true do
+    before do
+      project.team << [user, :master]
+      login_as user
+      visit namespace_project_mirror_path(project.namespace, project)
+    end
+
+    describe 'pressing "Update now"' do
+      it 'returns with the project updating (job enqueued)' do
+        Sidekiq::Testing.fake! { click_link('Update Now') }
+
+        expect(page).to have_content('Updating')
+      end
+    end
+  end
+end

spec/workers/repository_update_mirror_dispatch_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryUpdateMirrorDispatchWorker do
+  describe '#perform' do
+    it 'executes project#update_mirror if can obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(true)
+      expect_any_instance_of(Project).to receive(:update_mirror)
+
+      project = create(:empty_project, :mirror)
+      described_class.new.perform(project.id)
+    end
+
+    it 'just returns if cannot obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(false)
+      expect_any_instance_of(Project).not_to receive(:update_mirror)
+
+      project = create(:empty_project, :mirror)
+      described_class.new.perform(project.id)
+    end
+  end
+end

spec/workers/repository_update_mirror_worker_spec.rb

@@ -2,43 +2,22 @@ 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
+    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")
+      expect { described_class.new.perform(project.id) }
+        .to change { project.reload.import_status }.to('finished')
     end
 
-      it "sets import as failed when update mirror service executes with errors" do
+    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
+      expect { described_class.new.perform(project.id) }
+        .to change { project.reload.import_status }.to('failed')
     end
   end
 end

spec/workers/update_all_mirrors_worker_spec.rb

@@ -15,14 +15,14 @@ describe UpdateAllMirrorsWorker do
       worker.perform
     end
 
-    it 'updates all mirrored Projects' do
+    it 'enqueue a job on all mirrored Projects' do
       worker = described_class.new
 
-      create(:empty_project, :mirror)
+      mirror = create(:empty_project, :mirror)
       create(:empty_project)
 
       expect(worker).to receive(:rand).with(30.minutes).and_return(10)
-      expect_any_instance_of(Project).to receive(:update_mirror).with(delay: 10).once
+      expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(10, mirror.id)
 
       worker.perform
     end